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 10, 2020 · e97c1f1 · e97c1f1
1 parent e096d14
commit e97c1f1
Show file tree

Hide file tree

Showing 8 changed files with 199 additions and 127 deletions.
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 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 find 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}."
         )
   }
 

diff --git a/...kflow-ui/core-android/src/test/java/com/squareup/workflow/ui/CompositeViewRegistryTest.kt b/...kflow-ui/core-android/src/test/java/com/squareup/workflow/ui/CompositeViewRegistryTest.kt
@@ -15,30 +15,16 @@
  */
 package com.squareup.workflow.ui
 
-import android.content.Context
-import android.view.View
-import android.view.ViewGroup
 import com.google.common.truth.Truth.assertThat
-import com.nhaarman.mockito_kotlin.mock
 import org.junit.Test
 import kotlin.reflect.KClass
 import kotlin.test.assertFailsWith
 
 class CompositeViewRegistryTest {
 
-  @Test fun `throws on duplicates`() {
-    val fooBarRegistry = TestRegistry(
-        mapOf(
-            FooRendering::class to mock(),
-            BarRendering::class to mock()
-        )
-    )
-    val barBazRegistry = TestRegistry(
-        mapOf(
-            BarRendering::class to mock(),
-            BazRendering::class to mock()
-        )
-    )
+  @Test fun `constructor throws on duplicates`() {
+    val fooBarRegistry = TestRegistry(setOf(FooRendering::class, BarRendering::class))
+    val barBazRegistry = TestRegistry(setOf(BarRendering::class, BazRendering::class))
 
     val error = assertFailsWith<IllegalStateException> {
       CompositeViewRegistry(fooBarRegistry, barBazRegistry)
@@ -49,32 +35,43 @@ class CompositeViewRegistryTest {
         .contains(BarRendering::class.java.name)
   }
 
-  @Test fun `buildView delegates to composite registries`() {
-    val fooView = mock<View>()
-    val barView = mock<View>()
-    val bazView = mock<View>()
+  @Test fun `getFactoryFor delegates to composite registries`() {
+    val fooFactory = TestViewFactory(FooRendering::class)
+    val barFactory = TestViewFactory(BarRendering::class)
+    val bazFactory = TestViewFactory(BazRendering::class)
     val fooBarRegistry = TestRegistry(
         mapOf(
-            FooRendering::class to fooView,
-            BarRendering::class to barView
+            FooRendering::class to fooFactory,
+            BarRendering::class to barFactory
         )
     )
-    val bazRegistry = TestRegistry(mapOf(BazRendering::class to bazView))
+    val bazRegistry = TestRegistry(factories = mapOf(BazRendering::class to bazFactory))
     val registry = CompositeViewRegistry(fooBarRegistry, bazRegistry)
 
-    assertThat(registry.buildView(FooRendering)).isSameInstanceAs(fooView)
-    assertThat(registry.buildView(BarRendering)).isSameInstanceAs(barView)
-    assertThat(registry.buildView(BazRendering)).isSameInstanceAs(bazView)
+    assertThat(registry.getFactoryFor(FooRendering::class))
+        .isSameInstanceAs(fooFactory)
+    assertThat(registry.getFactoryFor(BarRendering::class))
+        .isSameInstanceAs(barFactory)
+    assertThat(registry.getFactoryFor(BazRendering::class))
+        .isSameInstanceAs(bazFactory)
   }
 
-  @Test fun `keys includes all composite registries' keys`() {
-    val fooBarRegistry = TestRegistry(
-        mapOf(
-            FooRendering::class to mock(),
-            BarRendering::class to mock()
+  @Test fun `getFactoryFor throws on missing registry`() {
+    val fooRegistry = TestRegistry(setOf(FooRendering::class))
+    val registry = CompositeViewRegistry(fooRegistry)
+
+    val error = assertFailsWith<IllegalArgumentException> {
+      registry.getFactoryFor(BarRendering::class)
+    }
+    assertThat(error).hasMessageThat()
+        .isEqualTo(
+            "A ${ViewFactory::class.java.name} should have been registered to display a ${BarRendering::class}."
         )
-    )
-    val bazRegistry = TestRegistry(mapOf(BazRendering::class to mock()))
+  }
+
+  @Test fun `keys includes all composite registries' keys`() {
+    val fooBarRegistry = TestRegistry(setOf(FooRendering::class, BarRendering::class))
+    val bazRegistry = TestRegistry(setOf(BazRendering::class))
     val registry = CompositeViewRegistry(fooBarRegistry, bazRegistry)
 
     assertThat(registry.keys).containsExactly(
@@ -84,31 +81,18 @@ class CompositeViewRegistryTest {
     )
   }
 
-  @Test fun `throws on missing binding`() {
-    val fooRegistry = TestRegistry(mapOf(FooRendering::class to mock()))
-    val registry = CompositeViewRegistry(fooRegistry)
-
-    val error = assertFailsWith<IllegalArgumentException> {
-      registry.buildView(BarRendering)
-    }
-    assertThat(error).hasMessageThat()
-        .isEqualTo(
-            "A ${ViewFactory::class.java.name} should have been registered to display $BarRendering."
-        )
-  }
-
   private object FooRendering
   private object BarRendering
   private object BazRendering
 
-  private class TestRegistry(private val bindings: Map<KClass<*>, View>) : ViewRegistry {
-    override val keys: Set<KClass<*>> get() = bindings.keys
+  private class TestRegistry(private val factories: Map<KClass<*>, ViewFactory<*>>) : ViewRegistry {
+    constructor(keys: Set<KClass<*>>) : this(keys.associateWith { TestViewFactory(it) })
+
+    override val keys: Set<KClass<*>> get() = factories.keys
 
-    override fun <RenderingT : Any> buildView(
-      initialRendering: RenderingT,
-      initialViewEnvironment: ViewEnvironment,
-      contextForNewView: Context,
-      container: ViewGroup?
-    ): View = bindings.getValue(initialRendering::class)
+    @Suppress("UNCHECKED_CAST")
+    override fun <RenderingT : Any> getFactoryFor(
+      renderingType: KClass<out RenderingT>
+    ): ViewFactory<RenderingT> = factories.getValue(renderingType) as ViewFactory<RenderingT>
   }
 }
diff --git a/...a/com/squareup/workflow/ui/TestBinding.kt → ...m/squareup/workflow/ui/TestViewFactory.kt b/...a/com/squareup/workflow/ui/TestBinding.kt → ...m/squareup/workflow/ui/TestViewFactory.kt
@@ -23,10 +23,9 @@ import kotlin.reflect.KClass
 import kotlin.test.fail
 
 fun <R : Any> ViewRegistry.buildView(rendering: R): View =
-  buildView(rendering, ViewEnvironment(this), mock())
+  buildView(rendering, ViewEnvironment(this), mock<Context>())
 
-class TestBinding<R : Any>(override val type: KClass<R>) :
-    ViewFactory<R> {
+class TestViewFactory<R : Any>(override val type: KClass<R>) : ViewFactory<R> {
   override fun buildView(
     initialRendering: R,
     initialViewEnvironment: ViewEnvironment,