wip: ViewRegistry overhaul, ViewRendering, ModalRendering #202

rjrjr · 2020-10-02T20:03:52Z

Builds the simpler, more flexible scheme for modals proposed in #195. Only
AlertDialog takes advantage of it so far, and only the hellobackbutton and
helloworkflow samples are ported.

closes #195

We introduce the ViewRendering and ModalRendering interfaces,
similar in sprit to AnyScreen in Swift.
ViewRegistry changes to be able to hold a collection of more arbitrary entry types, not just ViewFactory.
- Its buildView method is deprecated.
- Once we delete that method, we should be able to move ViewRegistry over to the
  non-Android, core UI module.
There are three implementations of ViewRegistry.Entry
- Our old friend ViewFactory<T: Any>, now deprecated.
- ViewBuilder<T: ViewRendering>, which replaces ViewFactory
- DialogBuilder<T: ModalRendering>
Extension method ViewRendering.buildView() replaces ViewRegistry.buildView(). We also introduce ModalRendering.buildDialog().
- These methods find the appropriate builder in ViewRegistry, based on the concrete type of the rendering, just like ViewRegistry.buildView() did.
- The new ability of ViewRegistry to hold arbitrary entries means that we can add as many rendering:view-machinery pairings as we want to. Maybe cleans up Compose integration? (@zach-klippenstein )
The abstract ModalContainer and its subclasses (AlertContainer, ModalViewContainer, our internal ones) are all deprecated in favor of a single final class: ModalContainerView, which is able to display any kind of ModalRendering associated with a DialogBuilder implementation.
LayoutRunner<T: Any> is deprecated, replaced by ViewRunner<T: ViewRendering>. That's going to be a tedious migration for existing LayoutRunner implementations, but it avoids a huge breaking change —
the new mechanism works in parallel with the deprecated one.
WorkflowViewStub.display(rendering: ViewRendering) replaces WorkflowViewStub.update(rendering: Any), now deprecated. But the latter is able to delegate to the former so incremental migration is possible.

rjrjr · 2020-10-02T20:11:51Z

workflow-ui/core-android/build.gradle.kts

  api(Dependencies.Kotlin.Stdlib.jdk6)

  implementation(Dependencies.AndroidX.activity)
+  implementation(Dependencies.AndroidX.appcompat)


Uh-oh. Did we move this out of ui-core to keep @Armaxis happy? Is that still a concern?

I think I can avoid it, but it'll be a bit tedious.

No longer a concern, with the amount of things we need these day, appcompat is imminent.
Bring it back. Sorry for bothering you remove it in first place :/

Yay! Glad I asked.

rjrjr · 2020-10-02T20:20:02Z

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

+ *    )
+ */
+@WorkflowUiExperimentalApi
+class BespokeViewBuilder<RenderingT : ViewRendering>(


Not even sure if this is worth providing. Should try just implementing the interface somewhere.

rjrjr · 2020-10-02T20:29:41Z

...rs/hello-back-button/src/main/java/com/squareup/sample/hellobackbutton/AreYouSureWorkflow.kt

      }
      Quitting -> {
-        val dialog = AlertScreen(
+        val dialog = AlertModalRendering(


This is the punchline. AlertModalRendering is bound in the ViewRegistry to AlertDialogBuilder. We didn't have to make a subclass of ModalContainerView in order to show this particular kind of modal.

rjrjr · 2020-10-02T21:10:20Z

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

+  contextForNewView: Context,
+  container: ViewGroup? = null
+): View {
+  val builder = initialViewEnvironment[ViewRegistry].getEntryFor(this::class)


Very tempting to allow the RenderingT itself to implement ViewBuilder

val builder = (this as? ViewBuilder<RenderingT>) ?: initialViewEnvironment[ViewRegistry].getEntryFor(this::class)

Actually, better to do it in the other order so that ViewRegistry can override default implementations.

val builder = initialViewEnvironment[ViewRegistry].getEntryFor(this::class) ?: (this as? ViewBuilder<RenderingT>)

IIRC the way we talked ourselves out of that was because we didn’t want to encourage screen value types from being coupled to the UI layer. But I think in reality, screen types and view factories are often defined in public-facing, API, android-dependent modules already. And this isn’t requiring that, it merely allows it, which can be super useful for things like the WorkflowEditText wiring, the ComposeRendering PoC, etc. So, I'm on board.

For context on the compose thing, see the comments on square/workflow-kotlin-compose#8.

Builds the simpler, more flexible scheme for modals proposed in #195. Only AlertDialog takes advantage of it so far, and only the `hellobackbutton` and `helloworkflow` samples are ported. closes #195 * We introduce the `ViewRendering` and `ModalRendering` interfaces, similar in sprit to `AnyScreen` in Swift. * `ViewRegistry` changes to be able to hold a collection of more arbitrary entry types, not just `ViewFactory`. * Its `buildView` method is deprecated. * Once we delete that method, we should be able to move `ViewRegistry` over to the non-Android, core UI module. * There are three implementations of `ViewRegistry.Entry` * Our old friend `ViewFactory<T: Any>`, now deprecated. * `ViewBuilder<T: ViewRendering>`, which replaces `ViewFactory` * `DialogBuilder<T: ModalRendering>` * Extension method `ViewRendering.buildView()` replaces `ViewRegistry.buildView()`. We also introduce `ModalRendering.buildDialog()`. * These methods find the appropriate builder in `ViewRegistry`, based on the concrete type of the rendering, just like `ViewRegistry.buildView()` did. * The new ability of `ViewRegistry` to hold arbitrary entries means that we can add as many rendering:view-machinery pairings as we want to. Maybe cleans up Compose integration? * The abstract `ModalContainer` and its subclasses (`AlertContainer`, `ModalViewContainer`, our internal ones) are all deprecated in favor of a single final class: `ModalContainerView`, which is able to display any kind of `ModalRendering` associated with a `DialogBuilder` implementation. * `LayoutRunner<T: Any>` is deprecated, replaced by `ViewRunner<T: ViewRendering>`. That's going to be a tedious migration for existing `LayoutRunner` implementations, but it avoids a huge breaking change — the new mechanism works in parallel with the deprecated one. * `WorkflowViewStub.display(rendering: ViewRendering)` replaces `WorkflowViewStub.update(rendering: Any)`, now deprecated. But the latter is able to delegate to the former so incremental migration is possible.

steve-the-edwards · 2020-10-05T15:51:54Z

...rs/hello-back-button/src/main/java/com/squareup/sample/hellobackbutton/AreYouSureWorkflow.kt

 import kotlinx.android.parcel.Parcelize

+@OptIn(WorkflowUiExperimentalApi::class)
+private typealias Rendering = ModalContainerViewRendering<*, AlertModalRendering>


I've noticed this 'generic type aliasing' pattern with our Actions and now Rendering/Screens for Workflows. I've always felt like it makes things harder to follow rather than easier because I need to constantly look up what the actual types are. What advantage am I missing with this pattern?

Note that this one is private -- it was strictly a convenience to avoid some copy / paste, and arguably a bad choice for the reaons you mentioned. Our trend has been to use fewer and fewer type aliases, especially public ones.

Some of the lingering ones are leftovers from before we had the more concise action {} lambdas.

steve-the-edwards · 2020-10-05T16:02:43Z

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

 import android.view.ViewGroup
 import kotlin.reflect.KClass

-/**


Do we need some kind of documentation here just for archaeology?

steve-the-edwards · 2020-10-07T14:00:55Z

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

+ * has never been called.
+ */
+@WorkflowUiExperimentalApi
+fun <RenderingT : ModalRendering> Dialog.getDisplaying(): RenderingT? {


Is it worth an interface with showRenderingTag and displaying so we can abstract some of these duplicated functions into the same implementation?

nvm I see these extension properties are defined below (which of course they are as this is Dialog and View). No clear way to pull out an abstraction as an extension on Dialog and View I guess. 🤔

I guess you do almost already have an extension abstraction in ShowRenderingTag - why can't all these other extensions delegate into functions defined on that class itself. That way we wouldn't have the double maintenance. Granted this may not scale much beyond Dialog and View.

Except for would it if we introduce Compose?

bnvinay92 · 2020-10-17T18:10:26Z

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

+    for ((i, modalRendering) in newScreen.modals.withIndex()) {
+      updateDialogs +=
+        if (i < dialogs.size && compatible(dialogs[i].getDisplaying()!!, modalRendering)) {
+          dialogs[i].apply { display(modalRendering, viewEnvironment) }


AFAIK alert dialogs can't be updated in place. Perhaps AlertModalRendering should implement Compatible?

zach-klippenstein · 2020-11-16T23:45:51Z

This change seems to require all screen renderings to be completely opinionated about whether they should be a modal or not. Is that true? What happens if a rendering type wants to be usable as either a modal or a non-modal, and so implements both marker interfaces? Do we explicitly allow/forbid that, do we check for it anywhere?

As a straw-view, consider ComposeRendering: This is a generic rendering type that can render itself. It doesn't care if it's in a modal or not, it just defines how to render. Could it implement both marker interfaces?

rjrjr · 2020-11-17T18:27:07Z

This change seems to require all screen renderings to be completely opinionated about whether they should be a modal or not. Is that true? What happens if a rendering type wants to be usable as either a modal or a non-modal, and so implements both marker interfaces? Do we explicitly allow/forbid that, do we check for it anywhere?

As a straw-view, consider ComposeRendering: This is a generic rendering type that can render itself. It doesn't care if it's in a modal or not, it just defines how to render. Could it implement both marker interfaces?

My original thought was that ComposeRendering would be a completely different beast, the fourth implementation of ViewRegistry.Entry. Haven't dug in at all to see if that makes sense. Seems like it should -- it'd identify a rendering that needs some kind of compose-friendly home. Perhaps we'll also have something like data class ComposeContainerView<C: ComposeRendering>(val composed: C): ViewRendering?

rjrjr commented Oct 2, 2020

View reviewed changes

rjrjr force-pushed the ray/better-modals-195 branch from f520e16 to 7818bf4 Compare October 2, 2020 21:13

zach-klippenstein added this to the post-v1.0.0 milestone Oct 6, 2020

steve-the-edwards reviewed Oct 7, 2020

View reviewed changes

rjrjr added 2 commits October 13, 2020 16:05

wip: naming

d12d35f

wip: updated backstack

499b8fd

bnvinay92 reviewed Oct 17, 2020

View reviewed changes

rjrjr mentioned this pull request Nov 17, 2020

wip: ViewRegistry overhaul, ViewRendering, ModalRendering #199

Closed

rjrjr mentioned this pull request Nov 19, 2020

Introduces AndroidViewRendering, de-emphasizes ViewRegistry #248

Merged

rjrjr mentioned this pull request Nov 5, 2021

Introduces Screen, WithEnvironment : Screen. Deprecates ViewFactory #577

Merged

rjrjr closed this Nov 5, 2021

rjrjr mentioned this pull request Nov 11, 2021

Decouple modality from modal visual treatment #195

Closed

rjrjr deleted the ray/better-modals-195 branch November 12, 2021 15:48

wip: ViewRegistry overhaul, ViewRendering, ModalRendering #202

wip: ViewRegistry overhaul, ViewRendering, ModalRendering #202

Uh oh!

Conversation

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

Uh oh!

Uh oh!

rjrjr Oct 2, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

zach-klippenstein Oct 6, 2020 • 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!

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!

zach-klippenstein commented Nov 16, 2020

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rjrjr commented Oct 2, 2020 •

edited

Loading

rjrjr Oct 2, 2020 •

edited

Loading

zach-klippenstein Oct 6, 2020 •

edited

Loading

rjrjr commented Nov 17, 2020 •

edited

Loading