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

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

Update element last offset in all scenes 

This CL fixes a bug that was caused by ag/25641284: even if a shared
element does not need to be placed/drawn in a scene, we should still
compute and update its lastOffset in that scene.

Bug: 308961608
Test: ElementTest
Flag: N/A
Change-Id: Ibe691a53e220ccf4ad64402f460c473a1d595d1d
parent 27960d71

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

+7 −5
Original line number Diff line number Diff line
@@ -531,11 +531,6 @@ private fun IntermediateMeasureScope.place( 
            sceneValues.targetOffset = targetOffsetInScene

        }



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

        if (!shouldDrawElement(layoutImpl, scene, element)) {

            return

        }



        val currentOffset = lookaheadScopeCoordinates.localPositionOf(coords, Offset.Zero)

        val lastSharedValues = element.lastSharedValues

        val lastValues = sceneValues.lastValues
@@ -558,6 +553,13 @@ private fun IntermediateMeasureScope.place( 
        lastSharedValues.offset = targetOffset

        lastValues.offset = targetOffset



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

        // it's still important to compute the target offset and update lastValues, otherwise it

        // will be out of date.

        if (!shouldDrawElement(layoutImpl, scene, element)) {

            return

        }



        val offset = (targetOffset - currentOffset).round()

        if (isElementOpaque(layoutImpl, element, scene, sceneValues)) {

            // TODO(b/291071158): Call placeWithLayer() if offset != IntOffset.Zero and size is not

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

+7 −7
Original line number Diff line number Diff line
@@ -65,11 +65,11 @@ fun SceneTransitionLayout( 
    SceneTransitionLayoutForTesting(

        currentScene,

        onChangeScene,

        modifier,

        transitions,

        state,

        edgeDetector,

        transitionInterceptionThreshold,

        modifier,

        onLayoutImpl = null,

        scenes,

    )
@@ -263,12 +263,12 @@ enum class SwipeDirection(val orientation: Orientation) { 
internal fun SceneTransitionLayoutForTesting(

    currentScene: SceneKey,

    onChangeScene: (SceneKey) -> Unit,

    transitions: SceneTransitions,

    state: SceneTransitionLayoutState,

    edgeDetector: EdgeDetector,

    transitionInterceptionThreshold: Float,

    modifier: Modifier,

    onLayoutImpl: ((SceneTransitionLayoutImpl) -> Unit)?,

    modifier: Modifier = Modifier,

    transitions: SceneTransitions = transitions {},

    state: SceneTransitionLayoutState = remember { SceneTransitionLayoutState(currentScene) },

    edgeDetector: EdgeDetector = DefaultEdgeDetector,

    transitionInterceptionThreshold: Float = 0f,

    onLayoutImpl: ((SceneTransitionLayoutImpl) -> Unit)? = null,

    scenes: SceneTransitionLayoutScope.() -> Unit,

) {

    val density = LocalDensity.current

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

+89 −16
Original line number Diff line number Diff line
@@ -16,6 +16,7 @@ 


package com.android.compose.animation.scene



import androidx.compose.animation.core.LinearEasing

import androidx.compose.animation.core.tween

import androidx.compose.foundation.ExperimentalFoundationApi

import androidx.compose.foundation.layout.Box
@@ -30,16 +31,21 @@ import androidx.compose.runtime.Composable 
import androidx.compose.runtime.SideEffect

import androidx.compose.runtime.getValue

import androidx.compose.runtime.mutableStateOf

import androidx.compose.runtime.remember

import androidx.compose.runtime.rememberCoroutineScope

import androidx.compose.runtime.setValue

import androidx.compose.ui.ExperimentalComposeUiApi

import androidx.compose.ui.Modifier

import androidx.compose.ui.geometry.Offset

import androidx.compose.ui.layout.intermediateLayout

import androidx.compose.ui.platform.LocalDensity

import androidx.compose.ui.test.junit4.createComposeRule

import androidx.compose.ui.unit.Density

import androidx.compose.ui.unit.Dp

import androidx.compose.ui.unit.DpOffset

import androidx.compose.ui.unit.dp

import androidx.test.ext.junit.runners.AndroidJUnit4

import com.android.compose.test.subjects.DpOffsetSubject

import com.android.compose.test.subjects.assertThat

import com.google.common.truth.Truth.assertThat

import kotlinx.coroutines.CoroutineScope

import kotlinx.coroutines.launch
@@ -259,11 +265,6 @@ class ElementTest { 
            SceneTransitionLayoutForTesting(

                currentScene = currentScene,

                onChangeScene = { currentScene = it },

                transitions = remember { transitions {} },

                state = remember { SceneTransitionLayoutState(currentScene) },

                edgeDetector = DefaultEdgeDetector,

                modifier = Modifier,

                transitionInterceptionThreshold = 0f,

                onLayoutImpl = { nullableLayoutImpl = it },

            ) {

                scene(TestScenes.SceneA) { /* Nothing */}
@@ -429,11 +430,6 @@ class ElementTest { 
            SceneTransitionLayoutForTesting(

                currentScene = TestScenes.SceneA,

                onChangeScene = {},

                transitions = remember { transitions {} },

                state = remember { SceneTransitionLayoutState(TestScenes.SceneA) },

                edgeDetector = DefaultEdgeDetector,

                modifier = Modifier,

                transitionInterceptionThreshold = 0f,

                onLayoutImpl = { nullableLayoutImpl = it },

            ) {

                scene(TestScenes.SceneA) { Box(Modifier.element(key)) }
@@ -484,11 +480,6 @@ class ElementTest { 
            SceneTransitionLayoutForTesting(

                currentScene = TestScenes.SceneA,

                onChangeScene = {},

                transitions = remember { transitions {} },

                state = remember { SceneTransitionLayoutState(TestScenes.SceneA) },

                edgeDetector = DefaultEdgeDetector,

                modifier = Modifier,

                transitionInterceptionThreshold = 0f,

                onLayoutImpl = { nullableLayoutImpl = it },

            ) {

                scene(TestScenes.SceneA) {
@@ -574,4 +565,86 @@ class ElementTest { 
            after { assertThat(fooCompositions).isEqualTo(1) }

        }

    }



    @Test

    fun sharedElementOffsetIsUpdatedEvenWhenNotPlaced() {

        var nullableLayoutImpl: SceneTransitionLayoutImpl? = null

        var density: Density? = null



        fun layoutImpl() = nullableLayoutImpl ?: error("nullableLayoutImpl was not set")



        fun density() = density ?: error("density was not set")



        fun Offset.toDpOffset() = with(density()) { DpOffset(x.toDp(), y.toDp()) }



        fun foo() = layoutImpl().elements[TestElements.Foo] ?: error("Foo not in elements map")



        fun Element.lastSharedOffset() = lastSharedValues.offset.toDpOffset()



        fun Element.lastOffsetIn(scene: SceneKey) =

            (sceneValues[scene] ?: error("$scene not in sceneValues map"))

                .lastValues

                .offset

                .toDpOffset()



        rule.testTransition(

            from = TestScenes.SceneA,

            to = TestScenes.SceneB,

            transitionLayout = { currentScene, onChangeScene ->

                density = LocalDensity.current



                SceneTransitionLayoutForTesting(

                    currentScene = currentScene,

                    onChangeScene = onChangeScene,

                    onLayoutImpl = { nullableLayoutImpl = it },

                    transitions =

                        transitions {

                            from(TestScenes.SceneA, to = TestScenes.SceneB) {

                                spec = tween(durationMillis = 4 * 16, easing = LinearEasing)

                            }

                        }

                ) {

                    scene(TestScenes.SceneA) { Box(Modifier.element(TestElements.Foo)) }

                    scene(TestScenes.SceneB) {

                        Box(Modifier.offset(x = 40.dp, y = 80.dp).element(TestElements.Foo))

                    }

                }

            }

        ) {

            val tolerance = DpOffsetSubject.DefaultTolerance



            before {

                val expected = DpOffset(0.dp, 0.dp)

                assertThat(foo().lastSharedOffset()).isWithin(tolerance).of(expected)

                assertThat(foo().lastOffsetIn(TestScenes.SceneA)).isWithin(tolerance).of(expected)

            }



            at(16) {

                val expected = DpOffset(10.dp, 20.dp)

                assertThat(foo().lastSharedOffset()).isWithin(tolerance).of(expected)

                assertThat(foo().lastOffsetIn(TestScenes.SceneA)).isWithin(tolerance).of(expected)

                assertThat(foo().lastOffsetIn(TestScenes.SceneB)).isWithin(tolerance).of(expected)

            }



            at(32) {

                val expected = DpOffset(20.dp, 40.dp)

                assertThat(foo().lastSharedOffset()).isWithin(tolerance).of(expected)

                assertThat(foo().lastOffsetIn(TestScenes.SceneA)).isWithin(tolerance).of(expected)

                assertThat(foo().lastOffsetIn(TestScenes.SceneB)).isWithin(tolerance).of(expected)

            }



            at(48) {

                val expected = DpOffset(30.dp, 60.dp)

                assertThat(foo().lastSharedOffset()).isWithin(tolerance).of(expected)

                assertThat(foo().lastOffsetIn(TestScenes.SceneA)).isWithin(tolerance).of(expected)

                assertThat(foo().lastOffsetIn(TestScenes.SceneB)).isWithin(tolerance).of(expected)

            }



            after {

                val expected = DpOffset(40.dp, 80.dp)

                assertThat(foo().lastSharedOffset()).isWithin(tolerance).of(expected)

                assertThat(foo().lastOffsetIn(TestScenes.SceneB)).isWithin(tolerance).of(expected)

            }

        }

    }

}