Skip to content

Commit

Permalink
Invalidate parent layer every time child layer is invalidated (#1316)
Browse files Browse the repository at this point in the history
Fixes JetBrains/compose-multiplatform#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
#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
  • Loading branch information
igordmn committed Apr 25, 2024
1 parent a3b1dd2 commit fa363b3
Show file tree
Hide file tree
Showing 3 changed files with 132 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -231,8 +231,8 @@ internal class RenderNodeLayer(
if (!isDestroyed && picture != null) {
picture?.close()
picture = null
invalidateParentLayer()
}
invalidateParentLayer()
}

override fun drawLayer(canvas: Canvas) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,18 @@
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
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertFalse
import kotlin.test.assertTrue
import org.jetbrains.skia.Surface

class RenderNodeLayerTest {

Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit fa363b3

Please sign in to comment.