Wrapper renderings introduce an extra AndroidView unnecessarily when the wrapped rendering's factory is a ComposeScreenViewFactory.

[zach-klippenstein]

trafficstars

I just realized this wrapping pattern (even before this change) might introduce an extra view unnecessarily when the wrapped rendering's factory is a ComposeViewFactory. It shouldn't affect correctness but something we should probably check/test and to keep in mind as yall adopt compose.

Originally posted by @zach-klippenstein in https://github.com/square/workflow-kotlin/pull/544#discussion_r704522274

I think this could probably be addressed if you came up with a way to generalize the wrapping pattern (as @rjrjr suggested on slack), since WorkflowRendering's asComposeViewFactory could use that to do the type check at the leaf.

[rjrjr]

This is still a problem. I've tried and failed to find a general wrapping pattern a number of times, and I'm not confident a practical solution will emerge. A lot of wrapping is done for very view-system-specific purposes, like manipulating View instances and messing with Context. We'd need to come up with a two pass system that allows platform neutral code to manipulate just the structure and ViewEnvironment, and another that actually builds a View or provides a Composable.

In the meantime, I think we need to have the workflow compose module provide an alternative ScreenViewFactoryFinder that replaces the stock bindings for our out of the box wrappers (NamedScreen, EnvironmentScreen, AsScreen) with Compose variants. And provide tools for app developers to do the same.

[rjrjr]

VisualFactory to be introduced with #874 should address this.

[rjrjr]

VisualFactory is still far from production ready, and migrating to it will be yet another big lift. In the meantime, we might be able to give asComposeViewFactory() hooks to put Compose or Classic alternative bindings in place -- perhaps alternative ScreenViewFactoryFinder implementations?

@helios175 has done something similar to this in a one-off.

[lucamtudor]

We're lucky enough to have almost 100% Compose UI so I'm planning to tackle this internally, and maybe just do this:

In the meantime, I think we need to have the workflow compose module provide an alternative ScreenViewFactoryFinder that replaces the stock bindings for our out of the box wrappers (NamedScreen, EnvironmentScreen, AsScreen) with Compose variants. And provide tools for app developers to do the same. (uplink)

@rjrjr or @helios175, any new thoughts or pointers would be very much appreciated.

[rjrjr]

I'm starting a spike on this today, hoping to apply lessons learned from #874 without rewriting the entire world. Basic idea is:

• Add ScreenComposableFactory<in ScreenT : Screen> and ScreenComposableFactoryFinder
• Create a ViewEnvironment transform function that replaces ScreenComposableFactoryFinder and ScreenViewFactoryFinder with wrappers that are able to do the on demand conversion tricks that today are hard coded into ScreenViewFactory.asComposeViewFactory() and ComposeScreenViewFactory, document that hybrid apps should apply it to their root ViewEnvironment
• Change @Composable fun RenderWorkflow() to use ScreenComposableFactoryFinder

I think this should allow us to provide both Classic and Compose flavors for whatever renderings we want.

Something will have to change about ViewRegistry, though, since TypedViewRegistry supports only one one binding per concrete rendering type, and is built into ViewRegistry.plus and such.

Perhaps ViewRegistry develops peers like DialogRegistry and ComposableRegistry, and they all extend a FactoryRegistry base type? And perhaps the ScreenViewFactoryFinder interface can be unified with FactoryRegistry?

[rjrjr]

And perhaps the ScreenViewFactoryFinder interface can be unified with FactoryRegistry?

As much as I like that notion, it likely means "productionize #874", and I don't know that I can land that in a reasonable amount of time.

[rjrjr]

Perhaps ViewRegistry develops peers like DialogRegistry and ComposableRegistry, and they all extend a FactoryRegistry base type?

Less clean but maybe a smaller delta:

interface ViewRegistry {
  data class Key<RenderingT, FactoryT>(
    val renderingType: KClass<RenderingT>,
    val factoryType: KClass<FactoryT>
  )

  interface Entry<RenderingT: Any, FactoryT: Any> {
    val key: Key
  }
  
  val keys: Set<Key<*, *>>

  fun <R: Any, F: Any> getEntryFor(key: Key<R, F>): Entry<R, F>?
}

[rjrjr]

Actually…holding off on this until we can delete the legacy stuff like LayoutRunner, which we're very close to. That will make any cross cutting work like this 10x simpler. Bootstrapping in particular (ScreenViewFactory.startShowing) is a real mess right now because of the need to keep the old world working.

[rjrjr]

Working on this again, looks doable.