asDialogHolderWithContent, ScreenViewHolder.startShowing

1. `setContent` as a method name gave no clue that we were returning a new manager object, so now it's called `asDialogHolderWithContent`

2. Hard coding the call to `Dialog.setContent` interferes with code that needs to make that call itself. We now accept a `setContent: (ScreenViewHolder<C>) -> Unit` lambda to do that work, which defaults to the current behavior.

3. Decouple building and starting views. This allows us to ensure that a Dialog's content view is attached to its window before its first rendering -- before the first call to `ScreenViewHolder.show`. Necessary to ensure that the strictly view-tree-based AndroidX `findViewTreeOnBackPressedDispatcherOwner()` call works immediately. This was always the original intention, but never actually found a use case until now.

4. Moves most of that default behavior to a public `Dialog.fixBackgroundAndDimming` extension function to keep it available and more self documenting (hard learned lessons in there). Pretty similar to how `ScreenOverlayDialogFactory` acts.

5. Big kdoc update

Author: rjrjr

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

@@ -9,8 +9,8 @@ import com.squareup.workflow1.ui.ViewEnvironment
 import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
 import com.squareup.workflow1.ui.container.OverlayDialogFactory
 import com.squareup.workflow1.ui.container.OverlayDialogHolder
+import com.squareup.workflow1.ui.container.asDialogHolderWithContent
 import com.squareup.workflow1.ui.container.setBounds
-import com.squareup.workflow1.ui.container.setContent
 import kotlin.reflect.KClass
 
 /**
@@ -27,8 +27,11 @@ internal object PanelOverlayDialogFactory : OverlayDialogFactory<PanelOverlay<Sc
   ): OverlayDialogHolder<PanelOverlay<Screen>> {
     val dialog = AppCompatDialog(context, R.style.PanelDialog)
 
-    val realHolder = dialog.setContent(initialRendering, initialEnvironment)
+    val realHolder = dialog.asDialogHolderWithContent(initialRendering, initialEnvironment)
 
+    // We replace the default onUpdateBounds function with one that gives the
+    // panel a square shape on tablets. See OverlayDialogFactory for more details
+    // on the bounds mechanism.
     return object : OverlayDialogHolder<PanelOverlay<Screen>> by realHolder {
       override val onUpdateBounds: ((Rect) -> Unit) = { bounds ->
         val refinedBounds: Rect = if (!dialog.context.isTablet) {

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

@@ -39,7 +39,7 @@ import com.squareup.workflow1.ui.container.BackStackScreen
 import com.squareup.workflow1.ui.container.BodyAndOverlaysScreen
 import com.squareup.workflow1.ui.container.OverlayDialogFactory
 import com.squareup.workflow1.ui.container.ScreenOverlay
-import com.squareup.workflow1.ui.container.setContent
+import com.squareup.workflow1.ui.container.asDialogHolderWithContent
 import com.squareup.workflow1.ui.internal.test.IdleAfterTestRule
 import com.squareup.workflow1.ui.internal.test.IdlingDispatcherRule
 import com.squareup.workflow1.ui.internal.test.WorkflowUiTestActivity
@@ -584,7 +584,7 @@ internal class ComposeViewTreeIntegrationTest {
 
     override val dialogFactory =
       OverlayDialogFactory<TestOverlay> { initialRendering, initialEnvironment, context: Context ->
-        ComponentDialog(context).setContent(initialRendering, initialEnvironment)
+        ComponentDialog(context).asDialogHolderWithContent(initialRendering, initialEnvironment)
       }
   }
 

workflow-ui/container-android/src/main/java/com/squareup/workflow1/ui/modal/ModalContainer.kt

@@ -90,7 +90,7 @@ public abstract class ModalContainer<ModalRenderingT : Any> @JvmOverloads constr
             // any, and so we can use our lifecycle to destroy-on-detach the dialog hierarchy.
             WorkflowLifecycleOwner.installOn(
               dialogView,
-              (ref.dialog as? OnBackPressedDispatcherOwner)
+              onBackPressedDispatcherOwner = (ref.dialog as? OnBackPressedDispatcherOwner)
                 ?: viewEnvironment.onBackPressedDispatcherOwner(this),
               findParentLifecycle = { parentLifecycleOwner.lifecycle }
             )

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

@@ -129,6 +129,7 @@ public final class com/squareup/workflow1/ui/ScreenViewFactoryFinder$DefaultImpl
 
 public final class com/squareup/workflow1/ui/ScreenViewFactoryKt {
 	public static final fun startShowing (Lcom/squareup/workflow1/ui/ScreenViewFactory;Lcom/squareup/workflow1/ui/Screen;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/content/Context;Landroid/view/ViewGroup;Lcom/squareup/workflow1/ui/ViewStarter;)Lcom/squareup/workflow1/ui/ScreenViewHolder;
+	public static final fun startShowing (Lcom/squareup/workflow1/ui/ScreenViewHolder;Lcom/squareup/workflow1/ui/Screen;Lcom/squareup/workflow1/ui/ViewEnvironment;Lcom/squareup/workflow1/ui/ViewStarter;)V
 	public static synthetic fun startShowing$default (Lcom/squareup/workflow1/ui/ScreenViewFactory;Lcom/squareup/workflow1/ui/Screen;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/content/Context;Landroid/view/ViewGroup;Lcom/squareup/workflow1/ui/ViewStarter;ILjava/lang/Object;)Lcom/squareup/workflow1/ui/ScreenViewHolder;
 	public static final fun toViewFactory (Lcom/squareup/workflow1/ui/Screen;Lcom/squareup/workflow1/ui/ViewEnvironment;)Lcom/squareup/workflow1/ui/ScreenViewFactory;
 }
@@ -306,7 +307,8 @@ public final class com/squareup/workflow1/ui/androidx/WorkflowSavedStateRegistry
 	public fun <init> ()V
 	public final fun attachToParentRegistry (Ljava/lang/String;Landroidx/savedstate/SavedStateRegistryOwner;)V
 	public final fun detachFromParentRegistry ()V
-	public final fun installChildRegistryOwnerOn (Landroid/view/View;Ljava/lang/String;)V
+	public final fun installChildRegistryOwnerOn (Landroid/view/View;Ljava/lang/String;Z)V
+	public static synthetic fun installChildRegistryOwnerOn$default (Lcom/squareup/workflow1/ui/androidx/WorkflowSavedStateRegistryAggregator;Landroid/view/View;Ljava/lang/String;ZILjava/lang/Object;)V
 	public final fun pruneAllChildRegistryOwnersExcept (Ljava/util/Collection;)V
 	public static synthetic fun pruneAllChildRegistryOwnersExcept$default (Lcom/squareup/workflow1/ui/androidx/WorkflowSavedStateRegistryAggregator;Ljava/util/Collection;ILjava/lang/Object;)V
 	public final fun saveAndPruneChildRegistryOwner (Ljava/lang/String;)V
@@ -331,6 +333,12 @@ public abstract interface class com/squareup/workflow1/ui/container/AndroidOverl
 	public abstract fun getDialogFactory ()Lcom/squareup/workflow1/ui/container/OverlayDialogFactory;
 }
 
+public final class com/squareup/workflow1/ui/container/AsDialogHolderWithContentKt {
+	public static final fun asDialogHolderWithContent (Landroidx/activity/ComponentDialog;Lcom/squareup/workflow1/ui/container/ScreenOverlay;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function1;)Lcom/squareup/workflow1/ui/container/OverlayDialogHolder;
+	public static synthetic fun asDialogHolderWithContent$default (Landroidx/activity/ComponentDialog;Lcom/squareup/workflow1/ui/container/ScreenOverlay;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function1;ILjava/lang/Object;)Lcom/squareup/workflow1/ui/container/OverlayDialogHolder;
+	public static final fun fixBackgroundAndDimming (Landroid/app/Dialog;)V
+}
+
 public final class com/squareup/workflow1/ui/container/BackButtonScreen : com/squareup/workflow1/ui/AndroidScreen, com/squareup/workflow1/ui/Wrapper {
 	public fun <init> (Lcom/squareup/workflow1/ui/Screen;ZLkotlin/jvm/functions/Function0;)V
 	public synthetic fun <init> (Lcom/squareup/workflow1/ui/Screen;ZLkotlin/jvm/functions/Function0;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
@@ -376,10 +384,6 @@ public final class com/squareup/workflow1/ui/container/BackStackContainer$SavedS
 	public synthetic fun newArray (I)[Ljava/lang/Object;
 }
 
-public final class com/squareup/workflow1/ui/container/ComponentDialogSetContentKt {
-	public static final fun setContent (Landroidx/activity/ComponentDialog;Lcom/squareup/workflow1/ui/container/ScreenOverlay;Lcom/squareup/workflow1/ui/ViewEnvironment;)Lcom/squareup/workflow1/ui/container/OverlayDialogHolder;
-}
-
 public final class com/squareup/workflow1/ui/container/LayeredDialogSessions {
 	public static final field Companion Lcom/squareup/workflow1/ui/container/LayeredDialogSessions$Companion;
 	public synthetic fun <init> (Landroid/content/Context;Lkotlinx/coroutines/flow/StateFlow;Lkotlin/jvm/functions/Function0;Lkotlin/jvm/functions/Function0;Lkotlin/jvm/internal/DefaultConstructorMarker;)V

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

@@ -66,7 +66,7 @@ internal class DialogIntegrationTest {
 
     override val dialogFactory = OverlayDialogFactory<DialogRendering> { r, e, c ->
       val dialog = ComponentDialog(c).also { latestDialog = it }
-      dialog.setContent(r, e)
+      dialog.asDialogHolderWithContent(r, e)
     }
   }
 

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

@@ -27,7 +27,7 @@ public typealias BackPressHandler = () -> Unit
  */
 @Suppress("DEPRECATION")
 @WorkflowUiExperimentalApi
-@Deprecated("Use setOrClearBackHandler")
+@Deprecated("Use setBackHandler")
 public var View.backPressedHandler: BackPressHandler?
   get() = observerOrNull?.handler
   set(value) {

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

@@ -25,18 +25,33 @@ public typealias ViewBindingInflater<BindingT> = (LayoutInflater, ViewGroup?, Bo
  * It is rare to call [buildView] directly. Instead the most common path is to pass [Screen]
  * instances to [WorkflowViewStub.show], which will apply the [ScreenViewFactory] machinery for you.
  *
- * If you are building a custom container and [WorkflowViewStub] is too restrictive, use
- * [ScreenViewFactory.startShowing].
+ * If you are building a custom container and for some reason can't delegate to
+ * [WorkflowViewStub], here is how to do the work yourself:
+ *
+ * - Call [Screen.toViewFactory], which uses the [ScreenViewFactoryFinder] in the
+ *   given [ViewEnvironment] to return the [ScreenViewFactory] bound to the type
+ *   of the receiving [Screen].
+ *
+ * - Call [ScreenViewFactory.buildView] to build a view wrapped in the [ScreenViewHolder]
+ *   that manages it.
+ *
+ * - Call [ScreenViewHolder.startShowing] to initialize the new view with its
+ *   first [Screen] rendering, taking care to include a [ViewStarter] lambda that invokes
+ *   [WorkflowLifecycleOwner.installOn][com.squareup.workflow1.ui.androidx.WorkflowLifecycleOwner.installOn]
+ *
+ * - On subsequent updates, if [ScreenViewHolder.canShow] returns true, call [ScreenViewHolder.show]
+ *
+ * - If [ScreenViewHolder.canShow] returns false, it is time to tear your [ScreenViewHolder]
+ *   down. Be sure to call
+ *   [WorkflowLifecycleOwner.destroyOnDetach][com.squareup.workflow1.ui.androidx.WorkflowLifecycleOwner.destroyOnDetach]
+ *   on the managed [view][ScreenViewHolder.view]!
  */
 @WorkflowUiExperimentalApi
 public interface ScreenViewFactory<in ScreenT : Screen> : ViewRegistry.Entry<ScreenT> {
   /**
    * It is rare to call this method directly. Instead the most common path is to pass [Screen]
    * instances to [WorkflowViewStub.show], which will apply the [ScreenViewFactory] machinery for
    * you.
-   *
-   * Called by [startShowing] to create a [ScreenViewHolder] wrapping a [View] able to display a
-   * stream of [ScreenT] renderings, starting with [initialRendering].
    */
   public fun buildView(
     initialRendering: ScreenT,
@@ -256,11 +271,8 @@ public interface ScreenViewFactory<in ScreenT : Screen> : ViewRegistry.Entry<Scr
 
 /**
  * It is rare to call this method directly. Instead the most common path is to pass [Screen]
- * instances to [WorkflowViewStub.show], which will apply the [ScreenViewFactory] machinery for you.
- *
- * Use the [ScreenViewFactoryFinder] in [environment] to return the [ScreenViewFactory] bound to the
- * type of the receiving [Screen]. Call [ScreenViewFactory.startShowing] to create and initialize a
- * new [View].
+ * instances to [WorkflowViewStub.show], which will apply the [ScreenViewFactory] and
+ * [ScreenViewHolder] machinery for you. See [ScreenViewFactory] for details.
  */
 @WorkflowUiExperimentalApi
 public fun <ScreenT : Screen> ScreenT.toViewFactory(
@@ -271,83 +283,74 @@ public fun <ScreenT : Screen> ScreenT.toViewFactory(
 
 /**
  * It is rare to call this method directly. Instead the most common path is to pass [Screen]
- * instances to [WorkflowViewStub.show], which will apply the [ScreenViewFactory] machinery for you.
+ * instances to [WorkflowViewStub.show], which will apply the [ScreenViewFactory] and
+ * [ScreenViewHolder] machinery for you. See [ScreenViewFactory] for details.
  *
- * Creates a [ScreenViewHolder] wrapping a [View] able to display a stream of [ScreenT] renderings,
- * starting with [initialRendering].
+ * Initializes a [ScreenViewHolder] that has just been created by [ScreenViewFactory.buildView].
  *
  * To add more initialization behavior (typically a call to
  * [WorkflowLifecycleOwner.installOn][com.squareup.workflow1.ui.androidx.WorkflowLifecycleOwner.installOn]),
  * provide a [viewStarter].
  */
 @Suppress("DEPRECATION")
 @WorkflowUiExperimentalApi
-public fun <ScreenT : Screen> ScreenViewFactory<ScreenT>.startShowing(
+public fun <ScreenT : Screen> ScreenViewHolder<ScreenT>.startShowing(
   initialRendering: ScreenT,
   initialEnvironment: ViewEnvironment,
-  contextForNewView: Context,
-  container: ViewGroup? = null,
-  viewStarter: ViewStarter? = null
-): ScreenViewHolder<ScreenT> {
-  return buildView(
-    initialRendering,
-    initialEnvironment,
-    contextForNewView,
-    container
-  ).also { holder ->
-    val resolvedStarter = viewStarter ?: ViewStarter { _, doStart -> doStart() }
+  viewStarter: ViewStarter?
+) {
+  val resolvedStarter = viewStarter ?: ViewStarter { _, doStart -> doStart() }
 
-    val legacyStarter: ((View) -> Unit)? = holder.view.starterOrNull
+  val legacyStarter: ((View) -> Unit)? = view.starterOrNull
 
-    if (legacyStarter != null) {
-      var shown = false
-      // This View was built by a legacy ViewFactory, and so it needs to be
-      // started in just the right way.
-      //
-      // The tricky bit is the old starter's default value, a function that calls
-      // View.showRendering(). Odds are it's wrapped and wrapped again deep inside
-      // legacyStarter. To ensure it gets called at the right time, and that we don't
-      // update the view redundantly, we use bindShowRendering to replace View.showRendering()
-      // with a call to our own holder.show(). (No need to call the original showRendering(),
-      // AsScreenViewFactory blanked it.)
-      //
-      // This same call to bindShowRendering will also update View.getRendering() and
-      // View.environment() to return what was passed in here, as expected.
-      holder.view.bindShowRendering(
-        initialRendering,
-        initialEnvironment
-      ) { rendering, environment ->
-        holder.show(rendering, environment)
-        shown = true
-      }
-      holder.view.starter = { startingView ->
-        resolvedStarter.startView(startingView) { legacyStarter(startingView) }
-      }
-      // We have to call View.start() to fire this off rather than calling the starter directly,
-      // to keep the rest of the legacy machinery happy.
-      holder.view.start()
-      check(shown) {
-        "A ViewStarter provided to ViewRegistry.buildView or a DecorativeViewFactory " +
-          "neglected to call the given doStart() function"
-      }
-    } else {
-      var shown = false
-      resolvedStarter.startView(holder.view) {
-        holder.show(initialRendering, initialEnvironment)
-        shown = true
-      }
-      check(shown) {
-        "A ViewStarter provided to ScreenViewFactory.startShowing " +
-          "neglected to call the given doStart() function"
-      }
+  if (legacyStarter != null) {
+    var shown = false
+    // This View was built by a legacy ViewFactory, and so it needs to be
+    // started in just the right way.
+    //
+    // The tricky bit is the old starter's default value, a function that calls
+    // View.showRendering(). Odds are it's wrapped and wrapped again deep inside
+    // legacyStarter. To ensure it gets called at the right time, and that we don't
+    // update the view redundantly, we use bindShowRendering to replace View.showRendering()
+    // with a call to our own holder.show(). (No need to call the original showRendering(),
+    // AsScreenViewFactory blanked it.)
+    //
+    // This same call to bindShowRendering will also update View.getRendering() and
+    // View.environment() to return what was passed in here, as expected.
+    view.bindShowRendering(
+      initialRendering,
+      initialEnvironment
+    ) { rendering, environment ->
+      show(rendering, environment)
+      shown = true
+    }
+    view.starter = { startingView ->
+      resolvedStarter.startView(startingView) { legacyStarter(startingView) }
+    }
+    // We have to call View.start() to fire this off rather than calling the starter directly,
+    // to keep the rest of the legacy machinery happy.
+    view.start()
+    check(shown) {
+      "$viewStarter neglected to call the given doStart() function when showing $initialRendering"
+    }
+  } else {
+    var shown = false
+    resolvedStarter.startView(view) {
+      show(initialRendering, initialEnvironment)
+      shown = true
+    }
+    check(shown) {
+      "$viewStarter neglected to call the given doStart() function when showing $initialRendering"
     }
   }
 }
 
 /**
- * A wrapper for the function invoked when [ScreenViewFactory.startShowing] is called, allowing
+ * A wrapper for the function invoked when [ScreenViewHolder.startShowing] is called, allowing
  * for custom initialization of a newly built [View] before or after the first call to
- * [ScreenViewHolder.show].
+ * [ScreenViewHolder.show]. Most typical use is to call
+ * [WorkflowLifecycleOwner.installOn][com.squareup.workflow1.ui.androidx.WorkflowLifecycleOwner.installOn]),
+ * at just the right moment.
  */
 @WorkflowUiExperimentalApi
 public fun interface ViewStarter {
@@ -358,6 +361,28 @@ public fun interface ViewStarter {
   )
 }
 
+/**
+ * Convenience that combines [ScreenViewFactory.buildView] and [ScreenViewHolder.startShowing],
+ * since we rarely need to do work between those two calls.
+ */
+@WorkflowUiExperimentalApi
+public fun <ScreenT : Screen> ScreenViewFactory<ScreenT>.startShowing(
+  initialRendering: ScreenT,
+  initialEnvironment: ViewEnvironment,
+  contextForNewView: Context,
+  container: ViewGroup? = null,
+  viewStarter: ViewStarter? = null
+): ScreenViewHolder<ScreenT> {
+  return buildView(
+    initialRendering,
+    initialEnvironment,
+    contextForNewView,
+    container
+  ).apply {
+    startShowing(initialRendering, initialEnvironment, viewStarter)
+  }
+}
+
 /**
  * Transforms a [ScreenViewFactory] of [TransformedT] into one that can handle
  * instances of [SourceT].

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

@@ -211,10 +211,18 @@ public class WorkflowSavedStateRegistryAggregator {
    * [WorkflowSavedStateRegistryAggregator]. Typically this is derived from the
    * [compatibility key][com.squareup.workflow1.ui.Compatible.keyFor] of the [view]'s
    * rendering.
+   *
+   * @param force when this is true we're asserting that it's okay to clobber an
+   * existing registry that may have been put in place by Android. If we find
+   * one of our own [KeyedSavedStateRegistryOwner], we will still throw an exception.
+   *
+   * @throws IllegalArgumentException is [key] is already in use or if [view] has an
+   * unexpected [SavedStateRegistryOwner] in place already.
    */
   public fun installChildRegistryOwnerOn(
     view: View,
-    key: String
+    key: String,
+    force: Boolean = false
   ) {
     val lifecycleOwner = requireNotNull(ViewTreeLifecycleOwner.get(view)) {
       "Expected $view($key) to have a ViewTreeLifecycleOwner. " +
@@ -224,9 +232,13 @@ public class WorkflowSavedStateRegistryAggregator {
     children.put(key, registryOwner)?.let {
       throw IllegalArgumentException("$key is already in use, it cannot be used to register $view")
     }
-    view.findViewTreeSavedStateRegistryOwner()?.let {
-      throw IllegalArgumentException("$view already has SavedStateRegistryOwner: $it")
-    }
+    view.findViewTreeSavedStateRegistryOwner()
+      ?.takeIf { !force || it is KeyedSavedStateRegistryOwner }
+      ?.let {
+        throw IllegalArgumentException(
+          "Using $key to register $view, but it already has SavedStateRegistryOwner: $it"
+        )
+      }
     view.setViewTreeSavedStateRegistryOwner(registryOwner)
     restoreIfOwnerReady(registryOwner)
   }

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

@@ -13,7 +13,8 @@ import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
  *
  * - [ScreenOverlay] for dialogs whose content is defined by a wrapped
  *   [Screen][com.squareup.workflow1.ui.Screen] instance. And in this case,
- *   also note the [ComponentDialog.setContent][setContent] extension function.
+ *   also note the [ComponentDialog.asDialogHolderWithContent][asDialogHolderWithContent]
+ *   extension function.
  *
  * For example:
  *