ScreenViewHolder replaces View.* extensions and DecorativeViewFactory

[rjrjr]

Replaces #703.

The legacy workflow-ui world was built around a set of extension methods on View, backed by a WorkflowViewState object hanging off of a tag. This was overly clever, needlessly threw away a good bit of compile time safety, and made it much harder than it should be to implement wrapper renderings.

In particular, every ViewFactory was required to call View.bindShowRendering to put all this machinery in place. When a screen rendering wrapped another, with the intention of delegating to its registered ViewFactory, that required creating a wrapper ViewFactory that made a new bindShowRendering call replacing and wrapping the WorkflowViewState put in place by the delegate. That's so tricky it required the introduction of DecorativeViewFactory, which was still pretty tricky to use.

To solve all this, we reverse the relationship between View and WorkflowViewState, with the latter being replaced by ScreenViewHolder. Instead of returing a raw View, ScreenViewFactory.buildView now returns the view in a ScreenViewHolder. The call to ScreenViewHolder() replaces the old call to bindShowRendering, and the compiler makes it impossible to forget.

ScreenViewHolder is a box that holds:

• a View
• the (Screen, ViewEnvironment) -> Unit function that updates it (still fun interface ScreenViewRunner for ease of implementation)
• and the latest ViewEnvironment that function received

It is worth noting that ScreenViewHolder.showing .canShow and .show() are all static extensions, built around the new Showing: ViewEnvironmentKey<Screen>. ScreenViewHolder itself is "write only", with an in ScreenT: Screen type parameter -- you can push strongly typed Screen renderings into it, but you can't pull them out again.

This minimalist ScreenViewHolder interface is trivial to wrap, so all the complexity that inspired DecorativeViewFactory is gone. It is replaced by the new ScreenViewFactory.unwrapping extension functions, whose use is demontrated in NamedScreenViewFactory, EnvironmentScreenViewFactory and BackButtonScreen.

The ViewStarter mechanism is also much simpler now, also no longer relying on any state stored in View tags. The implementation of ScreenViewFactory.start() remains a bit complex, but only because of the work it has to do to keep the legacy View.start() method working, in partnership with AsScreenViewFactory(). Once we delete the deprecated View code, most of the code in both of those can be deleted.

Fixes #413, fixes #598, fixes #626, fixes #551, fixes #443.

# https://sequencediagram.org/
title WorkflowViewStub.show()

participant "WorkflowViewStub" as stub
participant Screen
participant "ViewEnvironment" as env
participant "ScreenViewFactoryFinder" as finder
participant "ViewRegistry" as registry
participant "ScreenViewFactory" as factory
participant "ScreenViewHolder" as holder

[->stub:show(screen, env)

stub->Screen:buildView(env)
activate Screen
Screen->Screen:toViewFactory(env)
activate Screen
Screen->env:get(ScreenViewFactoryFinder)
Screen<--env:finder
Screen->finder:getViewFactoryForRendering(this)
activate finder
finder->registry:getEntryFor(screen)
finder<--registry:factory
Screen<--finder:factory
deactivate finder
deactivate Screen

Screen->factory:start(this, env)
activate factory
factory->factory:buildView(screen, env)
activate factory
factory->holder:init(screen, env, runner)
holder-->factory:holder
deactivate factory
factory->holder:show(screen, env)
activate holder
holder->holder:this.environment = env + (Showing to screen)
holder->holder:this.runner.show(screen, environment)
deactivate holder
Screen<--factory:holder
deactivate factory

stub<--Screen:holder
deactivate Screen


[->stub:show(screen, env)
activate stub
stub->holder:canShow(screen)
activate holder
holder->holder: compatible(this.environment[Showing], screen)
stub<--holder:true
deactivate holder
stub->holder:show(screen, env)
activate holder
holder->holder:this.environment = env + (Showing to screen)
holder->holder:this.runner.show(screen, environment)
deactivate holder
deactivate stub

[rjrjr]

I've opened this open for review again because it's FINALLY GREEN. It's a bit sprawling, though, because a bunch of method renames that were previously in a separate PR have been squashed into this one. Looks like I dropped some important kdoc too.

I'm used up for today, I'll see if I can make it easier to digest in the morning. If anyone wants to dig in in the meantime, the meaty bits are ScreenViewFactory.kt and ScreenViewHolder.kt.

[rjrjr]

Notice how bindShowRendering calls migrate to ScreenViewHolder instantiation.

[rjrjr]

This is our only remaining use of View.setTag. I don't see an alternative.

[rjrjr]

Compare this with #703, where we were forced to cast (stub as WorkflowViewStub)

[rjrjr]

This mistake is now impossible.

[rjrjr]

Less concise, but I hope also less mysterious.

[rjrjr]

Not crazy about this name. forBuilder? forConstructor? fromCode?

[steve-the-edwards]

Whether it is reading this for hours or not I am not sure, but it is clicking with me.

[RBusarow]

I didn't love forBuiltView in the previous PR, but eventually got comfortable with it. This week, I had to re-learn what it meant. Without the context of having just looked at its siblings, I don't think it makes a lot of sense.

Perhaps forProgrammaticView? Or forViewFromCode so that it's consistent with the for prefix. Or use from___ for them all.

[steve-the-edwards]

I liked forXYZ so much more than bind but now I'm jumping back on here to say I do love @RBusarow 's from__ suggestion! or even buildFrom__

buildFromCode buildFromLayoutResource buildFromBinding

[rjrjr]

I like from_ a lot. Re: build, I like building even better. But either one is verbose, and I've had to waste so much time coping with line-too-long lints.

[rjrjr]

TODONE

[steve-the-edwards]

Halfway through. will break until tomorrow. Looks good so far! Really like the shape of the holder more!

[steve-the-edwards]

Very grateful for these forXYZ renamings!

[steve-the-edwards]

You probably explained this and I read it but didn't fully understand but why don't we need the Screen/Rendering type passed in anymore? Isn't this still the key for ViewRegistry?

[steve-the-edwards]

Oh i see we type-parametrized the method.

[steve-the-edwards]

😓 . Ok. I think this will be great and much easier to follow. Lots of comments on naming and such though. If you think these can all just live as backlog items for while you are away I guess i'm OK with that?

[steve-the-edwards]

Was this just unnecessary or is there something significant to losing the explicit declared type?

[rjrjr]

Unnecessary, the factory function is already doing that for us.

[steve-the-edwards]

You use a mix of ScreenViewFactory.forBuiltView and just forBuiltView and a mix of named buildView vs anonymous.

Is this just to show options? Or do we want to standardize in the sample code?

[steve-the-edwards]

Personally I would lean towards the ScreenViewFactory.forBuiltView and the named argument as I think it reads more clearly.

[rjrjr]

Combination of rushing and avoiding long lines, and don't care very much about consistency in non-sample code.

[rjrjr]

In the sample code, agree with you that ScreenViewFactory.forBuiltView is much better.

[steve-the-edwards]

This is confusing. At first it sounded like forLayoutResource and forViewBinding are extensions on ScreenViewRunner - which they are not. They accept a functional argument that constructs a ScreenViewRunner for the named component - a 'layout resource' or a 'view binding'.

what about something like?

Suggested change

	* Use [forLayoutResource], [forViewBinding], etc., to create a [ScreenViewFactory]
	* from a [ScreenViewRunner].
	* Use [forLayoutResource], [forViewBinding], etc., to create a [ScreenViewFactory].
	* These helper methods take the resource, view, and/or view binding as arguments
	* along with a constructor function that creates the `ScreenViewRunner` `showRendering`
	* function.

As I type that out I realize how important it is that we just think of the ScreenViewRunner as a way to refer to the type parameterized showRendering function for any ScreenT. That is the important bit and I want to keep drawing back references to that.

[rjrjr]

Much better, thanks.

[rjrjr]

 * Use [forLayoutResource], [forViewBinding], etc., to create a [ScreenViewFactory].
 * These helper methods take a layout resource, view binding, or view building
 * function as arguments, along with a factory to create a [showRendering]
 * [ScreenViewRunner.showRendering] function.

[helios175]

I was about to ask the purpose of ScreenViewRunner if it was a way to keep Holder as static/fixed as possible, and place the polymorphism in the Runner. Like a "strategy". From the comments I think that's the reason. :)

[steve-the-edwards]

remind me again why these are extensions rather than implemented in the interface? We just want them to be final to avoid footguns?

[rjrjr]

More than that: when wrapping, it's to prevent recursive calls from clobbering environment[Showing] with the nested rendering type. I'll add an inline comment to call that out.

[rjrjr]

  // Why is this an extension rather than part of the interface?
  // When wrapping, we need to prevent recursive calls from clobbering
  // `environment[Showing]` with the nested rendering type.
  runner.showRendering(screen, environment + (Showing to screen))

[steve-the-edwards]

Is this actually what the extension is still called? I don't remember toView

[rjrjr]

It became buildView, but now I've gone ahead and inlined it! Will fix.

[steve-the-edwards]

Whether it is reading this for hours or not I am not sure, but it is clicking with me.

[steve-the-edwards]

Wha? Was this just for the NothingShowing case? Why did that have to be a function?

[rjrjr]

It didn't have to be a function, it's just a convenience that makes it easier to implement ViewEnvironmentKey, should have written it two years ago. I'd like to make a pass and use it universally after this merges.

[rjrjr]

#715.

[helios175]

nit: I think you could use private set:

override var environment: ViewEnvironment = initialEnvironment
  private set

[rjrjr]

I can't, the public API is val. But if I drop the interface, then hell yeah.

[helios175]

It sounded to me like the two examples were different in shape, but they look the same. Same structure, same calls? (not saying it's bad, just asking if I'm missing any difference)

[rjrjr]

You know, you're right. I originally had dropped the second one, but then tired me thought it actually covered more ground.

It really doesn't, deleting it.

[helios175]

Is the plan to make this type as sealed as possible? I'm asking because the polymorphism is extracted out to the runner.
If the answer is yes this could be a class, or a sealed class at least, or something. Definitely not important.

But maybe worth mentioning in the docs:

• holder: pretty static / fixed / controlled / not-instantiable from outside, etc
• runner: the "strategy" (strategy pattern) to implement a particular holder.

[rjrjr]

I kept bouncing back and forth btw. using an interface or just making RealScreenViewHolder public (and dropping the Real prefix, obv.). I forget what finally drove me back to the interface, something related to ease of wrapping. Unclear to me that it's still necessary, I'll take one more look at dropping

[rjrjr]

I think I can pull this off, but I'd prefer to merge what I have and tackle it in a follow up. In the meantime I'll try to punch up the docs as you suggest.

[rjrjr]

The important thing about the interface approach is that it makes it easy to wrap a ScreenViewHolder and delegate to the environment of the wrapped instance, which is crucial for our crazy showing semantics. I suppose we could achieve that another way, but I don't really see what the benefit would be.

This is the case that drove me to write EnvironmentScreenAndroidIntegrationTest last week.

[helios175]

I've been wandering around the code for 1h+ and everything makes sense so far.
This is awesome!

Some comments around making sure we understand the purpose (as in Matrix's Agent Smith purpose) of each class, and making sure !purpose(Holder).intersects(purpose(Runner))