Do not show `Dialog`s before `Activity` content view is attached. #1003

rjrjr · 2023-05-31T22:39:24Z

If the very first rendering would instantiate Dialog windows, their content views were getting attached before the Activity's content view. This would cause us to restore saved view state in the wrong order. If a ComposeView is in a Dialog in this situation, we would crash with an IllegalStateException from SavedStateRegistry.consumeRestoredStateForKey.

The fix is for LayeredDialogSessions and DialogCollator to conspire to decouple created Dialog instances from showing them the first time. Normally we go ahead and do both at once. But if LayeredDialogSessions.update is called before the corresponding BodyAndOverlaysContainer view is attached to the Activity window, we defer showing until it is.

Fixes #1001

In anticipation of fix for #1001

If the very first rendering would instantiate `Dialog` windows, their content views were getting attached before the `Activity`'s content view. This would cause us to restore saved view state in the wrong order. If a `ComposeView` is in a `Dialog` in this situation, we would crash with an `IllegalStateException` from `SavedStateRegistry.consumeRestoredStateForKey`. The fix is for `LayeredDialogSessions` to memoize update calls received while detached, and replay them at attach time. The details are a little more complicated than that, though: * We always build the `BodyAndOverlaysScreen.content` (typically the stuff in the `Activity` window) eagerly * When nesting `BodyAndOverlaysScreen`, we have to take care that only the outermost `LayeredDialogSessions` instance does the capture and replay thing. Otherwise `Dialog` windows get shown in the wrong order. Fortunately we have good test coverage around this stuff these days, and now with this PR it's even better. Fixes #1003

rjrjr · 2023-06-01T22:32:56Z

...re-android/src/androidTest/java/com/squareup/workflow1/ui/container/DialogIntegrationTest.kt


    scenario.onActivity { activity ->
      val root = WorkflowLayout(activity)
+      activity.setContentView(root)


Oops, these tests were never actually attaching anything to the activity window. Now they do.

If the very first rendering would instantiate `Dialog` windows, their content views were getting attached before the `Activity`'s content view. This would cause us to restore saved view state in the wrong order. If a `ComposeView` is in a `Dialog` in this situation, we would crash with an `IllegalStateException` from `SavedStateRegistry.consumeRestoredStateForKey`. The fix is for `LayeredDialogSessions` to memoize update calls received while detached, and replay them at attach time. The details are a little more complicated than that, though: * We always build the `BodyAndOverlaysScreen.content` (typically the stuff in the `Activity` window) eagerly * When nesting `BodyAndOverlaysScreen`, we have to take care that only the outermost `LayeredDialogSessions` instance does the capture and replay thing. Otherwise `Dialog` windows get shown in the wrong order. Fortunately we have good test coverage around this stuff these days, and now with this PR it's even better. Fixes #1003

rjrjr · 2023-06-02T19:52:35Z

I'm thinking that View.backPressedHandler and BackButtonScreen will still be solid enough even with this change, but I'll punch up some of the related samples to be more confident about that before merging.

@helios175 has been working on something in house that improves our ability to nest AndroidX back handlers, which would obviate this concern entirely. We should probably open source that anyway, but I don't know that doing so is required to merge this PR. (Tracking this as #1007.)

Update: in its final form this PR doesn't break the existing behavior where parent views can rely on their children being created / updated synchronously, so #1007 is less urgent. It still seems like it would be a good idea to opensource it though.

If the very first rendering would instantiate `Dialog` windows, their content views were getting attached before the `Activity`'s content view. This would cause us to restore saved view state in the wrong order. If a `ComposeView` is in a `Dialog` in this situation, we would crash with an `IllegalStateException` from `SavedStateRegistry.consumeRestoredStateForKey`. The fix is for `LayeredDialogSessions` to memoize update calls received while detached, and replay them at attach time. The details are a little more complicated than that, though: * We always build the `BodyAndOverlaysScreen.content` (typically the stuff in the `Activity` window) eagerly * When nesting `BodyAndOverlaysScreen`, we have to take care that only the outermost `LayeredDialogSessions` instance does the capture and replay thing. Otherwise `Dialog` windows get shown in the wrong order. Fortunately we have good test coverage around this stuff these days, and now with this PR it's even better. Fixes #1003

rjrjr · 2023-06-05T22:15:40Z

...i/core-android/src/main/java/com/squareup/workflow1/ui/container/BodyAndOverlaysContainer.kt


  override fun dispatchTouchEvent(event: MotionEvent): Boolean {
-    return !dialogs.allowEvents || super.dispatchTouchEvent(event)
+    return !dialogs.allowContentEvents || super.dispatchTouchEvent(event)


Unrelated method rename to try to make it clearer that this is strictly about event handling in the activity window, not the dialogs.

Why allowContentEvents and not allowBodyEvents? I think Body would help me understand what you mean.

I wrestled with that. I went with content because it matches ScreenOverlay.content, which in turn comes from the Wrapper interface. But really they're both still ambiguous, aren't they -- dialog content? dialog body?

How about allowBaseEvents? That would match the updateBase param to LayeredDialogSessions.update.

See insane stream of consciousness below, allowBodyEvents and updateBody() FTW.

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/DialogCollator.kt

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/DialogSession.kt

...w-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/LayeredDialogSessions.kt

steve-the-edwards

Nice! A few naming questions as usual 😄

steve-the-edwards · 2023-06-06T14:55:56Z

...ed-overlays/src/androidTest/java/com/squareup/sample/nestedoverlays/NestedOverlaysAppTest.kt


+  @Test fun backButtonWorks() {
+    onTopCoverEverything().perform(click())
+    onView(withText("Cover Body")).inRoot(isDialog()).perform(click())


nit: could use constants for these view texts.

I don't know why, but I always find that makes tests harder for me to read.

steve-the-edwards · 2023-06-06T14:57:08Z

...s/nested-overlays/src/main/java/com/squareup/sample/nestedoverlays/NestedOverlaysWorkflow.kt

    val outerSheet = if (!renderState.showOuterSheet) {
      null
    } else {
+      val close = context.eventHandler { state = state.copy(showOuterSheet = false) }


nit:

Suggested change

val close = context.eventHandler { state = state.copy(showOuterSheet = false) }

val closeOuter = context.eventHandler { state = state.copy(showOuterSheet = false) }

Actually I guess that won't compile below heh. Well, take it or leave it.

...s/nested-overlays/src/main/java/com/squareup/sample/nestedoverlays/NestedOverlaysWorkflow.kt

steve-the-edwards · 2023-06-06T15:04:52Z

...i/core-android/src/main/java/com/squareup/workflow1/ui/container/BodyAndOverlaysContainer.kt


  override fun dispatchTouchEvent(event: MotionEvent): Boolean {
-    return !dialogs.allowEvents || super.dispatchTouchEvent(event)
+    return !dialogs.allowContentEvents || super.dispatchTouchEvent(event)


Why allowContentEvents and not allowBodyEvents? I think Body would help me understand what you mean.

steve-the-edwards · 2023-06-06T15:09:37Z

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/DialogSession.kt

-    initialEnvironment: ViewEnvironment
-  ) {
+  /**
+   * One time call to set up our brand new [OverlayDialogHolder] instance.


Do we need to propagate this naming change down to the holder? A little bit incongruous here to say this is not showing, we are 'setting up' the holder, and then the first call is to holder.show.

That gets into another level of API inconsistency. holder.show is public, OverlayDialogHolder.show, and it parallels ScreenViewHolder.show. This is definitely an awkward seam, but it's all in non-public code.

Basically, Dialog.show() conflicts with our show(someRendering, environment) convention. But it's only us on the Workflow team who take the cognitive hit, b/c feature code never gets anywhere near Dialog.show() or Dialog.dismiss().

steve-the-edwards · 2023-06-06T15:13:10Z

...w-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/LayeredDialogSessions.kt

+  private var deferredShow: List<DialogSession>? = null
+
+  /**
+   * Clients should ignore UI events targeting their [content][ScreenOverlay.content]


so this is where the content naming comes from? Hmm. How do I map body to content? are they equivalent? 'Body' is the container for 'content'?

It was originally

class BodyAndOverlaysScreen<B: Screen, O: Overlay>( val body: B, val overlays: List<O> = emptyList() )

but I recently added the common Wrapper type, and standardized content (makes sense everywhere else :/):

class BodyAndOverlaysScreen<C: Screen, O: Overlay>( override val content: C, val overlays: List<O> = emptyList() ): Wrapper<Screen, C>

While we still have time to fuss with these names, would this be more or less confusing?

class BodyAndOverlaysScreen<B: Screen, O: Overlay>( val body: B, val overlays: List<O> = emptyList() ): Wrapper<Screen, B> { override val content: B = body }

I guess I like it?

Oh, derp: and that kdoc is TOTALLY WRONG! Link should be to BodyAndOverlays.content!

Tell you what, let's decouple this naming conversation from the ordering fix. Maybe even move it upstream.

Or for…IT'S ALREADY BodyAndOverlaysScreen.body.

I'll change things here accordingly.

/** * While this is true, clients should ignore UI events targeting the * [body][BodyAndOverlaysScreen.body] view beneath the * [Dialog][android.app.Dialog] windows they manage. It reflects * that a [ModalOverlay] is (or soon will be) showing over the body. */ public var allowBodyEvents: Boolean = true

I'm going to rename updateBase to updateBody as well.

If the very first rendering would instantiate `Dialog` windows, their content views were getting attached before the `Activity`'s content view. This would cause us to restore saved view state in the wrong order. If a `ComposeView` is in a `Dialog` in this situation, we would crash with an `IllegalStateException` from `SavedStateRegistry.consumeRestoredStateForKey`. The fix is for `LayeredDialogSessions` and `DialogCollator` to conspire to decouple created `Dialog` instances from showing them the first time. Normally we go ahead and do both at once. But if `LayeredDialogSessions.update` is called before the corresponding `BodyAndOverlaysContainer` view is attached to the `Activity` window, we defer showing until it is. Fixes #1001

rjrjr · 2023-06-06T20:39:58Z

@steve-the-edwards PTAL, adopted most of your naming fixes.

ijwhelan

Still a novice at Workflow internals but I'm not seeing any obvious issues, changes make sense to me.

steve-the-edwards

Love it. LGTM

rjrjr mentioned this pull request May 31, 2023

ComposeView and IllegalStateException: You can consumeRestoredStateForKey only after super.onCreate of corresponding component #1001

Closed

More Dialog test coverage

e5c7ff9

In anticipation of fix for #1001

rjrjr force-pushed the ray/restore-dialogs-later-like-a-sane-person branch from cbc58b5 to 5fd5d47 Compare June 1, 2023 18:52

rjrjr force-pushed the ray/restore-dialogs-later-like-a-sane-person branch from 5fd5d47 to c96d191 Compare June 1, 2023 22:22

rjrjr changed the title ~~wip: do not build dialogs before container is attached~~ Do not show Dialogs before Activity content view is attached. Jun 1, 2023

rjrjr force-pushed the ray/restore-dialogs-later-like-a-sane-person branch from c96d191 to 6a8f29a Compare June 1, 2023 22:30

rjrjr force-pushed the ray/restore-dialogs-later-like-a-sane-person branch from 6a8f29a to 17b2451 Compare June 1, 2023 22:32

rjrjr commented Jun 1, 2023

View reviewed changes

rjrjr force-pushed the ray/restore-dialogs-later-like-a-sane-person branch from 17b2451 to b68a0ef Compare June 1, 2023 23:11

rjrjr marked this pull request as ready for review June 1, 2023 23:11

rjrjr requested review from a team and zach-klippenstein as code owners June 1, 2023 23:11

rjrjr marked this pull request as draft June 2, 2023 00:15

rjrjr mentioned this pull request Jun 2, 2023

Consider open sourcing our wrappers for onbackpresseddispatcher #1007

Closed

rjrjr force-pushed the ray/restore-dialogs-later-like-a-sane-person branch from b68a0ef to 9d9ec5e Compare June 2, 2023 20:51

rjrjr force-pushed the ray/restore-dialogs-later-like-a-sane-person branch 2 times, most recently from 40781e4 to dafe01b Compare June 5, 2023 21:12

rjrjr marked this pull request as ready for review June 5, 2023 22:13

rjrjr commented Jun 5, 2023

View reviewed changes

rjrjr force-pushed the ray/restore-dialogs-later-like-a-sane-person branch from dafe01b to ea4d731 Compare June 5, 2023 22:58

rjrjr assigned ijwhelan and steve-the-edwards Jun 5, 2023

steve-the-edwards reviewed Jun 6, 2023

View reviewed changes

rjrjr force-pushed the ray/restore-dialogs-later-like-a-sane-person branch from ea4d731 to bf2b9cf Compare June 6, 2023 20:39

rjrjr requested a review from steve-the-edwards June 6, 2023 20:39

ijwhelan approved these changes Jun 6, 2023

View reviewed changes

steve-the-edwards approved these changes Jun 6, 2023

View reviewed changes

Merge branch 'main' into ray/restore-dialogs-later-like-a-sane-person

ab8def8

rjrjr enabled auto-merge June 6, 2023 22:08

rjrjr merged commit 81fe606 into main Jun 6, 2023

rjrjr deleted the ray/restore-dialogs-later-like-a-sane-person branch June 6, 2023 22:41

	val close = context.eventHandler { state = state.copy(showOuterSheet = false) }
	val closeOuter = context.eventHandler { state = state.copy(showOuterSheet = false) }

Do not show Dialogs before Activity content view is attached. #1003

Do not show Dialogs before Activity content view is attached. #1003

Uh oh!

Conversation

rjrjr commented May 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjrjr commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steve-the-edwards left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjrjr Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjrjr Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjrjr commented Jun 6, 2023

Uh oh!

ijwhelan left a comment

Choose a reason for hiding this comment

Uh oh!

steve-the-edwards left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Do not show `Dialog`s before `Activity` content view is attached. #1003

Do not show `Dialog`s before `Activity` content view is attached. #1003

rjrjr commented May 31, 2023 •

edited

Loading

rjrjr commented Jun 2, 2023 •

edited

Loading

rjrjr Jun 6, 2023 •

edited

Loading

rjrjr Jun 6, 2023 •

edited

Loading