Merge "Derive currentOverscrollSpec to avoid unnecessary layouts/placements" into main

2 files changed, 82 insertions, 10 deletions

diff --git a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/content/state/TransitionState.kt b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/content/state/TransitionState.kt
index 9d3f25e9af22..3bd59df16f12 100644
--- a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/content/state/TransitionState.kt
+++ b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/content/state/TransitionState.kt
@@ -21,6 +21,7 @@ import androidx.compose.animation.core.AnimationVector1D
 import androidx.compose.animation.core.spring
 import androidx.compose.foundation.gestures.Orientation
 import androidx.compose.runtime.Stable
+import androidx.compose.runtime.State
 import androidx.compose.runtime.derivedStateOf
 import androidx.compose.runtime.getValue
 import com.android.compose.animation.scene.ContentKey
@@ -249,18 +250,29 @@ sealed interface TransitionState {
         private var fromOverscrollSpec: OverscrollSpecImpl? = null
         private var toOverscrollSpec: OverscrollSpecImpl? = null
 
-        /** The current [OverscrollSpecImpl], if this transition is currently overscrolling. */
-        internal val currentOverscrollSpec: OverscrollSpecImpl?
-            get() {
-                if (this !is HasOverscrollProperties) return null
-                val progress = progress
-                val bouncingContent = bouncingContent
-                return when {
-                    progress < 0f || bouncingContent == fromContent -> fromOverscrollSpec
-                    progress > 1f || bouncingContent == toContent -> toOverscrollSpec
-                    else -> null
+        /**
+         * The current [OverscrollSpecImpl], if this transition is currently overscrolling.
+         *
+         * Note: This is backed by a State<OverscrollSpecImpl?> because the overscroll spec is
+         * derived from progress, and we don't want readers of currentOverscrollSpec to recompose
+         * every time progress is changed.
+         */
+        private val _currentOverscrollSpec: State<OverscrollSpecImpl?>? =
+            if (this !is HasOverscrollProperties) {
+                null
+            } else {
+                derivedStateOf {
+                    val progress = progress
+                    val bouncingContent = bouncingContent
+                    when {
+                        progress < 0f || bouncingContent == fromContent -> fromOverscrollSpec
+                        progress > 1f || bouncingContent == toContent -> toOverscrollSpec
+                        else -> null
+                    }
                 }
             }
+        internal val currentOverscrollSpec: OverscrollSpecImpl?
+            get() = _currentOverscrollSpec?.value
 
         /**
          * An animatable that animates from 1f to 0f. This will be used to nicely animate the sudden
diff --git a/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ElementTest.kt b/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ElementTest.kt
index a2c2729e09be..39d46990dc4b 100644
--- a/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ElementTest.kt
+++ b/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ElementTest.kt
@@ -46,6 +46,7 @@ import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.geometry.Offset
 import androidx.compose.ui.layout.approachLayout
+import androidx.compose.ui.layout.layout
 import androidx.compose.ui.platform.LocalViewConfiguration
 import androidx.compose.ui.platform.testTag
 import androidx.compose.ui.test.assertIsDisplayed
@@ -70,11 +71,13 @@ import androidx.test.ext.junit.runners.AndroidJUnit4
 import com.android.compose.animation.scene.TestScenes.SceneA
 import com.android.compose.animation.scene.TestScenes.SceneB
 import com.android.compose.animation.scene.TestScenes.SceneC
+import com.android.compose.animation.scene.content.state.TransitionState
 import com.android.compose.animation.scene.subjects.assertThat
 import com.android.compose.test.assertSizeIsEqualTo
 import com.android.compose.test.setContentAndCreateMainScope
 import com.android.compose.test.transition
 import com.google.common.truth.Truth.assertThat
+import com.google.common.truth.Truth.assertWithMessage
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.launch
 import org.junit.Assert.assertThrows
@@ -2581,4 +2584,61 @@ class ElementTest {
             }
         }
     }
+
+    @Test
+    fun staticSharedElementShouldNotRemeasureOrReplaceDuringOverscrollableTransition() {
+        val size = 30.dp
+        var numberOfMeasurements = 0
+        var numberOfPlacements = 0
+
+        // Foo is a simple element that does not move or resize during the transition.
+        @Composable
+        fun SceneScope.Foo(modifier: Modifier = Modifier) {
+            Box(
+                modifier
+                    .element(TestElements.Foo)
+                    .layout { measurable, constraints ->
+                        numberOfMeasurements++
+                        measurable.measure(constraints).run {
+                            numberOfPlacements++
+                            layout(width, height) { place(0, 0) }
+                        }
+                    }
+                    .size(size)
+            )
+        }
+
+        val state = rule.runOnUiThread { MutableSceneTransitionLayoutState(SceneA) }
+        val scope =
+            rule.setContentAndCreateMainScope {
+                SceneTransitionLayout(state) {
+                    scene(SceneA) { Box(Modifier.fillMaxSize()) { Foo() } }
+                    scene(SceneB) { Box(Modifier.fillMaxSize()) { Foo() } }
+                }
+            }
+
+        // Start an overscrollable transition driven by progress.
+        var progress by mutableFloatStateOf(0f)
+        val transition = transition(from = SceneA, to = SceneB, progress = { progress })
+        assertThat(transition).isInstanceOf(TransitionState.HasOverscrollProperties::class.java)
+        scope.launch { state.startTransition(transition) }
+
+        // Reset the counters after the first animation frame.
+        rule.waitForIdle()
+        numberOfMeasurements = 0
+        numberOfPlacements = 0
+
+        // Change the progress a bunch of times.
+        val nFrames = 20
+        repeat(nFrames) { i ->
+            progress = i / nFrames.toFloat()
+            rule.waitForIdle()
+
+            // We shouldn't have remeasured or replaced Foo.
+            assertWithMessage("Frame $i didn't remeasure Foo")
+                .that(numberOfMeasurements)
+                .isEqualTo(0)
+            assertWithMessage("Frame $i didn't replace Foo").that(numberOfPlacements).isEqualTo(0)
+        }
+    }
 }