From e081c2e36f93063583c507b533808ebf68169c9e Mon Sep 17 00:00:00 2001 From: Ray Ryan Date: Fri, 3 Mar 2023 17:23:31 -0800 Subject: [PATCH] Introduces `DialogCollator` Android notoriously give us no control over the z order of Dialog windows other than taking care to `show()` them in the right order, so keeping a pile of windows in sync with a list of `Overlay` rendering models is hard. We've already solved that for individual instances of `LayeredDialogSessions` (the delegate that does all of the work for `BodyAndOverlaysContainer`), but it was still a problem when you nested them, e.g. in the style of the recently introduced Nested Overlays sample. Also, the solution was kind of nasty because we would replace any out of order Dialogs with brand new ones, rather than preserving them by calling `dismiss()` and `show()`. To fix all that we introduce `internal class DialogCollator`. On each view update pass, a new `DialogCollator` instance is created by the outermost `LayeredDialogSessions` and made available to any nested instances via the `ViewEnvironment`. The shared collator collects a list of all existing `DialogSession` instances, and a set of `DialogSessionUpdate` functions to be asynchronously used to update existing instances or create new ones. Because a `DialogCollator` has complete knowledge of all the windows managed by the set of nested `LayerDialogSessions` instances it supports, it is able to ensure that inserts are supported by calling `dismiss()` / `show()` on existing windows in the sequence needed to reorder them. One important change to note: our `SavedStateRegistry` code requires that each window in a set has a unique name, which we derive by applying `Compatible.keyFor` to its defining `Overlay`. We used to append to that key an `Overlay`'s position in its `LayeredDialogSessions` list, as a poorly considered hack to simplify showing several of the same type at once. That approach conflicts with reordering. With this commit we no longer include the index value in `SavedStateRegistry` keys, meaning that each `Overlay` shown by a `LayeredDialogSession` must have a unique key -- `BackStackScreen` has long had a similar requirement. `NamedScreen` exists to simplify that kind of thing, and in particular can be used to wrap `ScreenOverlay.content` to meet the new requirement. Fixes #966 --- .../poetry/views/MayBeLoadingScreen.kt | 6 +- .../nestedoverlays/NestedOverlaysAppTest.kt | 121 ++++++-- .../sample/nestedoverlays/ButtonBar.kt | 29 +- .../nestedoverlays/NestedOverlaysWorkflow.kt | 45 +-- .../src/main/res/values/ids.xml | 4 + .../src/main/res/values/strings.xml | 22 +- .../compose/ComposeViewTreeIntegrationTest.kt | 28 +- .../ui/container/DialogIntegrationTest.kt | 36 ++- .../androidx/KeyedSavedStateRegistryOwner.kt | 6 +- .../WorkflowSavedStateRegistryAggregator.kt | 18 +- .../workflow1/ui/container/CoveredByModal.kt | 6 +- .../workflow1/ui/container/DialogCollator.kt | 261 ++++++++++++++++++ .../workflow1/ui/container/DialogSession.kt | 69 ++++- .../ui/container/LayeredDialogSessions.kt | 125 ++++----- .../container/OverlayDialogFactoryFinder.kt | 6 +- .../container/ScreenOverlayDialogFactory.kt | 4 +- workflow-ui/core-common/api/core-common.api | 4 +- ...ullScreenOverlay.kt => FullScreenModal.kt} | 8 +- 18 files changed, 630 insertions(+), 168 deletions(-) create mode 100644 samples/nested-overlays/src/main/res/values/ids.xml create mode 100644 workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/DialogCollator.kt rename workflow-ui/core-common/src/main/java/com/squareup/workflow1/ui/container/{FullScreenOverlay.kt => FullScreenModal.kt} (77%) diff --git a/benchmarks/performance-poetry/complex-poetry/src/main/java/com/squareup/benchmarks/performance/complex/poetry/views/MayBeLoadingScreen.kt b/benchmarks/performance-poetry/complex-poetry/src/main/java/com/squareup/benchmarks/performance/complex/poetry/views/MayBeLoadingScreen.kt index bb894a163a..42a9bee0cf 100644 --- a/benchmarks/performance-poetry/complex-poetry/src/main/java/com/squareup/benchmarks/performance/complex/poetry/views/MayBeLoadingScreen.kt +++ b/benchmarks/performance-poetry/complex-poetry/src/main/java/com/squareup/benchmarks/performance/complex/poetry/views/MayBeLoadingScreen.kt @@ -4,11 +4,11 @@ import com.squareup.sample.container.overviewdetail.OverviewDetailScreen import com.squareup.sample.container.panel.ScrimScreen import com.squareup.workflow1.ui.WorkflowUiExperimentalApi import com.squareup.workflow1.ui.container.BodyAndOverlaysScreen -import com.squareup.workflow1.ui.container.FullScreenOverlay +import com.squareup.workflow1.ui.container.FullScreenModal @OptIn(WorkflowUiExperimentalApi::class) typealias MayBeLoadingScreen = - BodyAndOverlaysScreen, FullScreenOverlay> + BodyAndOverlaysScreen, FullScreenModal> @OptIn(WorkflowUiExperimentalApi::class) fun MayBeLoadingScreen( @@ -17,6 +17,6 @@ fun MayBeLoadingScreen( ): MayBeLoadingScreen { return BodyAndOverlaysScreen( ScrimScreen(baseScreen, dimmed = loaders.isNotEmpty()), - loaders.map { FullScreenOverlay(it) } + loaders.map { FullScreenModal(it) } ) } diff --git a/samples/nested-overlays/src/androidTest/java/com/squareup/sample/nestedoverlays/NestedOverlaysAppTest.kt b/samples/nested-overlays/src/androidTest/java/com/squareup/sample/nestedoverlays/NestedOverlaysAppTest.kt index c616d62a14..2dabf7bd89 100644 --- a/samples/nested-overlays/src/androidTest/java/com/squareup/sample/nestedoverlays/NestedOverlaysAppTest.kt +++ b/samples/nested-overlays/src/androidTest/java/com/squareup/sample/nestedoverlays/NestedOverlaysAppTest.kt @@ -3,9 +3,12 @@ package com.squareup.sample.nestedoverlays import androidx.test.espresso.Espresso.onView import androidx.test.espresso.ViewInteraction import androidx.test.espresso.action.ViewActions.click +import androidx.test.espresso.action.ViewActions.typeText import androidx.test.espresso.assertion.ViewAssertions.doesNotExist import androidx.test.espresso.assertion.ViewAssertions.matches +import androidx.test.espresso.matcher.RootMatchers.isDialog import androidx.test.espresso.matcher.ViewMatchers.isDisplayed +import androidx.test.espresso.matcher.ViewMatchers.withId import androidx.test.espresso.matcher.ViewMatchers.withParent import androidx.test.espresso.matcher.ViewMatchers.withParentIndex import androidx.test.espresso.matcher.ViewMatchers.withText @@ -36,17 +39,17 @@ class NestedOverlaysAppTest { onBottomCoverEverything().assertDisplayed() onTopCoverBody().perform(click()) - onView(withText("Close")).perform(click()) + onView(withText("❌")).perform(click()) onTopCoverEverything().perform(click()) - onView(withText("Close")).perform(click()) + onView(withText("❌")).perform(click()) - onView(withText("Hide Top Bar")).perform(click()) + onView(withText("Hide Top")).perform(click()) onTopCoverBody().assertNotDisplayed() onTopCoverEverything().assertNotDisplayed() onBottomCoverBody().assertDisplayed() onBottomCoverEverything().assertDisplayed() - onView(withText("Hide Bottom Bar")).perform(click()) + onView(withText("Hide Bottom")).perform(click()) onTopCoverBody().assertNotDisplayed() onTopCoverEverything().assertNotDisplayed() onBottomCoverBody().assertNotDisplayed() @@ -56,25 +59,109 @@ class NestedOverlaysAppTest { // https://github.com/square/workflow-kotlin/issues/966 @Test fun canInsertDialog() { onTopCoverEverything().perform(click()) - onView(withText("Hide Top Bar")).check(doesNotExist()) - onView(withText("Cover Body")).perform(click()) - // This line fails due to https://github.com/square/workflow-kotlin/issues/966 - // onView(withText("Hide Top Bar")).check(doesNotExist()) + // Cannot see the inner dialog. + onView(withText("Hide Top")).inRoot(isDialog()).check(doesNotExist()) + + // Click the outer dialog's button to show the inner dialog. + onView(withText("Cover Body")).inRoot(isDialog()).perform(click()) + // Inner was created below outer, so we still can't see it. + onView(withText("Hide Top")).inRoot(isDialog()).check(doesNotExist()) + + // Close the outer dialog. + onView(withText("❌")).inRoot(isDialog()).perform(click()) + // Now we can see the inner. + onView(withText("Hide Top")).inRoot(isDialog()).check(matches(isDisplayed())) + // Close it to confirm it really works. + onView(withText("❌")).inRoot(isDialog()).perform(click()) + onTopCoverEverything().check(matches(isDisplayed())) + } + + @Test fun canInsertAndRemoveCoveredDialog() { + // Show the outer dialog + onTopCoverEverything().perform(click()) + // Show the inner dialog behind it + onView(withText("Cover Body")).inRoot(isDialog()).perform(click()) + // Close the (covered) inner dialog and don't crash. :/ + onView(withText("Reveal Body")).inRoot(isDialog()).perform(click()) + // Close the outer dialog + onView(withText("❌")).inRoot(isDialog()).perform(click()) + // We can see the activity window again + onTopCoverEverything().check(matches(isDisplayed())) + } + + @Test fun whenReorderingViewStateIsPreserved() { + // Show the outer dialog + onTopCoverEverything().perform(click()) + + // Type something on it + onView(withId(R.id.button_bar_text)).inRoot(isDialog()) + .perform(typeText("banana")) + + // Click the outer dialog's button to show the inner dialog. + onView(withText("Cover Body")).inRoot(isDialog()).perform(click()) + + // The original outer dialog was destroyed and replaced. + // Check that the text we entered made it to the replacement dialog via view state. + onView(withId(R.id.button_bar_text)).inRoot(isDialog()) + .check(matches(withText("banana"))) + } + + // https://github.com/square/workflow-kotlin/issues/314 + @Test fun whenBodyAndOverlaysStopsBeingRenderedDialogsAreDismissed() { + onBottomCoverBody().perform(click()) + onView(withText("💣")).inRoot(isDialog()).perform(click()) + + onBottomCoverBody().check(doesNotExist()) + onView(withText("Reset")).perform(click()) - // Should continue to close the top sheet and assert that the inner sheet is visible. + onBottomCoverBody().perform(click()) + onView(withText("💣")).inRoot(isDialog()).check(matches(isDisplayed())) } - // So far can't express this in Espresso. Considering move to Maestro - // @Test fun canClickPastInnerWindow() { - // onView(allOf(withText("Cover Everything"), withParent(withParentIndex(0)))) + // So far can't express this in Espresso, because it refuses to work + // w/a root that lacks window focus. Considering move to Maestro. + // In the meantime I'd like to keep this commented out block around + // as a reminder. + + // @Test fun canCoverDialogAndRemoveItWhileCovered() { + // // Show the inner dialog + // onTopCoverBody().perform(click()) + // + // lateinit var activity: Activity + // scenarioRule.scenario.onActivity { activity = it } + // + // // Show the outer dialog + // onTopCoverEverything() + // .inRoot( + // allOf( + // withDecorView(Matchers.`is`(activity.window.decorView)), + // Matchers.not(hasWindowFocus()) + // ) + // ) // .perform(click()) // - // scenario.onActivity { activity -> - // onView(allOf(withText("Cover Everything"), withParent(withParentIndex(0)))) - // .inRoot(withDecorView(not(`is`(activity.window.decorView)))) - // .perform(click()) - // } + // // Close the (covered) inner dialog + // onView(withText("Reveal Body")).inRoot(isDialog()).perform(click()) + // // Close the outer dialog + // onView(withText("❌")).inRoot(isDialog()).perform(click()) + // // We can see the activity window again + // onTopCoverEverything().check(matches(isDisplayed())) + // } + // + // /** + // * Like the private (why?) `hasWindowFocus` method in Espresso, but + // * built into a `Matcher` rather than a `Matcher` (since + // * that was our only use case). + // */ + // fun hasWindowFocus(): Matcher { + // return withDecorView(object : TypeSafeMatcher() { + // override fun describeTo(description: Description) { + // description.appendText("has window focus (Square fork)") + // } + // + // override fun matchesSafely(item: View): Boolean = item.hasWindowFocus() + // }) // } private fun ViewInteraction.assertNotDisplayed() { diff --git a/samples/nested-overlays/src/main/java/com/squareup/sample/nestedoverlays/ButtonBar.kt b/samples/nested-overlays/src/main/java/com/squareup/sample/nestedoverlays/ButtonBar.kt index 37f9f7726f..3f44b75377 100644 --- a/samples/nested-overlays/src/main/java/com/squareup/sample/nestedoverlays/ButtonBar.kt +++ b/samples/nested-overlays/src/main/java/com/squareup/sample/nestedoverlays/ButtonBar.kt @@ -2,6 +2,9 @@ package com.squareup.sample.nestedoverlays import android.graphics.drawable.ColorDrawable import android.view.Gravity +import android.view.View.GONE +import android.view.View.VISIBLE +import android.widget.EditText import android.widget.LinearLayout import androidx.annotation.ColorRes import androidx.annotation.StringRes @@ -21,23 +24,33 @@ data class Button( class ButtonBar( vararg buttons: Button?, @ColorRes val color: Int = -1, + val showEditText: Boolean = false, ) : AndroidScreen { private val buttons: List