Merge pull request #29 from square/zachklipp/inline-viewstub

Inline WorkflowViewStub logic in ViewFactory.showRendering to avoid unnecessary intermediate Views.

Author: zach-klippenstein

.buildscript/android-ui-tests.gradle

@@ -2,6 +2,9 @@ android {
   defaultConfig {
     testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner"
   }
+  testOptions {
+    animationsDisabled = true
+  }
 }
 
 dependencies {

.github/workflows/kotlin.yml

@@ -100,9 +100,7 @@ jobs:
       fail-fast: false
       matrix:
         api-level:
-          # Tests are failing on APIs <24.
-          #- 21
-          #- 23
+          - 21
           - 24
           - 29
     steps:

core-compose/src/main/java/com/squareup/workflow/ui/compose/ComposeViewFactory.kt

@@ -29,7 +29,8 @@ import androidx.compose.mutableStateOf
 import com.squareup.workflow.ui.ViewEnvironment
 import com.squareup.workflow.ui.ViewFactory
 import com.squareup.workflow.ui.bindShowRendering
-import com.squareup.workflow.ui.compose.internal.setOrContinueContent
+import com.squareup.workflow.ui.compose.internal.ParentComposition
+import com.squareup.workflow.ui.compose.internal.setOrSubcomposeContent
 import kotlin.reflect.KClass
 
 /**
@@ -111,23 +112,25 @@ internal class ComposeViewFactory<RenderingT : Any>(
         areEquivalent = StructurallyEqual
     )
 
-    // Models will throw if their properties are accessed when there is no frame open. Currently,
-    // that will be the case if the model is accessed before any other Compose infrastructure has
-    // ran, i.e. if this view factory is the first compose code to run in the app.
-    // I believe that eventually there will be a global frame that will make this unnecessary.
-    FrameManager.ensureStarted()
-
     // Update the state whenever a new rendering is emitted.
     composeContainer.bindShowRendering(
         initialRendering,
         initialViewEnvironment
     ) { rendering, environment ->
       // This lambda will be executed synchronously before bindShowRendering returns.
-      renderState.value = Pair(rendering, environment)
+
+      // Models will throw if their properties are accessed when there is no frame open. Currently,
+      // that will be the case if the model is accessed before any other Compose infrastructure has
+      // run, i.e. if this view factory is the first compose code to run in the app.
+      // I believe that eventually there will be a global frame that will make this unnecessary.
+      FrameManager.framed {
+        renderState.value = Pair(rendering, environment)
+      }
     }
 
     // Entry point to the world of Compose.
-    composeContainer.setOrContinueContent(initialViewEnvironment) {
+    val parentComposition = initialViewEnvironment[ParentComposition]
+    composeContainer.setOrSubcomposeContent(parentComposition.reference) {
       val (rendering, environment) = renderState.value!!
       showRenderingWrappedWithRoot(rendering, environment)
     }

core-compose/src/main/java/com/squareup/workflow/ui/compose/internal/ParentComposition.kt

@@ -22,24 +22,22 @@ import androidx.compose.Composable
 import androidx.compose.CompositionReference
 import androidx.compose.Recomposer
 import androidx.compose.compositionReference
-import androidx.compose.currentComposer
 import androidx.ui.core.setContent
 import com.squareup.workflow.ui.ViewEnvironment
 import com.squareup.workflow.ui.ViewEnvironmentKey
 
 /**
- * Holds a [CompositionReference] and a [Recomposer] that can be used to [setContent] to create a
- * composition that is a child of another composition. Child compositions get ambients and other
- * compose context from their parent, which allows ambients provided around a [showRendering] call
- * to be read by nested [bindCompose] factories.
+ * Holds a [CompositionReference] and that can be passed to [setOrSubcomposeContent] to create a
+ * composition that is a child of another composition. Subcompositions get ambients and other
+ * compose context from their parent, and propagate invalidations, which allows ambients provided
+ * around a [showRendering] call to be read by nested Compose-based view factories.
  *
  * When [showRendering] is called, it will store an instance of this class in the [ViewEnvironment].
- * [ComposeViewFactory] will then pull the continuation out of the environment and use it to link
- * its composition to the outer one.
+ * [ComposeViewFactory] pulls the reference out of the environment and uses it to link its
+ * composition to the outer one.
  */
-internal data class ParentComposition(
-  val reference: CompositionReference? = null,
-  val recomposer: Recomposer? = null
+internal class ParentComposition(
+  var reference: CompositionReference? = null
 ) {
   companion object : ViewEnvironmentKey<ParentComposition>(ParentComposition::class) {
     override val default: ParentComposition get() = ParentComposition()
@@ -50,31 +48,29 @@ internal data class ParentComposition(
  * Creates a [ParentComposition] from the current point in the composition and adds it to this
  * [ViewEnvironment].
  */
-@Composable internal fun ViewEnvironment.withParentComposition(): ViewEnvironment {
-  val compositionReference = ParentComposition(
-      reference = compositionReference(),
-      recomposer = currentComposer.recomposer
-  )
+@Composable internal fun ViewEnvironment.withParentComposition(
+  reference: CompositionReference = compositionReference()
+): ViewEnvironment {
+  val compositionReference = ParentComposition(reference = reference)
   return this + (ParentComposition to compositionReference)
 }
 
 /**
  * Starts composing [content] into this [ViewGroup].
  *
- * If there is a [ParentComposition] present in [initialViewEnvironment], it will start the
- * composition as a subcomposition of that continuation.
+ * If [parentComposition] is not null, [content] will be installed as a _subcomposition_ of the
+ * parent composition, meaning that it will propagate ambients and invalidation.
  *
  * This function corresponds to [withParentComposition].
  */
-internal fun ViewGroup.setOrContinueContent(
-  initialViewEnvironment: ViewEnvironment,
+internal fun ViewGroup.setOrSubcomposeContent(
+  parentComposition: CompositionReference?,
   content: @Composable() () -> Unit
 ) {
-  val (compositionReference, recomposer) = initialViewEnvironment[ParentComposition]
-  if (compositionReference != null && recomposer != null) {
+  if (parentComposition != null) {
     // Somewhere above us in the workflow rendering tree, there's another bindCompose factory.
     // We need to link to its composition reference so we inherit its ambients.
-    setContent(recomposer, compositionReference, content)
+    setContent(Recomposer.current(), parentComposition, content)
   } else {
     // This is the first bindCompose factory in the rendering tree, so it won't be a child
     // composition.

core-compose/src/main/java/com/squareup/workflow/ui/compose/internal/ViewFactories.kt

@@ -15,17 +15,24 @@
  */
 package com.squareup.workflow.ui.compose.internal
 
-import android.content.Context
-import android.view.ViewGroup.LayoutParams.MATCH_PARENT
-import android.widget.FrameLayout
+import android.view.View
+import android.view.ViewGroup
 import androidx.compose.Composable
+import androidx.compose.compositionReference
+import androidx.compose.onPreCommit
+import androidx.compose.remember
+import androidx.ui.core.AndroidOwner
+import androidx.ui.core.ContextAmbient
 import androidx.ui.core.Modifier
+import androidx.ui.core.OwnerAmbient
+import androidx.ui.core.Ref
 import androidx.ui.foundation.Box
+import androidx.ui.viewinterop.AndroidView
 import com.squareup.workflow.ui.ViewEnvironment
 import com.squareup.workflow.ui.ViewFactory
-import com.squareup.workflow.ui.WorkflowViewStub
+import com.squareup.workflow.ui.canShowRendering
 import com.squareup.workflow.ui.compose.ComposeViewFactory
-import com.squareup.workflow.ui.compose.internal.ComposableViewStubWrapper.Update
+import com.squareup.workflow.ui.showRendering
 
 /**
  * Renders [rendering] into the composition using the `ViewRegistry` from the [ViewEnvironment] to
@@ -49,49 +56,73 @@ import com.squareup.workflow.ui.compose.internal.ComposableViewStubWrapper.Updat
 ) {
   val viewFactory = this
   Box(modifier = modifier) {
-    // Fast path: If the child binding is also a Composable, we don't need to go through the legacy
-    // view system and can just invoke the binding's composable function directly.
+    // "Fast" path: If the child binding is also a Composable, we don't need to go through the
+    // legacy view system and can just invoke the binding's composable function directly.
     if (viewFactory is ComposeViewFactory) {
       viewFactory.showRenderingWrappedWithRoot(rendering, viewEnvironment)
-    } else {
-      // Plumb the current composition "context" through the ViewEnvironment so any nested
-      // composable factories get access to any ambients currently in effect.
-      // See setOrContinueContent().
-      val newEnvironment = viewEnvironment.withParentComposition()
-
-      // IntelliJ currently complains very loudly about this function call, but it actually
-      // compiles. The IDE tooling isn't currently able to recognize that the Compose compiler
-      // accepts this code.
-      ComposableViewStubWrapper(update = Update(rendering, newEnvironment))
+      return@Box
     }
+
+    // "Slow" path: Create a legacy Android View to show the rendering, like WorkflowViewStub.
+    ViewFactoryAndroidView(viewFactory, rendering, viewEnvironment)
   }
 }
 
 /**
- * Wraps a [WorkflowViewStub] with an API that is more Compose-friendly.
+ * This is effectively the logic of [com.squareup.workflow.ui.WorkflowViewStub], but translated
+ * into Compose idioms. This approach has a few advantages:
+ *
+ *  - Avoids extra custom views required to host `WorkflowViewStub` inside a Composition. Its trick
+ *    of replacing itself in its parent doesn't play nice with Compose.
+ *  - Allows us to pass the correct parent view for inflation (the root of the composition).
+ *  - Avoids `WorkflowViewStub` having to do its own lookup to find the correct [ViewFactory], since
+ *    we already have the correct one.
  *
- * In particular, Compose will only generate `Emittable`s for views with a single constructor
- * that takes a [Context].
+ * Like `WorkflowViewStub`, this function uses the [viewFactory] to create and memoize a [View] to
+ * display the [rendering], keeps it updated with the latest [rendering] and [viewEnvironment], and
+ * adds it to the composition.
  *
- * See [this slack message](https://kotlinlang.slack.com/archives/CJLTWPH7S/p1576264533012000?thread_ts=1576262311.008800&cid=CJLTWPH7S).
+ * This function also passes a [ParentComposition] down through the [ViewEnvironment] so that if the
+ * child view further nests any `ComposableViewFactory`s, they will be correctly subcomposed.
  */
-private class ComposableViewStubWrapper(context: Context) : FrameLayout(context) {
+@Composable private fun <R : Any> ViewFactoryAndroidView(
+  viewFactory: ViewFactory<R>,
+  rendering: R,
+  viewEnvironment: ViewEnvironment
+) {
+  val childView = remember { Ref<View>() }
 
-  data class Update(
-    val rendering: Any,
-    val viewEnvironment: ViewEnvironment
-  )
+  // Plumb the current composition through the ViewEnvironment so any nested composable factories
+  // get access to any ambients currently in effect. See setOrSubcomposeContent().
+  val parentComposition = remember { ParentComposition() }
+  parentComposition.reference = compositionReference()
+  val wrappedEnvironment = remember(viewEnvironment) {
+    viewEnvironment + (ParentComposition to parentComposition)
+  }
 
-  private val viewStub = WorkflowViewStub(context)
+  // A view factory can decide to recreate its view at any time. This also covers the case where
+  // the value of the viewFactory argument has changed, including to one with a different type.
+  if (childView.value?.canShowRendering(rendering) != true) {
+    // If we don't pass the parent Android View, the child will have the wrong LayoutParams.
+    // OwnerAmbient is deprecated, but the only way to get the root view currently. I've filed
+    // a feature request to expose this as first-class API, see
+    // https://issuetracker.google.com/issues/156875705.
+    @Suppress("DEPRECATION")
+    val parentView = (OwnerAmbient.current as? AndroidOwner)?.view as? ViewGroup
 
-  init {
-    addView(viewStub)
+    childView.value = viewFactory.buildView(
+        initialRendering = rendering,
+        initialViewEnvironment = wrappedEnvironment,
+        contextForNewView = ContextAmbient.current,
+        container = parentView
+    )
   }
 
-  // Compose turns this into a parameter when you invoke this class as a Composable.
-  fun setUpdate(update: Update) {
-    viewStub.update(update.rendering, update.viewEnvironment)
+  // Invoke the ViewFactory's update logic whenever the view, the rendering, or the ViewEnvironment
+  // change.
+  onPreCommit(childView.value, rendering, wrappedEnvironment) {
+    childView.value!!.showRendering(rendering, wrappedEnvironment)
   }
 
-  override fun getLayoutParams(): LayoutParams = LayoutParams(MATCH_PARENT, MATCH_PARENT)
+  AndroidView(childView.value!!)
 }

samples/nested-renderings/src/androidTest/java/com/squareup/sample/nestedrenderings/NestedRenderingsTest.kt

@@ -24,7 +24,6 @@ import androidx.ui.test.assertIsDisplayed
 import androidx.ui.test.doClick
 import androidx.ui.test.findAllByText
 import androidx.ui.test.findByText
-import androidx.ui.test.last
 import org.junit.Rule
 import org.junit.Test
 import org.junit.runner.RunWith
@@ -49,11 +48,32 @@ class NestedRenderingsTest {
     findAllByText(ADD_BUTTON_TEXT)
         .assertCountEquals(4)
 
-    findAllByText("Reset").last()
-        .doClick()
+    resetAll()
     findAllByText(ADD_BUTTON_TEXT).assertCountEquals(1)
   }
 
+  /**
+   * We can't rely on the order of nodes returned by [findAllByText], and the contents of the
+   * collection will change as we remove nodes, so we have to double-loop over all reset buttons and
+   * click them all until there is only one left.
+   */
+  private fun resetAll() {
+    var foundNodes = Int.MAX_VALUE
+    while (foundNodes > 1) {
+      foundNodes = 0
+      findAllByText("Reset").forEach {
+        try {
+          it.assertExists()
+        } catch (e: AssertionError) {
+          // No more reset buttons, we're done.
+          return@forEach
+        }
+        foundNodes++
+        it.doClick()
+      }
+    }
+  }
+
   private fun SemanticsNodeInteractionCollection.forEach(
     block: (SemanticsNodeInteraction) -> Unit
   ) {