From ef54cd7b73894a098c4f0fd409d3e772daa905f9 Mon Sep 17 00:00:00 2001 From: Alexander Maryanovsky Date: Thu, 11 Apr 2024 15:33:14 +0300 Subject: [PATCH] Move the effects and synthetic events dispatching to after the draw phase in the render loop. (#1260) --- .../androidx/compose/ui/ComposeSceneTest.kt | 129 +---------- .../compose/ui/platform/RenderingTestScope.kt | 17 +- .../FlushCoroutineDispatcher.skiko.kt | 2 +- .../ui/scene/BaseComposeScene.skiko.kt | 47 ++-- .../scene/ComposeSceneInputHandler.skiko.kt | 5 +- .../ui/scene/ComposeSceneRecomposer.skiko.kt | 23 +- .../platform/FlushCoroutineDispatcherTest.kt | 2 - .../compose/ui/platform/RenderPhasesTest.kt | 216 ++++++++++++++++++ 8 files changed, 269 insertions(+), 172 deletions(-) create mode 100644 compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/RenderPhasesTest.kt diff --git a/compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/ComposeSceneTest.kt b/compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/ComposeSceneTest.kt index 9f3703617dde8..4513292e8951c 100644 --- a/compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/ComposeSceneTest.kt +++ b/compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/ComposeSceneTest.kt @@ -73,31 +73,23 @@ import androidx.compose.ui.layout.Layout import androidx.compose.ui.layout.MeasurePolicy import androidx.compose.ui.layout.RootMeasurePolicy.measure import androidx.compose.ui.platform.renderingTest -import androidx.compose.ui.platform.testTag -import androidx.compose.ui.test.ExperimentalTestApi import androidx.compose.ui.test.InternalTestApi import androidx.compose.ui.test.junit4.DesktopScreenshotTestRule import androidx.compose.ui.test.junit4.createComposeRule -import androidx.compose.ui.test.onNodeWithTag import androidx.compose.ui.test.onRoot import androidx.compose.ui.test.performKeyPress -import androidx.compose.ui.test.performMouseInput -import androidx.compose.ui.test.runComposeUiTest import androidx.compose.ui.unit.dp -import androidx.compose.ui.window.Window -import androidx.compose.ui.window.runApplicationTest import com.google.common.truth.Truth.assertThat import kotlin.test.assertEquals import kotlin.test.assertTrue import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking import org.junit.Assert.assertFalse import org.junit.Ignore import org.junit.Rule import org.junit.Test -@OptIn(InternalTestApi::class, ExperimentalComposeUiApi::class, ExperimentalTestApi::class) +@OptIn(InternalTestApi::class, ExperimentalComposeUiApi::class) class ComposeSceneTest { @get:Rule val screenshotRule = DesktopScreenshotTestRule("compose/ui/ui-desktop") @@ -704,125 +696,6 @@ class ComposeSceneTest { assertThat(field2FocusState!!.isFocused).isFalse() } } - - // Test that we're draining the effect dispatcher completely before starting the draw phase - @Test - fun allEffectsRunBeforeDraw() = runApplicationTest { - var flag by mutableStateOf(false) - val events = mutableListOf() - launchTestApplication { - Window(onCloseRequest = {}) { - LaunchedEffect(flag) { - if (flag) { - launch { - launch { - launch { - launch { - launch { - launch { - events.add("effect") - } - } - } - } - } - } - } - } - - Canvas(Modifier.size(100.dp)) { - if (flag) { - events.add("draw") - } - } - } - } - awaitIdle() - - flag = true - awaitIdle() - assertEquals(listOf("effect", "draw"), events) - } - - // Test that effects are run before synthetic events are delivered. - // This is important because the effects may be registering to receive the events. For example - // when a new element appears under the mouse pointer and registers to mouse-enter events. - @Test - fun effectsRunBeforeSyntheticEvents() = runComposeUiTest { - var flag by mutableStateOf(false) - val events = mutableListOf() - - setContent { - Box( - modifier = Modifier - .size(100.dp) - .testTag("container"), - contentAlignment = Alignment.Center - ) { - if (flag) { - Box( - modifier = Modifier - .size(50.dp) - .onPointerEvent(eventType = PointerEventType.Enter) { - events.add("mouse-enter") - } - ) - - LaunchedEffect(Unit) { - events.add("effect") - } - } - } - } - - onNodeWithTag("container").performMouseInput { - moveTo(Offset(50f, 50f)) - } - flag = true - waitForIdle() - - assertEquals(listOf("effect", "mouse-enter"), events) - } - - // Test that synthetic events are dispatched before the drawing phase. - @Test - fun syntheticEventsDispatchedBeforeDraw() = runComposeUiTest { - var flag by mutableStateOf(false) - val events = mutableListOf() - - setContent { - Box( - modifier = Modifier - .size(100.dp) - .testTag("container"), - contentAlignment = Alignment.Center - ) { - if (flag) { - Box( - modifier = Modifier - .size(50.dp) - .onPointerEvent(eventType = PointerEventType.Enter) { - events.add("mouse-enter") - } - ) - - Canvas(Modifier.size(100.dp)) { - if (flag) { - events.add("draw") - } - } - } - } - } - - onNodeWithTag("container").performMouseInput { - moveTo(Offset(50f, 50f)) - } - flag = true - waitForIdle() - - assertEquals(listOf("mouse-enter", "draw"), events) - } } private object PressTestIndication : Indication { diff --git a/compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/platform/RenderingTestScope.kt b/compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/platform/RenderingTestScope.kt index 830bf19cc619a..1b517e6797b3a 100644 --- a/compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/platform/RenderingTestScope.kt +++ b/compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/platform/RenderingTestScope.kt @@ -17,19 +17,20 @@ package androidx.compose.ui.platform import androidx.compose.runtime.Composable -import androidx.compose.ui.ComposeScene import androidx.compose.ui.graphics.Color +import androidx.compose.ui.graphics.asComposeCanvas +import androidx.compose.ui.graphics.nativeCanvas import androidx.compose.ui.graphics.toArgb -import androidx.compose.ui.unit.Constraints +import androidx.compose.ui.scene.MultiLayerComposeScene import androidx.compose.ui.unit.Density +import androidx.compose.ui.unit.IntSize +import kotlin.coroutines.CoroutineContext import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.runBlocking import kotlinx.coroutines.yield -import org.jetbrains.skia.Canvas import org.jetbrains.skia.Surface import org.jetbrains.skiko.FrameDispatcher import org.jetbrains.skiko.MainUIDispatcher -import kotlin.coroutines.CoroutineContext internal fun renderingTest( width: Int, @@ -57,12 +58,12 @@ internal class RenderingTestScope( } val surface: Surface = Surface.makeRasterN32Premul(width, height) - private val canvas: Canvas = surface.canvas - val scene = ComposeScene( + private val canvas = surface.canvas.asComposeCanvas() + val scene = MultiLayerComposeScene( coroutineContext = coroutineContext, invalidate = frameDispatcher::scheduleFrame ).apply { - constraints = Constraints(maxWidth = width, maxHeight = height) + size = IntSize(width = width, height = height) } var density: Float @@ -85,7 +86,7 @@ internal class RenderingTestScope( } private fun onRender(timeNanos: Long) { - canvas.clear(Color.Transparent.toArgb()) + canvas.nativeCanvas.clear(Color.Transparent.toArgb()) scene.render(canvas, timeNanos) onRender.complete(Unit) } diff --git a/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/FlushCoroutineDispatcher.skiko.kt b/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/FlushCoroutineDispatcher.skiko.kt index 2ab5c01d8f439..02880d88a9437 100644 --- a/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/FlushCoroutineDispatcher.skiko.kt +++ b/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/FlushCoroutineDispatcher.skiko.kt @@ -40,7 +40,7 @@ internal class FlushCoroutineDispatcher( ) : CoroutineDispatcher(), Delay { // Dispatcher should always be alive, even if Job is cancelled. Otherwise coroutines which // use this dispatcher won't be properly cancelled. - // TODO replace it by scope.coroutineContext[Dispatcher] when it will be no longer experimental + // TODO replace it by scope.coroutineContext[CoroutineDispatcher] when it will be no longer experimental private val scope = CoroutineScope(scope.coroutineContext.minusKey(Job)) private val tasks = mutableSetOf() private val delayedTasks = mutableSetOf() diff --git a/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/BaseComposeScene.skiko.kt b/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/BaseComposeScene.skiko.kt index 0b1d401fda4a0..15637bbb3443b 100644 --- a/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/BaseComposeScene.skiko.kt +++ b/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/BaseComposeScene.skiko.kt @@ -63,6 +63,9 @@ internal abstract class BaseComposeScene( processKeyEvent = ::processKeyEvent ) + // Store this to avoid creating a lambda every frame + private val updatePointerPosition = inputHandler::updatePointerPosition + private val frameClock = BroadcastFrameClock(onNewAwaiters = ::invalidateIfNeeded) private val recomposer: ComposeSceneRecomposer = ComposeSceneRecomposer(coroutineContext, frameClock) @@ -95,8 +98,7 @@ internal abstract class BaseComposeScene( private var hasPendingDraws = true protected fun invalidateIfNeeded() { hasPendingDraws = frameClock.hasAwaiters || - snapshotInvalidationTracker.hasInvalidations || - inputHandler.hasInvalidations + snapshotInvalidationTracker.hasInvalidations if (hasPendingDraws && !isInvalidationDisabled && !isClosed && composition != null) { invalidate() } @@ -133,7 +135,7 @@ internal abstract class BaseComposeScene( * It's required before setting content to apply changed parameters * before first recomposition. Otherwise, it can lead to double recomposition. */ - recomposer.performScheduledTasks() + recomposer.performScheduledRecomposerTasks() composition?.dispose() composition = createComposition { @@ -143,31 +145,30 @@ internal abstract class BaseComposeScene( ) } - recomposer.performScheduledTasks() + recomposer.performScheduledRecomposerTasks() } override fun render(canvas: Canvas, nanoTime: Long) = postponeInvalidation("BaseComposeScene:render") { - // Note that on Android the order is slightly different: - // - Recomposition - // - Layout - // - Draw - // - Composition effects - // - Synthetic events - // We do this differently in order to be able to observe changes made by synthetic events - // in the drawing phase, thus reducing the time before they are visible on screen. - // - // It is important, however, to run the composition effects before the synthetic events are - // dispatched, in order to allow registering for these events before they are sent. - // Otherwise, events like a synthetic mouse-enter sent due to a new element appearing under - // the pointer would be missed by e.g. InteractionSource.collectHoverAsState - recomposer.performScheduledTasks() - frameClock.sendFrame(nanoTime) // Recomposition - doLayout() // Layout - recomposer.performScheduledEffects() // Composition effects (e.g. LaunchedEffect) - inputHandler.updatePointerPosition() // Synthetic move event + // We try to run the phases here in the same order Android does. + + // Flush composition effects (e.g. LaunchedEffect, coroutines launched in + // rememberCoroutineScope()) before everything else + recomposer.performScheduledEffects() + + recomposer.performScheduledRecomposerTasks() + frameClock.sendFrame(nanoTime) // withFrameMillis/Nanos and recomposition + + doLayout() // Layout + + // Schedule synthetic events to be sent after `render` completes + if (inputHandler.needUpdatePointerPosition) { + recomposer.scheduleAsEffect(updatePointerPosition) + } + + // Draw snapshotInvalidationTracker.onDraw() - draw(canvas) // Draw + draw(canvas) } override fun sendPointerEvent( diff --git a/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/ComposeSceneInputHandler.skiko.kt b/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/ComposeSceneInputHandler.skiko.kt index a89460c1f680a..d8063dbccdddc 100644 --- a/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/ComposeSceneInputHandler.skiko.kt +++ b/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/ComposeSceneInputHandler.skiko.kt @@ -64,10 +64,9 @@ internal class ComposeSceneInputHandler( get() = pointerPositions.values.firstOrNull() /** - * Indicates if there were invalidations and triggering [BaseComposeScene.measureAndLayout] - * is now required. + * Indicates whether [updatePointerPosition] needs to be called. */ - val hasInvalidations: Boolean + val needUpdatePointerPosition: Boolean get() = syntheticEventSender.needUpdatePointerPosition fun onPointerEvent( diff --git a/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/ComposeSceneRecomposer.skiko.kt b/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/ComposeSceneRecomposer.skiko.kt index 5aa082f683c5d..b026e3305b777 100644 --- a/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/ComposeSceneRecomposer.skiko.kt +++ b/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/ComposeSceneRecomposer.skiko.kt @@ -25,6 +25,7 @@ import kotlin.coroutines.CoroutineContext import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.CoroutineStart import kotlinx.coroutines.Job +import kotlinx.coroutines.Runnable import kotlinx.coroutines.launch /** @@ -44,8 +45,8 @@ internal class ComposeSceneRecomposer( private val coroutineScope = CoroutineScope(coroutineContext + job) /** - * We use [FlushCoroutineDispatcher] not because we need [flush] for - * LaunchEffect tasks, but because we need to know if it is idle (hasn't scheduled tasks) + * We use [FlushCoroutineDispatcher] not only because we need [FlushCoroutineDispatcher.flush] + * for LaunchEffect tasks, but also to know whether it is idle (has no scheduled tasks) */ private val effectDispatcher = FlushCoroutineDispatcher(coroutineScope) private val recomposeDispatcher = FlushCoroutineDispatcher(coroutineScope) @@ -75,12 +76,13 @@ internal class ComposeSceneRecomposer( } /** - * Perform all scheduled tasks and wait for the tasks which are already + * Perform all scheduled recomposer tasks and wait for the tasks which are already * performing in the recomposition scope. */ - fun performScheduledTasks() = trace("ComposeSceneRecomposer:performScheduledTasks") { - recomposeDispatcher.flush() - } + fun performScheduledRecomposerTasks() = + trace("ComposeSceneRecomposer:performScheduledRecomposerTasks") { + recomposeDispatcher.flush() + } /** * Perform all scheduled effects. @@ -90,7 +92,14 @@ internal class ComposeSceneRecomposer( } /** - * Permanently shut down this [Recomposer] for future use. + * Schedules the given [block] to run with the effects dispatcher. + */ + fun scheduleAsEffect(block: () -> Unit) { + effectDispatcher.dispatch(job, Runnable(block)) + } + + /** + * Permanently shut down this [ComposeSceneRecomposer] for future use. * * @see Recomposer.cancel */ diff --git a/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/FlushCoroutineDispatcherTest.kt b/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/FlushCoroutineDispatcherTest.kt index 09c12da85b39a..63eff8b1fbd85 100644 --- a/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/FlushCoroutineDispatcherTest.kt +++ b/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/FlushCoroutineDispatcherTest.kt @@ -17,14 +17,12 @@ package androidx.compose.ui.platform import kotlinx.coroutines.test.runTest -import kotlin.random.Random import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.assertTrue import kotlinx.coroutines.* -@OptIn(ExperimentalCoroutinesApi::class) class FlushCoroutineDispatcherTest { @Test diff --git a/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/RenderPhasesTest.kt b/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/RenderPhasesTest.kt new file mode 100644 index 0000000000000..64d2036ba9bbf --- /dev/null +++ b/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/RenderPhasesTest.kt @@ -0,0 +1,216 @@ +/* + * Copyright 2024 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package androidx.compose.ui.platform + +import androidx.compose.foundation.Canvas +import androidx.compose.foundation.layout.Box +import androidx.compose.foundation.layout.fillMaxSize +import androidx.compose.foundation.layout.size +import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.rememberCoroutineScope +import androidx.compose.runtime.setValue +import androidx.compose.runtime.withFrameMillis +import androidx.compose.runtime.withFrameNanos +import androidx.compose.ui.Modifier +import androidx.compose.ui.geometry.Offset +import androidx.compose.ui.input.pointer.PointerEventType +import androidx.compose.ui.input.pointer.onPointerEvent +import androidx.compose.ui.layout.Layout +import androidx.compose.ui.scene.BaseComposeScene +import androidx.compose.ui.test.ExperimentalTestApi +import androidx.compose.ui.test.InternalTestApi +import androidx.compose.ui.test.onNodeWithTag +import androidx.compose.ui.test.performMouseInput +import androidx.compose.ui.test.runInternalSkikoComposeUiTest +import androidx.compose.ui.unit.dp +import kotlin.test.Test +import kotlin.test.assertContentEquals +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.StandardTestDispatcher + +/** + * Tests the ordering of the phases of the [BaseComposeScene.render] loop. + */ +@OptIn(ExperimentalTestApi::class, InternalTestApi::class) +class RenderPhasesTest { + /** + * Test the order of the basic phases: + * 1. withFrameMillis/Nanos + * 2. Composition + * 3. Layout + * 4. Draw + * 5. Effects launched in the composition. + */ + @Test + fun testBasicOrder() = runInternalSkikoComposeUiTest( + coroutineDispatcher = StandardTestDispatcher() + ) { + mainClock.autoAdvance = false + + val phases = mutableListOf() + var logPhases by mutableStateOf(false) + + setContent { + LaunchedEffect(Unit) { + var stop = false + while(!stop) { + withFrameNanos { + if (logPhases) { + phases.add("frame") + stop = true + } + } + } + } + + if (logPhases) { + phases.add("composition") + } + + Layout( + modifier = Modifier.size(100.dp), + measurePolicy = { _, _ -> + if (logPhases) { + phases.add("layout") + } + + layout(100, 100) { } + } + ) + + Canvas(Modifier.size(100.dp)) { + if (logPhases) { + phases.add("draw") + } + } + + LaunchedEffect(logPhases) { + if (logPhases) { + phases.add("effects") + } + } + } + + assertContentEquals(emptyList(), phases) + logPhases = true + mainClock.advanceTimeByFrame() + waitForIdle() // Make the effects run + assertContentEquals( + expected = listOf("frame", "composition", "layout", "draw", "effects"), + actual = phases + ) + } + + /** + * Test that synthetic events are delivered after effects have run. + */ + @Test + fun syntheticEventsDispatchedAfterEffects() = runInternalSkikoComposeUiTest( + coroutineDispatcher = StandardTestDispatcher() + ) { + val phases = mutableListOf() + var showNewBox by mutableStateOf(false) + + setContent { + Box(Modifier.size(100.dp).testTag("box")) { + if (showNewBox) { + Box( + Modifier + .fillMaxSize() + .onPointerEvent(eventType = PointerEventType.Enter) { + phases.add("syntheticEvent") + } + ) + LaunchedEffect(Unit) { + phases.add("effects") + } + } + } + } + + assertContentEquals(emptyList(), phases) + onNodeWithTag("box").performMouseInput { + moveTo(Offset(50f, 50f)) + } + showNewBox = true + waitForIdle() + assertContentEquals( + expected = listOf("effects", "syntheticEvent"), + actual = phases + ) + } + + /** + * Test that coroutines launched in a composition's [rememberCoroutineScope] execute in the + * correct order, and only after the draw phase. + */ + @Test + fun coroutinesDispatchedAfterDraw() = runInternalSkikoComposeUiTest( + coroutineDispatcher = StandardTestDispatcher() + ) { + mainClock.autoAdvance = false + + val phases = mutableListOf() + var startTest by mutableStateOf(false) + setContent { + val coroutineScope = rememberCoroutineScope() + + LaunchedEffect(Unit) { + while(true) { + withFrameMillis { + if (startTest) { + coroutineScope.launch { + phases.add("launchedWithFrame") + } + } + } + } + } + + if (startTest) { + coroutineScope.launch { + phases.add("launchedFromComposition") + } + } + + LaunchedEffect(startTest) { + if (startTest) { + coroutineScope.launch { + phases.add("launchedFromEffect") + } + } + } + + Canvas(Modifier.size(100.dp)) { + if (startTest) { + phases.add("draw") + } + } + } + + assertContentEquals(emptyList(), phases) + startTest = true + mainClock.advanceTimeByFrame() + waitForIdle() // Make the effects run + assertContentEquals( + expected = listOf("draw", "launchedWithFrame", "launchedFromComposition", "launchedFromEffect"), + actual = phases + ) + } +} \ No newline at end of file