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

Commit 0ce8047f authored by Jordan Demeulenaere's avatar Jordan Demeulenaere
Browse files

Correctly clear last placement values on nested elements 

This CL ensures that we correctly clear the last placement values for
all elements that are not placed.

Before this CL, only the first element to not be placed would correctly
clear its last offset/alpha/scale given that measure() is not called on
children of elements that are not placed.

This CL fixes that by making ElementNode a traversable node that we can
recursively traverse to correctly clear the last placement values.

Bug: 290930950
Test: ElementTest
Flag: com.android.systemui.scene_container
Change-Id: I02361fcde32196ddaece01437ef727951e56d8a4
parent 6052526d

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

+28 −7
Original line number Original line Diff line number Diff line
@@ -39,6 +39,8 @@ import androidx.compose.ui.layout.MeasureScope 
import androidx.compose.ui.layout.Placeable

import androidx.compose.ui.layout.Placeable

import androidx.compose.ui.node.DrawModifierNode

import androidx.compose.ui.node.DrawModifierNode

import androidx.compose.ui.node.ModifierNodeElement

import androidx.compose.ui.node.ModifierNodeElement

import androidx.compose.ui.node.TraversableNode

import androidx.compose.ui.node.traverseDescendants

import androidx.compose.ui.platform.testTag

import androidx.compose.ui.platform.testTag

import androidx.compose.ui.unit.Constraints

import androidx.compose.ui.unit.Constraints

import androidx.compose.ui.unit.IntSize

import androidx.compose.ui.unit.IntSize
@@ -165,7 +167,7 @@ internal class ElementNode( 
    private var currentTransitions: List<TransitionState.Transition>,

    private var currentTransitions: List<TransitionState.Transition>,

    private var scene: Scene,

    private var scene: Scene,

    private var key: ElementKey,

    private var key: ElementKey,

) : Modifier.Node(), DrawModifierNode, ApproachLayoutModifierNode {

) : Modifier.Node(), DrawModifierNode, ApproachLayoutModifierNode, TraversableNode {

    private var _element: Element? = null

    private var _element: Element? = null

    private val element: Element

    private val element: Element

        get() = _element!!

        get() = _element!!
@@ -174,6 +176,8 @@ internal class ElementNode( 
    private val sceneState: Element.SceneState

    private val sceneState: Element.SceneState

        get() = _sceneState!!

        get() = _sceneState!!





    override val traverseKey: Any = ElementTraverseKey



    override fun onAttach() {

    override fun onAttach() {

        super.onAttach()

        super.onAttach()

        updateElementAndSceneValues()

        updateElementAndSceneValues()
@@ -289,9 +293,7 @@ internal class ElementNode( 
        val isOtherSceneOverscrolling = overscrollScene != null && overscrollScene != scene.key

        val isOtherSceneOverscrolling = overscrollScene != null && overscrollScene != scene.key

        val isNotPartOfAnyOngoingTransitions = transitions.isNotEmpty() && transition == null

        val isNotPartOfAnyOngoingTransitions = transitions.isNotEmpty() && transition == null

        if (isNotPartOfAnyOngoingTransitions || isOtherSceneOverscrolling) {

        if (isNotPartOfAnyOngoingTransitions || isOtherSceneOverscrolling) {

            sceneState.lastOffset = Offset.Unspecified

            recursivelyClearPlacementValues()

            sceneState.lastScale = Scale.Unspecified

            sceneState.lastAlpha = Element.AlphaUnspecified





            val placeable = measurable.measure(constraints)

            val placeable = measurable.measure(constraints)

            sceneState.lastSize = placeable.size()

            sceneState.lastSize = placeable.size()
@@ -319,9 +321,7 @@ internal class ElementNode( 




            // No need to place the element in this scene if we don't want to draw it anyways.

            // No need to place the element in this scene if we don't want to draw it anyways.

            if (!shouldPlaceElement(layoutImpl, scene.key, element, transition)) {

            if (!shouldPlaceElement(layoutImpl, scene.key, element, transition)) {

                sceneState.lastOffset = Offset.Unspecified

                recursivelyClearPlacementValues()

                sceneState.lastScale = Scale.Unspecified

                sceneState.lastAlpha = Element.AlphaUnspecified

                return

                return

            }

            }
@@ -402,6 +402,25 @@ internal class ElementNode( 
        }

        }

    }

    }





    /**

     * Recursively clear the last placement values on this node and all descendants ElementNodes.

     * This should be called when this node is not placed anymore, so that we correctly clear values

     * for the descendants for which approachMeasure() won't be called.

     */

    private fun recursivelyClearPlacementValues() {

        fun Element.SceneState.clearLastPlacementValues() {

            lastOffset = Offset.Unspecified

            lastScale = Scale.Unspecified

            lastAlpha = Element.AlphaUnspecified

        }



        sceneState.clearLastPlacementValues()

        traverseDescendants(ElementTraverseKey) { node ->

            (node as ElementNode).sceneState.clearLastPlacementValues()

            TraversableNode.Companion.TraverseDescendantsAction.ContinueTraversal

        }

    }



    override fun ContentDrawScope.draw() {

    override fun ContentDrawScope.draw() {

        element.wasDrawnInAnyScene = true

        element.wasDrawnInAnyScene = true
@@ -421,6 +440,8 @@ internal class ElementNode( 
    }

    }





    companion object {

    companion object {

        private val ElementTraverseKey = Any()



        private fun maybePruneMaps(

        private fun maybePruneMaps(

            layoutImpl: SceneTransitionLayoutImpl,

            layoutImpl: SceneTransitionLayoutImpl,

            element: Element,

            element: Element,

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

+1 −1
Original line number Original line Diff line number Diff line
@@ -768,7 +768,7 @@ internal class HoistedSceneTransitionLayoutState( 
/** A [MutableSceneTransitionLayoutState] that holds the value for the current scene. */

/** A [MutableSceneTransitionLayoutState] that holds the value for the current scene. */

internal class MutableSceneTransitionLayoutStateImpl(

internal class MutableSceneTransitionLayoutStateImpl(

    initialScene: SceneKey,

    initialScene: SceneKey,

    override var transitions: SceneTransitions,

    override var transitions: SceneTransitions = transitions {},

    private val canChangeScene: (SceneKey) -> Boolean = { true },

    private val canChangeScene: (SceneKey) -> Boolean = { true },

    stateLinks: List<StateLink> = emptyList(),

    stateLinks: List<StateLink> = emptyList(),

    enableInterruptions: Boolean = DEFAULT_INTERRUPTIONS_ENABLED,

    enableInterruptions: Boolean = DEFAULT_INTERRUPTIONS_ENABLED,

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

+72 −0
Original line number Original line Diff line number Diff line
@@ -1719,4 +1719,76 @@ class ElementTest { 
        rule.onNode(isElement(TestElements.Foo, SceneB)).assertIsNotDisplayed()

        rule.onNode(isElement(TestElements.Foo, SceneB)).assertIsNotDisplayed()

        rule.onNode(isElement(TestElements.Foo, SceneC)).assertPositionInRootIsEqualTo(40.dp, 40.dp)

        rule.onNode(isElement(TestElements.Foo, SceneC)).assertPositionInRootIsEqualTo(40.dp, 40.dp)

    }

    }



    @Test

    fun lastPlacementValuesAreClearedOnNestedElements() {

        val state = rule.runOnIdle { MutableSceneTransitionLayoutStateImpl(SceneA) }



        @Composable

        fun SceneScope.NestedFooBar() {

            Box(Modifier.element(TestElements.Foo)) {

                Box(Modifier.element(TestElements.Bar).size(10.dp))

            }

        }



        lateinit var layoutImpl: SceneTransitionLayoutImpl

        rule.setContent {

            SceneTransitionLayoutForTesting(state, onLayoutImpl = { layoutImpl = it }) {

                scene(SceneA) { NestedFooBar() }

                scene(SceneB) { NestedFooBar() }

            }

        }



        // Idle on A: composed and placed only in B.

        rule.onNode(isElement(TestElements.Foo, SceneA)).assertIsDisplayed()

        rule.onNode(isElement(TestElements.Bar, SceneA)).assertIsDisplayed()

        rule.onNode(isElement(TestElements.Foo, SceneB)).assertDoesNotExist()

        rule.onNode(isElement(TestElements.Bar, SceneB)).assertDoesNotExist()



        assertThat(layoutImpl.elements).containsKey(TestElements.Foo)

        assertThat(layoutImpl.elements).containsKey(TestElements.Bar)

        val foo = layoutImpl.elements.getValue(TestElements.Foo)

        val bar = layoutImpl.elements.getValue(TestElements.Bar)



        assertThat(foo.sceneStates).containsKey(SceneA)

        assertThat(bar.sceneStates).containsKey(SceneA)

        assertThat(foo.sceneStates).doesNotContainKey(SceneB)

        assertThat(bar.sceneStates).doesNotContainKey(SceneB)



        val fooInA = foo.sceneStates.getValue(SceneA)

        val barInA = bar.sceneStates.getValue(SceneA)

        assertThat(fooInA.lastOffset).isNotEqualTo(Offset.Unspecified)

        assertThat(fooInA.lastAlpha).isNotEqualTo(Element.AlphaUnspecified)

        assertThat(fooInA.lastScale).isNotEqualTo(Scale.Unspecified)



        assertThat(barInA.lastOffset).isNotEqualTo(Offset.Unspecified)

        assertThat(barInA.lastAlpha).isNotEqualTo(Element.AlphaUnspecified)

        assertThat(barInA.lastScale).isNotEqualTo(Scale.Unspecified)



        // A => B: composed in both and placed only in B.

        rule.runOnUiThread { state.startTransition(transition(from = SceneA, to = SceneB)) }

        rule.onNode(isElement(TestElements.Foo, SceneA)).assertExists().assertIsNotDisplayed()

        rule.onNode(isElement(TestElements.Bar, SceneA)).assertExists().assertIsNotDisplayed()

        rule.onNode(isElement(TestElements.Foo, SceneB)).assertIsDisplayed()

        rule.onNode(isElement(TestElements.Bar, SceneB)).assertIsDisplayed()



        assertThat(foo.sceneStates).containsKey(SceneB)

        assertThat(bar.sceneStates).containsKey(SceneB)



        val fooInB = foo.sceneStates.getValue(SceneB)

        val barInB = bar.sceneStates.getValue(SceneB)

        assertThat(fooInA.lastOffset).isEqualTo(Offset.Unspecified)

        assertThat(fooInA.lastAlpha).isEqualTo(Element.AlphaUnspecified)

        assertThat(fooInA.lastScale).isEqualTo(Scale.Unspecified)

        assertThat(fooInB.lastOffset).isNotEqualTo(Offset.Unspecified)

        assertThat(fooInB.lastAlpha).isNotEqualTo(Element.AlphaUnspecified)

        assertThat(fooInB.lastScale).isNotEqualTo(Scale.Unspecified)



        assertThat(barInA.lastOffset).isEqualTo(Offset.Unspecified)

        assertThat(barInA.lastAlpha).isEqualTo(Element.AlphaUnspecified)

        assertThat(barInA.lastScale).isEqualTo(Scale.Unspecified)

        assertThat(barInB.lastOffset).isNotEqualTo(Offset.Unspecified)

        assertThat(barInB.lastAlpha).isNotEqualTo(Element.AlphaUnspecified)

        assertThat(barInB.lastScale).isNotEqualTo(Scale.Unspecified)

    }

}
 
}

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

+2 −2
Original line number Original line Diff line number Diff line
@@ -17,7 +17,7 @@ 
package com.android.compose.animation.scene

package com.android.compose.animation.scene





import androidx.compose.ui.test.SemanticsMatcher

import androidx.compose.ui.test.SemanticsMatcher

import androidx.compose.ui.test.hasParent

import androidx.compose.ui.test.hasAnyAncestor

import androidx.compose.ui.test.hasTestTag

import androidx.compose.ui.test.hasTestTag





/** A [SemanticsMatcher] that matches [element], optionally restricted to scene [scene]. */

/** A [SemanticsMatcher] that matches [element], optionally restricted to scene [scene]. */
@@ -25,6 +25,6 @@ fun isElement(element: ElementKey, scene: SceneKey? = null): SemanticsMatcher { 
    return if (scene == null) {

    return if (scene == null) {

        hasTestTag(element.testTag)

        hasTestTag(element.testTag)

    } else {

    } else {

        hasTestTag(element.testTag) and hasParent(hasTestTag(scene.testTag))

        hasTestTag(element.testTag) and hasAnyAncestor(hasTestTag(scene.testTag))

    }

    }

}
 
}