From fa363b3a15dde6d07175bbfdc5245c8c418fe35d Mon Sep 17 00:00:00 2001 From: Igor Demin Date: Thu, 25 Apr 2024 03:12:07 +0200 Subject: [PATCH] Invalidate parent layer every time child layer is invalidated (#1316) Fixes https://github.com/JetBrains/compose-multiplatform/issues/4681 The issue was because of this situation: - the parent RenderNodeLayer is drawn and cached it's `picture` - the child RenderNodeLayer isn't drawn, but `invalidate` is called `invalidate` should have updated the parent layer (removing the `picture` cache), but didn't do that as we do that conditionally. Now `invalidate` invalidates the parent layer unconditionally. It is a regression after https://github.com/JetBrains/compose-multiplatform-core/pull/1260, but it just revealed the bug, didn't introduce it. ## Testing - new added tests fail before, work after the fix - the issue is no longer reproducible This should be tested by QA ## Release Notes > Fixes - Multiple Platforms - _(prerelease fix)_ Fix frozen composition with pager and text field --- .../ui/platform/RenderNodeLayer.skiko.kt | 2 +- .../ui/platform/GraphicLayerBugTest.kt | 80 +++++++++++++++++++ .../ui/platform/RenderNodeLayerTest.kt | 54 ++++++++++++- 3 files changed, 132 insertions(+), 4 deletions(-) create mode 100644 compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/GraphicLayerBugTest.kt diff --git a/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/RenderNodeLayer.skiko.kt b/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/RenderNodeLayer.skiko.kt index e676e3e04b3bb..428567b1f8a21 100644 --- a/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/RenderNodeLayer.skiko.kt +++ b/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/RenderNodeLayer.skiko.kt @@ -231,8 +231,8 @@ internal class RenderNodeLayer( if (!isDestroyed && picture != null) { picture?.close() picture = null - invalidateParentLayer() } + invalidateParentLayer() } override fun drawLayer(canvas: Canvas) { diff --git a/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/GraphicLayerBugTest.kt b/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/GraphicLayerBugTest.kt new file mode 100644 index 0000000000000..431ce42f5e298 --- /dev/null +++ b/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/GraphicLayerBugTest.kt @@ -0,0 +1,80 @@ +/* + * 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.ExperimentalFoundationApi +import androidx.compose.foundation.layout.Spacer +import androidx.compose.foundation.layout.fillMaxSize +import androidx.compose.foundation.layout.size +import androidx.compose.foundation.pager.HorizontalPager +import androidx.compose.foundation.pager.rememberPagerState +import androidx.compose.material.Scaffold +import androidx.compose.material.TextField +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue +import androidx.compose.ui.Modifier +import androidx.compose.ui.assertThat +import androidx.compose.ui.draw.drawBehind +import androidx.compose.ui.isEqualTo +import androidx.compose.ui.test.ExperimentalTestApi +import androidx.compose.ui.test.runComposeUiTest +import androidx.compose.ui.unit.dp +import kotlin.test.Test + +// Tests for fixed bugs +class GraphicLayerBugTest { + // https://github.com/JetBrains/compose-multiplatform/issues/4681 + @OptIn(ExperimentalTestApi::class, ExperimentalFoundationApi::class) + @Test + fun draw_invalidates_inside_complex_layout() = + runComposeUiTest { + var valueForDraw by mutableStateOf(0f) + var lastDrawnValue = -1f + + setContent { + val pagerState = rememberPagerState { 1 } + Scaffold( + modifier = Modifier.fillMaxSize(), + topBar = { + Spacer(Modifier.drawBehind { pagerState.targetPage }) + }, + ) { + HorizontalPager(modifier = Modifier.fillMaxSize(), state = pagerState) { + var value by remember { mutableStateOf("") } + TextField( + value = value, + onValueChange = { value = it }, + ) + + Canvas(Modifier.size(40.dp)) { + lastDrawnValue = valueForDraw + } + } + } + } + + waitForIdle() + assertThat(lastDrawnValue).isEqualTo(0f) + + valueForDraw = 1f + waitForIdle() + assertThat(lastDrawnValue).isEqualTo(1f) + } +} diff --git a/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/RenderNodeLayerTest.kt b/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/RenderNodeLayerTest.kt index a106c17d0a02d..a4afaa0b90678 100644 --- a/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/RenderNodeLayerTest.kt +++ b/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/RenderNodeLayerTest.kt @@ -17,8 +17,10 @@ package androidx.compose.ui.platform import androidx.compose.foundation.shape.CircleShape +import androidx.compose.ui.assertThat import androidx.compose.ui.geometry.Offset import androidx.compose.ui.graphics.* +import androidx.compose.ui.isEqualTo import androidx.compose.ui.unit.* import kotlin.math.PI import kotlin.math.cos @@ -26,6 +28,7 @@ import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.assertTrue +import org.jetbrains.skia.Surface class RenderNodeLayerTest { @@ -464,11 +467,56 @@ class RenderNodeLayerTest { assertTrue(layer.isInLayer(Offset(50f, 100f))) } - private fun TestRenderNodeLayer() = RenderNodeLayer( + @Test + fun invalidate_parent_layer() { + var parentDrawCount = 0 + + var childLayer: RenderNodeLayer? = null + val parentLayer = TestRenderNodeLayer( + drawBlock = { + parentDrawCount++ + childLayer?.drawLayer(it) + }, + ) + + val surface = Surface.makeNull(10, 10) + val canvas = surface.canvas.asComposeCanvas() + + assertThat(parentDrawCount).isEqualTo(0) + + parentLayer.drawLayer(canvas) + assertThat(parentDrawCount).isEqualTo(1) + + // shouldn't be drawn again, as it isn't changed + parentLayer.drawLayer(canvas) + assertThat(parentDrawCount).isEqualTo(1) + + // https://github.com/JetBrains/compose-multiplatform/issues/4681 + // parent should be drawn again, as we add a child + childLayer = TestRenderNodeLayer( + invalidateParentLayer = parentLayer::invalidate, + drawBlock = {}, + ) + childLayer.invalidate() + parentLayer.drawLayer(canvas) + assertThat(parentDrawCount).isEqualTo(2) + + parentLayer.drawLayer(canvas) + assertThat(parentDrawCount).isEqualTo(2) + + childLayer.invalidate() + parentLayer.drawLayer(canvas) + assertThat(parentDrawCount).isEqualTo(3) + } + + private fun TestRenderNodeLayer( + invalidateParentLayer: () -> Unit = {}, + drawBlock: (Canvas) -> Unit = {}, + ) = RenderNodeLayer( Density(1f, 1f), measureDrawBounds = false, - invalidateParentLayer = {}, - drawBlock = {} + invalidateParentLayer = invalidateParentLayer, + drawBlock = drawBlock, ) private fun assertMapping(from: Offset, to: Offset) {