fix(Android): drawing order for multiple disappearing screens#2806
fix(Android): drawing order for multiple disappearing screens#2806
Conversation
…ple disappearing screens
kkafar
left a comment
kkafar
left a comment
There was a problem hiding this comment.
This looks good!
I'll test it later in the day & we'll merge it after 4.10.0 release.
| 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 } |
There was a problem hiding this comment.
You moved it out from run scope to fix this compiler warnings, right?
There was a problem hiding this comment.
No, I used notDismissedWrappers sequence 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 the run scope important?
| ) { | ||
| super.applyTransformation(interpolatedTime, t) | ||
| // interpolated time should be the progress of the current transition | ||
| mFragment.dispatchTransitionProgressEvent(interpolatedTime, !mFragment.isResumed) |
There was a problem hiding this comment.
Action point for the future: Fragment ScreenFragment should have onTransitionProgress callback & it should be called here. This class should not dispatch the events directly.
| val disappearingCount = | ||
| notDismissedWrappers | ||
| .drop(1) | ||
| .takeWhile { it.screen.isTransparent() } | ||
| .count() + 1 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The problem is that notDismissedWrappers sequence is computed basing on screenWrappers - these represent current react view state. We don't have guarantees that this is consistent with currently HostTree state represented by stack list.
I think we need to find some time in upcoming weeks & design this algorithm properly from the ground up.
There was a problem hiding this comment.
Notation:
- uppercase letters, e.g. A, B - opaque screens,
- lowercase letters, e.g. a, b - translucent screens.
stack: Abc
screenWrappers: AD
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.
kkafar
left a comment
kkafar
left a comment
There was a problem hiding this comment.
I've also spotted problem while testing on Example app.
Screen.Recording.2025-03-28.at.19.06.15.mov
I do not know whether this is a regression or not. We must check before proceeding.
|
Yeah, this is a regression. Works fine on main. Don't know what the culprit is yet. Will look into it when I have a moment next week. |
There was a problem hiding this comment.
Okay, I think that current approach is reasonable.
When we decide to swap:
- we want to draw entering screen after exiting ones -> therefore we need to put entering screen at the end of the list,
- when we have multiple exiting screens we want to reverse the order among them, because framework draws them reversed (disappearing child with greater index in parent is drawn before one with smaller index).
Therefore reversing whole drawing operation list seems 🆗
|
I'll merge it once we manage to convince the CI to pass |
|
No idea why CI fails. It passes locally. 
|
kkafar
deleted the
@kligarski/drawing-order-for-multiple-disappearing-screens-2
branch
2 participants
Description
Fixes drawing order to account for multiple disappearing screens (e.g. formSheets).
Changes
ScreensCoordinatorLayout,ScreensAnimationto seperate packageChildDrawingOrderStrategyinterface to seperate fileChildDrawingOrderStrategywhen the transition is overReverseOrderStartingFromstrategy to replaceSwapLastTwoand use it inScreenStack'sonUpdateTest code and steps to reproduce
Currently, the transition from a
formSheettopushis not working properly (two animation mechanisms don't work correctly together) so it is difficult to verify drawing order. To temporarily disable formSheet animation, you canreturn nullat the beginning ofScreenStackFragment'sonCreateAnimatormethod. Then go toTestFormSheetexample sheet, open the sheet and then clickOpen second. New screen will be drawn over disappearing screens during the transition.Checklist