From 3ee72261947011130305a5838be6d42c0eaf520c Mon Sep 17 00:00:00 2001 From: Alexander Maryanovsky Date: Mon, 13 May 2024 17:38:42 +0300 Subject: [PATCH] Fix RootNodeOwner.OwnerImpl.onRequestRelayout to only schedule layout, not measure (#1355) --- .../compose/ui/node/RootNodeOwner.skiko.kt | 18 +++-- .../node/SnapshotInvalidationTracker.skiko.kt | 12 +-- .../ui/scene/BaseComposeScene.skiko.kt | 8 +- .../compose/ui/platform/RenderPhasesTest.kt | 77 +++++++++++++++++++ 4 files changed, 100 insertions(+), 15 deletions(-) diff --git a/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/node/RootNodeOwner.skiko.kt b/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/node/RootNodeOwner.skiko.kt index fb06553c89a17..fd53047b13f3a 100644 --- a/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/node/RootNodeOwner.skiko.kt +++ b/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/node/RootNodeOwner.skiko.kt @@ -338,12 +338,12 @@ internal class RootNodeOwner( if (measureAndLayoutDelegate.requestLookaheadRemeasure(layoutNode, forceRequest) && scheduleMeasureAndLayout ) { - snapshotInvalidationTracker.requestLayout() + snapshotInvalidationTracker.requestMeasureAndLayout() } } else if (measureAndLayoutDelegate.requestRemeasure(layoutNode, forceRequest) && scheduleMeasureAndLayout ) { - snapshotInvalidationTracker.requestLayout() + snapshotInvalidationTracker.requestMeasureAndLayout() } } @@ -352,12 +352,20 @@ internal class RootNodeOwner( affectsLookahead: Boolean, forceRequest: Boolean ) { - this.onRequestMeasure(layoutNode, affectsLookahead, forceRequest, scheduleMeasureAndLayout = true) + if (affectsLookahead) { + if (measureAndLayoutDelegate.requestLookaheadRelayout(layoutNode, forceRequest)) { + snapshotInvalidationTracker.requestMeasureAndLayout() + } + } else { + if (measureAndLayoutDelegate.requestRelayout(layoutNode, forceRequest)) { + snapshotInvalidationTracker.requestMeasureAndLayout() + } + } } override fun requestOnPositionedCallback(layoutNode: LayoutNode) { measureAndLayoutDelegate.requestOnPositionedCallback(layoutNode) - snapshotInvalidationTracker.requestLayout() + snapshotInvalidationTracker.requestMeasureAndLayout() } override fun createLayer( @@ -434,7 +442,7 @@ internal class RootNodeOwner( override fun registerOnLayoutCompletedListener(listener: Owner.OnLayoutCompletedListener) { measureAndLayoutDelegate.registerOnLayoutCompletedListener(listener) - snapshotInvalidationTracker.requestLayout() + snapshotInvalidationTracker.requestMeasureAndLayout() } } diff --git a/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/node/SnapshotInvalidationTracker.skiko.kt b/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/node/SnapshotInvalidationTracker.skiko.kt index 275a8360acfb8..52a793264ef88 100644 --- a/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/node/SnapshotInvalidationTracker.skiko.kt +++ b/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/node/SnapshotInvalidationTracker.skiko.kt @@ -32,7 +32,7 @@ internal class SnapshotInvalidationTracker( private val invalidate: () -> Unit = {} ) { private val snapshotChanges = CommandList(invalidate) - private var needLayout = true + private var needMeasureAndLayout = true private var needDraw = true /** @@ -43,15 +43,15 @@ internal class SnapshotInvalidationTracker( private var renderingThreadId: Long? by atomic(null) val hasInvalidations: Boolean - get() = needLayout || needDraw || snapshotChanges.hasCommands + get() = needMeasureAndLayout || needDraw || snapshotChanges.hasCommands - fun requestLayout() { - needLayout = true + fun requestMeasureAndLayout() { + needMeasureAndLayout = true invalidate() } - fun onLayout() { - needLayout = false + fun onMeasureAndLayout() { + needMeasureAndLayout = false } fun requestDraw() { 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 15637bbb3443b..960538b383fe2 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 @@ -58,7 +58,7 @@ internal abstract class BaseComposeScene( protected val snapshotInvalidationTracker = SnapshotInvalidationTracker(::invalidateIfNeeded) protected val inputHandler: ComposeSceneInputHandler = ComposeSceneInputHandler( - prepareForPointerInputEvent = ::doLayout, + prepareForPointerInputEvent = ::doMeasureAndLayout, processPointerInputEvent = ::processPointerInputEvent, processKeyEvent = ::processKeyEvent ) @@ -159,7 +159,7 @@ internal abstract class BaseComposeScene( recomposer.performScheduledRecomposerTasks() frameClock.sendFrame(nanoTime) // withFrameMillis/Nanos and recomposition - doLayout() // Layout + doMeasureAndLayout() // Layout // Schedule synthetic events to be sent after `render` completes if (inputHandler.needUpdatePointerPosition) { @@ -223,8 +223,8 @@ internal abstract class BaseComposeScene( inputHandler.onKeyEvent(keyEvent) } - private fun doLayout() { - snapshotInvalidationTracker.onLayout() + private fun doMeasureAndLayout() { + snapshotInvalidationTracker.onMeasureAndLayout() measureAndLayout() } 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 index 64d2036ba9bbf..23aecc4c3c37e 100644 --- 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 @@ -23,8 +23,10 @@ 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.neverEqualPolicy import androidx.compose.runtime.rememberCoroutineScope import androidx.compose.runtime.setValue +import androidx.compose.runtime.snapshots.Snapshot import androidx.compose.runtime.withFrameMillis import androidx.compose.runtime.withFrameNanos import androidx.compose.ui.Modifier @@ -41,6 +43,8 @@ import androidx.compose.ui.test.runInternalSkikoComposeUiTest import androidx.compose.ui.unit.dp import kotlin.test.Test import kotlin.test.assertContentEquals +import kotlin.test.assertEquals +import kotlin.test.assertTrue import kotlinx.coroutines.launch import kotlinx.coroutines.test.StandardTestDispatcher @@ -213,4 +217,77 @@ class RenderPhasesTest { actual = phases ) } + + @Test + fun layoutScopeInvalidationDoesNotCauseRemeasure() = runInternalSkikoComposeUiTest( + coroutineDispatcher = StandardTestDispatcher() + ) { + var measureCount = 0 + var layoutCount = 0 + + var layoutScopeInvalidation by mutableStateOf(Unit, neverEqualPolicy()) + + setContent { + Layout( + measurePolicy = { _, _ -> + measureCount += 1 + layout(100, 100) { + @Suppress("UNUSED_EXPRESSION") + layoutScopeInvalidation + layoutCount += 1 + } + } + ) + } + + assertEquals(measureCount, layoutCount) + + measureCount = 0 + layoutCount = 0 + layoutScopeInvalidation = Unit + waitForIdle() + + assertEquals(0, measureCount) + assertTrue(layoutCount > 0) + } + + @Test + fun readingStateInLayoutModifiedByMeasureDoesNotCauseInfiniteRemeasureAndLayout() { + // https://github.com/JetBrains/compose-multiplatform/issues/4760 + runInternalSkikoComposeUiTest(coroutineDispatcher = StandardTestDispatcher()) { + mainClock.autoAdvance = false + val state = mutableStateOf(0) + // Don't read the state initially so that the test fails rather than getting stuck + var readStateInLayout by mutableStateOf(false) + var layoutCount = 0 + setContent { + Layout( + measurePolicy = { _, _ -> + val prevValue = Snapshot.withoutReadObservation { + state.value + } + state.value = prevValue+1 + layout(100, 100) { + if (readStateInLayout) { + state.value // Read the state value! + } + layoutCount++ + } + }, + ) + } + + mainClock.advanceTimeByFrame() + assertEquals(1, state.value) + + readStateInLayout = true + layoutCount = 0 + mainClock.advanceTimeByFrame() + assertEquals(1, layoutCount) + + // Check that no more layout happens + mainClock.advanceTimeByFrame() + assertEquals(1, layoutCount) + } + } } \ No newline at end of file