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

Commit 88e15dd0 authored by Jordan Demeulenaere's avatar Jordan Demeulenaere
Browse files

Remove calls to invokeOnCompletion 

This CL removes all calls to invokeOnCompletion in STL codebase. As
mentioned in the KDoc, there is no guarantee on which thread the
invokeOnCompletion block is run, which can cause concurrency issues that
throw an exception since ag/27342795.

The calls to invokeOnCompletion were replaced by a try/finally block.
However, it is important to note that a try block launched inside a
coroutine might never run if the coroutine is cancelled before it
starts. For this reason, we must also specify `start = ATOMIC` when
launching the coroutine to make sure that the try/finally block will
definitely run until the first suspension point.

Bug: 290930950
Bug: 340824084
Test: atest PlatformComposeSceneTransitionLayoutTests
Flag: com.android.systemui.scene_container
Change-Id: Ia822d4a2e0f424cd7a6715fb7dbeaf46affcaba3
parent b93b830a

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

+10 −8
Original line number Diff line number Diff line
@@ -21,6 +21,7 @@ import androidx.compose.animation.core.AnimationVector1D 
import androidx.compose.animation.core.SpringSpec

import kotlin.math.absoluteValue

import kotlinx.coroutines.CoroutineScope

import kotlinx.coroutines.CoroutineStart

import kotlinx.coroutines.Job

import kotlinx.coroutines.launch
@@ -190,13 +191,14 @@ private fun CoroutineScope.animate( 
        }



    // Animate the progress to its target value.

    // Important: We start atomically to make sure that we start the coroutine even if it is

    // cancelled right after it is launched, so that finishTransition() is correctly called.

    // Otherwise, this transition will never be stopped and we will never settle to Idle.

    transition.job =

        launch { animatable.animateTo(targetProgress, animationSpec, initialVelocity) }

            .apply {

                invokeOnCompletion {

                    // Settle the state to Idle(target). Note that this will do nothing if this

                    // transition was replaced/interrupted by another one, and this also runs if

                    // this coroutine is cancelled, i.e. if [this] coroutine scope is cancelled.

        launch(start = CoroutineStart.ATOMIC) {

            try {

                animatable.animateTo(targetProgress, animationSpec, initialVelocity)

            } finally {

                layoutState.finishTransition(transition, targetScene)

            }

        }

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

+7 −5
Original line number Diff line number Diff line
@@ -33,6 +33,7 @@ import com.android.compose.animation.scene.TransitionState.HasOverscrollProperti 
import com.android.compose.nestedscroll.PriorityNestedScrollConnection

import kotlin.math.absoluteValue

import kotlinx.coroutines.CoroutineScope

import kotlinx.coroutines.CoroutineStart

import kotlinx.coroutines.Job

import kotlinx.coroutines.launch
@@ -684,7 +685,11 @@ private class SwipeTransition( 
            val isTargetGreater = targetOffset > animatable.value

            val job =

                coroutineScope

                    .launch {

                    // Important: We start atomically to make sure that we start the coroutine even

                    // if it is cancelled right after it is launched, so that snapToScene() is

                    // correctly called. Otherwise, this transition will never be stopped and we

                    // will never settle to Idle.

                    .launch(start = CoroutineStart.ATOMIC) {

                        // TODO(b/327249191): Refactor the code so that we don't even launch a

                        // coroutine if we don't need to animate.

                        if (skipAnimation) {
@@ -726,18 +731,15 @@ private class SwipeTransition( 
                            }

                        } finally {

                            bouncingScene = null

                            snapToScene(targetScene)

                        }

                    }

                    // Make sure that we settle to target scene at the end of the animation or if

                    // the animation is cancelled.

                    .apply { invokeOnCompletion { snapToScene(targetScene) } }



            OffsetAnimation(animatable, job)

        }

    }



    fun snapToScene(scene: SceneKey) {

        if (layoutState.transitionState != this) return

        cancelOffsetAnimation()

        layoutState.finishTransition(this, idleScene = scene)

    }

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

+3 −5
Original line number Diff line number Diff line
@@ -497,11 +497,9 @@ internal abstract class BaseSceneTransitionLayoutState( 
                transitionStates = listOf(transition)

            }

            is TransitionState.Transition -> {

                // Force the current transition to finish to currentScene.

                currentState.finish().invokeOnCompletion {

                    // Make sure [finishTransition] is called at the end of the transition.

                    finishTransition(currentState, currentState.currentScene)

                }

                // Force the current transition to finish to currentScene. The transition will call

                // [finishTransition] once it's finished.

                currentState.finish()



                val tooManyTransitions = transitionStates.size >= MAX_CONCURRENT_TRANSITIONS

                val clearCurrentTransitions = !chain || tooManyTransitions