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

Commit 526b787c authored by Jordan Demeulenaere's avatar Jordan Demeulenaere
Browse files

Fix the overscroll placement/composition logic 

This CL fixes a broken logic in shouldPlaceElement() and
shouldDrawOrComposeSharedElement(): when we are overscrolling, we should
only place (or compose in the case of MovableElements) in the scene that
is overscrolling. Before this CL, we would both place (compose) in the
overscrolling scene *and* in the scene picked by the scene picker. This
means that when overscrolling a MovableElement could be composed twice,
which should never happen.

Bug: 290930950
Test: ElementTest
Flag: com.android.systemui.scene_container
Change-Id: I8f76d7b30b43434ec2e71af268fc5b51fe106ae2
parent d233902b

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

+11 −8
Original line number Diff line number Diff line
@@ -596,12 +596,9 @@ private fun shouldPlaceElement( 
        return false

    }



    // Place the element if it is not shared or if the current scene is the one that is currently

    // overscrolling with [OverscrollSpec].

    // Place the element if it is not shared.

    if (

        transition.fromScene !in element.sceneStates ||

            transition.toScene !in element.sceneStates ||

            transition.currentOverscrollSpec?.scene == scene.key

        transition.fromScene !in element.sceneStates || transition.toScene !in element.sceneStates

    ) {

        return true

    }
@@ -611,7 +608,7 @@ private fun shouldPlaceElement( 
        return true

    }



    return shouldDrawOrComposeSharedElement(

    return shouldPlaceOrComposeSharedElement(

        layoutImpl,

        scene.key,

        element.key,
@@ -619,12 +616,18 @@ private fun shouldPlaceElement( 
    )

}



internal fun shouldDrawOrComposeSharedElement(

internal fun shouldPlaceOrComposeSharedElement(

    layoutImpl: SceneTransitionLayoutImpl,

    scene: SceneKey,

    element: ElementKey,

    transition: TransitionState.Transition,

): Boolean {

    // If we are overscrolling, only place/compose the element in the overscrolling scene.

    val overscrollScene = transition.currentOverscrollSpec?.scene

    if (overscrollScene != null) {

        return scene == overscrollScene

    }



    val scenePicker = element.scenePicker

    val fromScene = transition.fromScene

    val toScene = transition.toScene
@@ -637,7 +640,7 @@ internal fun shouldDrawOrComposeSharedElement( 
            toSceneZIndex = layoutImpl.scenes.getValue(toScene).zIndex,

        ) ?: return false



    return pickedScene == scene || transition.currentOverscrollSpec?.scene == scene

    return pickedScene == scene

}



private fun isSharedElementEnabled(

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

+1 −1
Original line number Diff line number Diff line
@@ -187,7 +187,7 @@ private fun shouldComposeMovableElement( 
        } ?: return false



    // Always compose movable elements in the scene picked by their scene picker.

    return shouldDrawOrComposeSharedElement(

    return shouldPlaceOrComposeSharedElement(

        layoutImpl,

        scene,

        element,

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

+103 −0
Original line number Diff line number Diff line
@@ -32,6 +32,7 @@ import androidx.compose.foundation.layout.offset 
import androidx.compose.foundation.layout.size

import androidx.compose.foundation.pager.HorizontalPager

import androidx.compose.foundation.pager.PagerState

import androidx.compose.material3.Text

import androidx.compose.runtime.Composable

import androidx.compose.runtime.LaunchedEffect

import androidx.compose.runtime.SideEffect
@@ -46,9 +47,12 @@ 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

import androidx.compose.ui.test.assertTopPositionInRootIsEqualTo

import androidx.compose.ui.test.hasText

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

import androidx.compose.ui.test.onNodeWithTag

import androidx.compose.ui.test.onRoot
@@ -1529,4 +1533,103 @@ class ElementTest { 
        rule.waitForIdle()

        assertThat(fooInB.lastAlpha).isEqualTo(0.3f)

    }



    @Test

    fun sharedElementIsOnlyPlacedInOverscrollingScene() {

        val state =

            rule.runOnUiThread {

                MutableSceneTransitionLayoutStateImpl(

                    SceneA,

                    transitions {

                        overscroll(SceneA, Orientation.Horizontal)

                        overscroll(SceneB, Orientation.Horizontal)

                    }

                )

            }



        @Composable

        fun SceneScope.Foo() {

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

        }



        rule.setContent {

            SceneTransitionLayout(state) {

                scene(SceneA) { Foo() }

                scene(SceneB) { Foo() }

            }

        }



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

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



        // A => B while overscrolling at scene B.

        var progress by mutableStateOf(2f)

        rule.runOnUiThread {

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

        }

        rule.waitForIdle()



        // Foo should only be placed in scene B.

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

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



        // Overscroll at scene A.

        progress = -1f

        rule.waitForIdle()



        // Foo should only be placed in scene A.

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

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

    }



    @Test

    fun sharedMovableElementIsOnlyComposedInOverscrollingScene() {

        val state =

            rule.runOnUiThread {

                MutableSceneTransitionLayoutStateImpl(

                    SceneA,

                    transitions {

                        overscroll(SceneA, Orientation.Horizontal)

                        overscroll(SceneB, Orientation.Horizontal)

                    }

                )

            }



        val fooInA = "fooInA"

        val fooInB = "fooInB"



        @Composable

        fun SceneScope.MovableFoo(text: String, modifier: Modifier = Modifier) {

            MovableElement(TestElements.Foo, modifier) { content { Text(text) } }

        }



        rule.setContent {

            SceneTransitionLayout(state) {

                scene(SceneA) { MovableFoo(text = fooInA) }

                scene(SceneB) { MovableFoo(text = fooInB) }

            }

        }



        rule.onNode(hasText(fooInA)).assertIsDisplayed()

        rule.onNode(hasText(fooInB)).assertDoesNotExist()



        // A => B while overscrolling at scene B.

        var progress by mutableStateOf(2f)

        rule.runOnUiThread {

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

        }

        rule.waitForIdle()



        // Foo content should only be composed in scene B.

        rule.onNode(hasText(fooInA)).assertDoesNotExist()

        rule.onNode(hasText(fooInB)).assertIsDisplayed()



        // Overscroll at scene A.

        progress = -1f

        rule.waitForIdle()



        // Foo content should only be composed in scene A.

        rule.onNode(hasText(fooInA)).assertIsDisplayed()

        rule.onNode(hasText(fooInB)).assertDoesNotExist()

    }

}