Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit b93b830a authored by Jordan Demeulenaere's avatar Jordan Demeulenaere
Browse files

Enforce that STLState is mutated on the right thread 

SceneTransitionLayoutState currently does not do any synchronization
to support mutation from different threads. Usually this is not an issue
because most state changes naturally happen on the main thread, but I
noticed that it was causing some flakiness issues in tests given that
those run in a thread different than the main thread.

This CL enforces that all mutations of a STLState are done in the same
thread in which the state was created. If that STLState is used by a
SceneTransitionLayout, we also check that the thread is the same (which
is usually the main thread).

Bug: 290930950
Test: atest PlatformComposeSceneTransitionLayoutTests
Flag: com.android.systemui.scene_container
Change-Id: Ia928cc43305f4c226358dabd74fafee9d340fcc5
parent d8ce27e6

packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/SceneTransitionLayoutImpl.kt

+4 −0
Original line number Diff line number Diff line
@@ -126,6 +126,10 @@ internal class SceneTransitionLayoutImpl( 
                orientation = Orientation.Vertical,

                coroutineScope = coroutineScope,

            )



        // Make sure that the state is created on the same thread (most probably the main thread)

        // than this STLImpl.

        state.checkThread()

    }



    internal fun draggableHandler(orientation: Orientation): DraggableHandlerImpl =

packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/SceneTransitionLayoutState.kt

+24 −0
Original line number Diff line number Diff line
@@ -376,6 +376,8 @@ internal abstract class BaseSceneTransitionLayoutState( 
    // TODO(b/290930950): Remove this flag.

    internal var enableInterruptions: Boolean,

) : SceneTransitionLayoutState {

    private val creationThread: Thread = Thread.currentThread()



    /**

     * The current [TransitionState]. This list will either be:

     * 1. A list with a single [TransitionState.Idle] element, when we are idle.
@@ -420,6 +422,20 @@ internal abstract class BaseSceneTransitionLayoutState( 
     */

    internal abstract fun CoroutineScope.onChangeScene(scene: SceneKey)



    internal fun checkThread() {

        val current = Thread.currentThread()

        if (current !== creationThread) {

            error(

                """

                    Only the original thread that created a SceneTransitionLayoutState can mutate it

                      Expected: ${creationThread.name}

                      Current: ${current.name}

                """

                    .trimIndent()

            )

        }

    }



    override fun isTransitioning(from: SceneKey?, to: SceneKey?): Boolean {

        val transition = currentTransition ?: return false

        return transition.isTransitioning(from, to)
@@ -444,6 +460,8 @@ internal abstract class BaseSceneTransitionLayoutState( 
        transitionKey: TransitionKey?,

        chain: Boolean = true,

    ) {

        checkThread()



        // Compute the [TransformationSpec] when the transition starts.

        val fromScene = transition.fromScene

        val toScene = transition.toScene
@@ -564,6 +582,8 @@ internal abstract class BaseSceneTransitionLayoutState( 
     * nothing if [transition] was interrupted since it was started.

     */

    internal fun finishTransition(transition: TransitionState.Transition, idleScene: SceneKey) {

        checkThread()



        val existingIdleScene = finishedTransitions[transition]

        if (existingIdleScene != null) {

            // This transition was already finished.
@@ -617,6 +637,8 @@ internal abstract class BaseSceneTransitionLayoutState( 
    }



    fun snapToScene(scene: SceneKey) {

        checkThread()



        // Force finish all transitions.

        while (currentTransitions.isNotEmpty()) {

            val transition = transitionStates[0] as TransitionState.Transition
@@ -755,6 +777,8 @@ internal class MutableSceneTransitionLayoutStateImpl( 
        coroutineScope: CoroutineScope,

        transitionKey: TransitionKey?,

    ): TransitionState.Transition? {

        checkThread()



        return coroutineScope.animateToScene(

            layoutState = this@MutableSceneTransitionLayoutStateImpl,

            target = targetScene,

packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ElementTest.kt

+75 −59
Original line number Diff line number Diff line
@@ -610,6 +610,7 @@ class ElementTest { 
    // TODO(b/341072461): Remove this test.

    fun layoutGetsCurrentTransitionStateFromComposition() {

        val state =

            rule.runOnUiThread {

                MutableSceneTransitionLayoutStateImpl(

                    SceneA,

                    transitions {
@@ -618,6 +619,7 @@ class ElementTest { 
                        }

                    }

                )

            }



        rule.setContent {

            SceneTransitionLayout(state) {
@@ -630,10 +632,12 @@ class ElementTest { 
        rule.mainClock.autoAdvance = false



        // Change the current transition.

        rule.runOnUiThread {

            state.startTransition(

                transition(from = SceneA, to = SceneB, progress = { 0.5f }),

                transitionKey = null,

            )

        }



        // The size of Foo should still be 20dp given that the new state was not composed yet.

        rule.onNode(isElement(TestElements.Foo)).assertSizeIsEqualTo(20.dp, 20.dp)
@@ -652,11 +656,13 @@ class ElementTest { 
        var touchSlop = 0f



        val state =

            rule.runOnUiThread {

                MutableSceneTransitionLayoutState(

                    initialScene = SceneA,

                    transitions = transitions(sceneTransitions),

                )

                    as MutableSceneTransitionLayoutStateImpl

            }



        rule.setContent {

            touchSlop = LocalViewConfiguration.current.touchSlop
@@ -762,6 +768,7 @@ class ElementTest { 
        val layoutHeight = 400.dp



        val state =

            rule.runOnUiThread {

                MutableSceneTransitionLayoutState(

                    initialScene = SceneB,

                    transitions =
@@ -772,6 +779,7 @@ class ElementTest { 
                        }

                )

                    as MutableSceneTransitionLayoutStateImpl

            }



        rule.setContent {

            touchSlop = LocalViewConfiguration.current.touchSlop
@@ -938,10 +946,12 @@ class ElementTest { 
        val duration = 4 * 16



        val state =

            rule.runOnUiThread {

                MutableSceneTransitionLayoutState(

                    SceneA,

                    transitions {

                    // Foo is at the top left corner of scene A. We make it disappear during A => B

                        // Foo is at the top left corner of scene A. We make it disappear during A

                        // => B

                        // to the right edge so it translates to the right.

                        from(SceneA, to = SceneB) {

                            spec = tween(duration, easing = LinearEasing)
@@ -952,7 +962,8 @@ class ElementTest { 
                            )

                        }



                    // Bar is at the top right corner of scene C. We make it appear during B => C

                        // Bar is at the top right corner of scene C. We make it appear during B =>

                        // C

                        // from the left edge so it translates to the right at same time as Foo.

                        from(SceneB, to = SceneC) {

                            spec = tween(duration, easing = LinearEasing)
@@ -964,6 +975,7 @@ class ElementTest { 
                        }

                    }

                )

            }



        val layoutSize = 150.dp

        val elemSize = 50.dp
@@ -1059,6 +1071,7 @@ class ElementTest { 
        val duration = 4 * 16



        val state =

            rule.runOnUiThread {

                MutableSceneTransitionLayoutStateImpl(

                    SceneA,

                    transitions {
@@ -1067,6 +1080,7 @@ class ElementTest { 
                    },

                    enableInterruptions = false,

                )

            }



        val layoutSize = DpSize(200.dp, 100.dp)

        val fooSize = DpSize(20.dp, 10.dp)
@@ -1177,8 +1191,10 @@ class ElementTest { 
            .assertPositionInRootIsEqualTo(offsetInC.x, offsetInC.y)



        // Manually finish the transition.

        rule.runOnUiThread {

            state.finishTransition(aToB, SceneB)

            state.finishTransition(bToC, SceneC)

        }

        rule.waitForIdle()

        assertThat(state.transitionState).isIdle()

packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/SwipeToSceneTest.kt

+6 −4
Original line number Diff line number Diff line
@@ -70,7 +70,9 @@ class SwipeToSceneTest { 
    private fun layoutState(

        initialScene: SceneKey = SceneA,

        transitions: SceneTransitions = EmptyTestTransitions,

    ) = MutableSceneTransitionLayoutState(initialScene, transitions)

    ): MutableSceneTransitionLayoutState {

        return rule.runOnUiThread { MutableSceneTransitionLayoutState(initialScene, transitions) }

    }



    /** The content under test. */

    @Composable
@@ -455,7 +457,7 @@ class SwipeToSceneTest { 


    @Test

    fun swipeEnabledLater() {

        val layoutState = MutableSceneTransitionLayoutState(SceneA)

        val layoutState = layoutState()

        var swipesEnabled by mutableStateOf(false)

        var touchSlop = 0f

        rule.setContent {
@@ -489,7 +491,7 @@ class SwipeToSceneTest { 
    fun transitionKey() {

        val transitionkey = TransitionKey(debugName = "foo")

        val state =

            MutableSceneTransitionLayoutStateImpl(

            layoutState(

                SceneA,

                transitions {

                    from(SceneA, to = SceneB) { fade(TestElements.Foo) }
@@ -553,7 +555,7 @@ class SwipeToSceneTest { 
            }



        val state =

            MutableSceneTransitionLayoutState(

            layoutState(

                SceneA,

                transitions { from(SceneA, to = SceneB) { distance = swipeDistance } }

            )