Introduces Overlay, and its first uses: BodyAndModalsScreen, AlertOverlay #588

rjrjr · 2021-11-17T19:27:06Z

New platform-neutral rendering types:

Overlay is a marker type that identifies a window-like rendering.
AlertOverlay is the first use of Overlay, models Android's
AlertDialog
BodyAndModalsScreen has a body Screen covered by zero or more
Overlays that are given a modal treatment -- its body is shielded
from events while any Overlay is present.

Android support code that uses them to manage Dialogs:

OverlayDialogFactory is to Overlay as
ScreenViewRunner and ScreenViewFactory are to Screen
DialogStack uses OverlayDialogFactory to display List<Overlay>
as a pile of Dialogs
BodyAndModalsContainer is a FrameLayout that uses DialogStack
to display BodyAndModalScreen
AlertOverlayDialogFactory creates AlertDialog instances for
AlertOverlay

Note that BodyAndModalsContainer and AlertOverlayDialogFactory are
internal, with default binings built into Overlay.toDialogFactory()
and Screen.buildView.

Also note that AndroidViewEnvironment.kt and the bulk of AndroidScreen.kt
have been moved to ScreenViewFactory.kt, which seemed like a less
surprising home for them.

Updates hellobackbutton sample to demonstrate the use of all of this.

Note the commented out deprecations. Turns out its impossible to suppress
deprecation warnings in typealias calls like the ones our sample apps are
still using, and those can't be ported until we have a replacement for
ModalViewContainer. Tracked in #589.

CLAassistant · 2021-11-17T19:27:11Z

All committers have signed the CLA.

rjrjr · 2021-11-17T22:22:10Z

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

@@ -31,3 +37,87 @@ public interface ScreenViewFactory<in RenderingT : Screen> : ViewRegistry.Entry<
    container: ViewGroup? = null
  ): View
 }
+
+/**


Outside of making getViewFactoryForRendering non-public, no real changes in this file, just moved code in from another location.

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

rjrjr · 2021-11-18T00:31:55Z

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

+    ?: (rendering as? BackStackScreen<*>)?.let {
+      BackStackScreenViewFactory as ScreenViewFactory<ScreenT>
+    }
+    ?: (rendering as? BodyAndModalsScreen<*, *>)?.let {


Okay, I lied -- this line is new

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

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

steve-the-edwards

Thank-you for the final kdoc commit. I will likely need more to understand everything going on here, but hopefully @helios175 has a better understanding?

...low-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/ListenToCoordinates.kt

steve-the-edwards · 2021-11-18T15:42:23Z

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

+import com.squareup.workflow1.ui.bindShowRendering
+
+@WorkflowUiExperimentalApi
+internal class BodyAndModalsContainer @JvmOverloads constructor(


At the risk of another bikeshed why isn't this BodyAndOverlaysContainer? And the screen BodyAndOverlaysScreen?

Well spotted, and not an accident. I've realized that modals are a subset of overlays, and this name is meant to respect that distinction -- a modal is an overlay whose presence implies that things beneath it are blocked from receiving any events. The presence of a modal is a visual indicator that we are in a different mode of the app -- channeling ancient Macintosh developer docs here.

Contrast this with tooltips and other pop-ups that are also naturally modeled via Overlay, and which should not have the side effect of modality.

Fell into this because the container itself actually has to participate -- dialogs simply can't do the job all by themselves, as we've re-learned painfully through BugSnag in the past year. (Thinking we should double down on that fact and put the container in charge of showing the scrim, which would fix double-shadow issues we have right now.)

Yes this makes total sense and is a great place to settle. We should document this.

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

...flow-ui/core-common/src/main/java/com/squareup/workflow1/ui/container/BodyAndModalsScreen.kt

rjrjr · 2021-11-18T18:55:42Z

Updated to remove the controversial bits (RectangleProvider and its uses), which are also unused at the moment. Figure we can get this merged and then sort that out. Squashed what's left and updated the commit message.

rjrjr · 2021-11-18T19:55:57Z

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

+            // Custom behavior from the container to be fired when we show a new dialog.
+            // The default modal container uses this to flush its body of partially
+            // processed touch events that should have been blocked by the modal.
+            beforeShowing()


Probably will need to tweak this to pass the Dialog itself in the next PR.

rjrjr · 2021-11-18T19:57:35Z

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

+      }
+    }
+
+    (runners - newRunners.toSet()).forEach { it.dismiss() }


The IDE prompted me to add the toSet() call as an optimization, and I'm very suggestible.

rjrjr · 2021-11-18T22:01:36Z

Damn espresso tests are so flaky, I can't tell if I've made them more so.

rjrjr · 2021-11-18T23:36:07Z

Simply cannot get a green build. #590 is awful, and clearly unrelated to this PR, we didn't change that app or anything it uses.

steve-the-edwards

Definitely understand now! Still have more naming change requests that I think coudl be helpful.

steve-the-edwards · 2021-11-19T20:26:21Z

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

+    return compatible(this.rendering, rendering)
+  }
+
+  fun showRendering(


I would change this to bindRendering as I'm thinking of the RecyclerView ViewHolder analogy. What you are doing is 'binding' the rendering to this dialog - nothing is actually changing with respect to its shown/hidden state.

I don't know why exactly but bind bugs me. How about canTakeRendering and takeRendering?

steve-the-edwards · 2021-11-19T20:27:21Z

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

+import com.squareup.workflow1.ui.compatible
+
+@WorkflowUiExperimentalApi
+internal class DialogRunner<T : Overlay>(


I would change this to DialogHolder. I realize we use Runner elsewhere but I also just realized that while I got the 'Runner' metaphor the 'ViewHolder' analogy (as in the RecyclerView sense) is much more powerful to me as an Android person with RecyclerView experience.

Great call. When we use Runner elsewhere it really means something else entirely.

steve-the-edwards · 2021-11-19T20:27:46Z

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

+    }
+  }
+
+  fun canShowRendering(rendering: Overlay): Boolean {


canBindRendering?

steve-the-edwards · 2021-11-19T20:29:03Z

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

+ * Provides a [LifecycleOwner] per managed dialog, and view persistence support.
+ */
+@WorkflowUiExperimentalApi
+public class DialogStack(


DialogStackContainer we use the Container prefix for these logic classes that deal with interaction between groups of Views bound to Renderings no?

Container isn't good b/c that's consistently meant "a workflow view that displays other workflow views". This is a support class to create such views.

And as @helios175 pointed out, Stack isn't good because this is so completely different from BackStack.

How about LayeredDialogHolders.

Oh, but DialogHolder is a private type. So just LayeredDialogs.

steve-the-edwards · 2021-11-19T20:31:41Z

...flow-ui/core-common/src/main/java/com/squareup/workflow1/ui/container/BodyAndModalsScreen.kt

+/**
+ * A screen that may stack a number of modal [Overlay]s over a body.
+ * While modals are present, the body is expected to ignore any
+ * input events -- touch, keyboard, etc.
+ */


Suggested change

/**

* A screen that may stack a number of modal [Overlay]s over a body.

* While modals are present, the body is expected to ignore any

* input events -- touch, keyboard, etc.

*/

/**

* A screen that may stack a number of modal [Overlay]s over a body.

* While modals are present, the body is expected to ignore any

* input events -- touch, keyboard, etc.

* Passing an [Overlay] rendering as part of [modals] here marks that

* Overlay as modal - that is, it should block input events around outside

* of its content area. The logic for that input event blocking is handled

* for users of this screen by the internal [BodyAndModalsContainer].

*/

it should block input events around outside of its content area

Oh, no no no no no. It should only block events within the bounds of the body. Blocking events outside of that area is a contract violation. And in fact turning on the "modal" flag for an Android Dialog implementation would be a bug precisely because of this.

I'm too tired to phrase this properly right now. Will come up with something before merging.

Actually…let me leave this as is and rephrase with the next PR, where I'll actually be implementing proper custom dialogs. There may be more nuances then.

rjrjr · 2021-11-22T19:04:31Z

Update with name changes proposed during review:

DialogRunner > DialogHolder
DialogStack > LayeredDialogs
canShowRendering > canTakeRendering
showRendering > takeRendering

Still need to find a home for the essay explaining Overlay v. Modal. Guess I'll put it right on the Overlay interface.

…rlay New platform-neutral rendering types: - `Overlay` is a marker type that identifies a window-like rendering. - `AlertOverlay` is the first use of `Overlay`, models Android's `AlertDialog` - `BodyAndModalsScreen` has a body `Screen` covered by zero or more `Overlay`s that are given a modal treatment -- its body is shielded from events while any `Overlay` is present. Android support code that uses them to manage `Dialog`s: - `OverlayDialogFactory` is to `Overlay` as `ScreenViewRunner` and `ScreenViewFactory` are to `Screen`. - `LayeredDialogs` is a support class that uses `OverlayDialogFactory` to display `List<Overlay>` as a pile of `Dialog`s. It's meant to simplify the implementation of container views that use `Overlay`. - `BodyAndModalsContainer` is a `FrameLayout` that uses `DialogStack` to display `BodyAndModalScreen`. First use of `LayeredDialogs`. - `AlertOverlayDialogFactory` creates `AlertDialog` instances for `AlertOverlay`. Note that `BodyAndModalsContainer` and `AlertOverlayDialogFactory` are `internal`, with default binings built into `Overlay.toDialogFactory()` and `Screen.buildView`. Also note that `AndroidViewEnvironment.kt` and the bulk of `AndroidScreen.kt` have been moved to `ScreenViewFactory.kt`, which seemed like a less surprising home for them. Updates `hellobackbutton` sample to demonstrate the use of all of this. Note the commented out deprecations. Turns out its impossible to suppress deprecation warnings in `typealias` calls like the ones our sample apps are still using, and those can't be ported until we have a replacement for `ModalViewContainer`. Tracked in #589.

rjrjr · 2021-11-22T19:12:21Z

Updated the kdoc on Overlay:

/**
 * Marker interface implemented by window-like renderings that map to a layer above
 * a base [Screen][com.squareup.workflow1.ui.Screen].
 *
 * Note that an Overlay is not necessarily a modal window, though that is how
 * they are used in [BodyAndModalsScreen]. An Overlay can be any part of the UI
 * that visually floats in a layer above the main UI, or above other Overlays.
 * Possible examples include alerts, drawers, and tooltips.
 * 
 * Whether or not an Overlay's presence indicates that events are blocked from lower
 * layers is a separate concern.
 */
@WorkflowUiExperimentalApi
public interface Overlay

If no one complains I'll merge this when CI blesses it.

rjrjr force-pushed the ray/overlay branch 4 times, most recently from bfa3a55 to b928193 Compare November 17, 2021 19:58

rjrjr changed the title ~~Ray/overlay~~ Introduces Overlay Nov 17, 2021

rjrjr changed the title ~~Introduces Overlay~~ Introduces Overlay et al to replace ModalContainer Nov 17, 2021

rjrjr force-pushed the ray/overlay branch 3 times, most recently from 232fe8d to 58ca707 Compare November 17, 2021 22:19

rjrjr commented Nov 17, 2021

View reviewed changes

rjrjr marked this pull request as ready for review November 17, 2021 22:26

rjrjr requested review from zach-klippenstein and a team as code owners November 17, 2021 22:26

rjrjr force-pushed the ray/overlay branch 2 times, most recently from fef7cac to 4976aeb Compare November 17, 2021 23:55

rjrjr assigned steve-the-edwards and helios175 Nov 18, 2021

rjrjr commented Nov 18, 2021

View reviewed changes

...-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/BodyAndModalsContainer.kt Outdated Show resolved Hide resolved

rjrjr force-pushed the ray/overlay branch 2 times, most recently from 59eaca3 to 7781b18 Compare November 18, 2021 00:30

rjrjr commented Nov 18, 2021

View reviewed changes

helios175 reviewed Nov 18, 2021

View reviewed changes

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/RectProvider.kt Outdated Show resolved Hide resolved

helios175 reviewed Nov 18, 2021

View reviewed changes

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/DialogRunner.kt Outdated Show resolved Hide resolved

steve-the-edwards approved these changes Nov 18, 2021

View reviewed changes

rjrjr force-pushed the ray/overlay branch from 7781b18 to 2891b18 Compare November 18, 2021 18:54

rjrjr changed the title ~~Introduces Overlay et al to replace ModalContainer~~ Introduces Overlay, and its first uses: BodyAndModalsScreen, AlertOverlay Nov 18, 2021

rjrjr force-pushed the ray/overlay branch from 2891b18 to 36ac8ee Compare November 18, 2021 19:51

rjrjr commented Nov 18, 2021

View reviewed changes

rjrjr requested a review from steve-the-edwards November 18, 2021 20:46

steve-the-edwards reviewed Nov 19, 2021

View reviewed changes

rjrjr force-pushed the ray/overlay branch from 36ac8ee to 5514589 Compare November 22, 2021 19:02

rjrjr force-pushed the ray/overlay branch from 5514589 to 89115ac Compare November 22, 2021 19:11

rjrjr merged commit 060019c into ray/ui-update Nov 22, 2021

rjrjr deleted the ray/overlay branch November 22, 2021 22:01

Introduces Overlay, and its first uses: BodyAndModalsScreen, AlertOverlay #588

Introduces Overlay, and its first uses: BodyAndModalsScreen, AlertOverlay #588

Uh oh!

Conversation

rjrjr commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rjrjr Nov 17, 2021 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

steve-the-edwards left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjrjr Nov 18, 2021 • 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!

Uh oh!

Uh oh!

Uh oh!

rjrjr commented Nov 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjrjr commented Nov 18, 2021

Uh oh!

rjrjr commented Nov 18, 2021

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjrjr Nov 20, 2021 • 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!

rjrjr commented Nov 22, 2021

Uh oh!

rjrjr commented Nov 22, 2021

Uh oh!

Uh oh!

rjrjr commented Nov 17, 2021 •

edited

Loading

CLAassistant commented Nov 17, 2021 •

edited

Loading

rjrjr Nov 17, 2021 •

edited

Loading

rjrjr Nov 18, 2021 •

edited

Loading

rjrjr Nov 20, 2021 •

edited

Loading