From fae88ea157bf3f2e15f091de830a6cd8898bcb3b Mon Sep 17 00:00:00 2001 From: Ray Ryan Date: Fri, 19 Jul 2024 18:31:17 -0700 Subject: [PATCH] DialogCollator destroys/rebuild instead of dismiss/show When `DialogCollator` has to reorder, it did so by calling `Dialog.dismiss()` and `Dialog.show()`. #1213 fixed a problem that caused for `ComposeView`, but that fix is undone by Compose 1.6.8. @vumngoc dug in to see why and discovered: > The `onClick` in `Modifier.clickable` is passed all the way down to `pointerInputHandler` in `SuspendingPointerInputModifierNodeImpl` (when the screen is responsive, you should see `pointerInputHandler()` being called in `SuspendingPointerInputModifierNodeImpl#onPointerEvent`). > > After shuffling the overlays, even though the `ClickableNode` is found and `SuspendingPointerInputModifierNodeImpl#onPointerEvent` is called, `pointerInputHandler()` is never called because the `pointerInputJob` in `SuspendingPointerInputModifierNodeImpl` was launched from a canceled coroutineScope. Tracking down that coroutineScope shows that it was canceled when `DialogCollator` calls `Dialog.dismiss` -> `onViewDetachedFromWindow` -> `Recomposer.cancel()`. So it makes sense why destroying/rebuilding instead of dismissing and reusing the dialog fixes this issue. Still need to put together a non-workflow repro sample and submit a bug to the Compose team, but this unblocks our move to Compose 1.6.8 in the meantime. --- .../workflow1/ui/navigation/DialogCollator.kt | 49 ++++++++++--------- .../workflow1/ui/navigation/DialogSession.kt | 11 ++--- 2 files changed, 29 insertions(+), 31 deletions(-) diff --git a/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/navigation/DialogCollator.kt b/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/navigation/DialogCollator.kt index 2ce5b54a7..efb20ac7e 100644 --- a/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/navigation/DialogCollator.kt +++ b/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/navigation/DialogCollator.kt @@ -1,9 +1,11 @@ package com.squareup.workflow1.ui.navigation +import com.squareup.workflow1.ui.Compatible import com.squareup.workflow1.ui.ViewEnvironment import com.squareup.workflow1.ui.ViewEnvironmentKey import com.squareup.workflow1.ui.WorkflowUiExperimentalApi import com.squareup.workflow1.ui.navigation.DialogCollator.IdAndSessions +import com.squareup.workflow1.ui.navigation.DialogSession.KeyAndBundle import java.util.UUID /** @@ -38,7 +40,11 @@ internal class DialogSessionUpdate( oldSessionFinder: OldSessionFinder, covered: Boolean ) -> DialogSession -) +) { + override fun toString(): String { + return "DialogSessionUpdate(overlay=${Compatible.keyFor(overlay)})" + } +} /** * Init method called at the start of [LayeredDialogSessions.update]. @@ -113,8 +119,7 @@ internal fun ViewEnvironment.establishDialogCollator( * updates that were enqueued with the shared [DialogCollator] are executed in a single * pass. Because this [DialogCollator] has complete knowledge of the existing stack * of `Dialog` windows and all updates, it is able to decide if any existing instances need to be - * [dismissed][android.app.Dialog.dismiss] and [re-shown][android.app.Dialog.show] - * to keep them in the correct order. + * [destroyed][DialogSession.destroyDialog] and rebuilt to keep them in the correct order. */ @WorkflowUiExperimentalApi internal class DialogCollator { @@ -149,7 +154,11 @@ internal class DialogCollator { val id: UUID, val updates: List, val onSessionsUpdated: (List) -> Unit - ) + ) { + override fun toString(): String { + return "IdAndUpdates(id=$id, updates=$updates)" + } + } /** * The [IdAndUpdates] instances accumulated by all calls to [scheduleUpdates]. @@ -199,10 +208,6 @@ internal class DialogCollator { .flatMap { it.sessions.map { session -> Pair(it.id, session) } } .iterator() - // Collects members of establishedSessions that are dismissed because they - // are out of order, so that we can try to show them again. - val hiddenSessions = mutableListOf>() - // Z index of the uppermost ModalOverlay. val topModalIndex = allUpdates.asSequence() .flatMap { it.updates.asSequence().map { update -> update.overlay } } @@ -212,14 +217,13 @@ internal class DialogCollator { var updatingSessionIndex = 0 val allNewSessions = mutableListOf() + val viewStates = mutableMapOf() allUpdates.forEach { idAndUpdates -> val updatedSessions = mutableListOf() // We're building an object that the next LayeredDialogSessions can use to // find an existing dialog that matches a given Overlay. Any - // incompatible dialog that we skip on the way to find a match is dismissed. - // If we later find a match for one of the dismissed dialogs, it is re-shown -- - // which moves it to the front, ensuring the correct Z order. + // incompatible dialog that we skip on the way to find a match is destroyed. val oldSessionFinder = OldSessionFinder { overlay -> // First we iterate through the existing windows to find one that belongs @@ -228,26 +232,23 @@ internal class DialogCollator { val (id, session) = establishedSessionsIterator.next() if (idAndUpdates.id == id && session.canShow(overlay)) return@OldSessionFinder session - // Can't update this session from this Overlay. Dismiss it (via Dialog.dismiss - // under the hood), but hold on to it in case it can except a later Overlay. - session.setVisible(false) - hiddenSessions.add(Pair(id, session)) + // Can't update this session from this Overlay, save its view state and destroy it. + // If it was just out of z order, a new one with matching id will be made and restored. + session.save()?.let { viewStates[id.toString() + session.savedStateKey] = it } + session.destroyDialog(saveViewState = true) continue } - // There are no established windows left. See if any of the ones that were - // dismissed because they were out of order can be shown again. - return@OldSessionFinder hiddenSessions.indexOfFirst { (hiddenId, dialogSession) -> - idAndUpdates.id == hiddenId && dialogSession.canShow(overlay) - }.takeUnless { it == -1 }?.let { compatibleIndex -> - val restoredSession = hiddenSessions.removeAt(compatibleIndex).second - restoredSession.apply { setVisible(true) } - } + // There are no established windows left. + return@OldSessionFinder null } idAndUpdates.updates.forEach { update -> val covered = updatingSessionIndex < topModalIndex - updatedSessions += update.doUpdate(oldSessionFinder, covered) + updatedSessions += update.doUpdate(oldSessionFinder, covered).also { session -> + viewStates.remove(idAndUpdates.id.toString() + session.savedStateKey) + ?.let { session.restore(it) } + } updatingSessionIndex++ } idAndUpdates.onSessionsUpdated(updatedSessions) diff --git a/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/navigation/DialogSession.kt b/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/navigation/DialogSession.kt index 38fec6d80..2f10d858f 100644 --- a/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/navigation/DialogSession.kt +++ b/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/navigation/DialogSession.kt @@ -5,9 +5,6 @@ import android.os.Parcel import android.os.Parcelable import android.os.Parcelable.Creator import android.view.KeyEvent -import android.view.KeyEvent.ACTION_UP -import android.view.KeyEvent.KEYCODE_BACK -import android.view.KeyEvent.KEYCODE_ESCAPE import android.view.MotionEvent import android.view.Window.Callback import androidx.activity.OnBackPressedDispatcher @@ -94,9 +91,6 @@ internal class DialogSession( */ val savedStateKey = Compatible.keyFor(initialOverlay) - private val KeyEvent.isBackPress: Boolean - get() = (keyCode == KEYCODE_BACK || keyCode == KEYCODE_ESCAPE) && action == ACTION_UP - /** * One time call to set up our brand new [OverlayDialogHolder] instance. * This will be followed by one time calls to [showNewDialog] and [destroyDialog]. @@ -242,10 +236,13 @@ internal class DialogSession( * We are never going to use this `Dialog` again. Tear down our lifecycle hooks * and dismiss it. */ - fun destroyDialog() { + fun destroyDialog(saveViewState: Boolean = false) { if (!destroyed) { destroyed = true with(holder.dialog) { + if (isShowing && saveViewState) { + stateRegistryAggregator.saveAndPruneChildRegistryOwner(savedStateKey) + } // The dialog's views are about to be detached, and when that happens we want to transition // the dialog view's lifecycle to a terminal state even though the parent is probably still // alive.