-
-
Notifications
You must be signed in to change notification settings - Fork 643
fix(Android): drawing order for multiple disappearing screens #2806
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 8 commits
4cbd032
97b0025
472e8a5
51e98e1
f482314
d8eab84
6655a66
56b2cf2
fc308c6
f44513a
d6857e8
611ed88
264cae7
2dc640c
2f15672
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 |
|---|---|---|
|
|
@@ -9,32 +9,13 @@ import com.facebook.react.uimanager.UIManagerHelper | |
| import com.swmansion.rnscreens.Screen.StackAnimation | ||
| import com.swmansion.rnscreens.bottomsheet.isSheetFitToContents | ||
| import com.swmansion.rnscreens.events.StackFinishTransitioningEvent | ||
| import com.swmansion.rnscreens.stack.views.ChildDrawingOrderStrategy | ||
| import com.swmansion.rnscreens.stack.views.ScreensCoordinatorLayout | ||
| import com.swmansion.rnscreens.utils.setTweenAnimations | ||
| import java.util.Collections | ||
| import kotlin.collections.ArrayList | ||
| import kotlin.math.max | ||
|
|
||
| internal interface ChildDrawingOrderStrategy { | ||
| /** | ||
| * Mutates the list of draw operations **in-place**. | ||
| */ | ||
| fun apply(drawingOperations: MutableList<ScreenStack.DrawingOp>) | ||
|
|
||
| /** | ||
| * Enables the given strategy. When enabled - the strategy **might** mutate the operations | ||
| * list passed to `apply` method. | ||
| */ | ||
| fun enable() | ||
|
|
||
| /** | ||
| * Disables the given strategy - even when `apply` is called it **must not** produce | ||
| * any side effect (it must not manipulate the drawing operations list passed to `apply` method). | ||
| */ | ||
| fun disable() | ||
|
|
||
| fun isEnabled(): Boolean | ||
| } | ||
|
|
||
| internal abstract class ChildDrawingOrderStrategyBase( | ||
| var enabled: Boolean = false, | ||
| ) : ChildDrawingOrderStrategy { | ||
|
|
@@ -49,28 +30,35 @@ internal abstract class ChildDrawingOrderStrategyBase( | |
| override fun isEnabled() = enabled | ||
| } | ||
|
|
||
| internal class SwapLastTwo : ChildDrawingOrderStrategyBase() { | ||
| internal class ReverseOrderInRange( | ||
| val range: IntRange, | ||
| ) : ChildDrawingOrderStrategyBase() { | ||
| override fun apply(drawingOperations: MutableList<ScreenStack.DrawingOp>) { | ||
| if (!isEnabled()) { | ||
| return | ||
| } | ||
| if (drawingOperations.size >= 2) { | ||
| Collections.swap(drawingOperations, drawingOperations.lastIndex, drawingOperations.lastIndex - 1) | ||
|
|
||
| var startRange = range.start | ||
| var endRange = range.endInclusive | ||
|
|
||
| while (startRange < endRange) { | ||
| Collections.swap(drawingOperations, startRange, endRange) | ||
| startRange += 1 | ||
| endRange -= 1 | ||
| } | ||
| } | ||
| } | ||
|
|
||
| internal class ReverseOrderInRange( | ||
| val range: IntRange, | ||
| internal class ReverseOrderStartingFrom( | ||
| val startIndex: Int, | ||
| ) : ChildDrawingOrderStrategyBase() { | ||
| override fun apply(drawingOperations: MutableList<ScreenStack.DrawingOp>) { | ||
| if (!isEnabled()) { | ||
| return | ||
| } | ||
|
|
||
| var startRange = range.start | ||
| var endRange = range.endInclusive | ||
|
|
||
| var startRange = startIndex | ||
| var endRange = drawingOperations.lastIndex | ||
| while (startRange < endRange) { | ||
| Collections.swap(drawingOperations, startRange, endRange) | ||
| startRange += 1 | ||
|
|
@@ -88,9 +76,9 @@ class ScreenStack( | |
| private var drawingOps: MutableList<DrawingOp> = ArrayList() | ||
| private var topScreenWrapper: ScreenStackFragmentWrapper? = null | ||
| private var removalTransitionStarted = false | ||
| private var previousChildrenCount = 0 | ||
|
|
||
| private var childDrawingOrderStrategy: ChildDrawingOrderStrategy? = null | ||
| private var disappearingTransitioningChildren: MutableList<View> = ArrayList() | ||
|
|
||
| var goingForward = false | ||
|
|
||
|
|
@@ -122,14 +110,25 @@ class ScreenStack( | |
| } | ||
|
|
||
| override fun startViewTransition(view: View) { | ||
| check(view is ScreensCoordinatorLayout) { "[RNScreens] Unexpected type of ScreenStack direct subview ${view.javaClass}" } | ||
| super.startViewTransition(view) | ||
| childDrawingOrderStrategy?.enable() | ||
| if (view.fragment.isRemoving) { | ||
| disappearingTransitioningChildren.add(view) | ||
| } | ||
| if (disappearingTransitioningChildren.isNotEmpty()) { | ||
| childDrawingOrderStrategy?.enable() | ||
| } | ||
| removalTransitionStarted = true | ||
| } | ||
|
|
||
| override fun endViewTransition(view: View) { | ||
| super.endViewTransition(view) | ||
| childDrawingOrderStrategy?.disable() | ||
|
|
||
| disappearingTransitioningChildren.remove(view) | ||
|
|
||
| if (disappearingTransitioningChildren.isEmpty()) { | ||
| childDrawingOrderStrategy?.disable() | ||
| } | ||
| if (removalTransitionStarted) { | ||
| removalTransitionStarted = false | ||
| dispatchOnFinishTransitioning() | ||
|
|
@@ -175,21 +174,21 @@ class ScreenStack( | |
| childDrawingOrderStrategy = null | ||
|
|
||
| // Determine new first & last visible screens. | ||
| // Scope function to limit the scope of locals. | ||
| run { | ||
| val notDismissedWrappers = | ||
| screenWrappers | ||
| .asReversed() | ||
| .asSequence() | ||
| .filter { !dismissedWrappers.contains(it) && it.screen.activityState !== Screen.ActivityState.INACTIVE } | ||
| val notDismissedWrappers = | ||
| screenWrappers | ||
| .asReversed() | ||
| .asSequence() | ||
| .filter { | ||
| !dismissedWrappers.contains(it) && | ||
| it.screen.activityState !== Screen.ActivityState.INACTIVE | ||
| } | ||
|
|
||
| newTop = notDismissedWrappers.firstOrNull() | ||
| visibleBottom = | ||
| notDismissedWrappers | ||
| .dropWhile { it.screen.isTransparent() } | ||
| .firstOrNull() | ||
| ?.takeUnless { it === newTop } | ||
| } | ||
| newTop = notDismissedWrappers.firstOrNull() | ||
| visibleBottom = | ||
| notDismissedWrappers | ||
| .dropWhile { it.screen.isTransparent() } | ||
| .firstOrNull() | ||
| ?.takeUnless { it === newTop } | ||
|
|
||
| var shouldUseOpenAnimation = true | ||
| var stackAnimation: StackAnimation? = null | ||
|
|
@@ -229,16 +228,28 @@ 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() | ||
|
|
||
| // Count how many screens are disappearing | ||
| // (all transparent screens beneath newTop + one screen under them) | ||
| val disappearingCount = | ||
| notDismissedWrappers | ||
| .drop(1) | ||
| .takeWhile { it.screen.isTransparent() } | ||
| .count() + 1 | ||
|
Member
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 reading the code once again now & my initial impression is that it won't work in general case. I need to come up with counter example -- will try to do that next week.
Member
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. The problem is that I think we need to find some time in upcoming weeks & design this algorithm properly from the ground up.
Member
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. Notation:
This case will have issue, where the count of dismissed screens will be 0 -> therefore we won't change drawing order, while we should. One thing to note that current solution might be good enough to work in simple stack operations (push, pop, replace). The one I've just described is not a stack operation. My aspiration is to make the screen stack work with arbitrary state changes, but I guess we can postpone this plant until we have more robust update algorithm. |
||
|
|
||
| childDrawingOrderStrategy = | ||
| ReverseOrderStartingFrom( | ||
| screenWrappers.lastIndex - disappearingCount, | ||
| ) | ||
| } else if (newTop != null && | ||
| newTopAlreadyInStack && | ||
| topScreenWrapper?.screen?.isTransparent() == true && | ||
|
|
@@ -354,12 +365,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 | ||
|
|
||
| childDrawingOrderStrategy?.apply(drawingOps) | ||
|
|
||
| drawAndRelease() | ||
|
|
@@ -410,7 +415,7 @@ class ScreenStack( | |
| fragmentWrapper: ScreenFragmentWrapper, | ||
| resolvedStackAnimation: StackAnimation?, | ||
| ): Boolean { | ||
| val stackAnimation = if (resolvedStackAnimation != null) resolvedStackAnimation else fragmentWrapper.screen.stackAnimation | ||
| val stackAnimation = resolvedStackAnimation ?: fragmentWrapper.screen.stackAnimation | ||
| // On Android sdk 33 and above the animation is different and requires draw reordering. | ||
| // For React Native 0.70 and lower versions, `Build.VERSION_CODES.TIRAMISU` is not defined yet. | ||
| // Hence, we're comparing numerical version here. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| package com.swmansion.rnscreens.stack.anim | ||
|
|
||
| import android.view.animation.Animation | ||
| import android.view.animation.Transformation | ||
| import com.swmansion.rnscreens.ScreenFragment | ||
|
|
||
| internal class ScreensAnimation( | ||
| private val mFragment: ScreenFragment, | ||
| ) : Animation() { | ||
| override fun applyTransformation( | ||
| interpolatedTime: Float, | ||
| t: Transformation, | ||
| ) { | ||
| super.applyTransformation(interpolatedTime, t) | ||
| // interpolated time should be the progress of the current transition | ||
| mFragment.dispatchTransitionProgressEvent(interpolatedTime, !mFragment.isResumed) | ||
|
Member
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. Action point for the future: Fragment |
||
| } | ||
| } | ||
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.
You moved it out from
runscope to fix this compiler warnings, right?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.
No, I used
notDismissedWrapperssequence to count disappearing views later in the code (line 244) and I didn't want to repeat the same logic there with reversing and filtering the sequence. Was therunscope important?