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

Commit d4b9cb40 authored by Omar Miatello's avatar Omar Miatello
Browse files

Revert "STL ElementStateScope exposes lastSize()" 

This reverts commit 1faa0111.

Reason for revert: Not needed anymore after ag/34941048

Remove `ElementKey.lastSize()` from public API, this API was initially
introduced with ag/32945988.

The function was backed by a `mutableStateOf` to provide an element's
current size during transitions. However, we cannot make assumptions
about when this state might be read by consumers. If it is read after
the layout phase, it can trigger an additional composition/layout pass,
leading to a series of unexpected bugs and performance issues (see
b/431146033).

To prevent this, `Element.State.lastSize` is no longer a
`mutableStateOf`. The public API and its corresponding tests are also
removed.

Test: Manually tested on the demo app
Bug: 419520966
Bug: 404526497
Flag: com.android.systemui.scene_container
Change-Id: I8138ddba0234ea43d7433fb3b7c4169ce315d5b1
parent 1faa0111

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

+2 −5
Original line number Diff line number Diff line
@@ -107,7 +107,7 @@ internal class Element(val key: ElementKey) { 


        /** The last state this element had in this content. */

        var lastOffset = Offset.Unspecified

        var lastSize by mutableStateOf(SizeUnspecified)

        var lastSize = SizeUnspecified

        var lastScale = Scale.Unspecified

        var lastAlpha = AlphaUnspecified
@@ -410,6 +410,7 @@ internal class ElementNode( 


        val placeable =

            measure(layoutImpl, element, transition, stateInContent, measurable, constraints)

        stateInContent.lastSize = placeable.size()

        return layout(placeable.width, placeable.height) { place(elementState, placeable) }

    }
@@ -1213,7 +1214,6 @@ private fun measure( 
    maybePlaceable?.let { placeable ->

        stateInContent.sizeBeforeInterruption = Element.SizeUnspecified

        stateInContent.sizeInterruptionDelta = IntSize.Zero

        stateInContent.lastSize = placeable.size()

        return placeable

    }
@@ -1236,9 +1236,6 @@ private fun measure( 
                )

            },

        )



    stateInContent.lastSize = interruptedSize



    return measurable.measure(

        Constraints.fixed(

            interruptedSize.width.coerceAtLeast(0),

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

+0 −11
Original line number Diff line number Diff line
@@ -159,17 +159,6 @@ interface ElementStateScope { 
     */

    fun ElementKey.targetSize(content: ContentKey): IntSize?



    /**

     * Return the *last known size* of [this] element in the given [content], i.e. the size of the

     * element, or `null` if the element is not composed and measured in that content (yet).

     *

     * Note: Usually updated **after** the measurement pass and after processing children. However,

     * if the target size is known **in advance** (like during transitions involving transformations

     * or shared elements), the update happens **before** measurement pass. This earlier update

     * allows children to potentially use this predetermined size during their own measurement.

     */

    fun ElementKey.lastSize(content: ContentKey): IntSize?



    /**

     * Return the *target* offset of [this] element in the given [content], i.e. the size of the

     * element when idle, or `null` if the element is not composed and placed in that content (yet).

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

+0 −6
Original line number Diff line number Diff line
@@ -31,12 +31,6 @@ internal class ElementStateScopeImpl(private val layoutImpl: SceneTransitionLayo 
        }

    }



    override fun ElementKey.lastSize(content: ContentKey): IntSize? {

        return layoutImpl.elements[this]?.stateByContent?.get(content)?.lastSize.takeIf {

            it != Element.SizeUnspecified

        }

    }



    override fun ElementKey.targetOffset(content: ContentKey): Offset? {

        return layoutImpl.elements[this]?.stateByContent?.get(content)?.targetOffset.takeIf {

            it != Offset.Unspecified

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

+0 −96
Original line number Diff line number Diff line
@@ -51,7 +51,6 @@ import androidx.compose.ui.platform.LocalDensity 
import androidx.compose.ui.platform.LocalViewConfiguration

import androidx.compose.ui.platform.testTag

import androidx.compose.ui.test.assertIsDisplayed

import androidx.compose.ui.test.assertIsEqualTo

import androidx.compose.ui.test.assertIsNotDisplayed

import androidx.compose.ui.test.assertPositionInRootIsEqualTo

import androidx.compose.ui.test.assertTopPositionInRootIsEqualTo
@@ -2313,99 +2312,4 @@ class ElementTest { 


        assertThat(compositions).isEqualTo(1)

    }



    @Test

    fun measureElementApproachSizeBeforeChildren() {

        val state =

            rule.runOnUiThread {

                MutableSceneTransitionLayoutStateForTests(SceneA, SceneTransitions.Empty)

            }



        lateinit var lastFooHeight: () -> Dp?

        var firstFooHeightBeforeMeasuringChild: Dp? = null



        val scope =

            rule.setContentAndCreateMainScope {

                val density = LocalDensity.current

                SceneTransitionLayoutForTesting(state) {

                    scene(SceneA) {

                        SideEffect {

                            lastFooHeight = {

                                with(density) { TestElements.Foo.lastSize(SceneA)?.height?.toDp() }

                            }

                        }

                        Box(Modifier.element(TestElements.Foo).size(200.dp)) {

                            Box(

                                Modifier.approachLayout(

                                    isMeasurementApproachInProgress = { false },

                                    approachMeasure = { measurable, constraints ->

                                        if (firstFooHeightBeforeMeasuringChild == null) {

                                            firstFooHeightBeforeMeasuringChild = lastFooHeight()

                                        }



                                        measurable.measure(constraints).run {

                                            layout(width, height) {}

                                        }

                                    },

                                )

                            )

                        }

                    }

                    scene(SceneB) { Box(Modifier.element(TestElements.Foo).size(100.dp)) }

                }

            }



        var progress by mutableFloatStateOf(0f)

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



        fun assertDp(actual: Dp?, expected: Dp, subject: String) {

            assertThat(actual).isNotNull()

            actual!!.assertIsEqualTo(expected, subject, tolerance = 0.5.dp)

        }



        // Idle state: Scene A.

        assertThat(state.isTransitioning()).isFalse()

        assertDp(actual = lastFooHeight(), expected = 200.dp, subject = "lastFooHeight")



        // Start transition: Scene A -> Scene B (progress 0%).

        firstFooHeightBeforeMeasuringChild = null

        scope.launch { state.startTransition(transition) }

        rule.waitForIdle()

        assertThat(state.isTransitioning()).isTrue()

        assertDp(

            actual = firstFooHeightBeforeMeasuringChild,

            expected = 200.dp,

            subject = "firstFooHeightBeforeMeasuringChild",

        )

        assertDp(actual = lastFooHeight(), expected = 200.dp, subject = "lastFooHeight")



        // progress 50%: height is going from 200dp to 100dp, so 150dp is expected now.

        firstFooHeightBeforeMeasuringChild = null

        progress = 0.5f

        rule.waitForIdle()

        assertDp(

            actual = firstFooHeightBeforeMeasuringChild,

            expected = 150.dp,

            subject = "firstFooHeightBeforeMeasuringChild",

        )

        assertDp(actual = lastFooHeight(), expected = 150.dp, subject = "lastFooHeight")



        firstFooHeightBeforeMeasuringChild = null

        progress = 1f

        rule.waitForIdle()

        assertDp(

            actual = firstFooHeightBeforeMeasuringChild,

            expected = 100.dp,

            subject = "firstFooHeightBeforeMeasuringChild",

        )

        assertDp(actual = lastFooHeight(), expected = 100.dp, subject = "lastFooHeight")



        firstFooHeightBeforeMeasuringChild = null

        transition.finish()

        rule.waitForIdle()

        assertThat(state.isTransitioning()).isFalse()

        assertThat(firstFooHeightBeforeMeasuringChild).isNull()

        // null because SceneA does not exist anymore.

        assertThat(lastFooHeight()).isNull()

    }

}