-
Notifications
You must be signed in to change notification settings - Fork 112
Introduces Screen, WithEnvironment : Screen. Deprecates ViewFactory #577
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1417717 to
23488af
Compare
| */ | ||
| @WorkflowUiExperimentalApi | ||
| public fun View.showFirstRendering() { | ||
| showRendering(getRendering()!!, environment!!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to replace showRendering(Any) with showScreen(Screen), but this PR is already big enough.
| */ | ||
| @WorkflowUiExperimentalApi | ||
| @PublishedApi | ||
| internal class LayoutScreenViewFactory<RenderingT : Screen>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tempted to change all of these RenderingT param types to ScreenT. Downside is that we lose the hint that these are expected to be the RenderingT from a workflow. Probably won't mess.
| import kotlin.reflect.KClass | ||
|
|
||
| /** | ||
| * The [ViewEnvironment] service that can be used to display the stream of renderings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be updated to be less Android specific, and fix broken links. Will get to it in follow up.
1e9de52 to
193ab93
Compare
193ab93 to
49183bb
Compare
49183bb to
24a7ba4
Compare
|
Spent the rest of the day polishing this up -- updating samples, muting deprecations, fixing docs. @steve-the-edwards, @Blisse, @mbrubin56-square, @helios175, I'd like to get LGs from at least a couple of you before merging. I don't know if it'll be green before I give up for the day, but it'll be close. I think I'll update the |
93ed6f1 to
bd6184c
Compare
workflow-ui/compose/src/main/java/com/squareup/workflow1/ui/compose/CompositionRoot.kt
Show resolved
Hide resolved
bd6184c to
cca7597
Compare
samples/containers/android/src/main/java/com/squareup/sample/container/BackButtonViewFactory.kt
Outdated
Show resolved
Hide resolved
cca7597 to
fed57f6
Compare
samples/tictactoe/app/src/main/java/com/squareup/sample/mainactivity/TicTacToeActivity.kt
Outdated
Show resolved
Hide resolved
workflow-ui/core-android/src/test/java/com/squareup/workflow1/ui/TestViewFactory.kt
Outdated
Show resolved
Hide resolved
2901a45 to
cc4cfcc
Compare
|
Oh joy. Notice the conspicuous absence of any workflow code in that stack. |
fef4c41 to
edd4fcc
Compare
Note that ModalContainer API isn't completely ported to Screen, and Compose support ignores it entirely.
ae60cee to
bd658fd
Compare
| * Typically the rendering type (`RenderingT`) of the root of a UI workflow, | ||
| * but can be used at any point to modify the [ViewEnvironment] received from | ||
| * a parent view. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Introduces the Screen marker interface, similar to the one in workflow-swift
- Introduces the RootScreen rendering type as the preferred way to manage the ViewEnvironment, and makes WorkflowLayout use it
What is RootScreen
I guess I'm trying more to understand what does "Root" mean in the context of Workflows here. In my mind Root almost means singular, but this seems more generic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be a bad name. Really it's just a screen that can wrap another and provide a new ViewEnvironment at the same time. The 99% use case is at the top of a view hierarchy, and so that's what I named it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense as a utility. I think that we need a new dominant metaphor for naming this as the 'tree' metaphor is lacking here when talking about transforming the ViewEnvironment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a huge bikeshed I'd like to paint as I wholeheartedly do not resonate with the Screen naming. This is especially a problem when we introduce Modal as that may begin to suggest that a Modal is anything less than a "full" "Screen" when that it is explicitly not what it wants to represent. Can talk in slack or elsewhere.
https://workflow-community.slack.com/archives/CHTFPR277/p1636572894038600 |
|
Gonna try to summarize the bikeshed here. On the Responding @steve-the-edwards's immediate concern:
The implementations that We don't want We can't co-opt
Both Some Q & A: What’s the name of the component/idea that the Workflow runtime uses to convert the Container into the various Screen+Modals that show up on screen? In How does the Workflow runtime convert a Container (composed of Screens and Modals) into Views+Dialogs? Like who does the mapping? Consider this example: A workflow might render this: To turn that into
That method calls |
|
Maybe I should add the comment above to the commit message. |
|
Hmmmm. Probably this: fun <T: Screen> ViewEnvironment.buildView(screenRendering: T): Viewshould be this: fun <T: Screen> T.buildView(environment: ViewEnvironment): ViewMakes the marker interface serve more of a purpose, and is an even closer parallel to how the Swift code works. |
I like it a lot. The As for |
I like the extra extension. How about |
|
Actually…I can't find any place that we would ever actually call the |
fade109 to
bde238a
Compare
|
Okey doke, I have…
I'll plan to merge later today, barring any further feedback. |
Renames `RootWorkflow` to `WithEnvironment`. Moves `ViewEnvironment.buildView` to `Screen`.
bde238a to
3f479a8
Compare
| * @throws IllegalArgumentException if no factory can be find for type [RenderingT] | ||
| */ | ||
| @WorkflowUiExperimentalApi | ||
| public fun <RenderingT : Any> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are nearly verbatim copies of existing extensions on ViewRegistry.
| import org.junit.Test | ||
|
|
||
| @OptIn(WorkflowUiExperimentalApi::class) | ||
| internal class DecorativeScreenViewFactoryTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy / paste
| import android.view.ViewGroup | ||
| import kotlin.reflect.KClass | ||
|
|
||
| /** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy / paste
| import kotlin.reflect.KClass | ||
|
|
||
| /** | ||
| * A [ScreenViewFactory] that creates [View]s that need to be generated from code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy / paste of BuilderViewFactory
| * implement this interface at all. For details, see the three overloads of [ScreenViewRunner.bind]. | ||
| */ | ||
| @WorkflowUiExperimentalApi | ||
| public fun interface ScreenViewRunner<RenderingT : Screen> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy / paste of LayoutRunner
First of several PRs replacing #202 and addressing #195, overhauling
workflow-ui. Aimed at long living branchray/ui-update.Step One (this PR):
ScreenandWithEnvironment : ScreenViewEnvironmentandViewRegistryfrom AndroidViewEnvironment.updateFromto simplify combining environments and their nestedViewRegistryentries.Screenmarker interface, similar to the one inworkflow-swiftWithEnvironmentrendering type as the preferred way to manage theViewEnvironment, and makesWorkflowLayoutuse itBackStackScreento workflow-ui:core-commonThis work is intended to be mostly backward compatible. My goal is that client code should require no more than updating some imports and ignoring a lot of
@Deprecatedwarnings to upgrade.RenderingT : AnyRenderingT : ScreenLayoutRunnerScreenViewRunnerViewFactory<T>ViewRegistry.Entry<T>,ScreenViewFactory<T: Screen> : Entry<T>AndroidViewRenderingAndroidScreenWorkflowViewStub.update(Any)WorkflowViewStub.show(Screen)ViewRegistry.buildView(Any, ViewEnvironment, ...)Screen.buildView(ViewEnvironment, ...)WorkflowLayout.start(Flow<Any>)WorkflowLayout.take(Flow<Screen>)Yes,
Screenis kind of a weird choice when what we really mean isView. But it's well established inworkflow-swift, and doesn't seem to confuse anyone. More on this below.Step Two:
OverlayreplacesModalContainerModalContainerand the Compose support are basically unchanged in this PR, and still built against the now deprecated machinery. They'll be addressed in follow ups, also merged toray/ui-update.The Modal changes will be pretty drastic. The idea is to introduce another marker interface,
Overlay<T>, and a correspondingOverlayDialogRunner<OverlayT : Modal> : ViewRegistry.Entry<ModalT>.OverlayDialogRunnerwill replace the abstract methods inModalContainer, and alsoModalContainer.DialogRef.With that, we won't have to make a new
FrameLayoutsubclass every time we want a new type of dialog. To show, say, an alert over a modal backstack panel over a body, we can render something like:Step Three: Compose
Hoping this lets us do something cleaner than extending
AndroidViewRendering<Nothing>. Current strawman:ScreenComposable<T: Screen> : ViewRegistry.Entry<T>Screen.buildViewScreenComposableif it fails to findScreenViewFactoryScreenComposableis found, returns aScreenViewFactorylike today'sComposeViewFactory-- creates aComposeViewand uses it to hostScreenComposable.content()@Composable Screen.BoxRendering(rendering: Screen, modifier: Modifier)WorkflowRendering, which usesBox()but didn't document that factWorkflowViewStubandgetViewFactoryForRendering(), is that okay?ComposeScreen : Screenis like today'sComposeRendering, analog toAndroidScreenThis will probably require replacing the
ViewRegistryinterface with something that it's actually possible to wrap, in order to:workflow-ui:androiddepend upon the Compose runtime, which would be rude to library authorsTerminology and concepts: Why "Screen"?
On the
mainbranch, the only thing that I can map a rendering type to is aViewFactory, which can only buildandroid.view.View. The driving goal of the work kicked off by this PR is to make it possible for us to map rendering types to other things. Most immediately, we'd like to be able to map some rendering types directly toandroid.app.Dialog, without having to define an entire new classes ofandroid.view.Viewto host them.Screenis the marker interface for something view-like.android.view.View@Composablefunction that gets wrapped inBox() {}Modalis a terrible name, so we'll replace it withOverlay. It is the marker interface for something window-like.android.app.Dialogat the momentFrameLayoutthat acted window-like. That might happen again.The implementations that
ScreenandOverlaybind to are not interesting. The fact that we could easily implement both viaView, or via@Composable, is not interesting. The key distinction is their behavior, and that they are definitely not interchangable.We don't want
Renderingas the marker interface for something view-like. TheRenderingTtype parameter of theWorkflowinterface exists already. Having an interface with the same name would be terribly misleading. A workflow can renderScreens, it can renderOverlays, or it can render anything else at all, even things completely unrelated to UI concerns.We can't co-opt
Viewas that marker interface either. In both Android and iOS,Viewis no longer just UI terminology, it indicates a concrete class. An Android developer readsFooViewto mean a subclass ofandroid.view.View. For the same reason,Overlayis more attractive thanDialogorWindow.Screenis nice because it isn't used yet, and yet it's familiar. "I was on the home screen, I was on the checkout screen." The term was in widespread use long before Workflow showed up, and when we started using it as our marker term in Swift, it was understood intuitively. And back to the "why not View" point, it is not uncommon in our code base for bothclass FooScreenandclass FooViewto exist, where the former is the model for the latter.Both
ScreenandOverlayare examples of "UI renderings." UI rendering is likely to be a useful term in documentation, but we have no need for a common parent interface for them to extend.Screen : Any, there is no need forScreen : UiRendering. Solves no problems.Some Q & A:
What’s the name of the component/idea that the Workflow runtime uses to convert the Container into the various Screen+Overlays that show up on screen?
In
maintoday, that’sViewFactory. In this PR, it’sViewRegistry.Entry. There is still only one type ofEntryin this PR, but I'm looking to follow up quickly with at least two more:How does the Workflow runtime convert a Container (composed of Screens and Overlays) into Views+Dialogs? Like who does the mapping?
Consider this example:
A workflow might render this:
To turn that into
ViewandDialoginstances, in the new world:BodyAndOverlays.buildView()is called, probably byWorklowViewStub.show().buildView()finds the associatedScreenViewFactory<BodyAndOverlays<*, *>>, which probably inflates aBodyAndOverlaysContainer : FrameLayout, and calls a privateshow(BodyAndOverlays, ViewEnvironment)method on it.That method calls
WorklowViewStub.show(rendering.body)to show the body, and something like [TBD]Overlay.buildDialogRunner(coveringView: this)for each entry inrendering.overlays