OverlayDialogHolder.onUpdateBounds replaces ScreenOverlayDialogFactory.updateBounds

I finally tried to use `ScreenOverlayDialogFactory.updateBounds` and was reminded once again why it's bad to create interfaces that build something and then take it back later to be updated. If you want the renderings to be involved in setting the bounds policy, there was no way to smuggle that information along with the Dialog returned from `buildDialogWithContent`, so that `updateBounds` could honor it. Also, it was always pretty ugly that the bounds hook was special to the `ScreenOverlay` world.

The fix is make bounds maintenance part of the job of the `OverlayDialogHolder` interface. Square reviewers will notice that we're now an even more faithful rip off of Mosaic's DialogRunner -- hi @helios175.

Author: rjrjr

gradle.properties

@@ -8,7 +8,7 @@ android.useAndroidX=true
 systemProp.org.gradle.internal.publish.checksums.insecure=true
 
 GROUP=com.squareup.workflow1
-VERSION_NAME=1.8.0-beta06-SNAPSHOT
+VERSION_NAME=1.8.0-rjrjrbeta06-SNAPSHOT
 
 POM_DESCRIPTION=Square Workflow
 

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

@@ -5,9 +5,13 @@ import android.graphics.Rect
 import com.squareup.sample.container.R
 import com.squareup.workflow1.ui.Screen
 import com.squareup.workflow1.ui.ScreenViewHolder
+import com.squareup.workflow1.ui.ViewEnvironment
 import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
+import com.squareup.workflow1.ui.container.OverlayDialogHolder
 import com.squareup.workflow1.ui.container.ScreenOverlayDialogFactory
+import com.squareup.workflow1.ui.container.setBounds
 import com.squareup.workflow1.ui.container.setContent
+import com.squareup.workflow1.ui.show
 
 /**
  * Android support for [PanelOverlay].
@@ -18,41 +22,48 @@ internal object PanelOverlayDialogFactory :
     type = PanelOverlay::class
   ) {
   /**
-   * Forks the default implementation to apply [R.style.PanelDialog], for
-   * enter and exit animation.
+   * Forks the default implementation to apply [R.style.PanelDialog] for
+   * enter and exit animation, and to customize [bounds][OverlayDialogHolder.onUpdateBounds].
    */
-  override fun buildDialogWithContent(content: ScreenViewHolder<Screen>): Dialog {
-    return Dialog(content.view.context, R.style.PanelDialog).also {
-      it.setContent(content)
-    }
-  }
+  override fun buildDialogWithContent(
+    initialRendering: PanelOverlay<Screen>,
+    initialEnvironment: ViewEnvironment,
+    content: ScreenViewHolder<Screen>
+  ): OverlayDialogHolder<PanelOverlay<Screen>> {
+    val dialog = Dialog(content.view.context, R.style.PanelDialog)
+    dialog.setContent(content)
 
-  override fun updateBounds(
-    dialog: Dialog,
-    bounds: Rect
-  ) {
-    val refinedBounds: Rect = if (!dialog.context.isTablet) {
-      // On a phone, fill the bounds entirely.
-      bounds
-    } else {
-      if (bounds.height() > bounds.width()) {
-        val margin = bounds.height() - bounds.width()
-        val topDelta = margin / 2
-        val bottomDelta = margin - topDelta
-        Rect(bounds).apply {
-          top = bounds.top + topDelta
-          bottom = bounds.bottom - bottomDelta
-        }
-      } else {
-        val margin = bounds.width() - bounds.height()
-        val leftDelta = margin / 2
-        val rightDelta = margin - leftDelta
-        Rect(bounds).apply {
-          left = bounds.left + leftDelta
-          right = bounds.right - rightDelta
+    return OverlayDialogHolder(
+      initialEnvironment = initialEnvironment,
+      dialog = dialog,
+      onUpdateBounds = { bounds ->
+        val refinedBounds: Rect = if (!dialog.context.isTablet) {
+          // On a phone, fill the bounds entirely.
+          bounds
+        } else {
+          if (bounds.height() > bounds.width()) {
+            val margin = bounds.height() - bounds.width()
+            val topDelta = margin / 2
+            val bottomDelta = margin - topDelta
+            Rect(bounds).apply {
+              top = bounds.top + topDelta
+              bottom = bounds.bottom - bottomDelta
+            }
+          } else {
+            val margin = bounds.width() - bounds.height()
+            val leftDelta = margin / 2
+            val rightDelta = margin - leftDelta
+            Rect(bounds).apply {
+              left = bounds.left + leftDelta
+              right = bounds.right - rightDelta
+            }
+          }
         }
+
+        dialog.setBounds(refinedBounds)
       }
+    ) { overlayRendering, environment ->
+      content.show(overlayRendering.content, environment)
     }
-    super.updateBounds(dialog, refinedBounds)
   }
 }

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

@@ -436,8 +436,8 @@ public class com/squareup/workflow1/ui/container/AlertOverlayDialogFactory : com
 }
 
 public final class com/squareup/workflow1/ui/container/AndroidDialogBoundsKt {
-	public static final fun maintainBounds (Landroid/app/Dialog;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function2;)V
-	public static final fun maintainBounds (Landroid/app/Dialog;Lkotlinx/coroutines/flow/StateFlow;Lkotlin/jvm/functions/Function2;)V
+	public static final fun maintainBounds (Landroid/app/Dialog;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function1;)V
+	public static final fun maintainBounds (Landroid/app/Dialog;Lkotlinx/coroutines/flow/StateFlow;Lkotlin/jvm/functions/Function1;)V
 	public static final fun setBounds (Landroid/app/Dialog;Landroid/graphics/Rect;)V
 }
 
@@ -672,6 +672,7 @@ public abstract interface class com/squareup/workflow1/ui/container/OverlayDialo
 	public static final field Companion Lcom/squareup/workflow1/ui/container/OverlayDialogHolder$Companion;
 	public abstract fun getDialog ()Landroid/app/Dialog;
 	public abstract fun getEnvironment ()Lcom/squareup/workflow1/ui/ViewEnvironment;
+	public abstract fun getOnUpdateBounds ()Lkotlin/jvm/functions/Function1;
 	public abstract fun getRunner ()Lkotlin/jvm/functions/Function2;
 }
 
@@ -689,16 +690,18 @@ public final class com/squareup/workflow1/ui/container/OverlayDialogHolder$Compa
 }
 
 public final class com/squareup/workflow1/ui/container/OverlayDialogHolderKt {
-	public static final fun OverlayDialogHolder (Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/app/Dialog;Lkotlin/jvm/functions/Function2;)Lcom/squareup/workflow1/ui/container/OverlayDialogHolder;
+	public static final fun OverlayDialogHolder (Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/app/Dialog;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function2;)Lcom/squareup/workflow1/ui/container/OverlayDialogHolder;
+	public static synthetic fun OverlayDialogHolder$default (Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/app/Dialog;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function2;ILjava/lang/Object;)Lcom/squareup/workflow1/ui/container/OverlayDialogHolder;
 	public static final fun canShow (Lcom/squareup/workflow1/ui/container/OverlayDialogHolder;Lcom/squareup/workflow1/ui/container/Overlay;)Z
 	public static final fun getShowing (Lcom/squareup/workflow1/ui/container/OverlayDialogHolder;)Lcom/squareup/workflow1/ui/container/Overlay;
 	public static final fun show (Lcom/squareup/workflow1/ui/container/OverlayDialogHolder;Lcom/squareup/workflow1/ui/container/Overlay;Lcom/squareup/workflow1/ui/ViewEnvironment;)V
 }
 
 public final class com/squareup/workflow1/ui/container/RealOverlayDialogHolder : com/squareup/workflow1/ui/container/OverlayDialogHolder {
-	public fun <init> (Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/app/Dialog;Lkotlin/jvm/functions/Function2;)V
+	public fun <init> (Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/app/Dialog;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function2;)V
 	public fun getDialog ()Landroid/app/Dialog;
 	public fun getEnvironment ()Lcom/squareup/workflow1/ui/ViewEnvironment;
+	public fun getOnUpdateBounds ()Lkotlin/jvm/functions/Function1;
 	public fun getRunner ()Lkotlin/jvm/functions/Function2;
 }
 
@@ -707,9 +710,8 @@ public class com/squareup/workflow1/ui/container/ScreenOverlayDialogFactory : co
 	public fun buildContent (Lcom/squareup/workflow1/ui/ScreenViewFactory;Lcom/squareup/workflow1/ui/Screen;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/content/Context;)Lcom/squareup/workflow1/ui/ScreenViewHolder;
 	public synthetic fun buildDialog (Lcom/squareup/workflow1/ui/container/Overlay;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/content/Context;)Lcom/squareup/workflow1/ui/container/OverlayDialogHolder;
 	public final fun buildDialog (Lcom/squareup/workflow1/ui/container/ScreenOverlay;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/content/Context;)Lcom/squareup/workflow1/ui/container/OverlayDialogHolder;
-	public fun buildDialogWithContent (Lcom/squareup/workflow1/ui/ScreenViewHolder;)Landroid/app/Dialog;
+	public fun buildDialogWithContent (Lcom/squareup/workflow1/ui/container/ScreenOverlay;Lcom/squareup/workflow1/ui/ViewEnvironment;Lcom/squareup/workflow1/ui/ScreenViewHolder;)Lcom/squareup/workflow1/ui/container/OverlayDialogHolder;
 	public fun getType ()Lkotlin/reflect/KClass;
-	public fun updateBounds (Landroid/app/Dialog;Landroid/graphics/Rect;)V
 }
 
 public final class com/squareup/workflow1/ui/container/ScreenOverlayDialogFactoryKt {

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

@@ -52,7 +52,11 @@ public open class AlertOverlayDialogFactory : OverlayDialogFactory<AlertOverlay>
           alertDialog.setButton(button.toId(), " ") { _, _ -> }
         }
 
-        OverlayDialogHolder(initialEnvironment, alertDialog) { rendering, _ ->
+        OverlayDialogHolder(
+          initialEnvironment = initialEnvironment,
+          dialog = alertDialog,
+          onUpdateBounds = null
+        ) { rendering, _ ->
           with(alertDialog) {
             if (rendering.cancelable) {
               setOnCancelListener { rendering.onEvent(Canceled) }

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

@@ -19,7 +19,7 @@ import kotlinx.coroutines.flow.onEach
  * [bounds] is expected to be in global display coordinates,
  * e.g. as returned from [View.getGlobalVisibleRect].
  *
- * @see ScreenOverlayDialogFactory.updateBounds
+ * @see OverlayDialogHolder.onUpdateBounds
  */
 @WorkflowUiExperimentalApi
 public fun Dialog.setBounds(bounds: Rect) {
@@ -37,23 +37,23 @@ public fun Dialog.setBounds(bounds: Rect) {
 @WorkflowUiExperimentalApi
 internal fun <D : Dialog> D.maintainBounds(
   environment: ViewEnvironment,
-  onBoundsChange: (D, Rect) -> Unit
+  onBoundsChange: (Rect) -> Unit
 ) {
   maintainBounds(environment[OverlayArea].bounds, onBoundsChange)
 }
 
 @WorkflowUiExperimentalApi
 internal fun <D : Dialog> D.maintainBounds(
   bounds: StateFlow<Rect>,
-  onBoundsChange: (D, Rect) -> Unit
+  onBoundsChange: (Rect) -> Unit
 ) {
   val window = requireNotNull(window) { "Dialog must be attached to a window." }
   window.callback = object : Window.Callback by window.callback {
     var scope: CoroutineScope? = null
 
     override fun onAttachedToWindow() {
       scope = CoroutineScope(Dispatchers.Main.immediate).also {
-        bounds.onEach { b -> onBoundsChange(this@maintainBounds, b) }
+        bounds.onEach { b -> onBoundsChange(b) }
           .launchIn(it)
       }
     }
@@ -65,5 +65,5 @@ internal fun <D : Dialog> D.maintainBounds(
   }
 
   // If already attached, set the bounds eagerly.
-  if (window.peekDecorView()?.isAttachedToWindow == true) onBoundsChange(this, bounds.value)
+  if (window.peekDecorView()?.isAttachedToWindow == true) onBoundsChange(bounds.value)
 }

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

@@ -26,7 +26,7 @@ internal class DialogSession(
   index: Int,
   holder: OverlayDialogHolder<Overlay>
 ) {
-  // Note similar code in LayeredDialogs
+  // Note similar code in LayeredDialogSessions
   private var allowEvents = true
     set(value) {
       val was = field

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

@@ -163,6 +163,10 @@ public class LayeredDialogSessions private constructor(
         overlay.toDialogFactory(dialogEnv)
           .buildDialog(overlay, dialogEnv, context)
           .let { holder ->
+            holder.onUpdateBounds?.let { updateBounds ->
+              holder.dialog.maintainBounds(holder.environment) { b -> updateBounds(b) }
+            }
+
             DialogSession(i, holder).also { newSession ->
               // Prime the pump, make the first call to OverlayDialog.show to update
               // the new dialog to reflect the first rendering.

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

@@ -1,6 +1,7 @@
 package com.squareup.workflow1.ui.container
 
 import android.app.Dialog
+import android.graphics.Rect
 import com.squareup.workflow1.ui.Screen
 import com.squareup.workflow1.ui.ViewEnvironment
 import com.squareup.workflow1.ui.ViewEnvironmentKey
@@ -29,6 +30,21 @@ public interface OverlayDialogHolder<in OverlayT : Overlay> {
    */
   public val runner: (rendering: OverlayT, environment: ViewEnvironment) -> Unit
 
+
+  /**
+   * Optional function called to report the bounds of the managing container view,
+   * as reported by [OverlayArea]. Well behaved [Overlay] dialogs are expected to
+   * be restricted to those bounds, to the extent practical -- you probably want to ignore
+   * this for AlertDialog, e.g.
+   *
+   * Honoring this contract makes it easy to define areas of the display
+   * that are outside of the "shadow" of a modal dialog. Imagine an app
+   * with a status bar that should not be covered by modals.
+   *
+   * Default implementation provided by the factory function below calls [Dialog.setBounds].
+   */
+  public val onUpdateBounds: ((Rect) -> Unit)?
+
   public companion object {
     /**
      * Default value returned for the [InOverlay] [ViewEnvironmentKey], and therefore the
@@ -87,7 +103,8 @@ public val OverlayDialogHolder<*>.showing: Overlay
 public fun <OverlayT : Overlay> OverlayDialogHolder(
   initialEnvironment: ViewEnvironment,
   dialog: Dialog,
+  onUpdateBounds: ((Rect) -> Unit)? = { dialog.setBounds(it) },
   runner: (rendering: OverlayT, environment: ViewEnvironment) -> Unit
 ): OverlayDialogHolder<OverlayT> {
-  return RealOverlayDialogHolder(initialEnvironment, dialog, runner)
+  return RealOverlayDialogHolder(initialEnvironment, dialog, onUpdateBounds, runner)
 }

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

@@ -1,13 +1,15 @@
 package com.squareup.workflow1.ui.container
 
 import android.app.Dialog
+import android.graphics.Rect
 import com.squareup.workflow1.ui.ViewEnvironment
 import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
 
 @WorkflowUiExperimentalApi
 internal class RealOverlayDialogHolder<OverlayT : Overlay>(
   initialEnvironment: ViewEnvironment,
   override val dialog: Dialog,
+  override val onUpdateBounds: ((Rect) -> Unit)?,
   runnerFunction: (rendering: OverlayT, environment: ViewEnvironment) -> Unit
 ) : OverlayDialogHolder<OverlayT> {
 

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

@@ -2,7 +2,6 @@ package com.squareup.workflow1.ui.container
 
 import android.app.Dialog
 import android.content.Context
-import android.graphics.Rect
 import android.graphics.drawable.ColorDrawable
 import android.util.TypedValue
 import android.view.KeyEvent
@@ -61,11 +60,6 @@ import kotlin.reflect.KClass
  * display those [Overlay] renderings look for [OverlayArea] value and restrict
  * themselves to the reported bounds.
  *
- * Dialogs created via [ScreenOverlayDialogFactory] implementations honor [OverlayArea]
- * automatically. [updateBounds] is called as the [OverlayArea] changes, and the
- * default implementation of that method sets created dialog windows to fill the given area --
- * not necessarily the entire display.
- *
  * Another [ViewEnvironment] value is maintained to support modality: [CoveredByModal].
  * When this value is true, it indicates that a dialog window driven by a [ModalOverlay]
  * is in play over the view, or is about to be, and so touch and click events should be
@@ -109,42 +103,33 @@ public open class ScreenOverlayDialogFactory<S : Screen, O : ScreenOverlay<S>>(
     initialEnvironment: ViewEnvironment,
     context: Context
   ): ScreenViewHolder<S> {
-    return viewFactory
-      .startShowing(initialContent, initialEnvironment, context)
+    return viewFactory.startShowing(initialContent, initialEnvironment, context)
   }
 
   /**
    * Build the [Dialog] for the [content] that was just created by [buildContent].
-   * Open to allow customization, typically theming.
-   *
-   * The default implementation delegates all work to the provided [Dialog.setContent]
-   * extension function. Subclasses need not call `super`.
+   * Open to allow customization, typically theming, subclasses need not call `super`.
+   *  - Note that the default implementation calls the provided [Dialog.setContent]
+   *    extension for typical setup.
+   *  - Be sure to call [ScreenViewHolder.show] from [OverlayDialogHolder.runner].
    */
-  public open fun buildDialogWithContent(content: ScreenViewHolder<S>): Dialog {
-    return Dialog(content.view.context).also { it.setContent(content) }
+  public open fun buildDialogWithContent(
+    initialRendering: O,
+    initialEnvironment: ViewEnvironment,
+    content: ScreenViewHolder<S>
+  ): OverlayDialogHolder<O> {
+    return OverlayDialogHolder(
+      initialEnvironment, Dialog(content.view.context).also { it.setContent(content) }
+    ) { overlayRendering, environment ->
+      content.show(overlayRendering.content, environment)
+    }
   }
 
   /**
-   * This method will be called to report the bounds of the managing container view,
-   * as reported by [OverlayArea]. Well behaved [ScreenOverlay] dialogs are expected to
-   * be restricted to those bounds.
-   *
-   * Honoring this contract makes it easy to define areas of the display
-   * that are outside of the "shadow" of a modal dialog. Imagine an app
-   * with a status bar that should not be covered by modals.
-   *
-   * The default implementation calls straight through to the [Dialog.setBounds] function
-   * provided below. Custom implementations are not required to call `super`.
-   *
-   * @see Dialog.setBounds
+   * Locked down implementation enforces [ModalOverlay] and supports
+   * [ModalScreenOverlayBackButtonHelper]. Delegates to [buildContent] to create the content view
+   * and [buildDialogWithContent] to create the [Dialog].
    */
-  public open fun updateBounds(
-    dialog: Dialog,
-    bounds: Rect
-  ) {
-    dialog.setBounds(bounds)
-  }
-
   final override fun buildDialog(
     initialRendering: O,
     initialEnvironment: ViewEnvironment,
@@ -159,8 +144,12 @@ public open class ScreenOverlayDialogFactory<S : Screen, O : ScreenOverlay<S>>(
     val contentViewHolder =
       buildContent(contentViewFactory, initialRendering.content, initialEnvironment, context)
 
-    return buildDialogWithContent(contentViewHolder).let { dialog ->
-      val window = requireNotNull(dialog.window) { "Dialog must be attached to a window." }
+    return buildDialogWithContent(
+      initialRendering,
+      initialEnvironment,
+      contentViewHolder
+    ).also { holder ->
+      val window = requireNotNull(holder.dialog.window) { "Dialog must be attached to a window." }
 
       if (modal) {
         val realWindowCallback = window.callback
@@ -183,13 +172,6 @@ public open class ScreenOverlayDialogFactory<S : Screen, O : ScreenOverlay<S>>(
       // notion of its modality. Even a modal dialog should only block events within
       // the appropriate bounds, but Android makes them block everywhere.
       window.setFlags(FLAG_NOT_TOUCH_MODAL, FLAG_NOT_TOUCH_MODAL)
-
-      // Keep an eye on the bounds StateFlow(Rect) put in place by [LayeredDialogSessions].
-      dialog.maintainBounds(contentViewHolder.environment) { d, b -> updateBounds(d, Rect(b)) }
-
-      OverlayDialogHolder(initialEnvironment, dialog) { overlayRendering, environment ->
-        contentViewHolder.show(overlayRendering.content, environment)
-      }
     }
   }