From 183f549a2a9f2a8e9541c98e8ce86b29f532e53a Mon Sep 17 00:00:00 2001
From: Ivan Matkov <ivan.matkov@jetbrains.com>
Date: Thu, 12 Sep 2024 18:36:10 +0200
Subject: [PATCH] Remove invalidation trigger in `GraphicsLayer.record` (#1555)

Fixes [CMP-6660](https://youtrack.jetbrains.com/issue/CMP-6660)

## Testing
`GraphicsLayerTest`

## Release Notes
### Fixes - Multiple Platforms
- _(prerelease fix)_ Fix possible infinity invalidation loop triggered
by `GraphicsLayer.record`
---
 .../graphics/layer/SkiaGraphicsLayer.skiko.kt | 17 +++--
 .../compose/ui/graphics/GraphicsLayerTest.kt  | 67 +++++++++++++++++++
 2 files changed, 79 insertions(+), 5 deletions(-)
 create mode 100644 compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/graphics/GraphicsLayerTest.kt

diff --git a/compose/ui/ui-graphics/src/skikoMain/kotlin/androidx/compose/ui/graphics/layer/SkiaGraphicsLayer.skiko.kt b/compose/ui/ui-graphics/src/skikoMain/kotlin/androidx/compose/ui/graphics/layer/SkiaGraphicsLayer.skiko.kt
index bd9a2356dfae8..9e3d94b099975 100644
--- a/compose/ui/ui-graphics/src/skikoMain/kotlin/androidx/compose/ui/graphics/layer/SkiaGraphicsLayer.skiko.kt
+++ b/compose/ui/ui-graphics/src/skikoMain/kotlin/androidx/compose/ui/graphics/layer/SkiaGraphicsLayer.skiko.kt
@@ -156,18 +156,20 @@ actual class GraphicsLayer internal constructor() {
 
     private var density: Density = Density(1f)
 
-    private fun invalidateMatrix() {
+    private fun invalidateMatrix(requestDraw: Boolean = true) {
         matrixDirty = true
-        requestDraw()
+        if (requestDraw) {
+            requestDraw()
+        }
     }
 
     private fun requestDraw() {
         drawState.value = Unit
     }
 
-    private fun updateLayerConfiguration() {
+    private fun updateLayerConfiguration(requestDraw: Boolean = true) {
         this.outlineDirty = true
-        invalidateMatrix()
+        invalidateMatrix(requestDraw)
     }
 
     actual fun record(
@@ -182,7 +184,12 @@ actual class GraphicsLayer internal constructor() {
 
         this.density = density
         this.size = size
-        updateLayerConfiguration()
+        updateLayerConfiguration(
+            // [record] doesn't change the state and should not explicitly request drawing
+            // (happens only on the next frame) to avoid infinity invalidation loop.
+            // It's designed to be handled externally.
+            requestDraw = false
+        )
         val bounds = size.toSize().toRect().toSkiaRect()
         val canvas = pictureRecorder.beginRecording(bounds)
         val skiaCanvas = canvas.asComposeCanvas() as SkiaBackedCanvas
diff --git a/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/graphics/GraphicsLayerTest.kt b/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/graphics/GraphicsLayerTest.kt
new file mode 100644
index 0000000000000..12e609d83df13
--- /dev/null
+++ b/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/graphics/GraphicsLayerTest.kt
@@ -0,0 +1,67 @@
+/*
+ * 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.graphics
+
+import androidx.compose.foundation.layout.Box
+import androidx.compose.foundation.layout.size
+import androidx.compose.runtime.DisposableEffect
+import androidx.compose.runtime.remember
+import androidx.compose.ui.Modifier
+import androidx.compose.ui.draw.drawBehind
+import androidx.compose.ui.draw.drawWithContent
+import androidx.compose.ui.graphics.layer.drawLayer
+import androidx.compose.ui.platform.LocalGraphicsContext
+import androidx.compose.ui.test.ExperimentalTestApi
+import androidx.compose.ui.test.runSkikoComposeUiTest
+import androidx.compose.ui.unit.dp
+import kotlin.test.Test
+import kotlin.test.assertEquals
+
+@ExperimentalTestApi
+class GraphicsLayerTest {
+
+    @Test // Bug: https://youtrack.jetbrains.com/issue/CMP-6660
+    fun recordInvalidation() = runSkikoComposeUiTest {
+        var backgroundCompositions = 0
+        var foregroundCompositions = 0
+        setContent {
+            val graphicsContext = LocalGraphicsContext.current
+            val graphicsLayer = remember { graphicsContext.createGraphicsLayer() }
+            Box(Modifier.size(100.dp).drawWithContent {
+                backgroundCompositions++
+                graphicsLayer.record {
+                    this@drawWithContent.drawContent()
+                }
+            })
+            Box(Modifier.size(50.dp).drawBehind {
+                foregroundCompositions++
+                drawLayer(graphicsLayer)
+            })
+            DisposableEffect(graphicsLayer) {
+                onDispose { graphicsContext.releaseGraphicsLayer(graphicsLayer) }
+            }
+        }
+        waitForIdle()
+
+        // UnconfinedTestDispatcher makes some difference here: instead of possible infinite
+        // invalidation cycle, it resolves invalidations during the same render.
+        // But even in this case, with incorrect behaviour composition counters won't be equal 1
+
+        assertEquals(1, backgroundCompositions)
+        assertEquals(1, foregroundCompositions)
+    }
+}