Replaces *.initializeView with *.viewStarter

[rjrjr]

Fixes #597.

It used to be the case that every DecorativeViewFactory called
showRendering() twice (#397). We fixed that (we thought) by introducing the
initializeView lambda to ViewRegistry.buildView and
DecorativeViewFactory (#408).

Unfortunately, that fix botched recursion. Individual DecorativeViewFactory
instances work fine, but if you wrap them you still get one showRendering
call from each. Worse, upstream initializeView lambdas are clobbered by
immediately downstream ones. e.g., when a WorkflowViewStub shows a
DecorativeViewFactory, the WorkflowLifecycleRunner.installOn call in the
former is clobbered.

The fix is to completely decouple building a view from from this kind of
initialization. ViewRegistry.buildView and its wrappers no longer try to
call showRendering at all.

Instead the caller of buildView (mostly WorkflowViewStub) is reponsible
for immediately calling View.start on the new View. View.start makes
the initial showRendering call that formerly was the job of
ViewFactory.buildView -- the factory builds the view, and the container
turns the key. Since View.start is called only after all wrapped
ViewFactory.buildView functions have executed, we're certain it will only
happen once.

Of course we still need the ability to customize view initialization via
wrapping, especially to invoke WorkflowLifecycleOwner.installOn. To
accomodate that, the function that View.start executes can be wrapped via
the new viewStarter argument to ViewRegistry.buildView and
DecorativeViewFactory, which replaces initializeView.

This required a pretty thorough overhaul of ViewShowRendering.kt
The ViewShowRenderingTag that it hangs off of a view tag is renamed
WorkflowViewState, and extracted to a separate file.

WorkflowViewState is a sealed class with two implementations (New and
Started) to help us enforce the order of the ViewRegistry.buildView,
View.bindShowRendering, View.start and View.showRendering calls.

[zach-klippenstein]

[steve-the-edwards]

TODO?

[rjrjr]

Oopsy, that was supposed to shame me into writing the doc before asking for review.

[rjrjr]

 *
 * @param viewStarter An optional wrapper for the function invoked when [View.start]
 * is called, allowing for last second initialization of a newly built [View].
 * See [ViewStarter] for details.

/**
 * A wrapper for the function invoked when [View.start] is called, allowing for
 * last second initialization of a newly built [View]. Provided via [ViewRegistry.buildView]
 * or [DecorativeViewFactory.viewStarter].
 *
 * While [View.getRendering] may be called from [startView], it is not safe to
 * assume that the type of the rendering retrieved matches the type the view was
 * originally built to display. [ViewFactories][ViewFactory] can be wrapped, and
 * renderings can be mapped to other types.
 */
@WorkflowUiExperimentalApi
public fun interface ViewStarter {
  /** Called from [View.start]. [doStart] must be invoked. */
  public fun startView(
    view: View,
    doStart: () -> Unit
  )
}

[steve-the-edwards]

What is the variance that is not matching here?

by nature of only being an input parameter on the function is it wanting you to make RenderingT explicitly contravariant to ViewShowRendering?

Haven't tested, but my guess is

Suggested change

	public typealias ViewShowRendering<RenderingT> =
	(@UnsafeVariance RenderingT, ViewEnvironment) -> Unit
	public typealias ViewShowRendering<in RenderingT> =
	(RenderingT, ViewEnvironment) -> Unit

Should work without the Unsafe annotation?

[rjrjr]

It's been a while, but I think it's because that change is theoretically required but you're not allowed to do it with a typealias.

[steve-the-edwards]

ah gotcha. I kind of remember you saying something like that!

[steve-the-edwards]

Suggested change

	private var View.workflowTag: WorkflowUiViewState<*>
	private var View.workflowUiViewState: WorkflowUiViewState<*>

Tag (while it is the implementation) kept tripping me up. Not convinced this must be done but just a thought.

[rjrjr]

Seems helpful to me, will do.

[rjrjr]

And maybe rename WorkflowUiViewState to just WorkflowViewState while we're at it.

[steve-the-edwards]

Suggested change

	* Establishes [showRendering] as the implementation of [View.showRendering]
	* for the receiver, possibly replacing the existing one. Likewise sets / updates
	* the values returned by [View.getRendering] and [View.environment].
	* Establishes [showRendering] as the implementation of [View.showRendering]
	* for the receiver, possibly replacing the existing one. Likewise sets / updates
	* the values returned by [View.getRendering] and [View.environment] by setting
	* the [WorkflowUiViewState] tag on the View receiver.

[rjrjr]

Remember that WorkflowUiViewState is a private type.

[steve-the-edwards]

gotcha.

[steve-the-edwards]

so if the View WorkflowUiViewState was Started we just recycle it back to New?

[steve-the-edwards]

I guess that's what you mean by 'possibly replacing the existing one', but it was not clear to me whether you can replace it only before it is Started or you can replace it after as well?

[rjrjr]

This is mostly about, if there is already a New tag we have to propagate the starter.

That actually happens a lot. When a view factory gets wrapped, this method gets called again to rebind the view to the wrapper rendering type, so repeated calls are the norm.

• FooScreen : Screen
• We render NamedScreen(FooScreen())
• The view is built by FooScreenFactory, which calls bindShowRendering<FooScreen>()
• NamedScreenFactory invokes FooScreenFactory.buildView, and calls bindShowRendering<NamedScreen<*>>() on the view that FooScreenFactory built.

I actually had a case for is Started<*> that would throw when this was called, but I got afraid of making it difficult to reuse views. I figured just letting it fall through to the else would be more like the current behavior.

[rjrjr]

I'll add doc to this effect.

[steve-the-edwards]

Ack. As long as its safe and understood that we could re-use them. Wasn't sure if we needed cleanup hooks before being re-used but I guess this is functionally cleaning up the WorkflowUiViewState which is one good reason to keep it all in that type/tag.

[steve-the-edwards]

Suggested change

	* It is usually more convenient to use [WorkflowViewStub] than to call this method directly.
	* This is not necessary when using [WorkflowViewStub] as it will handle this check for you.

[steve-the-edwards]

Goodness me this is the first time I have deeply pined for Swift's protocol extensions.

I get why we need all of this as extensions on the View but it is very difficult to follow the logic with all these (albeit handy) extension shortcuts.

I guess it's not possible (given the coupling) to extend View to a WorkflowManagedView to have all these extensions in a more understandable form?

[rjrjr]

Nope, this approach is entirely about avoiding the need for custom View classes.

We could create a facade interface that just wrapped a plain old View. I don't think it's worth the effort. Feature developers basically never bump into this stuff, thanks to WorkflowViewStub and such. That will be even more true when the UI overhaul is done, I think.

But just to scratch the nerdsnipe itch, here's a sketch of what that could be.

fun <RenderingT> ViewRegistry.buildView(...): WorkflowView<RenderingT>

interface WorkflowView<RenderingT> {
  fun asView(): View
  
  fun start()

  val rendering: RenderingT
  val environment: ViewEnvironment

  fun update(
    rendering: RenderingT,
    environment: ViewEnvironment
  )
  // ...
}

fun <RenderingT> View.asWorkflowView(): WorkflowView<T>? {
  // Looks for the tag and confirms the type is right? 
}

Still thinking this would be a distraction, but I'm wavering. Dammit Steve, are you proud that you've done this to me?

[rjrjr]

And WorkflowViewStub could implement WorkflowView<*>! All the stock containers could!

[rjrjr]

Sigh, tracked: #606

[rjrjr]

Goodness me this is the first time I have deeply pined for Swift's protocol extensions.

Man, I've been wishing for those since I left Objective-C for Java.

[steve-the-edwards]

Haha. Amazing. Very proud. I get your point though that the audience is small. This does make it a lot easier to grok though IMHO.

[steve-the-edwards]

Suggested change

	* Sets the rendering associated with this view, and displays it by invoking
	* the [ViewShowRendering] function previously set by [bindShowRendering].
	* Shows [rendering] in this View by invoking
	* the [ViewShowRendering] function previously set by [bindShowRendering].

I get its setting a tag but that is the implementation detail I think and not necessary to the outcome of the function.

[steve-the-edwards]

I get that maybe this makes it more fluent in showRendering above but adding another layer of indirection is a lot to carry in your mind when grokking.

Not a big deal but maybe can we just use compatible(tag.showing, rendering) above? Is there another use for this?

[rjrjr]

Update addresses @steve-the-edwards review.

• adds a lot of missing kdoc
• renames WorkflowUiViewState to WorkflowViewState
• brings back ViewShowRendering.kt for better diff'ing
• Moves WorkflowViewState to its own file for better readability.

[rjrjr]

Not true!

[rjrjr]

21 passed, 24 had a stupid socket error. I'm merging this.