BREAKING: Replaces `ComposeScreenViewFactory` with `ScreenComposableFactory` #1146

rjrjr · 2023-12-18T23:57:12Z

Please be sure to review workflow-ui/compose/README.md, which GitHub helpfully hides by default because it's big.

There are two commits in this PR, and it'd be wise to review them in sequence:

`ViewRegistry.Key` allows multiple factory types per rendering type

Paves the way for parallel Classic and Compose implementations of wrappers like NamedScreen, to fix #546. Ironically, breaks Compose support in the process, but that is fixed by the next commit.

Replaces `ComposeScreenViewFactory` with `ScreenComposableFactory`

This commit takes advantage of the new ViewRegistry.Key (which allows renderings to be bound to multiple UI factories) to fix a long standing problem where wrapper screens -- things like NamedScreen and EnvironmentScreen -- used in a Compose context would cause needless calls to @Composeable fun AndroidView() and ComposeView().

For example, consider this rendering:

BodyAndOverlaysScreen(
  body = SomeComposeScreen(
    EnvironmentScreen(
      SomeOtherComposeScreen
    )
  )
)

Before this change, that would create a View hierarchy something like this:

BodyAndOverlaysContainer : FrameLayout {
  mChildren[0] = ComposeView {
    // compose land
    SomeComposeScreen.Content {
      AndroidView {
        ComposeView {
          // nested compose land
            SomeOtherComposeScreen.Content()

Now it will look this way:

BodyAndOverlaysContainer : FrameLayout {
  mChildren[0] = ComposeView {
    // compose land
    SomeComposeScreen.Content {
      SomeOtherComposeScreen.Content()

ScreenComposableFactory replaces ComposeScreenViewFactory, and ComposeScreen no longer extends AndroidScreen. Compose support is now a first class citizen, instead of a hack bolted on to View support.

Unfortunately, ViewEnvironment.withComposeInteropSupport() (see below) now must be called near the root of a Workflow app to enable the seamless Compose > Classic > Compose handling that used to be built in to WorkflowRendering and ComposeScreenViewFactory. This means that call is required for Compose support for built in rendering types like BodyAndOverlaysScreen and BackStackScreen, which so far are backed only by classic View implementations.

Other introductions, changes:

Screen.toComposableFactory(), used by WorkflowRendering() in the same way that WorkflowViewStub uses Screen.toViewFactory()
ScreenComposableFactoryFinder, a ViewEnvironment-based strategy object used by Screen.toComposableFactory() the same way that Screen.toViewFactory() uses ScreenViewFactoryFinder. The default implementation provides Compose bindings for NamedScreen and EnvironmentScreen, fixing Wrapper renderings introduce an extra AndroidView unnecessarily when the wrapped rendering's factory is a ComposeScreenViewFactory. #546.
ScreenViewFactoryFinder.getViewFactoryForRendering() can now return null. A requireViewFactoryForRendering() extension is introduced for use when null is not acceptable.
ViewEnvironment.withComposeInteropSupport(), which wraps the found ScreenComposableFactoryFinder and ScreenViewFactoryFinder with implementations that allow Compose contexts to handle renderings bound only to ScreenViewFactory, and classic contexts to handle renderings bound only to ScreenComposableFactory. Replaces the logic that used to be in the private ScreenViewFactory.asComposeViewFactory() extension in WorkflowRendering().
Screen.Preview() is introduced. The existing Preview() extension functions were tied to ScreenViewFactory, making them much less useful. It is still the case that previews work for non-Compose UI code just fine. Which is pretty cool, really.

Fixes #546

rjrjr · 2024-01-08T19:44:53Z

This is looking really solid. I don't quite have our internal test suite passing yet, but so far everything I've found has been newly exposed bugs in our workflow integration, especially the hellish duct tape supporiting our remaining pre-workflow code. I'm pretty optimistic about shipping this month.

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

workflow-ui/compose/src/main/java/com/squareup/workflow1/ui/compose/ScreenComposableFactory.kt

…ing type Paves the way for parallel Classic and Compose implementations of wrappers like `NamedScreen`, to fix #546. Ironically, breaks Compose support in the process, but that is fixed by the next commit.

rjrjr · 2024-01-25T01:47:49Z

...w-ui/compose/src/androidTest/java/com/squareup/workflow1/ui/compose/WorkflowRenderingTest.kt

    }

    onView(withText("two")).check(matches(isDisplayed()))
    wrapperText.value = "OWT"
    onView(withText("OWT")).check(matches(isDisplayed()))
  }

+  @Test fun namedScreenStaysInTheSameComposeView() {


rjrjr · 2024-01-25T01:52:02Z

workflow-ui/compose/src/main/java/com/squareup/workflow1/ui/compose/WorkflowRendering.kt

- * otherwise it wraps the factory in one that manages a classic Android view.
- */
-@OptIn(WorkflowUiExperimentalApi::class)
-private fun <ScreenT : Screen> ScreenViewFactory<ScreenT>.asComposeViewFactory() =


This became ScreenComposableFactoryFinder

rjrjr · 2024-01-25T02:15:33Z

workflow-ui/compose/README.md


-The above two concepts coordinate, and when a Compose-based factory is delegating to a child rendering that is also bound to a composable factory, we can skip the detour out into the Android view world and simply call the child composable directly from the parent.
+> You'll note that there is no `OverlayComposableFactory` family of interfaces. So far, all of our window management is strictly via classic Android `Dialog` calls. There is nothing stopping us (or you) from adding Compose-based `Overlay` support in the future as a replacement, but we're definitely not making any promises on that front.


@ekeitho 👀

ekeitho · 2024-01-25T02:46:07Z

workflow-ui/compose-tooling/src/main/java/com/squareup/workflow1/ui/compose/tooling/Previews.kt

+
+/**
+ * Like [ScreenComposableFactory.Preview], but for non-Compose [ScreenViewFactory] instances.
+ * Yes, you can preview classic [View][android.view.View] code this way.


whoa, neat.

workflow-ui/compose/README.md

vgonda · 2024-01-25T20:18:20Z

workflow-ui/compose-tooling/README.md

@@ -1,3 +0,0 @@
-# Module compose-tooling
-
-TODO


Good riddance ;)

vgonda · 2024-01-25T20:22:19Z

workflow-ui/compose-tooling/src/main/java/com/squareup/workflow1/ui/compose/tooling/Previews.kt

+
+/**
+ * Like [ScreenComposableFactory.Preview], but for non-Compose [ScreenViewFactory] instances.
+ * Yes, you can preview classic [View][android.view.View] code this way.


workflow-ui/compose/README.md

...-ui/compose/src/main/java/com/squareup/workflow1/ui/compose/ScreenComposableFactoryFinder.kt

...compose/src/main/java/com/squareup/workflow1/ui/compose/ViewEnvironmentWithComposeSupport.kt

ijwhelan · 2024-01-25T23:14:51Z

workflow-ui/compose-tooling/src/main/java/com/squareup/workflow1/ui/compose/tooling/Previews.kt

+public fun Screen.Preview(
+  modifier: Modifier = Modifier,
+  placeholderModifier: Modifier = Modifier,
+  viewEnvironmentUpdater: ((ViewEnvironment) -> ViewEnvironment)? = null
+) {


…actory` This commit takes advantage of the new `ViewRegistry.Key` (which allows renderings to be bound to multiple UI factories) to fix a long standing problem where wrapper screens -- things like `NamedScreen` and `EnvironmentScreen` -- used in a Compose context would cause needless calls to `@Composeable fun AndroidView()` and `ComposeView()`. For example, consider this rendering: ``` BodyAndOverlaysScreen( body = SomeComposeScreen( EnvironmentScreen( SomeOtherComposeScreen ) ) ) ``` Before this change, that would create a View hierarchy something like this: ``` BodyAndOverlaysContainer : FrameLayout { mChildren[0] = ComposeView { // compose land SomeComposeScreen.Content { AndroidView { ComposeView { // nested compose land SomeOtherComposeScreen.Content() ``` Now it will look this way: ``` BodyAndOverlaysContainer : FrameLayout { mChildren[0] = ComposeView { // compose land SomeComposeScreen.Content { SomeOtherComposeScreen.Content() ``` `ScreenComposableFactory` replaces `ComposeScreenViewFactory`, and `ComposeScreen` no longer extends `AndroidScreen`. Compose support is now a first class citizen, instead of a hack bolted on to View support. Unfortunately, `ViewEnvironment.withComposeInteropSupport()` (see below) now must be called near the root of a Workflow app to enable the seamless Compose > Classic > Compose handling that used to be built in to `WorkflowRendering` and `ComposeScreenViewFactory`. This means that call is required for Compose support for built in rendering types like `BodyAndOverlaysScreen` and `BackStackScreen`, which so far are backed only by classic View implementations. Other introductions, changes: - `Screen.toComposableFactory()`, used by `WorkflowRendering()` in the same way that `WorkflowViewStub` uses `Screen.toViewFactory()` - `ScreenComposableFactoryFinder`, a `ViewEnvironment`-based strategy object used by `Screen.toComposableFactory()` the same way that `Screen.toViewFactory()` uses `ScreenViewFactoryFinder`. The default implementation provides Compose bindings for `NamedScreen` and `EnvironmentScreen`, fixing #546. - `ScreenViewFactoryFinder.getViewFactoryForRendering()` can now return `null`. A `requireViewFactoryForRendering()` extension is introduced for use when `null` is not acceptable. - `ViewEnvironment.withComposeInteropSupport()`, which wraps the found `ScreenComposableFactoryFinder` and `ScreenViewFactoryFinder` with implementations that allow Compose contexts to handle renderings bound only to `ScreenViewFactory`, and classic contexts to handle renderings bound only to `ScreenComposableFactory`. Replaces the logic that used to be in the private `ScreenViewFactory.asComposeViewFactory()` extension in `WorkflowRendering()`. - `Screen.Preview()` is introduced. The existing `Preview()` extension functions were tied to `ScreenViewFactory`, making them much less useful. It is still the case that previews work for non-Compose UI code just fine. Which is pretty cool, really. Fixes #546

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

workflow-ui/compose/src/main/java/com/squareup/workflow1/ui/compose/ScreenComposableFactory.kt

ekeitho · 2024-01-26T01:40:38Z

workflow-ui/compose/src/main/java/com/squareup/workflow1/ui/compose/ScreenComposableFactory.kt

+ *
+ */
+@WorkflowUiExperimentalApi
+public interface ScreenComposableFactory<in ScreenT : Screen> : ViewRegistry.Entry<ScreenT> {


outside of scope of this, but out of curiosity if we were wanting to add a compose overlay factory.

would that entail creating a new ComposeOverlayDialogFactory which implements ScreenComposableFactory & then adding to the checks in ScreenComposableFactoryFinder searching for ComposeOverlayDialogFactory?

or would that entail creating a new factory like this one, with it's relative factory finder?

That would require:

A Compose binding (ScreenComposableFactory implementation) for BodyAndOverlaysScreen -- an alternative to the existing BodyAndOverlaysContainer

A new OverlayComposableFactory / OverlayComposableFactoryFinder pairing

That's assuming you weren't going to use any non-Compose dialog code at all.

If you were going to try to make it coexist with the DialogOverlayFactory you would also need to enhance the logic in withComposeInteropSupport(). But that would only be feasible if your Compose version was still going to create Dialog instances, so that Compose-managed ones could interleave with legacy instances. Even then, you would somehow have to make DialogCollator et al agnostic between Dialog intances created via Compose v. those created the other way. No idea if that's practical.

yeah exactly not going to use any non-compose dialog code at all - thanks for explaining!

…tory

rjrjr · 2024-01-31T20:49:45Z

As soon as this merges (still trying to get our #*&$(& benchmarks to run), we should follow up with a new Compose-based BackStackScreen treatment that fixes #669.

rjrjr · 2024-02-02T18:51:46Z

As soon as this merges (still trying to get our #*&$(& benchmarks to run), we should follow up with a new Compose-based BackStackScreen treatment that fixes #669.

Benchmarks look good. No real improvement in the instrumented portion of our real world app, but that's to be expected since our Compose code is still almost entirely at the leaf level. But the reassurance that the latency around those leaves didn't climb is meaningful.

rjrjr · 2024-02-02T19:13:33Z

workflow-ui/compose/build.gradle.kts

@@ -13,6 +13,10 @@ android {
  composeOptions {
    kotlinCompilerExtensionVersion = libs.versions.androidx.compose.compiler.get()
  }
+  defaultConfig {


Oops, didn't mean to merge this.

No, not accident, required by a crummy old AVD version. But this ripples to break other things (how did I get a green PR?). Need to consider spreading the blast radius or else upgrading the AVDs.

@RBusarow @steve-the-edwards, what's your kneejerk?

rjrjr force-pushed the ray/dual-entry branch 3 times, most recently from c22a09a to 9f5afc2 Compare December 19, 2023 01:57

rjrjr mentioned this pull request Dec 19, 2023

Wrapping still is a pain, and losing type info from Screen.withEnvironment is too. #683

Closed

rjrjr changed the title ~~wip: BREAKING: Replaces ComposeScreenViewFactory with ScreenComposableFactory~~ BREAKING: Replaces ComposeScreenViewFactory with ScreenComposableFactory Dec 20, 2023

rjrjr force-pushed the ray/dual-entry branch from 9f5afc2 to 7ceee3d Compare December 20, 2023 01:30

rjrjr force-pushed the ray/delete-all-the-things branch from dea82c8 to ffb5279 Compare December 20, 2023 19:40

rjrjr force-pushed the ray/dual-entry branch from b060cc9 to ab4ae71 Compare December 21, 2023 19:10

rjrjr commented Jan 9, 2024

View reviewed changes

workflow-ui/core-common/src/main/java/com/squareup/workflow1/ui/ViewRegistry.kt Show resolved Hide resolved

rjrjr commented Jan 9, 2024

View reviewed changes

workflow-ui/compose/src/main/java/com/squareup/workflow1/ui/compose/ScreenComposableFactory.kt Outdated Show resolved Hide resolved

rjrjr force-pushed the ray/dual-entry branch from 9bfd511 to 03738d5 Compare January 9, 2024 01:43

rjrjr force-pushed the ray/delete-all-the-things branch 2 times, most recently from 22d5182 to 95d0e69 Compare January 20, 2024 01:06

rjrjr force-pushed the ray/dual-entry branch from 03738d5 to b3b6bf1 Compare January 20, 2024 01:06

rjrjr force-pushed the ray/delete-all-the-things branch from f1b5ba3 to ec95ab1 Compare January 20, 2024 01:10

rjrjr force-pushed the ray/dual-entry branch from b3b6bf1 to 2832349 Compare January 20, 2024 01:10

rjrjr force-pushed the ray/delete-all-the-things branch from 529c40b to 4f3163e Compare January 20, 2024 01:27

rjrjr force-pushed the ray/dual-entry branch from 2832349 to 66da029 Compare January 20, 2024 01:27

Base automatically changed from ray/delete-all-the-things to main January 22, 2024 19:43

BREAKING: ViewRegistry.Key allows multiple factory types per render…

09bf09f

…ing type Paves the way for parallel Classic and Compose implementations of wrappers like `NamedScreen`, to fix #546. Ironically, breaks Compose support in the process, but that is fixed by the next commit.

rjrjr force-pushed the ray/dual-entry branch 3 times, most recently from 01585e9 to 94e9ff8 Compare January 25, 2024 00:48

rjrjr commented Jan 25, 2024

View reviewed changes

rjrjr force-pushed the ray/dual-entry branch 3 times, most recently from 13968d1 to 2ca9cad Compare January 25, 2024 02:10

rjrjr marked this pull request as ready for review January 25, 2024 02:11

rjrjr requested review from a team and zach-klippenstein as code owners January 25, 2024 02:11

rjrjr assigned ijwhelan and ekeitho Jan 25, 2024

rjrjr commented Jan 25, 2024

View reviewed changes

rjrjr assigned vgonda Jan 25, 2024

rjrjr force-pushed the ray/dual-entry branch from 589288b to d3279af Compare January 25, 2024 02:33

ekeitho reviewed Jan 25, 2024

View reviewed changes

workflow-ui/compose/README.md Outdated Show resolved Hide resolved

vgonda approved these changes Jan 25, 2024

View reviewed changes

rjrjr force-pushed the ray/dual-entry branch 2 times, most recently from 200a131 to dd63fdf Compare January 25, 2024 23:10

ijwhelan approved these changes Jan 25, 2024

View reviewed changes

rjrjr force-pushed the ray/dual-entry branch from d0f0a49 to 4211c0c Compare January 25, 2024 23:20

Apply changes from artifactsDump

7d6e47b

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

ekeitho approved these changes Jan 26, 2024

View reviewed changes

Punched up kdoc for AndroidScreen, ComposeScreen, ScreenComposableFac…

9c22132

…tory

rjrjr force-pushed the ray/dual-entry branch from 0f2fac6 to 9c22132 Compare January 26, 2024 21:19

rjrjr merged commit cd68b19 into main Feb 2, 2024
33 checks passed

rjrjr deleted the ray/dual-entry branch February 2, 2024 18:51

rjrjr commented Feb 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BREAKING: Replaces `ComposeScreenViewFactory` with `ScreenComposableFactory` #1146

BREAKING: Replaces `ComposeScreenViewFactory` with `ScreenComposableFactory` #1146

rjrjr commented Dec 18, 2023 •

edited

Loading

rjrjr commented Jan 8, 2024

rjrjr Jan 25, 2024

rjrjr Jan 25, 2024

rjrjr Jan 25, 2024

ekeitho Jan 25, 2024

vgonda Jan 25, 2024

vgonda Jan 25, 2024

vgonda Jan 25, 2024

ijwhelan Jan 25, 2024

ekeitho Jan 26, 2024

rjrjr Jan 26, 2024 •

edited

Loading

ekeitho Jan 26, 2024

rjrjr commented Jan 31, 2024

rjrjr commented Feb 2, 2024

rjrjr Feb 2, 2024

rjrjr Feb 2, 2024


		The above two concepts coordinate, and when a Compose-based factory is delegating to a child rendering that is also bound to a composable factory, we can skip the detour out into the Android view world and simply call the child composable directly from the parent.
		> You'll note that there is no `OverlayComposableFactory` family of interfaces. So far, all of our window management is strictly via classic Android `Dialog` calls. There is nothing stopping us (or you) from adding Compose-based `Overlay` support in the future as a replacement, but we're definitely not making any promises on that front.

BREAKING: Replaces ComposeScreenViewFactory with ScreenComposableFactory #1146

BREAKING: Replaces ComposeScreenViewFactory with ScreenComposableFactory #1146

Conversation

rjrjr commented Dec 18, 2023 • edited Loading

ViewRegistry.Key allows multiple factory types per rendering type

Replaces ComposeScreenViewFactory with ScreenComposableFactory

rjrjr commented Jan 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjrjr Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjrjr commented Jan 31, 2024

rjrjr commented Feb 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BREAKING: Replaces `ComposeScreenViewFactory` with `ScreenComposableFactory` #1146

BREAKING: Replaces `ComposeScreenViewFactory` with `ScreenComposableFactory` #1146

rjrjr commented Dec 18, 2023 •

edited

Loading

`ViewRegistry.Key` allows multiple factory types per rendering type

Replaces `ComposeScreenViewFactory` with `ScreenComposableFactory`

rjrjr Jan 26, 2024 •

edited

Loading