Introduces ScreenViewFactory.updateView, drastically reduces cleverness.

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

In particular, every `ViewFactory` is required to call `View.bindShowRendering` to put all this machinery in place. When a screen rendering wraps another, with the intention of delegating to its registered `ViewFactory`, that requires creating a wrapper `ViewFactory` that makes 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 is still pretty tricky to use.

To solve all this, we do two things:

- deprecate all the existing `View` extension methods and abandon `WorkflowViewState`, replacing it with `ScreenViewHolder`
- add an `updateView(View)` method to `ScreenViewFactory`

`ScreenViewHolder` is inverted from `WorkflowViewState` -- instead of belonging to the `View`, nestled in a tag, the `View` belongs to it. Most of the old `View` extension methods are replaced by methods on `ScreenViewHolder`. It is very similar to `DialogHolder`, but is part of the public API.

The change to `ScreenViewFactory`, giving it an `update` method, makes it much more like `OverlayDialogFactory`. I toyed a bit with the idea of trying to unify `ScreenViewFactory` / `OverlayDialogFactory` and `ScreenViewHolder` / `DialogHolder`, but it didn't seem worth the added complexity, especially around parameter types.

Reviewers should also note that the **`buildView` method no longer has access to the rendering**, a choice I'm still second guessing. So far I haven't hit a use case that required it, beyond a couple of changes to expected strings in `BackStackContainerPersistenceTest`. Perhaps that's reason enough to bring it back?

It is also worth calling out that `ScreenViewFactory` still is not involved in the `canShow` path. That was a very conscious choice. Making that call dynamic would add a lot of flexibility, but we've never needed that flexibility. The downside of adding it is that we would lose our 1:1 relationship between factories and rendering types, and suddenly would need a resolution mechanism. The cost benefit analysis there seems clear to me.

One of the deprecated `View` extensions did not move to `ScreenViewHolder`: `View.Start` is replaced by `ScreenViewFactory.start`. This extension method is what calls `ScreenViewFactory.buildView`. It creates and returns a `ScreenViewHolder` that wraps the new view and accept updates for it, which it does by calling `ScreenViewFactory.update`, as you'd expect. `ScreenViewFactory.start` is also the new home for the `ViewStarter` mechanism that is used by `WorkflowViewStub` and other containers to call `WorkflowLifecycleOwner.installOn`.

We are not completely out of the `View.setTag` business. `ScreenViewFactory` implementations must be stateless -- it is common for separate instances to field calls to `buildView` and `updateView`, or for a single factory instance to build and update multiple views. `View.setTag` is the most practical way for `buildView` to set up state that `updateView` can rely on. This technique is used as an implementation detail of several standard `ScreenViewFactory` implementations, but is never a public concern. In particular, the factories built by the various `ScreenViewRunner.bind` methods rely on this technique to keep a `View` paired with its runner.

And `ScreenViewRunner` is still very much needed under the new scheme, so that we don't force every implementation of `ScreenViewFactory` to solve this problem of maintaining per-view state themselves. But `ScreenViewRunner` is now a convenience, rather than a fundamental mechanism.

By making `ScreenViewFactory` responsible for both building and updating, it becomes trivial for one factory to delegate to another -- the update method no longer lives in a fragile `View` tag, and so no longer needs to be replaced. A wrapper factory can just delegate to another one. This means there is no longer a problem to be solved by `DecorativeScreenViewFactory`, so that class is deleted. `ManualScreenViewFactory` is also gone, replaced by a `bindBuiltView()` function that returns `Pair<View, ScreenViewRunner>`.

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.

samples/containers/android/src/main/java/com/squareup/sample/container/ScrimContainer.kt

@@ -7,11 +7,11 @@ import android.view.View
 import android.view.ViewGroup
 import androidx.core.content.ContextCompat
 import com.squareup.sample.container.panel.ScrimScreen
-import com.squareup.workflow1.ui.ManualScreenViewFactory
 import com.squareup.workflow1.ui.ScreenViewFactory
+import com.squareup.workflow1.ui.ScreenViewRunner
+import com.squareup.workflow1.ui.ScreenViewRunner.Companion.bindBuiltView
 import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
 import com.squareup.workflow1.ui.WorkflowViewStub
-import com.squareup.workflow1.ui.bindShowRendering
 
 /**
  * A view that renders only its first child, behind a smoke scrim if
@@ -91,23 +91,20 @@ internal class ScrimContainer @JvmOverloads constructor(
   }
 
   @OptIn(WorkflowUiExperimentalApi::class)
-  companion object : ScreenViewFactory<ScrimScreen<*>> by ManualScreenViewFactory(
-    type = ScrimScreen::class,
-    viewConstructor = { initialRendering, initialViewEnvironment, contextForNewView, _ ->
-      val stub = WorkflowViewStub(contextForNewView)
-
-      ScrimContainer(contextForNewView)
+  companion object : ScreenViewFactory<ScrimScreen<*>> by bindBuiltView(
+    buildViewAndRunner = { _, context, _ ->
+      val stub = WorkflowViewStub(context)
+      val scrimContainer = ScrimContainer(context)
         .also { view ->
           view.layoutParams = LayoutParams(LayoutParams.MATCH_PARENT, LayoutParams.MATCH_PARENT)
           view.addView(stub)
-
-          view.bindShowRendering(
-            initialRendering, initialViewEnvironment
-          ) { rendering, environment ->
-            stub.show(rendering.content, environment)
-            view.isDimmed = rendering.dimmed
-          }
         }
+      val runner = ScreenViewRunner<ScrimScreen<*>> { rendering, viewEnvironment ->
+        stub.show(rendering.content, viewEnvironment)
+        scrimContainer.isDimmed = rendering.dimmed
+      }
+
+      Pair(scrimContainer, runner)
     }
   )
 }

samples/dungeon/app/src/main/java/com/squareup/sample/dungeon/BoardView.kt

@@ -8,10 +8,8 @@ import android.graphics.Rect
 import android.view.View
 import androidx.core.content.ContextCompat
 import com.squareup.sample.dungeon.board.Board
-import com.squareup.workflow1.ui.ManualScreenViewFactory
 import com.squareup.workflow1.ui.ScreenViewFactory
 import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
-import com.squareup.workflow1.ui.bindShowRendering
 import kotlin.math.abs
 import kotlin.math.min
 
@@ -83,11 +81,8 @@ class BoardView(context: Context) : View(context) {
   }
 
   @OptIn(WorkflowUiExperimentalApi::class)
-  companion object : ScreenViewFactory<Board> by ManualScreenViewFactory(
-    type = Board::class,
-    viewConstructor = { initialRendering, initialEnv, contextForNewView, _ ->
-      BoardView(contextForNewView)
-        .apply { bindShowRendering(initialRendering, initialEnv) { r, _ -> update(r) } }
-    }
+  companion object : ScreenViewFactory<Board> by ScreenViewFactory(
+    buildView = { _, context, _ -> BoardView(context) },
+    updateView = { view, rendering, _ -> (view as BoardView).update(rendering) }
   )
 }

samples/stub-visibility/src/main/java/com/squareup/sample/stubvisibility/ClickyTextRendering.kt

@@ -9,31 +9,29 @@ import android.view.ViewGroup.LayoutParams
 import android.view.ViewGroup.LayoutParams.MATCH_PARENT
 import android.widget.TextView
 import com.squareup.workflow1.ui.AndroidScreen
-import com.squareup.workflow1.ui.ManualScreenViewFactory
+import com.squareup.workflow1.ui.ScreenViewFactory
 import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
-import com.squareup.workflow1.ui.bindShowRendering
 
 @OptIn(WorkflowUiExperimentalApi::class)
 data class ClickyTextRendering(
   val message: String,
   val visible: Boolean = true,
   val onClick: (() -> Unit)? = null
 ) : AndroidScreen<ClickyTextRendering> {
-  override val viewFactory = ManualScreenViewFactory(
-    type = ClickyTextRendering::class,
-    viewConstructor = { initialRendering, initialEnv, context, _ ->
+  override val viewFactory = ScreenViewFactory<ClickyTextRendering>(
+    buildView = { _, context, _ ->
       TextView(context).also { textView ->
         textView.layoutParams = LayoutParams(MATCH_PARENT, MATCH_PARENT)
         textView.gravity = CENTER
-
-        textView.bindShowRendering(initialRendering, initialEnv) { clickyText, _ ->
-          textView.text = clickyText.message
-          textView.isVisible = clickyText.visible
-          textView.setOnClickListener(
-            clickyText.onClick?.let { oc -> OnClickListener { oc() } }
-          )
-        }
       }
+    },
+    updateView = { view, rendering, _ ->
+      val textView = view as TextView
+      textView.text = rendering.message
+      textView.isVisible = rendering.visible
+      textView.setOnClickListener(
+        rendering.onClick?.let { oc -> OnClickListener { oc() } }
+      )
     }
   )
 }

samples/tictactoe/app/src/androidTest/java/com/squareup/sample/TicTacToeEspressoTest.kt

@@ -2,14 +2,12 @@ package com.squareup.sample
 
 import android.content.pm.ActivityInfo.SCREEN_ORIENTATION_LANDSCAPE
 import android.content.pm.ActivityInfo.SCREEN_ORIENTATION_PORTRAIT
-import android.view.View
 import androidx.test.espresso.IdlingRegistry
 import androidx.test.espresso.ViewInteraction
 import androidx.test.espresso.action.ViewActions.click
 import androidx.test.espresso.action.ViewActions.closeSoftKeyboard
 import androidx.test.espresso.action.ViewActions.typeText
 import androidx.test.espresso.assertion.ViewAssertions.matches
-import androidx.test.espresso.matcher.ViewMatchers.hasDescendant
 import androidx.test.espresso.matcher.ViewMatchers.isDisplayed
 import androidx.test.espresso.matcher.ViewMatchers.withClassName
 import androidx.test.espresso.matcher.ViewMatchers.withId
@@ -18,16 +16,9 @@ import androidx.test.espresso.matcher.ViewMatchers.withParentIndex
 import androidx.test.espresso.matcher.ViewMatchers.withText
 import androidx.test.ext.junit.rules.ActivityScenarioRule
 import androidx.test.ext.junit.runners.AndroidJUnit4
-import com.google.common.truth.Truth.assertThat
-import com.squareup.sample.gameworkflow.GamePlayScreen
-import com.squareup.sample.gameworkflow.Player
-import com.squareup.sample.gameworkflow.symbol
 import com.squareup.sample.mainactivity.TicTacToeActivity
 import com.squareup.sample.tictactoe.R
-import com.squareup.workflow1.ui.ViewEnvironment
 import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
-import com.squareup.workflow1.ui.environment
-import com.squareup.workflow1.ui.getRendering
 import com.squareup.workflow1.ui.internal.test.DetectLeaksAfterTestSuccess
 import com.squareup.workflow1.ui.internal.test.IdlingDispatcherRule
 import com.squareup.workflow1.ui.internal.test.actuallyPressBack
@@ -40,7 +31,6 @@ import org.junit.Rule
 import org.junit.Test
 import org.junit.rules.RuleChain
 import org.junit.runner.RunWith
-import java.util.concurrent.atomic.AtomicReference
 
 /**
  * This app is our most complex sample, which makes it a great candidate for
@@ -74,52 +64,6 @@ class TicTacToeEspressoTest {
     }
   }
 
-  @Test fun showRenderingTagStaysFresh() {
-    // Start a game so that there's something interesting in the Activity window.
-    // (Prior screens are all in a dialog window.)
-
-    inAnyView(withId(R.id.login_email)).type("foo@bar")
-    inAnyView(withId(R.id.login_password)).type("password")
-    inAnyView(withId(R.id.login_button)).perform(click())
-
-    inAnyView(withId(R.id.start_game)).perform(click())
-
-    val environment = AtomicReference<ViewEnvironment>()
-
-    // Why should I learn how to write a matcher when I can just grab the activity
-    // and work with it directly?
-    scenario.onActivity { activity ->
-      val button = activity.findViewById<View>(R.id.game_play_board)
-      val parent = button.parent as View
-      val rendering = parent.getRendering<GamePlayScreen>()!!
-      assertThat(rendering.gameState.playing).isSameInstanceAs(Player.X)
-      val firstEnv = parent.environment
-      assertThat(firstEnv).isNotNull()
-      environment.set(firstEnv)
-
-      // Make a move.
-      rendering.onClick(0, 0)
-    }
-
-    // I'm not an animal, though. Pop back out to the test to check that the update
-    // has happened, to make sure the idle check is allowed to do its thing. (Didn't
-    // actually seem to be necessary, originally did everything synchronously in the
-    // lambda above and it all worked just fine. But that seems like a land mine.)
-
-    inAnyView(withId(R.id.game_play_toolbar))
-      .check(matches(hasDescendant(withText("O, place your ${Player.O.symbol}"))))
-
-    // Now that we're confident the views have updated, back to the activity
-    // to mess with what should be the updated rendering.
-    scenario.onActivity { activity ->
-      val button = activity.findViewById<View>(R.id.game_play_board)
-      val parent = button.parent as View
-      val rendering = parent.getRendering<GamePlayScreen>()!!
-      assertThat(rendering.gameState.playing).isSameInstanceAs(Player.O)
-      assertThat(parent.environment).isEqualTo(environment.get())
-    }
-  }
-
   @Test fun configChangeReflectsWorkflowState() {
     inAnyView(withId(R.id.login_email)).type("bad email")
     inAnyView(withId(R.id.login_button)).perform(click())

workflow-core/src/main/java/com/squareup/workflow1/BaseRenderContext.kt

@@ -101,9 +101,6 @@ public interface BaseRenderContext<out PropsT, StateT, in OutputT> {
     sideEffect: suspend CoroutineScope.() -> Unit
   )
 
-  // TODO(218): We'd prefer the eventHandler methods to be extensions, but the
-  // compiler disagrees. https://youtrack.jetbrains.com/issue/KT-42741
-
   /**
    * Creates a function which builds a [WorkflowAction] from the
    * given [update] function, and immediately passes it to [actionSink]. Handy for

workflow-ui/compose/api/compose.api

@@ -30,7 +30,8 @@ public abstract class com/squareup/workflow1/ui/compose/ComposeScreenViewFactory
 	public static final field $stable I
 	public fun <init> ()V
 	public abstract fun Content (Lcom/squareup/workflow1/ui/Screen;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroidx/compose/runtime/Composer;I)V
-	public final fun buildView (Lcom/squareup/workflow1/ui/Screen;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/content/Context;Landroid/view/ViewGroup;)Landroid/view/View;
+	public final fun buildView (Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/content/Context;Landroid/view/ViewGroup;)Landroid/view/View;
+	public final fun updateView (Landroid/view/View;Lcom/squareup/workflow1/ui/Screen;Lcom/squareup/workflow1/ui/ViewEnvironment;)V
 }
 
 public final class com/squareup/workflow1/ui/compose/ComposeScreenViewFactoryKt {

workflow-ui/compose/src/androidTest/java/com/squareup/workflow1/ui/compose/ComposeViewTreeIntegrationTest.kt

@@ -32,7 +32,6 @@ import com.squareup.workflow1.ui.ScreenViewFactory
 import com.squareup.workflow1.ui.ViewEnvironment
 import com.squareup.workflow1.ui.ViewRegistry
 import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
-import com.squareup.workflow1.ui.bindShowRendering
 import com.squareup.workflow1.ui.container.AndroidOverlay
 import com.squareup.workflow1.ui.container.BackStackScreen
 import com.squareup.workflow1.ui.container.BodyAndModalsScreen
@@ -583,24 +582,26 @@ internal class ComposeViewTreeIntegrationTest {
     override val viewFactory: ScreenViewFactory<TestComposeRendering> get() = this
 
     override fun buildView(
-      initialRendering: TestComposeRendering,
-      initialViewEnvironment: ViewEnvironment,
-      contextForNewView: Context,
+      environment: ViewEnvironment,
+      context: Context,
       container: ViewGroup?
     ): View {
-      var lastCompositionStrategy = initialRendering.disposeStrategy
-
-      return ComposeView(contextForNewView).apply {
-        lastCompositionStrategy?.let(::setViewCompositionStrategy)
-
-        bindShowRendering(initialRendering, initialViewEnvironment) { rendering, _ ->
-          if (rendering.disposeStrategy != lastCompositionStrategy) {
-            lastCompositionStrategy = rendering.disposeStrategy
-            lastCompositionStrategy?.let(::setViewCompositionStrategy)
-          }
+      return ComposeView(context)
+    }
 
-          setContent(rendering.content)
+    override fun updateView(
+      view: View,
+      rendering: TestComposeRendering,
+      environment: ViewEnvironment
+    ) {
+      (view as ComposeView).let { composeView ->
+        val lastCompositionStrategy = composeView.tag as? ViewCompositionStrategy
+        composeView.tag = rendering.disposeStrategy
+        if (rendering.disposeStrategy != lastCompositionStrategy) {
+          lastCompositionStrategy?.let { composeView.setViewCompositionStrategy(it) }
         }
+
+        composeView.setContent(rendering.content)
       }
     }
   }

workflow-ui/compose/src/androidTest/java/com/squareup/workflow1/ui/compose/NoTransitionBackStackContainer.kt

@@ -1,12 +1,11 @@
 package com.squareup.workflow1.ui.compose
 
 import android.content.Context
-import android.view.View
 import android.view.ViewGroup.LayoutParams.MATCH_PARENT
-import com.squareup.workflow1.ui.ManualScreenViewFactory
+import com.squareup.workflow1.ui.NamedScreen
 import com.squareup.workflow1.ui.ScreenViewFactory
+import com.squareup.workflow1.ui.ScreenViewHolder
 import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
-import com.squareup.workflow1.ui.bindShowRendering
 import com.squareup.workflow1.ui.container.BackStackContainer
 import com.squareup.workflow1.ui.container.BackStackScreen
 import com.squareup.workflow1.ui.container.R
@@ -21,28 +20,24 @@ import com.squareup.workflow1.ui.container.R
 internal class NoTransitionBackStackContainer(context: Context) : BackStackContainer(context) {
 
   override fun performTransition(
-    oldViewMaybe: View?,
-    newView: View,
+    oldHolderMaybe: ScreenViewHolder<NamedScreen<*>>?,
+    newHolder: ScreenViewHolder<NamedScreen<*>>,
     popped: Boolean
   ) {
-    oldViewMaybe?.let(::removeView)
-    addView(newView)
+    oldHolderMaybe?.view?.let(::removeView)
+    addView(newHolder.view)
   }
 
-  companion object : ScreenViewFactory<BackStackScreen<*>>
-  by ManualScreenViewFactory(
-    type = BackStackScreen::class,
-    viewConstructor = { initialRendering, initialEnv, context, _ ->
+  companion object : ScreenViewFactory<BackStackScreen<*>> by ScreenViewFactory(
+    buildView = { _, context, _ ->
       NoTransitionBackStackContainer(context)
         .apply {
           id = R.id.workflow_back_stack_container
           layoutParams = LayoutParams(MATCH_PARENT, MATCH_PARENT)
-          bindShowRendering(
-            initialRendering,
-            initialEnv,
-            { newRendering, newViewEnvironment -> update(newRendering, newViewEnvironment) }
-          )
         }
+    },
+    updateView = { view, rendering, environment ->
+      (view as NoTransitionBackStackContainer).update(rendering, environment)
     }
   )
 }