-
-
Notifications
You must be signed in to change notification settings - Fork 643
fix(Android): drawing order for multiple disappearing screens #2795
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
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ import com.swmansion.rnscreens.utils.setTweenAnimations | |
| import java.util.Collections | ||
| import kotlin.collections.ArrayList | ||
| import kotlin.math.max | ||
| import kotlin.math.min | ||
|
|
||
| internal interface ChildDrawingOrderStrategy { | ||
| /** | ||
|
|
@@ -68,8 +69,12 @@ internal class ReverseOrderInRange( | |
| return | ||
| } | ||
|
|
||
| var startRange = range.start | ||
| var endRange = range.endInclusive | ||
| if (drawingOperations.isEmpty()) { | ||
| return | ||
| } | ||
|
|
||
| var startRange = min(range.start, drawingOperations.lastIndex) | ||
| var endRange = min(range.endInclusive, drawingOperations.lastIndex) | ||
|
|
||
| while (startRange < endRange) { | ||
| Collections.swap(drawingOperations, startRange, endRange) | ||
|
|
@@ -87,8 +92,8 @@ class ScreenStack( | |
| private val drawingOpPool: MutableList<DrawingOp> = ArrayList() | ||
| private var drawingOps: MutableList<DrawingOp> = ArrayList() | ||
| private var topScreenWrapper: ScreenStackFragmentWrapper? = null | ||
| private var removalTransitionStarted = false | ||
| private var previousChildrenCount = 0 | ||
| private var removalTransitionChildrenCount = 0 | ||
|
|
||
| private var childDrawingOrderStrategy: ChildDrawingOrderStrategy? = null | ||
|
|
||
|
|
@@ -97,7 +102,7 @@ class ScreenStack( | |
| /** | ||
| * Marks given fragment as to-be-dismissed and performs updates on container | ||
| * | ||
| * @param fragmentWrapper to-be-dismissed wrapper | ||
| * @param screenFragment to-be-dismissed wrapper | ||
| */ | ||
| fun dismiss(screenFragment: ScreenStackFragmentWrapper) { | ||
| dismissedWrappers.add(screenFragment) | ||
|
|
@@ -124,20 +129,22 @@ class ScreenStack( | |
| override fun startViewTransition(view: View) { | ||
| super.startViewTransition(view) | ||
| childDrawingOrderStrategy?.enable() | ||
| removalTransitionStarted = true | ||
| removalTransitionChildrenCount += 1 | ||
| // println("+1, now: " + removalTransitionChildrenCount) | ||
| } | ||
|
|
||
| override fun endViewTransition(view: View) { | ||
| super.endViewTransition(view) | ||
| childDrawingOrderStrategy?.disable() | ||
| if (removalTransitionStarted) { | ||
| removalTransitionStarted = false | ||
| removalTransitionChildrenCount = max(0, removalTransitionChildrenCount - 1) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without this, the count would go negative - in the formSheet example (TestFormSheet -> Open Sheet -> Open Second) it prints: |
||
| // println("-1, now: " + removalTransitionChildrenCount) | ||
| if (removalTransitionChildrenCount == 0) { | ||
| childDrawingOrderStrategy?.disable() | ||
| dispatchOnFinishTransitioning() | ||
| } | ||
| } | ||
|
|
||
| fun onViewAppearTransitionEnd() { | ||
| if (!removalTransitionStarted) { | ||
| if (removalTransitionChildrenCount == 0) { | ||
| dispatchOnFinishTransitioning() | ||
| } | ||
| } | ||
|
Comment on lines
129
to
149
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure whether this properly mirrors logic from before. |
||
|
|
@@ -229,16 +236,27 @@ class ScreenStack( | |
| needsDrawReordering(newTop, stackAnimation) && | ||
| visibleBottom == null | ||
| ) { | ||
| // When using an open animation in which two screens overlap (eg. fade_from_bottom or | ||
| // slide_from_bottom), we want to draw the previous screen under the new one, | ||
| // which is apparently not the default option. Android always draws the disappearing view | ||
| // When using an open animation in which screens overlap (eg. fade_from_bottom or | ||
| // slide_from_bottom), we want to draw the previous screens under the new one, | ||
| // which is apparently not the default option. Android always draws the disappearing views | ||
| // on top of the appearing one. We then reverse the order of the views so the new screen | ||
| // appears on top of the previous one. You can read more about in the comment | ||
| // appears on top of the previous ones. You can read more about in the comment | ||
| // for the code we use to change that behavior: | ||
| // https://github.com/airbnb/native-navigation/blob/9cf50bf9b751b40778f473f3b19fcfe2c4d40599/lib/android/src/main/java/com/airbnb/android/react/navigation/ScreenCoordinatorLayout.java#L18 | ||
| // Note: This should not be set in case there is only a single screen in stack or animation `none` is used. | ||
| // Atm needsDrawReordering implementation guards that assuming that first screen on stack uses `NONE` animation. | ||
| childDrawingOrderStrategy = SwapLastTwo() | ||
|
|
||
| val disappearingCount = screenWrappers | ||
| .asReversed() | ||
| .asSequence() | ||
| .filter { !dismissedWrappers.contains(it) | ||
| && it.screen.activityState !== Screen.ActivityState.INACTIVE } | ||
| .drop(1) | ||
| .takeWhile { it.screen.isTransparent() } | ||
| .count() + 1 | ||
|
|
||
| childDrawingOrderStrategy = ReverseOrderInRange( | ||
| screenWrappers.lastIndex - disappearingCount..screenWrappers.lastIndex) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what reference should I use here to count disappearing screens - I mirrored logic from |
||
| } else if (newTop != null && | ||
| newTopAlreadyInStack && | ||
| topScreenWrapper?.screen?.isTransparent() == true && | ||
|
|
@@ -354,12 +372,6 @@ class ScreenStack( | |
| override fun dispatchDraw(canvas: Canvas) { | ||
| super.dispatchDraw(canvas) | ||
|
|
||
| // check the view removal is completed (by comparing the previous children count) | ||
| if (drawingOps.size < previousChildrenCount) { | ||
| childDrawingOrderStrategy = null | ||
| } | ||
| previousChildrenCount = drawingOps.size | ||
|
|
||
|
Comment on lines
-357
to
-362
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think with disable() method on childDrawingOrderStrategy this probably isn't needed anymore but I am not 100% sure. |
||
| childDrawingOrderStrategy?.apply(drawingOps) | ||
|
|
||
| drawAndRelease() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like handling this kind of API misuse here. This shouldn't be job of the
ChildDrawingOrderStrategyto determine what is the range, it should get the range as argument and assume it makes sense.