Avoid calling Window.setVisible(true) on every AwtWindow recomposit…

…ion.

compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/window/AwtWindow.desktop.kt

@@ -18,15 +18,13 @@ package androidx.compose.ui.window
 
 import androidx.compose.runtime.Composable
 import androidx.compose.runtime.DisposableEffect
-import androidx.compose.runtime.SideEffect
 import androidx.compose.runtime.getValue
 import androidx.compose.runtime.remember
 import androidx.compose.runtime.rememberUpdatedState
 import androidx.compose.ui.node.Ref
 import androidx.compose.ui.util.UpdateEffect
 import kotlinx.coroutines.DelicateCoroutinesApi
 import kotlinx.coroutines.GlobalScope
-import kotlinx.coroutines.Job
 import kotlinx.coroutines.launch
 import org.jetbrains.skiko.MainUIDispatcher
 import java.awt.Window
@@ -41,8 +39,8 @@ import java.awt.Window
  *
  * The [update] block can be run multiple times (on the UI thread as well) due to recomposition,
  * and it is the right place to set [Window] properties depending on state.
- * When state changes, the block will be reexecuted to set the new properties.
- * Note the block will also be ran once right after the [create] block completes.
+ * When state changes, the block will be re-executed to set the new properties.
+ * Note the block will also be run once right after the [create] block completes.
  *
  * [AwtWindow] is needed for creating window's / dialog's that still can't be created with
  * the default Compose functions [androidx.compose.ui.window.Window] or
@@ -65,8 +63,6 @@ fun <T : Window> AwtWindow(
     dispose: (T) -> Unit,
     update: (T) -> Unit = {}
 ) {
-    val currentVisible by rememberUpdatedState(visible)
-
     val windowRef = remember { Ref<T>() }
     fun window() = windowRef.value!!
 
@@ -82,45 +78,38 @@ fun <T : Window> AwtWindow(
         update(window)
     }
 
-    val showJob = Ref<Job?>()
-
-    SideEffect {
+    DisposableEffect(visible) {
         // Why we dispatch showing in the next AWT tick:
         //
         // 1.
         // window.isVisible = true can be a blocking operation.
-        // So we have to schedule it when we will be outside of Compose render frame.
+        // So we have to schedule it outside the Compose render frame.
         //
-        // This happens in the when we create a modal dialog.
-        // When we call `window.isVisible = true`, internally will be created a new AWT event loop,
+        // This happens in when we create a modal dialog.
+        // When we call `window.isVisible = true`, internally a new AWT event loop will be created,
         // which will handle all the future Swing events while dialog is visible.
         //
-        // We can't use LaunchedEffect or rememberCoroutineScope, because they have a dispatcher
-        // which is controlled by the Compose rendering loop (ComposeScene.dispatcher) and we
-        // will block coroutine.
+        // We can't show the window directly inside LaunchedEffect or rememberCoroutineScope because
+        // their dispatcher is controlled by the Compose rendering loop (ComposeScene.dispatcher)
+        // and we will block the coroutine.
         //
         // 2.
-        // We achieve the correct order when we open nested
-        // window at the same time when we open the parent window. If we would show the window
-        // immediately we will have this sequence in case of nested Window's:
+        // We achieve the correct order when we open nested window at the same time when we open the
+        // parent window. If we had shown the window immediately we would have had this sequence in
+        // case of nested windows:
         //
         // 1. window1.setContent
         // 2. window2.setContent
         // 3. window2.isVisible = true
         // 4. window1.isVisible = true
         //
-        // So we will have a wrong active window (window1).
-
-        showJob.value?.cancel()
-        showJob.value = GlobalScope.launch(MainUIDispatcher) {
-            window().isVisible = currentVisible
+        // So we will have the wrong window active (window1).
+        val showJob = GlobalScope.launch(MainUIDispatcher) {
+            window().isVisible = visible
         }
-    }
 
-    DisposableEffect(Unit) {
         onDispose {
-            showJob.value?.cancel()
-            window().isVisible = false
+            showJob.cancel()
         }
     }
 }

compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/window/DialogWindowTest.kt

@@ -41,6 +41,8 @@ import androidx.compose.ui.input.key.onKeyEvent
 import androidx.compose.ui.input.key.onPreviewKeyEvent
 import androidx.compose.ui.platform.LocalLayoutDirection
 import androidx.compose.ui.sendKeyEvent
+import androidx.compose.ui.text.drawText
+import androidx.compose.ui.text.rememberTextMeasurer
 import androidx.compose.ui.toInt
 import androidx.compose.ui.unit.DpSize
 import androidx.compose.ui.unit.LayoutDirection
@@ -606,4 +608,38 @@ class DialogWindowTest {
         awaitIdle()
         assertThat(localLayoutDirection).isEqualTo(LayoutDirection.Ltr)
     }
+
+    @Test
+    fun `modal DialogWindow does not block parent window rendering`() {
+        runApplicationTest(useDelay = true) {
+            var text by mutableStateOf("1")
+            var renderedText: String? = null
+            lateinit var dialog: ComposeDialog
+
+            launchTestApplication {
+                Window(onCloseRequest = {}) {
+                    val textMeasurer = rememberTextMeasurer()
+                    Canvas(Modifier.size(200.dp)) {
+                        renderedText = text
+                        drawText(
+                            textMeasurer = textMeasurer,
+                            text = text
+                        )
+                    }
+
+                    DialogWindow(onCloseRequest = {}) {
+                        dialog = window
+                    }
+                }
+            }
+
+            awaitIdle()
+            assertThat(dialog.isModal).isTrue()
+            assertThat(renderedText).isEqualTo("1")
+
+            text = "2"
+            awaitIdle()
+            assertThat(renderedText).isEqualTo("2")
+        }
+    }
 }

compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/window/window/WindowTest.kt

@@ -47,6 +47,7 @@ import androidx.compose.ui.window.runApplicationTest
 import com.google.common.truth.Truth.assertThat
 import java.awt.Dimension
 import java.awt.GraphicsEnvironment
+import java.awt.SystemColor.window
 import java.awt.Toolkit
 import java.awt.event.WindowAdapter
 import java.awt.event.WindowEvent
@@ -696,4 +697,44 @@ class WindowTest {
         assertThat(windowLayoutDirectionResult).isEqualTo(LayoutDirection.Ltr)
         assertThat(popupLayoutDirectionResult).isEqualTo(LayoutDirection.Rtl)
     }
+
+    @Test
+    fun `window does not move to front on recomposition`() = runApplicationTest {
+        var window1: ComposeWindow? = null
+        var window2: ComposeWindow? = null
+
+        var window1Title by mutableStateOf("Window 1")
+
+        launchTestApplication {
+            Window(
+                onCloseRequest = ::exitApplication,
+                title = window1Title,
+            ) {
+                window1 = this.window
+                Box(Modifier.size(32.dp))
+            }
+
+            Window(
+                onCloseRequest = ::exitApplication,
+                title = "Window 2"
+            ) {
+                window2 = this.window
+                Box(Modifier.size(32.dp))
+                LaunchedEffect(Unit) {
+                    window.toFront()
+                }
+            }
+        }
+
+        awaitIdle()
+        assertThat(window1?.isShowing).isTrue()
+        assertThat(window2?.isShowing).isTrue()
+        assertThat(window1?.isActive).isFalse()
+        assertThat(window2?.isActive).isTrue()
+
+        window1Title = "Retitled Window"
+        awaitIdle()
+        assertThat(window1?.isActive).isFalse()
+        assertThat(window2?.isActive).isTrue()
+    }
 }