Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Translucent fragments #69

Merged
merged 7 commits into from
Mar 31, 2017
Merged

Translucent fragments #69

merged 7 commits into from
Mar 31, 2017

Conversation

comigor
Copy link
Contributor

@comigor comigor commented Mar 28, 2017

Keep old screen below the new one to be presented if the new screen's screenColor is translucent (to use like modals, for instance).

Supersedes #60

@comigor
Copy link
Contributor Author

comigor commented Mar 28, 2017

c/c @gpeal @lelandrichardson

@comigor
Copy link
Contributor Author

comigor commented Mar 29, 2017

Actually, this PR won't work due to the addition of ScreenCoordinatorLayout. By controlling the drawing order there, when we keep the old view (don't .detach() it), the reverseDrawing check would be true and so the new view would be drawn below the old one.

@gusbicalho
Copy link

gusbicalho commented Mar 29, 2017

The logic on ScreenCoordinatorLayout to reverse the order of the drawing only works if it has exactly two children. If we do not detach the fragment, we'll end up with several children, and reversing the child draw order will not have the desired effect.

I think the correct behavior is to reverse only the last two views

@gpeal
Copy link
Collaborator

gpeal commented Mar 30, 2017

@gusbicalho Yeah, reversing the last two views is probably correct for ScreenCoordinatorLayout. It has only been tested with the normal push/present case but could be modified to be more flexible.

reverse only the last 2 children if willDetachCurrentScreen() is called
@comigor
Copy link
Contributor Author

comigor commented Mar 30, 2017

@gpeal Our last commit seems to solve the issue we were discussing.

reverseLastTwoChildren = false;
}
previousChildrenCount = drawingOps.size();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gpeal I guess we can keep track on children count on our side then ☝️


private int previousBackStackEntryCount = 0;
private boolean reverseDrawing = false;
private List<DrawingOp> drawingOps = new ArrayList<>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can both be final

super.removeView(view);
}

@Override
protected void dispatchDraw(Canvas canvas) {
super.dispatchDraw(canvas);

while (!drawingOps.isEmpty()) {
DrawingOp op = drawingOps.remove(drawingOps.size() - 1);
if (drawingOps.size() < previousChildrenCount) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment here?

}
previousChildrenCount = drawingOps.size();

while (drawingOps.size() > 2) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Maybe we can make a drawAndRelease helper
  2. Can this loop just be in the else case and loop until !empty? Seems like all it's doing is finishing this loop. Then make the if case include this loop plus what's currently there.

}
previousChildrenCount = drawingOps.size();

if (reverseLastTwoChildren) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Igor1201 Would this be even more readable if you did an explicit swap of the last 2 items in the if block then just loop through them all?

@gpeal gpeal merged commit ba5ef8a into airbnb:master Mar 31, 2017
@comigor
Copy link
Contributor Author

comigor commented Mar 31, 2017

Thanks for your patience! By the way, do we have a schedule of when to tag releases/push to npm?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants