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

Commit 17df31be authored by Jordan Demeulenaere's avatar Jordan Demeulenaere
Browse files

Remove @Composable from Modifier factories 

This CL removes the @Composable annotation from Modifier factories and
replace them by Modifier.composed {} instead. As discussed in
http://shortn/_lBPKSM7VLn, making a Modifier factory @Composable can
have unexpected side effects where values are remember-ed longer than
they should or disposable not disposed when the associated element is
removed from composition.

In later CLs, we will migrate those APIs to the Node API instead.

Bug: 291566282
Test: atest PlatformComposeSceneTransitionLayoutTests
Change-Id: Ibb36ff90e8d438e1a18c3488109cfacf658d3d00
parent c737d35e

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

+3 −3
Original line number Diff line number Diff line
@@ -29,6 +29,7 @@ import androidx.compose.runtime.snapshots.Snapshot 
import androidx.compose.runtime.snapshots.SnapshotStateMap

import androidx.compose.ui.ExperimentalComposeUiApi

import androidx.compose.ui.Modifier

import androidx.compose.ui.composed

import androidx.compose.ui.draw.drawWithContent

import androidx.compose.ui.geometry.Offset

import androidx.compose.ui.geometry.isSpecified
@@ -110,13 +111,12 @@ internal class Element(val key: ElementKey) { 
}



/** The implementation of [SceneScope.element]. */

@Composable

@OptIn(ExperimentalComposeUiApi::class)

internal fun Modifier.element(

    layoutImpl: SceneTransitionLayoutImpl,

    scene: Scene,

    key: ElementKey,

): Modifier {

): Modifier = composed {

    val sceneValues = remember(scene, key) { Element.TargetValues() }

    val element =

        // Get the element associated to [key] if it was already composed in another scene,
@@ -160,7 +160,7 @@ internal fun Modifier.element( 
        }

    }



    return drawWithContent {

    drawWithContent {

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

                drawContent()

            }

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

+3 −4
Original line number Diff line number Diff line
@@ -23,11 +23,11 @@ import androidx.compose.foundation.gestures.awaitHorizontalTouchSlopOrCancellati 
import androidx.compose.foundation.gestures.awaitVerticalTouchSlopOrCancellation

import androidx.compose.foundation.gestures.horizontalDrag

import androidx.compose.foundation.gestures.verticalDrag

import androidx.compose.runtime.Composable

import androidx.compose.runtime.getValue

import androidx.compose.runtime.remember

import androidx.compose.runtime.rememberUpdatedState

import androidx.compose.ui.Modifier

import androidx.compose.ui.composed

import androidx.compose.ui.geometry.Offset

import androidx.compose.ui.input.pointer.PointerEventPass

import androidx.compose.ui.input.pointer.PointerId
@@ -56,7 +56,6 @@ import androidx.compose.ui.util.fastForEach 
 * change in the future.

 */

// TODO(b/291055080): Migrate to the Modifier.Node API.

@Composable

internal fun Modifier.multiPointerDraggable(

    orientation: Orientation,

    enabled: Boolean,
@@ -64,7 +63,7 @@ internal fun Modifier.multiPointerDraggable( 
    onDragStarted: (startedPosition: Offset, pointersDown: Int) -> Unit,

    onDragDelta: (Float) -> Unit,

    onDragStopped: (velocity: Float) -> Unit,

): Modifier {

): Modifier = composed {

    val onDragStarted by rememberUpdatedState(onDragStarted)

    val onDragStopped by rememberUpdatedState(onDragStopped)

    val onDragDelta by rememberUpdatedState(onDragDelta)
@@ -77,7 +76,7 @@ internal fun Modifier.multiPointerDraggable( 
            Velocity(maxF, maxF)

        }



    return this.pointerInput(enabled, orientation, maxFlingVelocity) {

    pointerInput(enabled, orientation, maxFlingVelocity) {

        if (!enabled) {

            return@pointerInput

        }

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

+0 −1
Original line number Diff line number Diff line
@@ -60,7 +60,6 @@ private class SceneScopeImpl( 
    private val layoutImpl: SceneTransitionLayoutImpl,

    private val scene: Scene,

) : SceneScope {

    @Composable

    override fun Modifier.element(key: ElementKey): Modifier {

        return element(layoutImpl, scene, key)

    }

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

+1 −1
Original line number Diff line number Diff line
@@ -117,7 +117,7 @@ interface SceneScope { 
     * TODO(b/291566282): Migrate this to the new Modifier Node API and remove the @Composable

     *   constraint.

     */

    @Composable fun Modifier.element(key: ElementKey): Modifier

    fun Modifier.element(key: ElementKey): Modifier



    /**

     * Create a *movable* element identified by [key].

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

+7 −4
Original line number Diff line number Diff line
@@ -30,7 +30,6 @@ import androidx.compose.runtime.mutableStateOf 
import androidx.compose.runtime.remember

import androidx.compose.runtime.setValue

import androidx.compose.runtime.snapshots.SnapshotStateMap

import androidx.compose.ui.ExperimentalComposeUiApi

import androidx.compose.ui.Modifier

import androidx.compose.ui.draw.drawWithContent

import androidx.compose.ui.layout.LookaheadScope
@@ -131,15 +130,19 @@ class SceneTransitionLayoutImpl( 
    }



    @Composable

    @OptIn(ExperimentalComposeUiApi::class)

    internal fun Content(modifier: Modifier) {

        val horizontalGestureHandler =

            rememberSceneGestureHandler(layoutImpl = this, Orientation.Horizontal)

        val verticalGestureHandler =

            rememberSceneGestureHandler(layoutImpl = this, Orientation.Vertical)



        Box(

            modifier

                // Handle horizontal and vertical swipes on this layout.

                // Note: order here is important and will give a slight priority to the vertical

                // swipes.

                .swipeToScene(layoutImpl = this, Orientation.Horizontal)

                .swipeToScene(layoutImpl = this, Orientation.Vertical)

                .swipeToScene(horizontalGestureHandler)

                .swipeToScene(verticalGestureHandler)

                .onSizeChanged { size = it }

        ) {

            LookaheadScope {