Simplify ViewRegistry to act more like a simple map of rendering type…

… to ViewFactory. `ViewRegistry` is now just responsible for returning a `ViewFactory` for a given rendering type, as well as checking that views returned from the factory have been correctly bound with `bindShowRendering`. `buildView` has been extracted into an extension method on `ViewRegistry`. This change simplifies the responsibility of each `ViewRegistry` implementation. It also makes `ViewRegistry` more flexible for extension. For example, allows a Compose-based `ViewFactory` to detect recursive Compose bindings and stay in Compose instead of jumping back out into the legacy view layer every time.
square · May 11, 2020 · 9ffea7d · 9ffea7d
1 parent acc6ab2
commit 9ffea7d
Show file tree

Hide file tree

Showing 10 changed files with 205 additions and 129 deletions.
diff --git a/...amples/containers/android/src/main/java/com/squareup/sample/container/BackButtonScreen.kt b/...amples/containers/android/src/main/java/com/squareup/sample/container/BackButtonScreen.kt
@@ -20,6 +20,7 @@ import com.squareup.workflow.ui.ViewFactory
 import com.squareup.workflow.ui.ViewRegistry
 import com.squareup.workflow.ui.backPressedHandler
 import com.squareup.workflow.ui.bindShowRendering
+import com.squareup.workflow.ui.buildView
 import com.squareup.workflow.ui.getShowRendering
 
 /**

diff --git a/kotlin/workflow-ui/core-android/api/core-android.api b/kotlin/workflow-ui/core-android/api/core-android.api
@@ -87,8 +87,9 @@ public final class com/squareup/workflow/ui/ViewFactory$DefaultImpls {
 
 public abstract interface class com/squareup/workflow/ui/ViewRegistry {
 	public static final field Companion Lcom/squareup/workflow/ui/ViewRegistry$Companion;
-	public abstract fun buildView (Ljava/lang/Object;Lcom/squareup/workflow/ui/ViewEnvironment;Landroid/content/Context;Landroid/view/ViewGroup;)Landroid/view/View;
+	public abstract fun getFactoryFor (Lkotlin/reflect/KClass;)Lcom/squareup/workflow/ui/ViewFactory;
 	public abstract fun getKeys ()Ljava/util/Set;
+	public abstract fun hasViewBeenBound (Landroid/view/View;)Z
 }
 
 public final class com/squareup/workflow/ui/ViewRegistry$Companion : com/squareup/workflow/ui/ViewEnvironmentKey {
@@ -97,14 +98,16 @@ public final class com/squareup/workflow/ui/ViewRegistry$Companion : com/squareu
 }
 
 public final class com/squareup/workflow/ui/ViewRegistry$DefaultImpls {
-	public static synthetic fun buildView$default (Lcom/squareup/workflow/ui/ViewRegistry;Ljava/lang/Object;Lcom/squareup/workflow/ui/ViewEnvironment;Landroid/content/Context;Landroid/view/ViewGroup;ILjava/lang/Object;)Landroid/view/View;
+	public static fun hasViewBeenBound (Lcom/squareup/workflow/ui/ViewRegistry;Landroid/view/View;)Z
 }
 
 public final class com/squareup/workflow/ui/ViewRegistryKt {
 	public static final fun ViewRegistry ()Lcom/squareup/workflow/ui/ViewRegistry;
 	public static final fun ViewRegistry ([Lcom/squareup/workflow/ui/ViewFactory;)Lcom/squareup/workflow/ui/ViewRegistry;
 	public static final fun ViewRegistry ([Lcom/squareup/workflow/ui/ViewRegistry;)Lcom/squareup/workflow/ui/ViewRegistry;
+	public static final fun buildView (Lcom/squareup/workflow/ui/ViewRegistry;Ljava/lang/Object;Lcom/squareup/workflow/ui/ViewEnvironment;Landroid/content/Context;Landroid/view/ViewGroup;)Landroid/view/View;
 	public static final fun buildView (Lcom/squareup/workflow/ui/ViewRegistry;Ljava/lang/Object;Lcom/squareup/workflow/ui/ViewEnvironment;Landroid/view/ViewGroup;)Landroid/view/View;
+	public static synthetic fun buildView$default (Lcom/squareup/workflow/ui/ViewRegistry;Ljava/lang/Object;Lcom/squareup/workflow/ui/ViewEnvironment;Landroid/content/Context;Landroid/view/ViewGroup;ILjava/lang/Object;)Landroid/view/View;
 	public static final fun plus (Lcom/squareup/workflow/ui/ViewRegistry;Lcom/squareup/workflow/ui/ViewFactory;)Lcom/squareup/workflow/ui/ViewRegistry;
 	public static final fun plus (Lcom/squareup/workflow/ui/ViewRegistry;Lcom/squareup/workflow/ui/ViewRegistry;)Lcom/squareup/workflow/ui/ViewRegistry;
 }

diff --git a/...in/workflow-ui/core-android/src/main/java/com/squareup/workflow/ui/BindingViewRegistry.kt b/...in/workflow-ui/core-android/src/main/java/com/squareup/workflow/ui/BindingViewRegistry.kt
@@ -15,9 +15,6 @@
  */
 package com.squareup.workflow.ui
 
-import android.content.Context
-import android.view.View
-import android.view.ViewGroup
 import kotlin.reflect.KClass
 
 /**
@@ -35,34 +32,18 @@ internal class BindingViewRegistry private constructor(
             check(keys.size == bindings.size) {
               "${bindings.map { it.type }} must not have duplicate entries."
             }
-          }
+          } as Map<KClass<*>, ViewFactory<*>>
   )
 
   override val keys: Set<KClass<*>> get() = bindings.keys
 
-  override fun <RenderingT : Any> buildView(
-    initialRendering: RenderingT,
-    initialViewEnvironment: ViewEnvironment,
-    contextForNewView: Context,
-    container: ViewGroup?
-  ): View {
+  override fun <RenderingT : Any> getFactoryFor(
+    renderingType: KClass<out RenderingT>
+  ): ViewFactory<RenderingT> {
     @Suppress("UNCHECKED_CAST")
-    return (bindings[initialRendering::class] as? ViewFactory<RenderingT>)
-        ?.buildView(
-            initialRendering,
-            initialViewEnvironment,
-            contextForNewView,
-            container
-        )
-        ?.apply {
-          checkNotNull(getRendering<RenderingT>()) {
-            "View.bindShowRendering should have been called for $this, typically by the " +
-                "${ViewFactory::class.java.name} that created it."
-          }
-        }
-        ?: throw IllegalArgumentException(
-            "A ${ViewFactory::class.java.name} should have been registered " +
-                "to display $initialRendering."
-        )
+    return requireNotNull(bindings[renderingType] as? ViewFactory<RenderingT>) {
+      "A ${ViewFactory::class.java.name} should have been registered " +
+          "to display a $renderingType."
+    }
   }
 }
diff --git a/.../workflow-ui/core-android/src/main/java/com/squareup/workflow/ui/CompositeViewRegistry.kt b/.../workflow-ui/core-android/src/main/java/com/squareup/workflow/ui/CompositeViewRegistry.kt
@@ -15,9 +15,6 @@
  */
 package com.squareup.workflow.ui
 
-import android.content.Context
-import android.view.View
-import android.view.ViewGroup
 import kotlin.reflect.KClass
 
 /**
@@ -42,18 +39,15 @@ internal class CompositeViewRegistry private constructor(
 
   override val keys: Set<KClass<*>> get() = registriesByKey.keys
 
-  override fun <RenderingT : Any> buildView(
-    initialRendering: RenderingT,
-    initialViewEnvironment: ViewEnvironment,
-    contextForNewView: Context,
-    container: ViewGroup?
-  ): View {
-    val registry = registriesByKey[initialRendering::class]
-        ?: throw IllegalArgumentException(
-            "A ${ViewFactory::class.java.name} should have been registered " +
-                "to display $initialRendering."
-        )
-    return registry.buildView(initialRendering, initialViewEnvironment, contextForNewView, container)
+  override fun <RenderingT : Any> getFactoryFor(
+    renderingType: KClass<out RenderingT>
+  ): ViewFactory<RenderingT> = getRegistryFor(renderingType).getFactoryFor(renderingType)
+
+  private fun getRegistryFor(renderingType: KClass<out Any>): ViewRegistry {
+    return requireNotNull(registriesByKey[renderingType]) {
+      "A ${ViewFactory::class.java.name} should have been registered " +
+          "to display a $renderingType."
+    }
   }
 
   companion object {

diff --git a/kotlin/workflow-ui/core-android/src/main/java/com/squareup/workflow/ui/ViewFactory.kt b/kotlin/workflow-ui/core-android/src/main/java/com/squareup/workflow/ui/ViewFactory.kt
@@ -27,8 +27,8 @@ import kotlin.reflect.KClass
  *
  * Sets of bindings are gathered in [ViewRegistry] instances.
  */
-interface ViewFactory<RenderingT : Any> {
-  val type: KClass<RenderingT>
+interface ViewFactory<in RenderingT : Any> {
+  val type: KClass<in RenderingT>
 
   /**
    * Returns a View ready to display [initialRendering] (and any succeeding values)

diff --git a/kotlin/workflow-ui/core-android/src/main/java/com/squareup/workflow/ui/ViewRegistry.kt b/kotlin/workflow-ui/core-android/src/main/java/com/squareup/workflow/ui/ViewRegistry.kt
@@ -67,22 +67,23 @@ interface ViewRegistry {
   val keys: Set<KClass<*>>
 
   /**
-   * It is usually more convenient to use [WorkflowViewStub] than to call this method directly.
+   * This method is not for general use, use [WorkflowViewStub] instead.
    *
-   * Creates a [View] to display [initialRendering], which can be updated via calls
-   * to [View.showRendering].
+   * Returns the [ViewFactory] that was registered for the given [renderingType].
    *
-   * @throws IllegalArgumentException if no binding can be find for type [RenderingT]
+   * @throws IllegalArgumentException if no factory can be found for type [RenderingT]
+   */
+  fun <RenderingT : Any> getFactoryFor(
+    renderingType: KClass<out RenderingT>
+  ): ViewFactory<RenderingT>
+
+  /**
+   * This method is not for general use, it's called by [buildView] to validate views returned by
+   * [ViewFactory]s.
    *
-   * @throws IllegalStateException if the matching [ViewFactory] fails to call
-   * [View.bindShowRendering] when constructing the view
+   * Returns true iff [view] has been bound to a [ShowRenderingTag] by calling [bindShowRendering].
    */
-  fun <RenderingT : Any> buildView(
-    initialRendering: RenderingT,
-    initialViewEnvironment: ViewEnvironment,
-    contextForNewView: Context,
-    container: ViewGroup? = null
-  ): View
+  fun hasViewBeenBound(view: View): Boolean = view.getRendering<Any>() != null
 
   companion object : ViewEnvironmentKey<ViewRegistry>(ViewRegistry::class) {
     override val default: ViewRegistry
@@ -104,6 +105,38 @@ fun ViewRegistry(vararg registries: ViewRegistry): ViewRegistry = CompositeViewR
  */
 fun ViewRegistry(): ViewRegistry = BindingViewRegistry()
 
+/**
+ * It is usually more convenient to use [WorkflowViewStub] than to call this method directly.
+ *
+ * Creates a [View] to display [initialRendering], which can be updated via calls
+ * to [View.showRendering].
+ *
+ * @throws IllegalArgumentException if no factory can be find for type [RenderingT]
+ *
+ * @throws IllegalStateException if [ViewRegistry.hasViewBeenBound] returns false (i.e. if the
+ * matching [ViewFactory] fails to call [View.bindShowRendering] when constructing the view)
+ */
+fun <RenderingT : Any> ViewRegistry.buildView(
+  initialRendering: RenderingT,
+  initialViewEnvironment: ViewEnvironment,
+  contextForNewView: Context,
+  container: ViewGroup? = null
+): View {
+  return getFactoryFor(initialRendering::class)
+      .buildView(
+          initialRendering,
+          initialViewEnvironment,
+          contextForNewView,
+          container
+      )
+      .apply {
+        check(hasViewBeenBound(this)) {
+          "View.bindShowRendering should have been called for $this, typically by the " +
+              "${ViewFactory::class.java.name} that created it."
+        }
+      }
+}
+
 /**
  * It is usually more convenient to use [WorkflowViewStub] than to call this method directly.
  *

diff --git a/...orkflow-ui/core-android/src/test/java/com/squareup/workflow/ui/BindingViewRegistryTest.kt b/...orkflow-ui/core-android/src/test/java/com/squareup/workflow/ui/BindingViewRegistryTest.kt
@@ -23,36 +23,44 @@ import kotlin.test.assertTrue
 class BindingViewRegistryTest {
 
   @Test fun `keys from bindings`() {
-    val binding1 = TestBinding(FooRendering::class)
-    val binding2 = TestBinding(BarRendering::class)
-    val registry = BindingViewRegistry(binding1, binding2)
+    val factory1 = TestViewFactory(FooRendering::class)
+    val factory2 = TestViewFactory(BarRendering::class)
+    val registry = BindingViewRegistry(factory1, factory2)
 
-    assertThat(registry.keys).containsExactly(binding1.type, binding2.type)
+    assertThat(registry.keys).containsExactly(factory1.type, factory2.type)
   }
 
-  @Test fun `throws on duplicates`() {
-    val binding1 = TestBinding(FooRendering::class)
-    val binding2 = TestBinding(FooRendering::class)
+  @Test fun `constructor throws on duplicates`() {
+    val factory1 = TestViewFactory(FooRendering::class)
+    val factory2 = TestViewFactory(FooRendering::class)
 
     val error = assertFailsWith<IllegalStateException> {
-      BindingViewRegistry(binding1, binding2)
+      BindingViewRegistry(factory1, factory2)
     }
     assertThat(error).hasMessageThat()
         .endsWith("must not have duplicate entries.")
     assertThat(error).hasMessageThat()
         .contains(FooRendering::class.java.name)
   }
 
-  @Test fun `throws on missing binding`() {
-    val fooBinding = TestBinding(FooRendering::class)
-    val registry = BindingViewRegistry(fooBinding)
+  @Test fun `getFactoryFor works`() {
+    val fooFactory = TestViewFactory(FooRendering::class)
+    val registry = BindingViewRegistry(fooFactory)
+
+    val factory = registry.getFactoryFor(FooRendering::class)
+    assertThat(factory).isSameInstanceAs(fooFactory)
+  }
+
+  @Test fun `getFactoryFor throws on missing binding`() {
+    val fooFactory = TestViewFactory(FooRendering::class)
+    val registry = BindingViewRegistry(fooFactory)
 
     val error = assertFailsWith<IllegalArgumentException> {
-      registry.buildView(BarRendering)
+      registry.getFactoryFor(BarRendering::class)
     }
     assertThat(error).hasMessageThat()
         .isEqualTo(
-            "A ${ViewFactory::class.java.name} should have been registered to display $BarRendering."
+            "A ${ViewFactory::class.java.name} should have been registered to display a ${BarRendering::class}."
         )
   }