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

Commit 5def44c3 authored by Jordan Demeulenaere's avatar Jordan Demeulenaere
Browse files

Replace AnimatedState.valueOrNull by unsafeCompositionState (1/2) 

This CL replaces AnimatedState.valueOrNull by a new function
unsafeCompositionState() that returns a State that won't throw if it is
read during composition. This ensures that callers don't try to access
this value when the associated Element object is not in its map yet,
which would happen once we add the Element in a DisposableEffect or in a
Node.onAttach() method.

Before this CL, values coming from valueOrNull could sometimes be one
frame behind and sometimes not, depending on timings and composition
order. With this CL, values read through unsafeCompositionState() will
consistently be one frame behind. That way, the behavior is consistent
and easy to reason about.

Bug: 291071158
Test: AnimatedSharedAsStateTest
Flag: N/A
Change-Id: I75dcadaabdd6b3d314744fdde336b79115af0755
parent 4b9cd743

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

+27 −7
Original line number Diff line number Diff line
@@ -18,15 +18,19 @@ package com.android.compose.animation.scene 


import androidx.compose.runtime.Composable

import androidx.compose.runtime.DisposableEffect

import androidx.compose.runtime.LaunchedEffect

import androidx.compose.runtime.SideEffect

import androidx.compose.runtime.Stable

import androidx.compose.runtime.State

import androidx.compose.runtime.mutableStateOf

import androidx.compose.runtime.remember

import androidx.compose.runtime.snapshotFlow

import androidx.compose.ui.graphics.Color

import androidx.compose.ui.graphics.lerp

import androidx.compose.ui.unit.Dp

import androidx.compose.ui.unit.lerp

import com.android.compose.ui.util.lerp

import kotlinx.coroutines.flow.collect



/**

 * A [State] whose [value] is animated.
@@ -42,13 +46,20 @@ import com.android.compose.ui.util.lerp 
 *

 * If you don't have the choice and *have to* get the value during composition, for instance because

 * a Modifier or Composable reading this value does not have a lazy/lambda-based API, then you can

 * access [valueOrNull] and use a fallback value for the first frame where this animated value can

 * not be computed yet. Note however that doing so will be bad for performance and might lead to

 * late-by-one-frame flickers.

 * access [unsafeCompositionState] and use a fallback value for the first frame where this animated

 * value can not be computed yet. Note however that doing so will be bad for performance and might

 * lead to late-by-one-frame flickers.

 */

@Stable

interface AnimatedState<out T> : State<T> {

    val valueOrNull: T?

interface AnimatedState<T> : State<T> {

    /**

     * Return a [State] that can be read during composition.

     *

     * Important: You should avoid using this as much as possible and instead read [value] during

     * layout/drawing, otherwise you will probably end up with a few frames that have a value that

     * is not correctly interpolated.

     */

    @Composable fun unsafeCompositionState(initialValue: T): State<T>

}



/**
@@ -188,8 +199,17 @@ internal fun <T> animateSharedValueAsState( 
            override val value: T

                get() = value(layoutImpl, sceneKey, element, key, lerp, canOverflow)



            override val valueOrNull: T?

                get() = valueOrNull(layoutImpl, sceneKey, element, key, lerp, canOverflow)

            @Composable

            override fun unsafeCompositionState(initialValue: T): State<T> {

                val state = remember { mutableStateOf(initialValue) }



                val animatedState = this

                LaunchedEffect(animatedState) {

                    snapshotFlow { animatedState.value }.collect { state.value = it }

                }



                return state

            }

        }

    }

}

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

+66 −13
Original line number Diff line number Diff line
@@ -18,10 +18,9 @@ package com.android.compose.animation.scene 


import androidx.compose.animation.core.LinearEasing

import androidx.compose.animation.core.tween

import androidx.compose.foundation.layout.Box

import androidx.compose.foundation.layout.size

import androidx.compose.runtime.Composable

import androidx.compose.runtime.LaunchedEffect

import androidx.compose.runtime.SideEffect

import androidx.compose.runtime.getValue

import androidx.compose.runtime.snapshotFlow

import androidx.compose.ui.Modifier
@@ -32,7 +31,6 @@ import androidx.compose.ui.unit.Dp 
import androidx.compose.ui.unit.dp

import androidx.compose.ui.unit.lerp

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

import com.android.compose.test.assertSizeIsEqualTo

import com.android.compose.ui.util.lerp

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

import org.junit.Assert.assertThrows
@@ -296,35 +294,90 @@ class AnimatedSharedAsStateTest { 
    @Test

    fun readingAnimatedStateValueDuringCompositionIsStillPossible() {

        @Composable

        fun SceneScope.Bar(targetSize: Dp, modifier: Modifier = Modifier) {

            val size = animateSceneDpAsState(targetSize, TestValues.Value1)

            Box(modifier.element(TestElements.Bar).size(size.valueOrNull ?: targetSize))

        fun SceneScope.SceneValuesDuringComposition(

            targetValues: Values,

            onCurrentValueChanged: (Values) -> Unit,

        ) {

            val int by

                animateSceneIntAsState(targetValues.int, key = TestValues.Value1)

                    .unsafeCompositionState(targetValues.int)

            val float by

                animateSceneFloatAsState(targetValues.float, key = TestValues.Value2)

                    .unsafeCompositionState(targetValues.float)

            val dp by

                animateSceneDpAsState(targetValues.dp, key = TestValues.Value3)

                    .unsafeCompositionState(targetValues.dp)

            val color by

                animateSceneColorAsState(targetValues.color, key = TestValues.Value4)

                    .unsafeCompositionState(targetValues.color)



            val values = Values(int, float, dp, color)

            SideEffect { onCurrentValueChanged(values) }

        }



        val fromValues = Values(int = 0, float = 0f, dp = 0.dp, color = Color.Red)

        val toValues = Values(int = 100, float = 100f, dp = 100.dp, color = Color.Blue)



        var lastValueInFrom = fromValues

        var lastValueInTo = toValues



        rule.testTransition(

            fromSceneContent = { Bar(targetSize = 10.dp) },

            toSceneContent = { Bar(targetSize = 50.dp) },

            fromSceneContent = {

                SceneValuesDuringComposition(

                    targetValues = fromValues,

                    onCurrentValueChanged = { lastValueInFrom = it },

                )

            },

            toSceneContent = {

                SceneValuesDuringComposition(

                    targetValues = toValues,

                    onCurrentValueChanged = { lastValueInTo = it },

                )

            },

            transition = {

                // The transition lasts 64ms = 4 frames.

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

            },

        ) {

            before { onElement(TestElements.Bar).assertSizeIsEqualTo(10.dp, 10.dp) }

            before {

                assertThat(lastValueInFrom).isEqualTo(fromValues)



                // to was not composed yet, so lastValueInTo was not set yet.

                assertThat(lastValueInTo).isEqualTo(toValues)

            }



            at(16) {

                onElement(TestElements.Bar, TestScenes.SceneB).assertSizeIsEqualTo(20.dp, 20.dp)

                // Because we are using unsafeCompositionState(), values are one frame behind their

                // expected progress so at this first frame we are at progress = 0% instead of 25%.

                val expectedValues = lerp(fromValues, toValues, fraction = 0f)

                assertThat(lastValueInFrom).isEqualTo(expectedValues)

                assertThat(lastValueInTo).isEqualTo(expectedValues)

            }



            at(32) {

                onElement(TestElements.Bar, TestScenes.SceneB).assertSizeIsEqualTo(30.dp, 30.dp)

                // One frame behind, so 25% instead of 50%.

                val expectedValues = lerp(fromValues, toValues, fraction = 0.25f)

                assertThat(lastValueInFrom).isEqualTo(expectedValues)

                assertThat(lastValueInTo).isEqualTo(expectedValues)

            }



            at(48) {

                onElement(TestElements.Bar, TestScenes.SceneB).assertSizeIsEqualTo(40.dp, 40.dp)

                // One frame behind, so 50% instead of 75%.

                val expectedValues = lerp(fromValues, toValues, fraction = 0.5f)

                assertThat(lastValueInFrom).isEqualTo(expectedValues)

                assertThat(lastValueInTo).isEqualTo(expectedValues)

            }



            after {

                onElement(TestElements.Bar, TestScenes.SceneB).assertSizeIsEqualTo(50.dp, 50.dp)

                // from should have been last composed at progress = 100% before it is removed from

                // composition, but given that we are one frame behind the last values are stuck at

                // 75%.

                assertThat(lastValueInFrom).isEqualTo(lerp(fromValues, toValues, fraction = 0.75f))



                // The after {} block resumes the clock and will run as many frames as necessary so

                // that the application is idle, so the toScene settle to the idle state and to the

                // final values.

                assertThat(lastValueInTo).isEqualTo(toValues)

            }

        }

    }