RA-64264 Make WorkflowLayout sample based on the frame clock

[rjrjr]

trafficstars

The renderings: Flow<Screen> provided to WorkflowLayout.take() can fire a lot more often than is useful, especially in large apps that don't use initialState / StateFlow.value effectively. Can take buffer what it receives, keeping only the latest, and take care to call [show] either once or never for frame clock tick?

Remember that it is absolutely fine to drop renderings; each time a new rendering appears it means that the combined workflow state has changed, and the previous ones are now invalid. We're not an animation tool.

[zach-klippenstein]

That would be the default behavior if WorkflowLayout were compose (since it would presumably collectAsState or collectAsStateWithLifecycle). I think for a View we could do something like this:

suspend fun <T> Flow<T>.collectAtFrame(choreographer: Choreographer, collector: () -> Unit) {
  var frameScheduled = false
  var mostRecentValue: T? = null
  val frameCallback = FrameCallback {
    // Not worried about races since everything's on the main thread, otherwise we'd need
    // a lock probably.
    frameScheduled = false
    collector(mostRecentValue)
  }

  try {
    collect { value ->
      mostRecentValue = value
      if (!frameScheduled) {
        frameScheduled = true
        // This is a fire-once callback: It automatically gets removed after firing.
        choreographer.postFrameCallback(frameCallback)
      }
    }
  } finally {
    // Noop if no callback currently posted.
    choreographer.removeFrameCallback(frameCallback)
  }
}

Caveat is I'm not sure how this will sync with Compose's frame callback. Compose also does postFrameCallback, so it would end up being a race and if collectAtFrame lost then the new renderings wouldn't show in Compose until the next frame, I think.

[zach-klippenstein]

We're not an animation tool.

If we were, we'd still want to only operate on frames 😂

[zach-klippenstein]

Actually I think we should probably just make show debounce itself until frame boundary, not put the logic in take.

[zach-klippenstein]

I haven't thought about nesting. If show is called as a result of a parent WorkflowLayout propagating the renderings down the view tree, then it shouldn't defer ever. (Compose also automatically handles this.) I'm not sure if this is a case we need to explicitly handle for views.

Ok so maybe take is the right place to put this debouncing. Assuming, IIRC, that when propagating down the view tree we call show directly and don't use take.

[rjrjr]

In the few instances where we have nested WorkflowLayout it's due to a separate workflow runtime being spun up by a legacy presenter object and feeding it directly. There is no recursion built into WorkflowLayout itself; and I can't see a reason for feature code to ever do that beause WorkflowViewStub and @Composable WorkflowRendering() exist and are attractive.

[rjrjr]

Did you just make an argument for us to get WorkflowLayout out of the top of our apps and do this instead? https://github.com/square/workflow-kotlin/blob/main/samples/compose-samples/src/main/java/com/squareup/sample/compose/hellocompose/HelloComposeActivity.kt

[steve-the-edwards]

Note that using a dispatcher hooked up to the frame clock - like Compose's AndroidUiDispatcher.Main- to dispatch the collection of the renderings from the Workflow runtime will have this effect as well.

Or rather I should say, that will work once we merge this - https://github.com/square/workflow-kotlin/pull/1424 - which will stop overriding the context.

That solves this problem that @zach-klippenstein raised as well:

Caveat is I'm not sure how this will sync with Compose's frame callback.