API overhaul because splitting the keys and the composables don't work. #64

rjrjr · 2022-08-27T00:59:54Z

All of our tests and demos were built using String as the key, with content that does nothing but render the key.
This approach doesn't reflect reality very well, and masked #63, where keys for more interesting objects can get out of sync with the content lambda that can render them.
When popping, you would wind up crashing when the up to date lambda is unable to interpret the key for the screen that is being animated away.

The fix is to change the API from something that takes a list of keys and a function that can render them, to a list of model objects that themselves are able to provide @Composable Content().
IMHO the updated API actually feels pretty good, more like the conventional hoisted-state @Composable Foo(model: FooModel) idiom.
(Of course I've been working on this all day, so I'm biased.)

We provide a new interface:

interface BackstackFrame<out K : Any> {
  val key: K
  @Composable fun Content()
}

And change the signature of the Backstack() function:

fun <K : Any> Backstack(
  frames: List<BackstackFrame<K>>,
  modifier: Modifier = Modifier,
  frameController: FrameController<K>
)

Note that the param type, K, is still the type of the key, not the type of a particular flavor of BackstackFrame.
This makes it easy for us to provide convenience functions to map lists of arbitrary model objects to BackstackFrame instances, so it's not much more verbose than it used to be to make it go.

Before:

Backstack(backstack) { screen ->
  when(screen) {
    Screen.ContactList -> ShowContactList(navigator)
    is Screen.ContactDetails -> ShowContact(screen.id, navigator)
    is Screen.EditContact -> ShowEditContact(screen.id, navigator)
  }
}

After:

Backstack(
  backstack.toBackstackModel { screen ->
    when(screen) {
      Screen.ContactList -> ShowContactList(navigator)
      is Screen.ContactDetails -> ShowContact(screen.id, navigator)
      is Screen.EditContact -> ShowEditContact(screen.id, navigator)
    }
  }
)

Note that there are two flavors of toBackstackModel. The second one supports models with more interesting keys.

data class Portrait(
  val id: Int,
  val url: String
)

Backstack(
  backstack.toBackstackModel(
    getKey = { it.id }
  ) {
    PrettyPicture(it.url)
  }
)

Fixes #63

All of our tests and demos were built using `String` as the key, with `content` that does nothing but render the key. This approach doesn't reflect reality very well, and masked #63, where keys for more interesting objects can get out of sync with the `content` lambda that can render them. When popping, you would wind up crashing when the up to date lambda is unable to interpret the key for the screen that is being animated away. The fix is to change the API from something that takes a list of keys and a function that can render them, to a list of model objects that themselves are able to provide `@Composable Content()`. IMHO the updated API actually feels pretty good, more like the conventional hoisted-state `@Composable Foo(model: FooModel)` idiom. (Of course I've been working on this all day, so I'm biased.) We provide a new interface: ```kotlin interface BackstackFrame<out K : Any> { val key: K @composable fun Content() } ``` And change the signature of the `Backstack()` function: ```kotlin fun <K : Any> Backstack( frames: List<BackstackFrame<K>>, modifier: Modifier = Modifier, frameController: FrameController<K> ) ``` Note that the param type, `K`, is still the type of the key, not the type of a particular flavor of `BackstackFrame`. This makes it easy for us to provide convenience functions to map lists of arbitrary model objects to `BackstackFrame` instances, so it's not much more verbose than it used to be to make it go. Before: ```kotlin Backstack(backstack) { screen -> when(screen) { Screen.ContactList -> ShowContactList(navigator) is Screen.ContactDetails -> ShowContact(screen.id, navigator) is Screen.EditContact -> ShowEditContact(screen.id, navigator) } } ``` After: ```kotlin Backstack( backstack.toBackstackModel { screen -> when(screen) { Screen.ContactList -> ShowContactList(navigator) is Screen.ContactDetails -> ShowContact(screen.id, navigator) is Screen.EditContact -> ShowEditContact(screen.id, navigator) } } ) ``` Note that there are two flavors of `toBackstackModel`. The second one supports models with more interesting keys. ```kotlin data class Portrait( val id: Int, val url: String ) Backstack( backstack.toBackstackModel( getKey = { it.id } ) { PrettyPicture(it.url) } ) ``` Fixes #63

zach-klippenstein

I know you're in a time crunch and I like the overall direction so approving. But this API feels very unidiomatic for Compose.

I really want to be able to write:

Backstack(listOfMyModels, key = { it.key }) { model ->
  Content()
}

I think the only thing preventing you from making that API right now is that both the Backstack function and the FrameController class need to know of the frame type, and right now that would require passing the key function in twice. I think we could solve that by passing in a frame controller factory instead of the controller itself, and the factory would create a controller given the key function. Then Backstack could instantiate the actual controller itself.

Anyway, that's quite a significant change, and I'm not 100% confident it is the right move either, so this is fine for now.

zach-klippenstein · 2022-08-30T15:23:54Z

compose-backstack/src/main/java/com/zachklipp/compose/backstack/BackstackFrame.kt

+  model: M,
+  key: K,
+  crossinline content: @Composable (M) -> Unit


Why do you need to take both the model and a function that takes the model? If you just take the function, the caller can just capture the model, right?

zach-klippenstein · 2022-08-30T15:25:02Z

compose-backstack/src/main/java/com/zachklipp/compose/backstack/BackstackFrame.kt

+  }
+}
+
+inline fun <reified M: Any> List<M>.toBackstackModel(


Nit: either call this mapToBackstackModels or at least pluralize?

Counter nit: List is a Backstack model.

zach-klippenstein · 2022-08-30T15:27:45Z

compose-backstack/src/main/java/com/zachklipp/compose/backstack/BackstackFrame.kt

+ * with [Composable] code that can display them, suitable for use
+ * with [Backstack].
+ */
+interface BackstackFrame<out K : Any> {


Nit: I think you might notice better recomposition performance if you mark this interface as @Stable. But also very possibly not, since it's going to always be in a List which is not stable. Would be something to measure/test.

zach-klippenstein · 2022-08-30T15:30:18Z

compose-backstack/src/main/java/com/zachklipp/compose/backstack/BackstackFrame.kt

+
+/**
+ * Models a frame in a Backstack, with a unique [key] to identify it,
+ * and a [Content] function to display it.


Nit: it would be helpful to explain to maybe what the key is used for so that people would have some idea of what sort of thing to pass in.

zach-klippenstein · 2022-08-30T15:32:36Z

compose-backstack/src/main/java/com/zachklipp/compose/backstack/FrameController.kt


  /**
   * A frame controlled by a [FrameController], to be shown by [Backstack].
   */
  @Immutable
-  data class BackstackFrame<out T : Any>(
-    val key: T,
+  data class FrameAndModifier<out K : Any>(


Nit: Modified(Backstack)Frame?

zach-klippenstein · 2022-08-30T15:45:22Z

compose-backstack/src/main/java/com/zachklipp/compose/backstack/TransitionController.kt

-    val popping = toKey in fromKeys
+    val fromFrame = fromFrames.last()
+    val toFrame = toFrames.last()
+    val popping = fromFrames.firstOrNull { it.key == toFrame.key } != null


Suggested change

val popping = fromFrames.firstOrNull { it.key == toFrame.key } != null

val popping = fromFrames.any { it.key == toFrame.key }

zach-klippenstein · 2022-08-30T15:47:28Z

compose-backstack/src/main/java/com/zachklipp/compose/backstack/TransitionController.kt

    val progress = Animatable(0f)

    val fromVisibility = derivedStateOf { 1f - progress.value }
    val toVisibility = progress.asState()

    // Wrap modifier functions in each their own recompose scope so that if they read the visibility
    // (or any other state) directly, the modified node will actually be updated.
+    @SuppressLint("UnnecessaryComposedModifier")


So glad I put that comment in – I hope i wrote a test too 😅

zach-klippenstein · 2022-08-30T15:50:11Z

compose-backstack/src/main/java/com/zachklipp/compose/backstack/Backstack.kt

 *
- * The [backstack] must follow some rules:
+ * The [frames] list must follow some rules:


"Frames list" is a lot less meaningful than "backstack", since the frame list semantically is a backstack, so I would prefer keeping backstack here.

Suggested change

* The [frames] list must follow some rules:

* The [backstack][frames] must follow some rules:

zach-klippenstein · 2022-08-30T15:50:50Z

compose-backstack/src/main/java/com/zachklipp/compose/backstack/Backstack.kt

 *  - Must always contain at least one item.
- *  - Items in the stack must implement `equals` and not change over the lifetime of the screen.
- *    If an item changes, it will be considered a new screen and any state held by the screen will
+ *  - Keys must implement `equals` and cannot change over the lifetime of the screen.


Suggested change

* - Keys must implement `equals` and cannot change over the lifetime of the screen.

* - [Keys][BackstackFrame.key] must implement `equals` and cannot change over the lifetime of the screen.

lucamtudor · 2023-01-16T14:11:30Z

following some talks & chats, I've landed on this PR. @rjrjr can you drop an update, just so I understand what's your current thinking on all this? I'm looking for a "pure Compose backstack container" solution (because of square/workflow-kotlin#750, square/workflow-kotlin#669)

rjrjr · 2023-01-18T19:37:14Z

Wow, we left you dangling. Very sorry.

I want to go back to that PR and clean up the API in the way that @zach-klippenstein suggested, but haven't managed to make time for it yet. What I should really do is ask him to pair with me.

But the PR worked well as is, if you'd want to just fork. :/

rjrjr force-pushed the ray/take-two branch 4 times, most recently from 8b55940 to a3d85d7 Compare August 27, 2022 01:20

rjrjr force-pushed the ray/take-two branch from a3d85d7 to 0fc1c2c Compare August 27, 2022 01:23

rjrjr marked this pull request as ready for review August 27, 2022 02:50

rjrjr requested a review from zach-klippenstein August 29, 2022 00:43

zach-klippenstein approved these changes Aug 30, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API overhaul because splitting the keys and the composables don't work. #64

API overhaul because splitting the keys and the composables don't work. #64

rjrjr commented Aug 27, 2022 •

edited

Loading

zach-klippenstein left a comment

zach-klippenstein Aug 30, 2022

zach-klippenstein Aug 30, 2022

rjrjr Aug 30, 2022

zach-klippenstein Aug 30, 2022

zach-klippenstein Aug 30, 2022

zach-klippenstein Aug 30, 2022

zach-klippenstein Aug 30, 2022

zach-klippenstein Aug 30, 2022

zach-klippenstein Aug 30, 2022

zach-klippenstein Aug 30, 2022

lucamtudor commented Jan 16, 2023

rjrjr commented Jan 18, 2023

	val popping = fromFrames.firstOrNull { it.key == toFrame.key } != null
	val popping = fromFrames.any { it.key == toFrame.key }

	* The [frames] list must follow some rules:
	* The [backstack][frames] must follow some rules:

	* - Keys must implement `equals` and cannot change over the lifetime of the screen.
	* - [Keys][BackstackFrame.key] must implement `equals` and cannot change over the lifetime of the screen.

API overhaul because splitting the keys and the composables don't work. #64

Are you sure you want to change the base?

API overhaul because splitting the keys and the composables don't work. #64

Conversation

rjrjr commented Aug 27, 2022 • edited Loading

zach-klippenstein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucamtudor commented Jan 16, 2023

rjrjr commented Jan 18, 2023

rjrjr commented Aug 27, 2022 •

edited

Loading