Merge pull request #1349 from square/ray/shiny-compose-samples

Updated ComposeScreen idioms in samples.

Author: rjrjr

samples/compose-samples/src/androidTest/java/com/squareup/sample/compose/preview/PreviewTest.kt

@@ -24,9 +24,9 @@ class PreviewTest {
     .around(IdlingDispatcherRule)
 
   @Test fun showsPreviewRendering() {
-    composeRule.onNodeWithText(ContactDetailsRendering::class.java.simpleName, substring = true)
+    composeRule.onNodeWithText(ContactDetailsScreen::class.java.simpleName, substring = true)
       .assertIsDisplayed()
-      .assertTextContains(previewContactRendering.details.phoneNumber, substring = true)
-      .assertTextContains(previewContactRendering.details.address, substring = true)
+      .assertTextContains(previewContactScreen.details.phoneNumber, substring = true)
+      .assertTextContains(previewContactScreen.details.address, substring = true)
   }
 }

samples/compose-samples/src/main/java/com/squareup/sample/compose/hellocompose/HelloComposeScreen.kt

@@ -16,16 +16,33 @@ data class HelloComposeScreen(
   val onClick: () -> Unit
 ) : ComposeScreen {
   @Composable override fun Content() {
-    Text(
-      message,
-      modifier = Modifier
-        .clickable(onClick = onClick)
-        .fillMaxSize()
-        .wrapContentSize(Alignment.Center)
-    )
+    // It is best to keep this method as empty as possible to avoid
+    // capturing state from stale ComposeScreen instances,
+    // and to keep from interfering with Compose's stability checks.
+    // https://developer.android.com/develop/ui/compose/performance/stability
+    Hello(this)
   }
 }
 
+/**
+ * @param modifier even though we use the default [Modifier] when calling
+ * from [HelloComposeScreen.Content], a habit of accepting this param from the
+ * Composable itself is handy for screenshot tests and previews.
+ */
+@Composable
+private fun Hello(
+  screen: HelloComposeScreen,
+  modifier: Modifier = Modifier
+) {
+  Text(
+    screen.message,
+    modifier = modifier
+      .clickable(onClick = screen.onClick)
+      .fillMaxSize()
+      .wrapContentSize(Alignment.Center)
+  )
+}
+
 @Preview(heightDp = 150, showBackground = true)
 @Composable
 private fun HelloPreview() {

samples/compose-samples/src/main/java/com/squareup/sample/compose/inlinerendering/InlineRenderingActivity.kt

@@ -20,7 +20,8 @@ import com.squareup.workflow1.ui.workflowContentView
 import kotlinx.coroutines.flow.StateFlow
 
 /**
- * A workflow that returns an anonymous `ComposeRendering`.
+ * A workflow that returns an anonymous
+ * [ComposeScreen][com.squareup.workflow1.ui.compose.ComposeScreen].
  */
 class InlineRenderingActivity : AppCompatActivity() {
   override fun onCreate(savedInstanceState: Bundle?) {

samples/compose-samples/src/main/java/com/squareup/sample/compose/inlinerendering/InlineRenderingWorkflow.kt

@@ -39,20 +39,28 @@ object InlineRenderingWorkflow : StatefulWorkflow<Unit, Int, Nothing, Screen>()
   ): ComposeScreen {
     val onClick = context.eventHandler("increment") { state += 1 }
     return ComposeScreen {
-      Box {
-        Button(onClick = onClick) {
-          Text("Counter: ")
-          AnimatedCounter(renderState) { counterValue ->
-            Text(counterValue.toString())
-          }
-        }
-      }
+      Content(renderState, onClick)
     }
   }
 
   override fun snapshotState(state: Int): Snapshot = Snapshot.of(state)
 }
 
+@Composable
+private fun Content(
+  count: Int,
+  onClick: () -> Unit
+) {
+  Box {
+    Button(onClick = onClick) {
+      Text("Counter: ")
+      AnimatedCounter(count) { counterValue ->
+        Text(counterValue.toString())
+      }
+    }
+  }
+}
+
 @Composable
 fun InlineRenderingWorkflowRendering() {
   val rendering by InlineRenderingWorkflow.renderAsState(

samples/compose-samples/src/main/java/com/squareup/sample/compose/preview/PreviewActivity.kt

@@ -33,9 +33,9 @@ class PreviewActivity : AppCompatActivity() {
   }
 }
 
-val previewContactRendering = ContactRendering(
+val previewContactScreen = ContactScreen(
   name = "Dim Tonnelly",
-  details = ContactDetailsRendering(
+  details = ContactDetailsScreen(
     phoneNumber = "555-555-5555",
     address = "1234 Apgar Lane"
   )
@@ -46,27 +46,30 @@ val previewContactRendering = ContactRendering(
 fun PreviewApp() {
   MaterialTheme {
     Surface {
-      previewContactRendering.Preview()
+      previewContactScreen.Preview()
     }
   }
 }
 
-data class ContactRendering(
+data class ContactScreen(
   val name: String,
-  val details: ContactDetailsRendering
+  val details: ContactDetailsScreen
 ) : ComposeScreen {
   @Composable override fun Content() {
-    ContactDetails(this)
+    Contact(this)
   }
 }
 
-data class ContactDetailsRendering(
+// Note, not a ComposeScreen and has no view binding of any kind,
+// which would normally be a runtime error. We're demonstrating that
+// the preview is able to stub out the WorkflowRendering call below.
+data class ContactDetailsScreen(
   val phoneNumber: String,
   val address: String
 ) : Screen
 
 @Composable
-private fun ContactDetails(rendering: ContactRendering) {
+private fun Contact(screen: ContactScreen) {
   Card(
     modifier = Modifier
       .padding(8.dp)
@@ -76,9 +79,9 @@ private fun ContactDetails(rendering: ContactRendering) {
       modifier = Modifier.padding(16.dp),
       verticalArrangement = spacedBy(8.dp),
     ) {
-      Text(rendering.name, style = MaterialTheme.typography.body1)
+      Text(screen.name, style = MaterialTheme.typography.body1)
       WorkflowRendering(
-        rendering = rendering.details,
+        rendering = screen.details,
         modifier = Modifier
           .aspectRatio(1f)
           .border(0.dp, Color.LightGray)

workflow-ui/compose/README.md

@@ -157,20 +157,30 @@ renderWorkflowIn(
 
 #### Defining Compose-based UI factories
 
-The most straightforward and common way to tie a `Screen` rendering type to a `@Composable` function is to implement [`ComposeScreen`](https://github.com/square/workflow-kotlin/blob/9bfd5119fabd0a3dfbc25bf7d93e52c7b31bb4cd/workflow-ui/compose/src/main/java/com/squareup/workflow1/ui/compose/ComposeScreen.kt), the Compose-friendly analog to [`AndroidScreen`](https://github.com/square/workflow-kotlin/blob/v1.12.1-beta06/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/AndroidScreen.kt).
+The most straightforward and common way to tie a `Screen` rendering type to a `@Composable` function
+is to implement [`ComposeScreen`](https://github.com/square/workflow-kotlin/blob/9bfd5119fabd0a3dfbc25bf7d93e52c7b31bb4cd/workflow-ui/compose/src/main/java/com/squareup/workflow1/ui/compose/ComposeScreen.kt), the Compose-friendly analog to [`AndroidScreen`](https://github.com/square/workflow-kotlin/blob/v1.12.1-beta06/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/AndroidScreen.kt).
 
 ```kotlin
- data class HelloScreen(
-   val message: String,
-   val onClick: () -> Unit
- ) : ComposeScreen {
-
-   @Composable override fun Content(viewEnvironment: ViewEnvironment) {
-     Button(onClick) {
-       Text(message)
-     }
-   }
- }
+import java.nio.file.WatchEvent.Modifier
+
+data class HelloScreen(
+  val message: String,
+  val onClick: () -> Unit
+) : ComposeScreen {
+  @Composable override fun Content() {
+    Hello(this)
+  }
+}
+
+@Composable
+private fun Hello(
+  screen: HelloScreen,
+  modifier: Modifier = Modifier
+) {
+  Button(screen.onClick, modifier) {
+    Text(message)
+  }
+}
 ```
 
 `ComposeScreen` is a convenience that automates creating a `ScreenComposableFactory` implementation responsible for expressing, say, `HelloScreen` instances by calling `HelloScreen.Content()`.
@@ -186,10 +196,10 @@ data class ContactScreen(
 ): Screen
 ```
 ```kotlin
-val contactUiFactory = ScreenComposableFactory<ContactScreen> { rendering, viewEnvironment ->
+val contactUiFactory = ScreenComposableFactory<ContactScreen> { screen ->
   Column {
-    Text(rendering.name)
-    Text(rendering.phoneNumber)
+    Text(screen.name)
+    Text(screen.phoneNumber)
   }
 }
 
@@ -215,7 +225,6 @@ Aka, `WorkflowViewStub` — Compose Edition! The idea of “view stub” is nons
 ```kotlin
 @Composable fun WorkflowRendering(
   rendering: Screen,
-  viewEnvironment: ViewEnvironment,
   modifier: Modifier = Modifier
 )
 ```
@@ -230,13 +239,12 @@ data class ContactScreen(
   val details: Screen
 ): Screen
 
-val contactUiFactory = ScreenComposableFactory<ContactScreen> { rendering, viewEnvironment ->
+val contactUiFactory = ScreenComposableFactory<ContactScreen> { screen ->
   Column {
-    Text(rendering.name)
+    Text(screen.name)
 
     WorkflowRendering(
-      rendering.details,
-      viewEnvironment,
+      screen.details,
       Modifier.fillMaxWidth()
     )
   }
@@ -307,36 +315,51 @@ Here’s an example:
 ```kotlin
 @Composable fun App(rootWorkflow: Workflow<...>) {
   var rootProps by remember { mutableStateOf(...) }
-  val viewEnvironment = ...
 
   val rootRendering by rootWorkflow.renderAsState(
     props = rootProps
   ) { output ->
     handleOutput(output)
   }
 
-  WorkflowRendering(rootRendering, viewEnvironment)
+  WorkflowRendering(rootRendering)
 }
 ```
 
 ----
 
 ## Potential risk: Data model
 
-Passing both the rendering and view environment down as parameters through the entire UI tree means that every time a rendering updates, we’ll recompose a lot of composables. This is how Workflow was designed, and because compose does some automatic deduping we’ll automatically avoid recomposing the leaves of the UI for a particular view factory unless the data for those bits of ui actually change. However, any time a leaf rendering changes, we’ll also be recomposing all the parent view factories just in order to propagate that leaf to its composable. That means we’re not able to take advantage of a lot of the other optimizations that compose tries to do both now and potentially in the future.
+Passing both rendering down as a parameter through the entire UI tree means that
+every time a rendering updates, we’ll recompose a lot of composables.
+This is how Workflow was designed, and because compose does some automatic deduping
+we’ll automatically avoid recomposing the leaves of the UI for a particular view factory
+unless the data for those bits of ui actually change. However, any time a leaf rendering changes,
+we’ll also be recomposing all the parent view factories just in order to propagate that leaf to its composable.
+That means we’re not able to take advantage of a lot of the other optimizations that compose tries to do both now and potentially in the future.
 
 In other words: “Workflow+views” < “Workflow+compose” < “data model designed specifically for compose + compose”.
 
-It should be straightforward to address this issue for view environments - see the _Alternative design_ section for more information. However, it’s not clear how to solve this for renderings without abandoning our current rendering data model. Today, renderings are an immutable tree of immutable value types that require the entire tree to be recreated any time any single piece of data changes. The reason for this design is that it was the only way to safely propagate changes without adding a bunch of reactive streams to renderings everywhere. The key word in that sentence is “was”: Compose’s snapshot state system makes it possible to expose simple mutable properties and still get change notifications that will ensure that the UI stays up-to-date (For an example of how this system can be used to model complex state systems with dependencies, see [this blog post](https://dev.to/zachklipp/plumbing-data-with-derived-state-in-compose-53ka)).
-
-Workflow could take advantage of this by allowing renderings to actually be mutable, so that when one Workflow deep in the tree wants to change something, it can do so independently and without requiring every rendering above it in the tree to also change. Making such a change to such a fundamental piece of Workflow design could have significant implications on other aspects of Workflow design, and doing so is very far outside the scope of this post.
-
-We want to call this out because it seems like we’ll be losing out on one of Compose’s optimization tricks, but we’re not sure how much of a problem this will turn out to be in the real world. The only performance issues that we’re aware of that we’ve run into with Workflow UI so far are issues with recreating leaf views on every rerender, and that in particular _*is*_ something Compose will automatically win at, even with our current data model.
-
-## Alternative design: Propagating `ViewEnvironment`s through `CompositionLocal`s
-
-You’ll notice that all the APIs described above explicitly pass `ViewEnvironment` objects around. This mirrors how other Workflow UI code works, as well as the Mosaic integration. Compose has the concept of “composition local” — which is similar in spirit to `ViewEnvironment` itself (and SwiftUI’s [`Environment`](https://developer.apple.com/documentation/swiftui/environment)). So why not just pass view environments implicitly through composition locals?
-
-This is what we did at first, but it made the API awkward for testing and other cases. Google advises against using composition locals in most cases for a reason. Because Workflow UI requires a `ViewRegistry` to be provided through the `ViewEnvironment`, there’s no obvious default value — what is the correct behavior when no `ViewEnvironment` local has been specified? Crashing at runtime is not ideal. We could provide an empty `ViewRegistry`, but that’s just another way to crash at runtime a few levels deeper in the call stack. Requiring explicit parameters for `ViewEnvironment` solves all these problems at the expense of a little more typing, and matches how the existing `ViewFactory` APIs work.
-
-On the other hand, providing an API to access individual view environment elements from a composable that hides the actual mechanism and uses composition locals under the hood would let us take much better advantage of Compose’s fine-grained UI updates. We could ensure that, when a view environment changes, only the parts of the UI that actually care about the modified part of the environment are recomposed. However, renderings typically change an order of magnitude more frequently than view environments, so there’s probably not much point solving this problem until we’ve solved the same problem with renderings (discussed above under _Potential risk: Data model_).
+It’s not clear how to solve this for renderings without abandoning our current rendering data model.
+Today, renderings are an immutable tree of immutable value types
+that require the entire tree to be recreated any time any single piece of data changes.
+The reason for this design is that it was the only way to safely propagate changes
+without adding a bunch of reactive streams to renderings everywhere.
+
+The key word in that sentence is “was”: Compose’s snapshot state system makes it possible
+to expose simple mutable properties and still get change notifications that will ensure
+that the UI stays up-to-date.
+For an example of how this system can be used to model complex state systems with dependencies,
+see [this blog post](https://dev.to/zachklipp/plumbing-data-with-derived-state-in-compose-53ka).
+
+Workflow could take advantage of this by allowing renderings to actually be mutable,
+so that when one Workflow deep in the tree wants to change something, it can do so independently
+and without requiring every rendering above it in the tree to also change.
+Making such a change to such a fundamental piece of Workflow design could have significant implications
+on other aspects of Workflow design, and doing so is very far outside the scope of this post.
+
+We want to call this out because it seems like we’ll be losing out on one of Compose’s optimization tricks,
+but we’re not sure how much of a problem this will turn out to be in the real world.
+The only performance issues that we’re aware of that we’ve run into with Workflow UI so far are issues
+with recreating leaf views on every rerender, and that in particular _*is*_ something Compose will automatically win at,
+even with our current data model.

workflow-ui/compose/src/main/java/com/squareup/workflow1/ui/compose/ComposeScreen.kt

@@ -24,24 +24,39 @@ import com.squareup.workflow1.ui.ViewRegistry
  *
  * Note that unlike most workflow view functions, [Content] does not take the rendering as a
  * parameter. Instead, the rendering is the receiver, i.e. the current value of `this`.
+ * Despite this (perhaps unfortunate) convenience, it is best to keep your `Content()`
+ * function as lean as possible to avoid interfering with Composes
+ * [stability calculations](https://developer.android.com/develop/ui/compose/performance/stability).
  *
  * Example:
  *
  *     data class HelloScreen(
  *       val message: String,
  *       val onClick: () -> Unit
  *     ) : ComposeScreen {
+ *       @Composable override fun Content() {
+ *         Hello(this)
+ *       }
+ *     }
  *
- *       @Composable override fun Content(viewEnvironment: ViewEnvironment) {
- *         Button(onClick) {
- *           Text(message)
+ *     @Composable
+ *     private fun Hello(
+ *       screen: HelloScreen,
+ *       modifier: Modifier = Modifier
+ *     ) {
+ *         Button(screen.onClick, modifier) {
+ *           Text(screen.message)
  *         }
- *       }
  *     }
  *
- * This is the simplest way to bridge the gap between your workflows and the UI, but using it
- * requires your workflows code to reside in Android modules and depend upon the Compose runtime,
- * instead of being pure Kotlin. If this is a problem, or you need more flexibility for any other
+ * (Note that the example includes a `modifier` parameter that is not used by
+ * the `HelloScreen` itself. We recommend this approach to simplify
+ * previews and snapshot tests.)
+ *
+ * [ComposeScreen] is the simplest way to bridge the gap between your workflows and the UI,
+ * but using it requires your workflows code to reside in Android modules
+ * and depend upon the Compose runtime, instead of being pure Kotlin.
+ * If this is a problem, or you need more flexibility for any other
  * reason, you can use [ViewRegistry] to bind your renderings to [ScreenComposableFactory]
  * implementations at runtime.
  *