bindShowRendering no longer calls showRendering.

That was always a weird coupling, and it caused the nasty compounding double
update behavior in `DecorativeViewFactory`.

To prevent `ViewFactory` implementations from having to remember to call
`showRendering`, we make `ViewRegistry.buildView` do so. And to allow more
sophisticiated `ViewFactory` implementations to prevent that call from
happening prematurely, we build it into the default value for a new
`initializeView` method on `ViewRegistry.buildView`.

`DecorativeViewFactory` takes advantage by passing a no-op function in for
`initializeView` when the view is built, and then making its own explicit call
to `showRendering` after it's done playing wrapping games.

Another interesting change is that the `showRendering` function now gets the
`View` as its receiver. Made it that much easier for `ViewRegistry.buildView`
to invoke it, and I suspect it will come in handy in other places too.

Closes #397

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

@@ -21,7 +21,7 @@ by DecorativeViewFactory(
         view.backPressedHandler = outerRendering.onBackPressed
       }
 
-      innerShowRendering.invoke(outerRendering.wrapped, viewEnvironment)
+      innerShowRendering.invoke(view, outerRendering.wrapped, viewEnvironment)
 
       if (outerRendering.override) {
         // Place our handler after invoking innerShowRendering, so that ours wins.

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

@@ -59,13 +59,15 @@ class PanelContainer @JvmOverloads constructor(
   }
 
   companion object : ViewFactory<PanelContainerScreen<*, *>> by BuilderViewFactory(
-      type = PanelContainerScreen::class,
-      viewConstructor = { initialRendering, initialEnv, contextForNewView, _ ->
-        PanelContainer(contextForNewView).apply {
-          id = R.id.panel_container
-          layoutParams = ViewGroup.LayoutParams(MATCH_PARENT, MATCH_PARENT)
-          bindShowRendering(initialRendering, initialEnv, ::update)
+    type = PanelContainerScreen::class,
+    viewConstructor = { initialRendering, initialEnv, contextForNewView, _ ->
+      PanelContainer(contextForNewView).apply {
+        id = R.id.panel_container
+        layoutParams = ViewGroup.LayoutParams(MATCH_PARENT, MATCH_PARENT)
+        bindShowRendering(initialRendering, initialEnv) { rendering, env ->
+          update(rendering, env)
         }
       }
+    }
   )
 }

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

@@ -7,8 +7,8 @@ import android.view.View
 import android.view.ViewGroup
 import com.squareup.sample.container.R
 import com.squareup.workflow1.ui.BuilderViewFactory
-import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
 import com.squareup.workflow1.ui.ViewFactory
+import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
 import com.squareup.workflow1.ui.WorkflowViewStub
 import com.squareup.workflow1.ui.bindShowRendering
 
@@ -33,7 +33,7 @@ class ScrimContainer @JvmOverloads constructor(
 
   private val child: View
     get() = getChildAt(0)
-        ?: error("Child must be set immediately upon creation.")
+      ?: error("Child must be set immediately upon creation.")
 
   var isDimmed: Boolean = false
     set(value) {
@@ -84,30 +84,28 @@ class ScrimContainer @JvmOverloads constructor(
       ValueAnimator.ofFloat(1f, 0f)
     }.apply {
       duration = resources.getInteger(android.R.integer.config_shortAnimTime)
-          .toLong()
+        .toLong()
       addUpdateListener { animation -> scrim.alpha = animation.animatedValue as Float }
       start()
     }
   }
 
   @OptIn(WorkflowUiExperimentalApi::class)
   companion object : ViewFactory<ScrimContainerScreen<*>> by BuilderViewFactory(
-      type = ScrimContainerScreen::class,
-      viewConstructor = { initialRendering, initialViewEnvironment, contextForNewView, _ ->
-        val stub = WorkflowViewStub(contextForNewView)
+    type = ScrimContainerScreen::class,
+    viewConstructor = { initialRendering, initialViewEnvironment, contextForNewView, _ ->
+      val stub = WorkflowViewStub(contextForNewView)
 
-        ScrimContainer(contextForNewView)
-            .apply {
-              layoutParams = LayoutParams(LayoutParams.MATCH_PARENT, LayoutParams.MATCH_PARENT)
-              addView(stub)
+      ScrimContainer(contextForNewView)
+        .apply {
+          layoutParams = LayoutParams(LayoutParams.MATCH_PARENT, LayoutParams.MATCH_PARENT)
+          addView(stub)
 
-              bindShowRendering(
-                  initialRendering, initialViewEnvironment
-              ) { rendering, environment ->
-                stub.update(rendering.wrapped, environment)
-                isDimmed = rendering.dimmed
-              }
-            }
-      }
+          bindShowRendering(initialRendering, initialViewEnvironment) { rendering, environment ->
+            stub.update(rendering.wrapped, environment)
+            isDimmed = rendering.dimmed
+          }
+        }
+    }
   )
 }

workflow-ui/backstack-android/src/androidTest/java/com/squareup/workflow1/ui/backstack/test/BackStackContainerTest.kt

@@ -21,7 +21,7 @@ import org.junit.runner.RunWith
 
 @OptIn(WorkflowUiExperimentalApi::class)
 @RunWith(AndroidJUnit4::class)
-class BackStackContainerTest {
+internal class BackStackContainerTest {
 
   @Rule @JvmField val scenarioRule = ActivityScenarioRule(BackStackTestActivity::class.java)
   private val scenario get() = scenarioRule.scenario

workflow-ui/backstack-android/src/androidTest/java/com/squareup/workflow1/ui/backstack/test/fixtures/BackStackTestActivity.kt

@@ -25,7 +25,7 @@ import com.squareup.workflow1.ui.showRendering
  * [onRetainNonConfigurationInstance].
  */
 @OptIn(WorkflowUiExperimentalApi::class)
-class BackStackTestActivity : Activity() {
+internal class BackStackTestActivity : Activity() {
 
   /**
    * A simple string holder that creates [ViewStateTestView]s with their ID and tag derived from
@@ -35,7 +35,7 @@ class BackStackTestActivity : Activity() {
    * @param onViewCreated An optional function that will be called by the view factory after the
    * view is created but before [bindShowRendering].
    */
-  class TestRendering(
+  internal class TestRendering(
     val name: String,
     val onViewCreated: (ViewStateTestView) -> Unit = {},
     val onShowRendering: (ViewStateTestView) -> Unit = {},
@@ -58,7 +58,7 @@ class BackStackTestActivity : Activity() {
             viewHooks = initialRendering.viewHooks
             initialRendering.onViewCreated(this)
             bindShowRendering(initialRendering, initialViewEnvironment) { rendering, _ ->
-              rendering.onShowRendering(this)
+              rendering.onShowRendering(this as ViewStateTestView)
               viewHooks = rendering.viewHooks
             }
       }
@@ -96,6 +96,7 @@ class BackStackTestActivity : Activity() {
     check(backstackContainer == null)
     backstackContainer =
       NoTransitionBackStackContainer.buildView(backstack!!, viewEnvironment, this)
+    backstackContainer!!.showRendering(backstack!!, viewEnvironment)
     setContentView(backstackContainer)
   }
 

workflow-ui/backstack-android/src/androidTest/java/com/squareup/workflow1/ui/backstack/test/fixtures/NoTransitionBackStackContainer.kt

@@ -16,7 +16,7 @@ import com.squareup.workflow1.ui.bindShowRendering
  * actual backstack logic. Views are just swapped instantly.
  */
 @OptIn(WorkflowUiExperimentalApi::class)
-class NoTransitionBackStackContainer(context: Context) : BackStackContainer(context) {
+internal class NoTransitionBackStackContainer(context: Context) : BackStackContainer(context) {
 
   override fun performTransition(oldViewMaybe: View?, newView: View, popped: Boolean) {
     oldViewMaybe?.let(::removeView)
@@ -31,7 +31,9 @@ class NoTransitionBackStackContainer(context: Context) : BackStackContainer(cont
         .apply {
           id = R.id.workflow_back_stack_container
           layoutParams = LayoutParams(MATCH_PARENT, MATCH_PARENT)
-          bindShowRendering(initialRendering, initialEnv, ::update)
+          bindShowRendering(initialRendering, initialEnv) { rendering, env ->
+            update(rendering, env)
+          }
         }
     }
   )

workflow-ui/backstack-android/src/main/java/com/squareup/workflow1/ui/backstack/BackStackContainer.kt

@@ -66,7 +66,12 @@ public open class BackStackContainer @JvmOverloads constructor(
           return
         }
 
-    val newView = environment[ViewRegistry].buildView(named.top, environment, this)
+    val newView = environment[ViewRegistry].buildView(
+      initialRendering = named.top,
+      initialViewEnvironment = environment,
+      contextForNewView = this.context,
+      container = this
+    )
     viewStateCache.update(named.backStack, oldViewMaybe, newView)
 
     val popped = currentRendering?.backStack?.any { compatible(it, named.top) } == true
@@ -146,7 +151,9 @@ public open class BackStackContainer @JvmOverloads constructor(
             .apply {
               id = R.id.workflow_back_stack_container
               layoutParams = (ViewGroup.LayoutParams(MATCH_PARENT, MATCH_PARENT))
-              bindShowRendering(initialRendering, initialEnv, ::update)
+              bindShowRendering(initialRendering, initialEnv) { rendering, env ->
+                update(rendering, env)
+              }
             }
       }
   )

workflow-ui/core-android/api/core-android.api

@@ -59,15 +59,15 @@ public final class com/squareup/workflow1/ui/NamedViewFactory : com/squareup/wor
 }
 
 public final class com/squareup/workflow1/ui/ShowRenderingTag {
-	public fun <init> (Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function2;)V
+	public fun <init> (Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function3;)V
 	public final fun component1 ()Ljava/lang/Object;
 	public final fun component2 ()Lcom/squareup/workflow1/ui/ViewEnvironment;
-	public final fun component3 ()Lkotlin/jvm/functions/Function2;
-	public final fun copy (Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function2;)Lcom/squareup/workflow1/ui/ShowRenderingTag;
-	public static synthetic fun copy$default (Lcom/squareup/workflow1/ui/ShowRenderingTag;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function2;ILjava/lang/Object;)Lcom/squareup/workflow1/ui/ShowRenderingTag;
+	public final fun component3 ()Lkotlin/jvm/functions/Function3;
+	public final fun copy (Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function3;)Lcom/squareup/workflow1/ui/ShowRenderingTag;
+	public static synthetic fun copy$default (Lcom/squareup/workflow1/ui/ShowRenderingTag;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function3;ILjava/lang/Object;)Lcom/squareup/workflow1/ui/ShowRenderingTag;
 	public fun equals (Ljava/lang/Object;)Z
 	public final fun getEnvironment ()Lcom/squareup/workflow1/ui/ViewEnvironment;
-	public final fun getShowRendering ()Lkotlin/jvm/functions/Function2;
+	public final fun getShowRendering ()Lkotlin/jvm/functions/Function3;
 	public final fun getShowing ()Ljava/lang/Object;
 	public fun hashCode ()I
 	public fun toString ()Ljava/lang/String;
@@ -127,19 +127,19 @@ public final class com/squareup/workflow1/ui/ViewRegistry$Companion : com/square
 public final class com/squareup/workflow1/ui/ViewRegistryKt {
 	public static final fun ViewRegistry ()Lcom/squareup/workflow1/ui/ViewRegistry;
 	public static final fun ViewRegistry ([Lcom/squareup/workflow1/ui/ViewFactory;)Lcom/squareup/workflow1/ui/ViewRegistry;
-	public static final fun buildView (Lcom/squareup/workflow1/ui/ViewRegistry;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/content/Context;Landroid/view/ViewGroup;)Landroid/view/View;
-	public static final fun buildView (Lcom/squareup/workflow1/ui/ViewRegistry;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/view/ViewGroup;)Landroid/view/View;
-	public static synthetic fun buildView$default (Lcom/squareup/workflow1/ui/ViewRegistry;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/content/Context;Landroid/view/ViewGroup;ILjava/lang/Object;)Landroid/view/View;
+	public static final fun buildView (Lcom/squareup/workflow1/ui/ViewRegistry;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/content/Context;Landroid/view/ViewGroup;Lkotlin/jvm/functions/Function3;)Landroid/view/View;
+	public static synthetic fun buildView$default (Lcom/squareup/workflow1/ui/ViewRegistry;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/content/Context;Landroid/view/ViewGroup;Lkotlin/jvm/functions/Function3;ILjava/lang/Object;)Landroid/view/View;
+	public static final fun getFactoryForRendering (Lcom/squareup/workflow1/ui/ViewRegistry;Ljava/lang/Object;)Lcom/squareup/workflow1/ui/ViewFactory;
 	public static final fun plus (Lcom/squareup/workflow1/ui/ViewRegistry;Lcom/squareup/workflow1/ui/ViewFactory;)Lcom/squareup/workflow1/ui/ViewRegistry;
 	public static final fun plus (Lcom/squareup/workflow1/ui/ViewRegistry;Lcom/squareup/workflow1/ui/ViewRegistry;)Lcom/squareup/workflow1/ui/ViewRegistry;
 }
 
 public final class com/squareup/workflow1/ui/ViewShowRenderingKt {
-	public static final fun bindShowRendering (Landroid/view/View;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function2;)V
+	public static final fun bindShowRendering (Landroid/view/View;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function3;)V
 	public static final fun canShowRendering (Landroid/view/View;Ljava/lang/Object;)Z
 	public static final fun getEnvironment (Landroid/view/View;)Lcom/squareup/workflow1/ui/ViewEnvironment;
 	public static final fun getRendering (Landroid/view/View;)Ljava/lang/Object;
-	public static final fun getShowRendering (Landroid/view/View;)Lkotlin/jvm/functions/Function2;
+	public static final fun getShowRendering (Landroid/view/View;)Lkotlin/jvm/functions/Function3;
 	public static final fun showRendering (Landroid/view/View;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;)V
 }
 

workflow-ui/core-android/src/androidTest/java/com/squareup/workflow1/ui/DecorativeViewFactoryTest.kt

@@ -8,7 +8,7 @@ import com.google.common.truth.Truth.assertThat
 import org.junit.Test
 
 @OptIn(WorkflowUiExperimentalApi::class)
-class DecorativeViewFactoryTest {
+internal class DecorativeViewFactoryTest {
 
   private val instrumentation = InstrumentationRegistry.getInstrumentation()
 
@@ -28,11 +28,19 @@ class DecorativeViewFactoryTest {
         }
       }
     }
+
+    val envString = object : ViewEnvironmentKey<String>(String::class) {
+      override val default: String get() = "Not set"
+    }
+
     val outerViewFactory = DecorativeViewFactory(
       type = OuterRendering::class,
-      map = { outer -> outer.wrapped },
-      initView = { outerRendering, _ ->
-        events += "initView $outerRendering"
+      map = { outer, env ->
+        val enhancedEnv = env + (envString to "Updated environment")
+        Pair(outer.wrapped, enhancedEnv)
+      },
+      initView = { outerRendering, view ->
+        events += "initView $outerRendering ${view.environment!![envString]}"
       }
     )
     val viewRegistry = ViewRegistry(innerViewFactory)
@@ -44,11 +52,9 @@ class DecorativeViewFactoryTest {
       instrumentation.context
     )
 
-    // Note that showRendering is called twice. Technically this behavior is not incorrect, although
-    // it's not necessary. Fix coming soon.
     assertThat(events).containsExactly(
-      "inner showRendering InnerRendering(innerData=inner)",
-      "initView OuterRendering(outerData=outer, wrapped=InnerRendering(innerData=inner))",
+      "initView OuterRendering(outerData=outer, wrapped=InnerRendering(innerData=inner)) " +
+        "Updated environment",
       "inner showRendering InnerRendering(innerData=inner)"
     )
   }
@@ -72,9 +78,9 @@ class DecorativeViewFactoryTest {
     val outerViewFactory = DecorativeViewFactory(
       type = OuterRendering::class,
       map = { outer -> outer.wrapped },
-      doShowRendering = { _, innerShowRendering, outerRendering, env ->
+      doShowRendering = { view, innerShowRendering, outerRendering, env ->
         events += "doShowRendering $outerRendering"
-        innerShowRendering(outerRendering.wrapped, env)
+        view.innerShowRendering(outerRendering.wrapped, env)
       }
     )
     val viewRegistry = ViewRegistry(innerViewFactory)
@@ -86,10 +92,7 @@ class DecorativeViewFactoryTest {
       instrumentation.context
     )
 
-    // Note that showRendering is called twice. Technically this behavior is not incorrect, although
-    // it's not necessary. Fix coming soon.
     assertThat(events).containsExactly(
-      "inner showRendering InnerRendering(innerData=inner)",
       "doShowRendering OuterRendering(outerData=outer, wrapped=InnerRendering(innerData=inner))",
       "inner showRendering InnerRendering(innerData=inner)"
     )
@@ -114,9 +117,9 @@ class DecorativeViewFactoryTest {
     val outerViewFactory = DecorativeViewFactory(
       type = OuterRendering::class,
       map = { outer -> outer.wrapped },
-      doShowRendering = { _, innerShowRendering, outerRendering, env ->
+      doShowRendering = { view, innerShowRendering, outerRendering, env ->
         events += "doShowRendering $outerRendering"
-        innerShowRendering(outerRendering.wrapped, env)
+        view.innerShowRendering(outerRendering.wrapped, env)
       }
     )
     val viewRegistry = ViewRegistry(innerViewFactory)

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/DecorativeViewFactory.kt

@@ -116,9 +116,9 @@ public class DecorativeViewFactory<OuterT : Any, InnerT : Any>(
     innerShowRendering: ViewShowRendering<InnerT>,
     outerRendering: OuterT,
     env: ViewEnvironment
-  ) -> Unit = { _, innerShowRendering, outerRendering, viewEnvironment ->
+  ) -> Unit = { view, innerShowRendering, outerRendering, viewEnvironment ->
     val (innerRendering, processedEnv) = map(outerRendering, viewEnvironment)
-    innerShowRendering(innerRendering, processedEnv)
+    view.innerShowRendering(innerRendering, processedEnv)
   }
 ) : ViewFactory<OuterT> {
   /**
@@ -133,14 +133,14 @@ public class DecorativeViewFactory<OuterT : Any, InnerT : Any>(
       innerShowRendering: ViewShowRendering<InnerT>,
       outerRendering: OuterT,
       env: ViewEnvironment
-    ) -> Unit = { _, innerShowRendering, outerRendering, viewEnvironment ->
-      innerShowRendering(map(outerRendering), viewEnvironment)
+    ) -> Unit = { view, innerShowRendering, outerRendering, viewEnvironment ->
+      view.innerShowRendering(map(outerRendering), viewEnvironment)
     }
   ) : this(
-      type,
-      map = { outer, viewEnvironment -> Pair(map(outer), viewEnvironment) },
-      initView = initView,
-      doShowRendering = doShowRendering
+    type,
+    map = { outer, viewEnvironment -> Pair(map(outer), viewEnvironment) },
+    initView = initView,
+    doShowRendering = doShowRendering
   )
 
   override fun buildView(
@@ -152,18 +152,27 @@ public class DecorativeViewFactory<OuterT : Any, InnerT : Any>(
     val (innerInitialRendering, processedInitialEnv) = map(initialRendering, initialViewEnvironment)
 
     return processedInitialEnv[ViewRegistry]
-        .buildView(
-            innerInitialRendering,
-            processedInitialEnv,
-            contextForNewView,
-            container
-        )
-        .also { view ->
-          val innerShowRendering: ViewShowRendering<InnerT> = view.getShowRendering()!!
-          initView(initialRendering, view)
-          view.bindShowRendering(initialRendering, processedInitialEnv) { rendering, env ->
-            doShowRendering(view, innerShowRendering, rendering, env)
-          }
-        }
+      .buildView(
+        innerInitialRendering,
+        processedInitialEnv,
+        contextForNewView,
+        container,
+        // Don't call showRendering yet, we need to wrap the function first.
+        initializeView = { }
+      )
+      .also { view ->
+        val innerShowRendering: ViewShowRendering<InnerT> = view.getShowRendering()!!
+        view.bindShowRendering(
+          initialRendering,
+          processedInitialEnv
+        ) { rendering, env -> doShowRendering(view, innerShowRendering, rendering, env) }
+
+        // Call this after we fuss with the bindings, to ensure it can pull the updated
+        // ViewEnvironment.
+        initView(initialRendering, view)
+
+        // Our showRendering wrapper is in place, now call it.
+        view.showRendering(initialRendering, processedInitialEnv)
+      }
   }
 }