From 4b375c3ebcfab8593baafe29bffc751c7e31ca28 Mon Sep 17 00:00:00 2001 From: Jordan Demeulenaere Date: Thu, 6 Jun 2024 11:52:37 +0200 Subject: [PATCH 1/3] Fix interruption + overscroll bug This CL fixes a tricky interruption + overscroll bug: because the scene in which an element is placed can change when overscrolling, the computation of the interruption deltas for placement-related values should also be saved on the scene we are not placing the element in if the element is shared in both scenes. This avoids jumpcuts that can happen when an interruption is ongoing and that we start overscrolling, placing the element in another scene. Bug: 290930950 Test: atest ElementTest Flag: com.android.systemui.scene_container Change-Id: Id9ba18e70013ca4561ad8e35e25be5d8d69d6789 --- .../compose/animation/scene/Element.kt | 121 +++++++++++++++--- .../compose/animation/scene/ElementTest.kt | 94 +++++++++++++- 2 files changed, 193 insertions(+), 22 deletions(-) diff --git a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/Element.kt b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/Element.kt index edf8943509df..f074bdda9212 100644 --- a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/Element.kt +++ b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/Element.kt @@ -280,7 +280,7 @@ internal class ElementNode( constraints: Constraints, ): MeasureResult { val transitions = currentTransitions - val transition = elementTransition(element, transitions) + val transition = elementTransition(layoutImpl, element, transitions) // If this element is not supposed to be laid out now, either because it is not part of any // ongoing transition or the other scene of its transition is overscrolling, then lay out @@ -318,13 +318,10 @@ internal class ElementNode( val targetOffsetInScene = lookaheadScopeCoordinates.localLookaheadPositionOf(coords) // No need to place the element in this scene if we don't want to draw it anyways. - if (!shouldPlaceElement(layoutImpl, scene, element, transition)) { + if (!shouldPlaceElement(layoutImpl, scene.key, element, transition)) { sceneState.lastOffset = Offset.Unspecified sceneState.lastScale = Scale.Unspecified sceneState.lastAlpha = Element.AlphaUnspecified - - sceneState.clearValuesBeforeInterruption() - sceneState.clearInterruptionDeltas() return } @@ -353,7 +350,17 @@ internal class ElementNode( getValueBeforeInterruption = { sceneState.offsetBeforeInterruption }, setValueBeforeInterruption = { sceneState.offsetBeforeInterruption = it }, getInterruptionDelta = { sceneState.offsetInterruptionDelta }, - setInterruptionDelta = { sceneState.offsetInterruptionDelta = it }, + setInterruptionDelta = { delta -> + setPlacementInterruptionDelta( + element = element, + sceneState = sceneState, + transition = transition, + delta = delta, + setter = { sceneState, delta -> + sceneState.offsetInterruptionDelta = delta + }, + ) + }, diff = { a, b -> a - b }, add = { a, b, bProgress -> a + b * bProgress }, ) @@ -363,7 +370,7 @@ internal class ElementNode( val offset = (interruptedOffset - currentOffset).round() if ( isElementOpaque(scene, element, transition) && - interruptedAlpha(layoutImpl, transition, sceneState, alpha = 1f) == 1f + interruptedAlpha(layoutImpl, element, transition, sceneState, alpha = 1f) == 1f ) { sceneState.lastAlpha = 1f @@ -379,8 +386,8 @@ internal class ElementNode( // also need to recompute the current transition to make sure that we are using // the current transition and not a reference to an old one. See b/343138966 for // details. - val transition = elementTransition(element, currentTransitions) - if (!shouldPlaceElement(layoutImpl, scene, element, transition)) { + val transition = elementTransition(layoutImpl, element, currentTransitions) + if (!shouldPlaceElement(layoutImpl, scene.key, element, transition)) { return@placeWithLayer } @@ -394,7 +401,7 @@ internal class ElementNode( override fun ContentDrawScope.draw() { element.wasDrawnInAnyScene = true - val transition = elementTransition(element, currentTransitions) + val transition = elementTransition(layoutImpl, element, currentTransitions) val drawScale = getDrawScale(layoutImpl, scene, element, transition, sceneState) if (drawScale == Scale.Default) { drawContent() @@ -435,6 +442,7 @@ internal class ElementNode( * its scenes contains the element. */ private fun elementTransition( + layoutImpl: SceneTransitionLayoutImpl, element: Element, transitions: List, ): TransitionState.Transition? { @@ -448,7 +456,7 @@ private fun elementTransition( if (transition != previousTransition && transition != null && previousTransition != null) { // The previous transition was interrupted by another transition. - prepareInterruption(element, transition, previousTransition) + prepareInterruption(layoutImpl, element, transition, previousTransition) } else if (transition == null && previousTransition != null) { // The transition was just finished. element.sceneStates.values.forEach { @@ -461,18 +469,43 @@ private fun elementTransition( } private fun prepareInterruption( + layoutImpl: SceneTransitionLayoutImpl, element: Element, transition: TransitionState.Transition, previousTransition: TransitionState.Transition, ) { val sceneStates = element.sceneStates - sceneStates[previousTransition.fromScene]?.selfUpdateValuesBeforeInterruption() - sceneStates[previousTransition.toScene]?.selfUpdateValuesBeforeInterruption() - sceneStates[transition.fromScene]?.selfUpdateValuesBeforeInterruption() - sceneStates[transition.toScene]?.selfUpdateValuesBeforeInterruption() + fun updatedSceneState(key: SceneKey): Element.SceneState? { + return sceneStates[key]?.also { it.selfUpdateValuesBeforeInterruption() } + } + + val previousFromState = updatedSceneState(previousTransition.fromScene) + val previousToState = updatedSceneState(previousTransition.toScene) + val fromState = updatedSceneState(transition.fromScene) + val toState = updatedSceneState(transition.toScene) reconcileStates(element, previousTransition) reconcileStates(element, transition) + + // Remove the interruption values to all scenes but the scene(s) where the element will be + // placed, to make sure that interruption deltas are computed only right after this interruption + // is prepared. + fun maybeCleanPlacementValuesBeforeInterruption(sceneState: Element.SceneState) { + if (!shouldPlaceElement(layoutImpl, sceneState.scene, element, transition)) { + sceneState.offsetBeforeInterruption = Offset.Unspecified + sceneState.alphaBeforeInterruption = Element.AlphaUnspecified + sceneState.scaleBeforeInterruption = Scale.Unspecified + + sceneState.offsetInterruptionDelta = Offset.Zero + sceneState.alphaInterruptionDelta = 0f + sceneState.scaleInterruptionDelta = Scale.Zero + } + } + + previousFromState?.let { maybeCleanPlacementValuesBeforeInterruption(it) } + previousToState?.let { maybeCleanPlacementValuesBeforeInterruption(it) } + fromState?.let { maybeCleanPlacementValuesBeforeInterruption(it) } + toState?.let { maybeCleanPlacementValuesBeforeInterruption(it) } } /** @@ -579,9 +612,38 @@ private inline fun computeInterruptedValue( } } +/** + * Set the interruption delta of a *placement/drawing*-related value (offset, alpha, scale). This + * ensures that the delta is also set on the other scene in the transition for shared elements, so + * that there is no jump cut if the scene where the element is placed has changed. + */ +private inline fun setPlacementInterruptionDelta( + element: Element, + sceneState: Element.SceneState, + transition: TransitionState.Transition?, + delta: T, + setter: (Element.SceneState, T) -> Unit, +) { + // Set the interruption delta on the current scene. + setter(sceneState, delta) + + if (transition == null) { + return + } + + // If the element is shared, also set the delta on the other scene so that it is used by that + // scene if we start overscrolling it and change the scene where the element is placed. + val otherScene = + if (sceneState.scene == transition.fromScene) transition.toScene else transition.fromScene + val otherSceneState = element.sceneStates[otherScene] ?: return + if (isSharedElementEnabled(element.key, transition)) { + setter(otherSceneState, delta) + } +} + private fun shouldPlaceElement( layoutImpl: SceneTransitionLayoutImpl, - scene: Scene, + scene: SceneKey, element: Element, transition: TransitionState.Transition?, ): Boolean { @@ -592,7 +654,7 @@ private fun shouldPlaceElement( // Don't place the element in this scene if this scene is not part of the current element // transition. - if (scene.key != transition.fromScene && scene.key != transition.toScene) { + if (scene != transition.fromScene && scene != transition.toScene) { return false } @@ -610,7 +672,7 @@ private fun shouldPlaceElement( return shouldPlaceOrComposeSharedElement( layoutImpl, - scene.key, + scene, element.key, transition, ) @@ -740,13 +802,14 @@ private fun elementAlpha( element.sceneStates.forEach { it.value.alphaBeforeInterruption = 0f } } - val interruptedAlpha = interruptedAlpha(layoutImpl, transition, sceneState, alpha) + val interruptedAlpha = interruptedAlpha(layoutImpl, element, transition, sceneState, alpha) sceneState.lastAlpha = interruptedAlpha return interruptedAlpha } private fun interruptedAlpha( layoutImpl: SceneTransitionLayoutImpl, + element: Element, transition: TransitionState.Transition?, sceneState: Element.SceneState, alpha: Float, @@ -760,7 +823,15 @@ private fun interruptedAlpha( getValueBeforeInterruption = { sceneState.alphaBeforeInterruption }, setValueBeforeInterruption = { sceneState.alphaBeforeInterruption = it }, getInterruptionDelta = { sceneState.alphaInterruptionDelta }, - setInterruptionDelta = { sceneState.alphaInterruptionDelta = it }, + setInterruptionDelta = { delta -> + setPlacementInterruptionDelta( + element = element, + sceneState = sceneState, + transition = transition, + delta = delta, + setter = { sceneState, delta -> sceneState.alphaInterruptionDelta = delta }, + ) + }, diff = { a, b -> a - b }, add = { a, b, bProgress -> a + b * bProgress }, ) @@ -867,7 +938,15 @@ private fun ContentDrawScope.getDrawScale( getValueBeforeInterruption = { sceneState.scaleBeforeInterruption }, setValueBeforeInterruption = { sceneState.scaleBeforeInterruption = it }, getInterruptionDelta = { sceneState.scaleInterruptionDelta }, - setInterruptionDelta = { sceneState.scaleInterruptionDelta = it }, + setInterruptionDelta = { delta -> + setPlacementInterruptionDelta( + element = element, + sceneState = sceneState, + transition = transition, + delta = delta, + setter = { sceneState, delta -> sceneState.scaleInterruptionDelta = delta }, + ) + }, diff = { a, b -> Scale( scaleX = a.scaleX - b.scaleX, diff --git a/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ElementTest.kt b/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ElementTest.kt index 47c9b9cbfd10..7827d85bcfb0 100644 --- a/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ElementTest.kt +++ b/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ElementTest.kt @@ -47,7 +47,6 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.geometry.Offset import androidx.compose.ui.layout.approachLayout import androidx.compose.ui.platform.LocalViewConfiguration -import androidx.compose.ui.platform.testTag import androidx.compose.ui.test.assertIsDisplayed import androidx.compose.ui.test.assertIsNotDisplayed import androidx.compose.ui.test.assertPositionInRootIsEqualTo @@ -1632,4 +1631,97 @@ class ElementTest { rule.onNode(hasText(fooInA)).assertIsDisplayed() rule.onNode(hasText(fooInB)).assertDoesNotExist() } + + @Test + fun interruptionThenOverscroll() = runTest { + val state = + rule.runOnUiThread { + MutableSceneTransitionLayoutStateImpl( + SceneA, + transitions { + overscroll(SceneB, Orientation.Vertical) { + translate(TestElements.Foo, y = 15.dp) + } + } + ) + } + + @Composable + fun SceneScope.SceneWithFoo(offset: DpOffset, modifier: Modifier = Modifier) { + Box(modifier.fillMaxSize()) { + Box(Modifier.offset(offset.x, offset.y).element(TestElements.Foo).size(100.dp)) + } + } + + rule.setContent { + SceneTransitionLayout(state, Modifier.size(200.dp)) { + scene(SceneA) { SceneWithFoo(offset = DpOffset.Zero) } + scene(SceneB) { SceneWithFoo(offset = DpOffset(x = 40.dp, y = 0.dp)) } + scene(SceneC) { SceneWithFoo(offset = DpOffset(x = 40.dp, y = 40.dp)) } + } + } + + // Start A => B at 75%. + rule.runOnUiThread { + state.startTransition( + transition( + from = SceneA, + to = SceneB, + progress = { 0.75f }, + onFinish = neverFinish(), + ) + ) + } + + // Foo should be at offset (30dp, 0dp) and placed in scene B. + rule.onNode(isElement(TestElements.Foo, SceneA)).assertIsNotDisplayed() + rule.onNode(isElement(TestElements.Foo, SceneB)).assertPositionInRootIsEqualTo(30.dp, 0.dp) + rule.onNode(isElement(TestElements.Foo, SceneC)).assertIsNotDisplayed() + + // Interrupt A => B with B => C at 0%. + var progress by mutableStateOf(0f) + var interruptionProgress by mutableStateOf(1f) + rule.runOnUiThread { + state.startTransition( + transition( + from = SceneB, + to = SceneC, + progress = { progress }, + interruptionProgress = { interruptionProgress }, + orientation = Orientation.Vertical, + onFinish = neverFinish(), + ) + ) + } + + // Because interruption progress is at 100M, Foo should still be at offset (30dp, 0dp) but + // placed in scene C. + rule.onNode(isElement(TestElements.Foo, SceneA)).assertIsNotDisplayed() + rule.onNode(isElement(TestElements.Foo, SceneB)).assertIsNotDisplayed() + rule.onNode(isElement(TestElements.Foo, SceneC)).assertPositionInRootIsEqualTo(30.dp, 0.dp) + + // Overscroll B => C on scene B at -100%. Because overscrolling on B => C translates Foo + // vertically by -15dp and that interruptionProgress is still 100%, we should now be at + // (30dp, -15dp) + progress = -1f + rule.onNode(isElement(TestElements.Foo, SceneA)).assertIsNotDisplayed() + rule + .onNode(isElement(TestElements.Foo, SceneB)) + .assertPositionInRootIsEqualTo(30.dp, -15.dp) + rule.onNode(isElement(TestElements.Foo, SceneC)).assertIsNotDisplayed() + + // Finish the interruption, we should now be at (40dp, -15dp), still on scene B. + interruptionProgress = 0f + rule.onNode(isElement(TestElements.Foo, SceneA)).assertIsNotDisplayed() + rule + .onNode(isElement(TestElements.Foo, SceneB)) + .assertPositionInRootIsEqualTo(40.dp, -15.dp) + rule.onNode(isElement(TestElements.Foo, SceneC)).assertIsNotDisplayed() + + // Finish the transition, we should be at the final position (40dp, 40dp) on scene C. + progress = 1f + rule.onNode(isElement(TestElements.Foo, SceneA)).assertIsNotDisplayed() + rule.onNode(isElement(TestElements.Foo, SceneB)).assertIsNotDisplayed() + rule.onNode(isElement(TestElements.Foo, SceneC)).assertPositionInRootIsEqualTo(40.dp, 40.dp) + } } From 906dad1ce10e03030d98600a22cd554b8bf7cc61 Mon Sep 17 00:00:00 2001 From: Jordan Demeulenaere Date: Thu, 6 Jun 2024 15:33:09 +0200 Subject: [PATCH 2/3] Move TransitionKey to Transition This CL simply moves the TransitionKey associated to a transition inside Transition itself, rather than binding it in STLState.startTransition. Bug: 290930950 Test: atest PlatformComposeSceneTransitionLayoutTests Flag: com.android.systemui.scene_container Change-Id: I371c8b93ab6c216e7f89b9c06e41361287cf2094 --- .../compose/animation/scene/AnimateToScene.kt | 8 ++- .../animation/scene/DraggableHandler.kt | 4 +- .../scene/SceneTransitionLayoutState.kt | 19 ++++--- .../scene/transition/link/LinkedTransition.kt | 2 + .../compose/animation/scene/ElementTest.kt | 21 +++---- .../scene/InterruptionHandlerTest.kt | 6 +- .../scene/ObservableTransitionStateTest.kt | 2 +- .../scene/SceneTransitionLayoutStateTest.kt | 57 ++++++++----------- 8 files changed, 58 insertions(+), 61 deletions(-) diff --git a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/AnimateToScene.kt b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/AnimateToScene.kt index 48a348b9d1c5..c2dd80375d5a 100644 --- a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/AnimateToScene.kt +++ b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/AnimateToScene.kt @@ -109,8 +109,7 @@ internal fun CoroutineScope.animateToScene( layoutState.transitions.interruptionHandler.onInterruption( transitionState, target, - ) - ?: DefaultInterruptionHandler.onInterruption(transitionState, target) + ) ?: DefaultInterruptionHandler.onInterruption(transitionState, target) val animateFrom = interruptionResult.animateFrom if ( @@ -159,6 +158,7 @@ private fun CoroutineScope.animate( val transition = if (reversed) { OneOffTransition( + key = transitionKey, fromScene = targetScene, toScene = fromScene, currentScene = targetScene, @@ -167,6 +167,7 @@ private fun CoroutineScope.animate( ) } else { OneOffTransition( + key = transitionKey, fromScene = fromScene, toScene = targetScene, currentScene = targetScene, @@ -178,7 +179,7 @@ private fun CoroutineScope.animate( // Change the current layout state to start this new transition. This will compute the // TransformationSpec associated to this transition, which we need to initialize the Animatable // that will actually animate it. - layoutState.startTransition(transition, transitionKey, chain) + layoutState.startTransition(transition, chain) // The transition now contains the transformation spec that we should use to instantiate the // Animatable. @@ -207,6 +208,7 @@ private fun CoroutineScope.animate( } private class OneOffTransition( + override val key: TransitionKey?, fromScene: SceneKey, toScene: SceneKey, override val currentScene: SceneKey, diff --git a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/DraggableHandler.kt b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/DraggableHandler.kt index e9633c2f6603..60d78feec8c2 100644 --- a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/DraggableHandler.kt +++ b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/DraggableHandler.kt @@ -257,7 +257,7 @@ private class DragControllerImpl( fun updateTransition(newTransition: SwipeTransition, force: Boolean = false) { if (isDrivingTransition || force) { - layoutState.startTransition(newTransition, newTransition.key) + layoutState.startTransition(newTransition) } swipeTransition = newTransition @@ -555,7 +555,7 @@ private class SwipeTransition( val layoutImpl: SceneTransitionLayoutImpl, val layoutState: BaseSceneTransitionLayoutState, val coroutineScope: CoroutineScope, - val key: TransitionKey?, + override val key: TransitionKey?, val _fromScene: Scene, val _toScene: Scene, val userActionDistanceScope: UserActionDistanceScope, diff --git a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/SceneTransitionLayoutState.kt b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/SceneTransitionLayoutState.kt index 44affd968513..6a178c8b0c25 100644 --- a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/SceneTransitionLayoutState.kt +++ b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/SceneTransitionLayoutState.kt @@ -225,6 +225,12 @@ sealed interface TransitionState { /** The scene this transition is going to. Can't be the same as fromScene */ val toScene: SceneKey, ) : TransitionState { + /** + * The key of this transition. This should usually be null, but it can be specified to use a + * specific set of transformations associated to this transition. + */ + open val key: TransitionKey? = null + /** * The progress of the transition. This is usually in the `[0; 1]` range, but it can also be * less than `0` or greater than `1` when using transitions with a spring AnimationSpec or @@ -455,11 +461,7 @@ internal abstract class BaseSceneTransitionLayoutState( * * Important: you *must* call [finishTransition] once the transition is finished. */ - internal fun startTransition( - transition: TransitionState.Transition, - transitionKey: TransitionKey? = null, - chain: Boolean = true, - ) { + internal fun startTransition(transition: TransitionState.Transition, chain: Boolean = true) { checkThread() // Compute the [TransformationSpec] when the transition starts. @@ -469,7 +471,9 @@ internal abstract class BaseSceneTransitionLayoutState( // Update the transition specs. transition.transformationSpec = - transitions.transitionSpec(fromScene, toScene, key = transitionKey).transformationSpec() + transitions + .transitionSpec(fromScene, toScene, key = transition.key) + .transformationSpec() if (orientation != null) { transition.updateOverscrollSpecs( fromSpec = transitions.overscrollSpec(fromScene, orientation), @@ -568,9 +572,10 @@ internal abstract class BaseSceneTransitionLayoutState( originalTransition = transitionState, fromScene = targetCurrentScene, toScene = matchingLink.targetTo, + key = matchingLink.targetTransitionKey, ) - stateLink.target.startTransition(linkedTransition, matchingLink.targetTransitionKey) + stateLink.target.startTransition(linkedTransition) activeTransitionLinks[stateLink] = linkedTransition } } diff --git a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/transition/link/LinkedTransition.kt b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/transition/link/LinkedTransition.kt index 79f126d24561..ed9888560f05 100644 --- a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/transition/link/LinkedTransition.kt +++ b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/transition/link/LinkedTransition.kt @@ -17,6 +17,7 @@ package com.android.compose.animation.scene.transition.link import com.android.compose.animation.scene.SceneKey +import com.android.compose.animation.scene.TransitionKey import com.android.compose.animation.scene.TransitionState import kotlinx.coroutines.Job @@ -25,6 +26,7 @@ internal class LinkedTransition( private val originalTransition: TransitionState.Transition, fromScene: SceneKey, toScene: SceneKey, + override val key: TransitionKey? = null, ) : TransitionState.Transition(fromScene, toScene) { override val currentScene: SceneKey diff --git a/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ElementTest.kt b/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ElementTest.kt index 7827d85bcfb0..41cacb4c71fc 100644 --- a/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ElementTest.kt +++ b/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ElementTest.kt @@ -638,10 +638,7 @@ class ElementTest { // Change the current transition. rule.runOnUiThread { - state.startTransition( - transition(from = SceneA, to = SceneB, progress = { 0.5f }), - transitionKey = null, - ) + state.startTransition(transition(from = SceneA, to = SceneB, progress = { 0.5f })) } // The size of Foo should still be 20dp given that the new state was not composed yet. @@ -1170,7 +1167,7 @@ class ElementTest { val offsetInAToB = lerp(offsetInA, offsetInB, aToBProgress) val sizeInAToB = lerp(sizeInA, sizeInB, aToBProgress) val valueInAToB = lerp(valueInA, valueInB, aToBProgress) - rule.runOnUiThread { state.startTransition(aToB, transitionKey = null) } + rule.runOnUiThread { state.startTransition(aToB) } rule .onNode(isElement(TestElements.Foo, SceneB)) .assertSizeIsEqualTo(sizeInAToB) @@ -1190,7 +1187,7 @@ class ElementTest { progress = { bToCProgress }, interruptionProgress = { interruptionProgress }, ) - rule.runOnUiThread { state.startTransition(bToC, transitionKey = null) } + rule.runOnUiThread { state.startTransition(bToC) } // The interruption deltas, which will be multiplied by the interruption progress then added // to the current transition offset and size. @@ -1332,9 +1329,9 @@ class ElementTest { interruptionProgress = { bToCInterruptionProgress }, onFinish = neverFinish(), ) - rule.runOnUiThread { state.startTransition(aToB, transitionKey = null) } + rule.runOnUiThread { state.startTransition(aToB) } rule.waitForIdle() - rule.runOnUiThread { state.startTransition(bToC, transitionKey = null) } + rule.runOnUiThread { state.startTransition(bToC) } // Foo is placed in both B and C given that the shared transition is disabled. In B, its // offset is impacted by the interruption but in C it is not. @@ -1370,7 +1367,7 @@ class ElementTest { progress = { 0.7f }, interruptionProgress = { 1f }, ) - rule.runOnUiThread { state.startTransition(bToA, transitionKey = null) } + rule.runOnUiThread { state.startTransition(bToA) } // Foo should have the position it had in B right before the interruption. rule @@ -1394,8 +1391,7 @@ class ElementTest { to = SceneB, progress = { -1f }, orientation = Orientation.Horizontal - ), - transitionKey = null, + ) ) } } @@ -1512,8 +1508,7 @@ class ElementTest { to = SceneB, progress = { 0.6f }, interruptionProgress = { interruptionProgress }, - ), - transitionKey = null + ) ) } rule.waitForIdle() diff --git a/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/InterruptionHandlerTest.kt b/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/InterruptionHandlerTest.kt index 85d4165b4bf6..09d1a827d0c7 100644 --- a/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/InterruptionHandlerTest.kt +++ b/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/InterruptionHandlerTest.kt @@ -40,7 +40,7 @@ class InterruptionHandlerTest { val state = MutableSceneTransitionLayoutState( SceneA, - transitions { /* default interruption handler */}, + transitions { /* default interruption handler */ }, ) state.setTargetScene(SceneB, coroutineScope = this) @@ -160,7 +160,7 @@ class InterruptionHandlerTest { progressVelocity = { progressVelocity }, onFinish = { launch {} }, ) - state.startTransition(aToB, transitionKey = null) + state.startTransition(aToB) // Animate back to A. The previous transition is reversed, i.e. it has the same (from, to) // pair, and its velocity is used when animating the progress back to 0. @@ -186,7 +186,7 @@ class InterruptionHandlerTest { progressVelocity = { progressVelocity }, onFinish = { launch {} }, ) - state.startTransition(aToB, transitionKey = null) + state.startTransition(aToB) // Animate to B. The previous transition is reversed, i.e. it has the same (from, to) pair, // and its velocity is used when animating the progress to 1. diff --git a/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ObservableTransitionStateTest.kt b/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ObservableTransitionStateTest.kt index 2a75e13066df..55431354b693 100644 --- a/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ObservableTransitionStateTest.kt +++ b/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ObservableTransitionStateTest.kt @@ -135,7 +135,7 @@ class ObservableTransitionStateTest { var transitionCurrentScene by mutableStateOf(SceneA) val transition = transition(from = SceneA, to = SceneB, current = { transitionCurrentScene }) - state.startTransition(transition, transitionKey = null) + state.startTransition(transition) assertThat(currentScene.value).isEqualTo(SceneA) // Change the transition current scene. diff --git a/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/SceneTransitionLayoutStateTest.kt b/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/SceneTransitionLayoutStateTest.kt index d2c8bd6928ee..de6f1cc518f4 100644 --- a/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/SceneTransitionLayoutStateTest.kt +++ b/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/SceneTransitionLayoutStateTest.kt @@ -57,7 +57,7 @@ class SceneTransitionLayoutStateTest { @Test fun isTransitioningTo_transition() { val state = MutableSceneTransitionLayoutStateImpl(SceneA, SceneTransitions.Empty) - state.startTransition(transition(from = SceneA, to = SceneB), transitionKey = null) + state.startTransition(transition(from = SceneA, to = SceneB)) assertThat(state.isTransitioning()).isTrue() assertThat(state.isTransitioning(from = SceneA)).isTrue() @@ -175,7 +175,7 @@ class SceneTransitionLayoutStateTest { val childTransition = transition(SceneA, SceneB) - childState.startTransition(childTransition, null) + childState.startTransition(childTransition) assertThat(childState.isTransitioning(SceneA, SceneB)).isTrue() assertThat(parentState.isTransitioning(SceneC, SceneD)).isTrue() @@ -211,7 +211,7 @@ class SceneTransitionLayoutStateTest { val childTransition = transition(SceneA, SceneB) - childState.startTransition(childTransition, null) + childState.startTransition(childTransition) assertThat(childState.isTransitioning(SceneA, SceneB)).isTrue() assertThat(parentState.isTransitioning(SceneC, SceneD)).isTrue() assertThat(parentParentState.isTransitioning(SceneB, SceneC)).isTrue() @@ -229,7 +229,7 @@ class SceneTransitionLayoutStateTest { var progress = 0f val childTransition = transition(SceneA, SceneB, progress = { progress }) - childState.startTransition(childTransition, null) + childState.startTransition(childTransition) assertThat(parentState.currentTransition?.progress).isEqualTo(0f) progress = .5f @@ -242,7 +242,7 @@ class SceneTransitionLayoutStateTest { val childTransition = transition(SceneB, SceneA) - childState.startTransition(childTransition, null) + childState.startTransition(childTransition) assertThat(childState.isTransitioning(SceneB, SceneA)).isTrue() assertThat(parentState.transitionState).isEqualTo(TransitionState.Idle(SceneC)) @@ -256,7 +256,7 @@ class SceneTransitionLayoutStateTest { val (parentState, childState) = setupLinkedStates() val childTransition = transition(SceneA, SceneB) - childState.startTransition(childTransition, null) + childState.startTransition(childTransition) childState.finishTransition(childTransition, SceneA) assertThat(childState.transitionState).isEqualTo(TransitionState.Idle(SceneA)) @@ -268,7 +268,7 @@ class SceneTransitionLayoutStateTest { val (parentState, childState) = setupLinkedStates() val childTransition = transition(SceneA, SceneB) - childState.startTransition(childTransition, null) + childState.startTransition(childTransition) childState.finishTransition(childTransition, SceneD) assertThat(childState.transitionState).isEqualTo(TransitionState.Idle(SceneD)) @@ -283,16 +283,16 @@ class SceneTransitionLayoutStateTest { transition( SceneA, SceneB, - onFinish = { launch { /* Do nothing. */} }, + onFinish = { launch { /* Do nothing. */ } }, ) val parentTransition = transition( SceneC, SceneA, - onFinish = { launch { /* Do nothing. */} }, + onFinish = { launch { /* Do nothing. */ } }, ) - childState.startTransition(childTransition, null) - parentState.startTransition(parentTransition, null) + childState.startTransition(childTransition) + parentState.startTransition(parentTransition) childState.finishTransition(childTransition, SceneB) assertThat(childState.transitionState).isEqualTo(TransitionState.Idle(SceneB)) @@ -341,10 +341,7 @@ class SceneTransitionLayoutStateTest { @Test fun snapToIdleIfClose_snapToStart() = runMonotonicClockTest { val state = MutableSceneTransitionLayoutStateImpl(SceneA, SceneTransitions.Empty) - state.startTransition( - transition(from = SceneA, to = SceneB, progress = { 0.2f }), - transitionKey = null - ) + state.startTransition(transition(from = SceneA, to = SceneB, progress = { 0.2f })) assertThat(state.isTransitioning()).isTrue() // Ignore the request if the progress is not close to 0 or 1, using the threshold. @@ -360,10 +357,7 @@ class SceneTransitionLayoutStateTest { @Test fun snapToIdleIfClose_snapToEnd() = runMonotonicClockTest { val state = MutableSceneTransitionLayoutStateImpl(SceneA, SceneTransitions.Empty) - state.startTransition( - transition(from = SceneA, to = SceneB, progress = { 0.8f }), - transitionKey = null - ) + state.startTransition(transition(from = SceneA, to = SceneB, progress = { 0.8f })) assertThat(state.isTransitioning()).isTrue() // Ignore the request if the progress is not close to 0 or 1, using the threshold. @@ -385,13 +379,13 @@ class SceneTransitionLayoutStateTest { from = SceneA, to = SceneB, progress = { 0.5f }, - onFinish = { launch { /* do nothing */} }, + onFinish = { launch { /* do nothing */ } }, ) - state.startTransition(aToB, transitionKey = null) + state.startTransition(aToB) assertThat(state.currentTransitions).containsExactly(aToB).inOrder() val bToC = transition(from = SceneB, to = SceneC, progress = { 0.8f }) - state.startTransition(bToC, transitionKey = null) + state.startTransition(bToC) assertThat(state.currentTransitions).containsExactly(aToB, bToC).inOrder() // Ignore the request if the progress is not close to 0 or 1, using the threshold. @@ -409,7 +403,7 @@ class SceneTransitionLayoutStateTest { val (parentState, childState) = setupLinkedStates(SceneC, SceneA, null, null, null, SceneD) val childTransition = transition(SceneA, SceneB) - childState.startTransition(childTransition, null) + childState.startTransition(childTransition) assertThat(childState.isTransitioning(SceneA, SceneB)).isTrue() assertThat(parentState.isTransitioning(SceneC, SceneD)).isTrue() @@ -425,7 +419,7 @@ class SceneTransitionLayoutStateTest { val childTransition = transition(SceneA, SceneB) - childState.startTransition(childTransition, null) + childState.startTransition(childTransition) assertThat(childState.isTransitioning(SceneA, SceneB)).isTrue() assertThat(parentState.isTransitioning(SceneC, SceneD)).isTrue() @@ -440,7 +434,7 @@ class SceneTransitionLayoutStateTest { setupLinkedStates(SceneC, SceneA, SceneB, null, SceneC, SceneD) val childTransition = transition(SceneA, SceneB) - childState.startTransition(childTransition, null) + childState.startTransition(childTransition) assertThat(childState.isTransitioning(SceneA, SceneB)).isTrue() assertThat(parentState.isTransitioning(SceneC, SceneD)).isFalse() } @@ -460,8 +454,7 @@ class SceneTransitionLayoutStateTest { to = SceneB, progress = progress, orientation = Orientation.Vertical, - ), - transitionKey = null + ) ) assertThat(state.isTransitioning()).isTrue() return state @@ -583,19 +576,19 @@ class SceneTransitionLayoutStateTest { assertThat(state.currentTransitions).isEmpty() // A => B. - state.startTransition(aToB, transitionKey = null) + state.startTransition(aToB) assertThat(finishingTransitions).isEmpty() assertThat(state.finishedTransitions).isEmpty() assertThat(state.currentTransitions).containsExactly(aToB).inOrder() // B => C. This should automatically call finish() on aToB. - state.startTransition(bToC, transitionKey = null) + state.startTransition(bToC) assertThat(finishingTransitions).containsExactly(aToB) assertThat(state.finishedTransitions).isEmpty() assertThat(state.currentTransitions).containsExactly(aToB, bToC).inOrder() // C => A. This should automatically call finish() on bToC. - state.startTransition(cToA, transitionKey = null) + state.startTransition(cToA) assertThat(finishingTransitions).containsExactly(aToB, bToC) assertThat(state.finishedTransitions).isEmpty() assertThat(state.currentTransitions).containsExactly(aToB, bToC, cToA).inOrder() @@ -617,8 +610,8 @@ class SceneTransitionLayoutStateTest { val state = MutableSceneTransitionLayoutStateImpl(SceneA, EmptyTestTransitions) fun startTransition() { - val transition = transition(SceneA, SceneB, onFinish = { launch { /* do nothing */} }) - state.startTransition(transition, transitionKey = null) + val transition = transition(SceneA, SceneB, onFinish = { launch { /* do nothing */ } }) + state.startTransition(transition) } var hasLoggedWtf = false From e3c540ad3e60f85ad5969fa0d0767ef29c3d35a2 Mon Sep 17 00:00:00 2001 From: Jordan Demeulenaere Date: Fri, 7 Jun 2024 15:47:01 +0200 Subject: [PATCH 3/3] Prevent NPE when placing fading elements This CL fixes an NPE that can happen since http://ag/27685078: the placeWithLayer block can still run even if an element is removed from composition. Bug: 290930950 Test: Manual. Unfortunately it's impossible to add tests for this because the timings solely depend on the invalidation of the graphics layer by the Compose runtime. I have not observed the NPE with this change, but I also didn't have manual reproduction steps. Flag: com.android.systemui.scene_container Change-Id: Ic8bd91b3d6fd3f8b3d697c72a42bb5f278f5d207 --- .../com/android/compose/animation/scene/Element.kt | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/Element.kt b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/Element.kt index f074bdda9212..980982a30926 100644 --- a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/Element.kt +++ b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/Element.kt @@ -381,11 +381,15 @@ internal class ElementNode( } else { placeable.placeWithLayer(offset) { // This layer might still run on its own (outside of the placement phase) even - // if this element is not placed anymore, so we need to double check again here - // before calling [elementAlpha] (which will update [SceneState.lastAlpha]). We - // also need to recompute the current transition to make sure that we are using - // the current transition and not a reference to an old one. See b/343138966 for - // details. + // if this element is not placed or composed anymore, so we need to double check + // again here before calling [elementAlpha] (which will update + // [SceneState.lastAlpha]). We also need to recompute the current transition to + // make sure that we are using the current transition and not a reference to an + // old one. See b/343138966 for details. + if (_element == null) { + return@placeWithLayer + } + val transition = elementTransition(layoutImpl, element, currentTransitions) if (!shouldPlaceElement(layoutImpl, scene.key, element, transition)) { return@placeWithLayer