Add lifecycle aware Presenters (#1282)

This PR adds a new `Lifecycle` interface, which presenters and UI can observe to know when they are 'paused'. The API is rudimentary at the moment and will change before this is ready to land. A bundled `PauseablePresenter` class can be extended, enabling clients to automatically add pausing ability to their presenters. This impl will simply return the last emitted `UiState` when the presenter is paused. Again, name TBD. ### Other things: - `GestureNavigationRetainedStateTest` and `GestureNavigationSaveableStateTest` have been combined into a parameterized `GestureNavigationStateTest`. This new test also now tests `CupertinoGestureNavigationDecoration` so we get extra coverage. - Changed `rememberRetained`'s `key` param to `Any` to be consistent with all of the other `remember` functions. ### TODO: - [x] Saveable is currently broken by these changes. This needs to be fixed before landing. - [x] Remove all of the logging code. - [x] Add comments - [x] Fix the last remaining failing test
slackhq · May 26, 2024 · bb09273 · bb09273
1 parent 9e7d982
commit bb09273
Show file tree

Hide file tree

Showing 12 changed files with 392 additions and 310 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,8 @@ Changelog
 - **New**: Add `FakeNavigator` functions to check for the lack of pop/resetRoot events.
 - **New**: Add `FakeNavigator` constructor param to add additional screens to the backstack.
 - **New**: Add support for static UIs. In some cases, a UI may not need a presenter to compute or manage its state. Examples of this include UIs that are stateless or can derive their state from a single static input or an input [Screen]'s properties. In these cases, make your _screen_ implement the `StaticScreen` interface. When a `StaticScreen` is used, Circuit will internally allow the UI to run on its own and won't connect it to a presenter if no presenter is provided.
+- **New**: Add `RecordLifecycle` and `LocalRecordLifecycle` composition local, allowing UIs and presenters to observe when they are 'active'. Currently, a record is 'active' when it is the top record on the back stack.
+- **Behaviour Change**: Presenters are now 'paused' and replay their last emitted `CircuitUiState` when they are not active. Presenters can opt-out of this behavior by implementing `NonPausablePresenter`.
 - **Behaviour Change**: `NavigatorImpl.goTo` no longer navigates if the `Screen` is equal to `Navigator.peek()`.
 - **Behaviour Change**: `Presenter.present` is now annotated with `@ComposableTarget("presenter")`. This helps prevent use of Compose UI in the presentation logic as the compiler will emit a warning if you do. Note this does not appear in the IDE, so it's recommended to use `allWarningsAsErrors` to fail the build on this event.
 - **Change**: `Navigator.goTo` now returns a Bool indicating navigation success.

diff --git a/...UnitTest/kotlin/com/slack/circuit/foundation/NavigableCircuitViewModelStateAndroidTest.kt b/...UnitTest/kotlin/com/slack/circuit/foundation/NavigableCircuitViewModelStateAndroidTest.kt
@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 package com.slack.circuit.foundation
 
+import androidx.compose.ui.test.MainTestClock
 import androidx.compose.ui.test.SemanticsMatcher
 import androidx.compose.ui.test.assertAll
 import androidx.compose.ui.test.assertAny
@@ -36,13 +37,12 @@ class NavigableCircuitViewModelStateAndroidTest {
   @Test
   fun retainedStateScopedToBackstackWithRecreations() {
     composeTestRule.run {
-      mainClock.autoAdvance = false
+      mainClock.autoAdvance = true
 
       // Current: Screen A. Increase count to 1
       onNodeWithTag(TAG_LABEL).assertTextEquals("A")
       onNodeWithTag(TAG_COUNT).assertTextEquals("0")
       onNodeWithTag(TAG_INCREASE_COUNT).performClick()
-      mainClock.advanceTimeByFrame()
       onNodeWithTag(TAG_COUNT).assertTextEquals("1")
 
       // Now recreate the Activity and assert that the values were retained
@@ -52,11 +52,9 @@ class NavigableCircuitViewModelStateAndroidTest {
 
       // Navigate to Screen B. Increase count to 1
       onNodeWithTag(TAG_GO_NEXT).performClick()
-      mainClock.advanceTimeBy(1_000)
       onNodeWithTag(TAG_LABEL).assertTextEquals("B")
       onNodeWithTag(TAG_COUNT).assertTextEquals("0")
       onNodeWithTag(TAG_INCREASE_COUNT).performClick()
-      mainClock.advanceTimeByFrame()
       onNodeWithTag(TAG_COUNT).assertTextEquals("1")
 
       // Now recreate the Activity and assert that the values were retained
@@ -66,49 +64,53 @@ class NavigableCircuitViewModelStateAndroidTest {
 
       // Navigate to Screen C. Increase count to 1
       onNodeWithTag(TAG_GO_NEXT).performClick()
-      mainClock.advanceTimeBy(1_000)
       onNodeWithTag(TAG_LABEL).assertTextEquals("C")
       onNodeWithTag(TAG_COUNT).assertTextEquals("0")
       onNodeWithTag(TAG_INCREASE_COUNT).performClick()
-      mainClock.advanceTimeByFrame()
       onNodeWithTag(TAG_COUNT).assertTextEquals("1")
 
       // Now recreate the Activity and assert that the values were retained
       scenario.recreate()
       onNodeWithTag(TAG_LABEL).assertTextEquals("C")
       onNodeWithTag(TAG_COUNT).assertTextEquals("1")
 
-      // Pop to Screen B. Increase count from 1 to 2.
-      onNodeWithTag(TAG_POP).performClick()
-
-      // Part-way through pop, both screens should be visible
-      onEachFrameWhileMultipleScreens(hasTestTag(TAG_LABEL)) {
-        onAllNodesWithTag(TAG_LABEL)
-          .assertCountEquals(2)
-          .assertAny(hasTextExactly("C"))
-          .assertAny(hasTextExactly("B"))
-        onAllNodesWithTag(TAG_COUNT).assertCountEquals(2).assertAll(hasTextExactly("1"))
+      mainClock.withAutoAdvance(false) {
+        // Pop to Screen B
+        onNodeWithTag(TAG_POP).performClick()
+
+        // Part-way through pop, both screens should be visible
+        onEachFrameWhileMultipleScreens(hasTestTag(TAG_LABEL)) {
+          onAllNodesWithTag(TAG_LABEL)
+            .assertCountEquals(2)
+            .assertAny(hasTextExactly("C"))
+            .assertAny(hasTextExactly("B"))
+          onAllNodesWithTag(TAG_COUNT).assertCountEquals(2).assertAll(hasTextExactly("1"))
+        }
       }
+
+      // Increase count from 1 to 2.
       onNodeWithTag(TAG_LABEL).assertTextEquals("B")
       onNodeWithTag(TAG_COUNT).assertTextEquals("1")
       onNodeWithTag(TAG_INCREASE_COUNT).performClick()
-      mainClock.advanceTimeByFrame()
       onNodeWithTag(TAG_COUNT).assertTextEquals("2")
 
-      // Navigate to Screen C. Assert that it's state was not retained
-      onNodeWithTag(TAG_GO_NEXT).performClick()
-
-      // Part-way through push, both screens should be visible
-      onEachFrameWhileMultipleScreens(hasTestTag(TAG_LABEL)) {
-        onAllNodesWithTag(TAG_LABEL)
-          .assertCountEquals(2)
-          .assertAny(hasTextExactly("C"))
-          .assertAny(hasTextExactly("B"))
-        onAllNodesWithTag(TAG_COUNT)
-          .assertCountEquals(2)
-          .assertAny(hasTextExactly("0"))
-          .assertAny(hasTextExactly("2"))
+      mainClock.withAutoAdvance(false) {
+        // Navigate to Screen C
+        onNodeWithTag(TAG_GO_NEXT).performClick()
+
+        // Part-way through push, both screens should be visible
+        onEachFrameWhileMultipleScreens(hasTestTag(TAG_LABEL)) {
+          onAllNodesWithTag(TAG_LABEL)
+            .assertCountEquals(2)
+            .assertAny(hasTextExactly("C"))
+            .assertAny(hasTextExactly("B"))
+          onAllNodesWithTag(TAG_COUNT)
+            .assertCountEquals(2)
+            .assertAny(hasTextExactly("0"))
+            .assertAny(hasTextExactly("2"))
+        }
       }
+      // Assert that Screen C's state was retained
       onNodeWithTag(TAG_LABEL).assertTextEquals("C")
       onNodeWithTag(TAG_COUNT).assertTextEquals("0")
 
@@ -117,20 +119,23 @@ class NavigableCircuitViewModelStateAndroidTest {
       onNodeWithTag(TAG_LABEL).assertTextEquals("C")
       onNodeWithTag(TAG_COUNT).assertTextEquals("0")
 
-      // Pop to Screen B. Assert that it's state was retained
-      onNodeWithTag(TAG_POP).performClick()
-
-      // Part-way through pop, both screens should be visible
-      onEachFrameWhileMultipleScreens(hasTestTag(TAG_LABEL)) {
-        onAllNodesWithTag(TAG_LABEL)
-          .assertCountEquals(2)
-          .assertAny(hasTextExactly("C"))
-          .assertAny(hasTextExactly("B"))
-        onAllNodesWithTag(TAG_COUNT)
-          .assertCountEquals(2)
-          .assertAny(hasTextExactly("0"))
-          .assertAny(hasTextExactly("2"))
+      mainClock.withAutoAdvance(false) {
+        // Pop to Screen B
+        onNodeWithTag(TAG_POP).performClick()
+
+        // Part-way through pop, both screens should be visible
+        onEachFrameWhileMultipleScreens(hasTestTag(TAG_LABEL)) {
+          onAllNodesWithTag(TAG_LABEL)
+            .assertCountEquals(2)
+            .assertAny(hasTextExactly("C"))
+            .assertAny(hasTextExactly("B"))
+          onAllNodesWithTag(TAG_COUNT)
+            .assertCountEquals(2)
+            .assertAny(hasTextExactly("0"))
+            .assertAny(hasTextExactly("2"))
+        }
       }
+      // Assert that Screen B's state was retained
       onNodeWithTag(TAG_LABEL).assertTextEquals("B")
       onNodeWithTag(TAG_COUNT).assertTextEquals("2")
 
@@ -139,20 +144,23 @@ class NavigableCircuitViewModelStateAndroidTest {
       onNodeWithTag(TAG_LABEL).assertTextEquals("B")
       onNodeWithTag(TAG_COUNT).assertTextEquals("2")
 
-      // Pop to Screen A. Assert that it's state was retained
-      onNodeWithTag(TAG_POP).performClick()
-
-      // Part-way through pop, both screens should be visible
-      onEachFrameWhileMultipleScreens(hasTestTag(TAG_LABEL)) {
-        onAllNodesWithTag(TAG_LABEL)
-          .assertCountEquals(2)
-          .assertAny(hasTextExactly("B"))
-          .assertAny(hasTextExactly("A"))
-        onAllNodesWithTag(TAG_COUNT)
-          .assertCountEquals(2)
-          .assertAny(hasTextExactly("2"))
-          .assertAny(hasTextExactly("1"))
+      mainClock.withAutoAdvance(false) {
+        // Pop to Screen A
+        onNodeWithTag(TAG_POP).performClick()
+
+        // Part-way through pop, both screens should be visible
+        onEachFrameWhileMultipleScreens(hasTestTag(TAG_LABEL)) {
+          onAllNodesWithTag(TAG_LABEL)
+            .assertCountEquals(2)
+            .assertAny(hasTextExactly("B"))
+            .assertAny(hasTextExactly("A"))
+          onAllNodesWithTag(TAG_COUNT)
+            .assertCountEquals(2)
+            .assertAny(hasTextExactly("2"))
+            .assertAny(hasTextExactly("1"))
+        }
       }
+      // Assert that Screen B's state was retained
       onNodeWithTag(TAG_LABEL).assertTextEquals("A")
       onNodeWithTag(TAG_COUNT).assertTextEquals("1")
 
@@ -163,7 +171,6 @@ class NavigableCircuitViewModelStateAndroidTest {
 
       // Navigate to Screen B. Assert that it's state was not retained
       onNodeWithTag(TAG_GO_NEXT).performClick()
-      mainClock.advanceTimeBy(1_000)
       onNodeWithTag(TAG_LABEL).assertTextEquals("B")
       onNodeWithTag(TAG_COUNT).assertTextEquals("0")
     }
@@ -188,3 +195,13 @@ class NavigableCircuitViewModelStateAndroidTest {
     }
   }
 }
+
+private fun MainTestClock.withAutoAdvance(value: Boolean, block: () -> Unit) {
+  val currentAutoAdvance = this.autoAdvance
+  try {
+    this.autoAdvance = value
+    block()
+  } finally {
+    this.autoAdvance = currentAutoAdvance
+  }
+}
diff --git a/circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/CircuitContent.kt b/circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/CircuitContent.kt
@@ -147,7 +147,11 @@ public fun <UiState : CircuitUiState> CircuitContent(
         onDispose(eventListener::onDisposePresent)
       }
 
-      val state = presenter.present()
+      val state =
+        when (presenter) {
+          is NonPausablePresenter<UiState> -> presenter.present()
+          else -> presenter.presentWithLifecycle()
+        }
 
       // TODO not sure why stateFlow + LaunchedEffect + distinctUntilChanged doesn't work here
       SideEffect { eventListener.onState(state) }

diff --git a/...-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/NavigableCircuitContent.kt b/...-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/NavigableCircuitContent.kt
@@ -58,7 +58,8 @@ public fun <R : Record> NavigableCircuitContent(
     circuit.onUnavailableContent,
 ) {
   val activeContentProviders =
-    backStack.buildCircuitContentProviders(
+    buildCircuitContentProviders(
+      backStack = backStack,
       navigator = navigator,
       circuit = circuit,
       unavailableRoute = unavailableRoute,
@@ -103,33 +104,15 @@ public fun <R : Record> NavigableCircuitContent(
 
   CompositionLocalProvider(LocalRetainedStateRegistry provides outerRegistry) {
     decoration.DecoratedContent(activeContentProviders, backStack.size, modifier) { provider ->
-      // We retain the record's retained state registry for as long as the back stack
-      // contains the record
       val record = provider.record
-      val recordInBackStackRetainChecker =
-        remember(backStack, record) {
-          CanRetainChecker { backStack.containsRecord(record, includeSaved = true) }
-        }
 
-      CompositionLocalProvider(LocalCanRetainChecker provides recordInBackStackRetainChecker) {
-        // Remember the `providedValues` lookup because this composition can live longer than
-        // the record is present in the backstack, if the decoration is animated for example.
-        val values = remember(record) { providedValues[record] }?.provideValues()
-        val providedLocals = remember(values) { values?.toTypedArray() ?: emptyArray() }
+      // Remember the `providedValues` lookup because this composition can live longer than
+      // the record is present in the backstack, if the decoration is animated for example.
+      val values = remember(record) { providedValues[record] }?.provideValues()
+      val providedLocals = remember(values) { values?.toTypedArray() ?: emptyArray() }
 
-        // Now provide a new registry to the content for it to store any retained state in,
-        // along with a retain checker which is always true (as upstream registries will
-        // maintain the lifetime), and the other provided values
-        val recordRetainedStateRegistry =
-          rememberRetained(key = record.registryKey) { RetainedStateRegistry() }
-        CompositionLocalProvider(
-          LocalRetainedStateRegistry provides recordRetainedStateRegistry,
-          LocalCanRetainChecker provides CanRetainChecker.Always,
-          LocalBackStack provides backStack,
-          *providedLocals,
-        ) {
-          provider.content(record)
-        }
+      CompositionLocalProvider(LocalBackStack provides backStack, *providedLocals) {
+        provider.content(record)
       }
     }
   }
@@ -163,37 +146,60 @@ public class RecordContentProvider<R : Record>(
 }
 
 @Composable
-private fun <R : Record> BackStack<R>.buildCircuitContentProviders(
+private fun <R : Record> buildCircuitContentProviders(
+  backStack: BackStack<R>,
   navigator: Navigator,
   circuit: Circuit,
   unavailableRoute: @Composable (screen: Screen, modifier: Modifier) -> Unit,
 ): ImmutableList<RecordContentProvider<R>> {
   val previousContentProviders = remember { mutableMapOf<String, RecordContentProvider<R>>() }
 
+  val lastBackStack by rememberUpdatedState(backStack)
   val lastNavigator by rememberUpdatedState(navigator)
   val lastCircuit by rememberUpdatedState(circuit)
   val lastUnavailableRoute by rememberUpdatedState(unavailableRoute)
 
-  return iterator()
+  fun createRecordContent() =
+    movableContentOf<R> { record ->
+      val recordInBackStackRetainChecker =
+        remember(lastBackStack, record) {
+          CanRetainChecker { lastBackStack.containsRecord(record, includeSaved = true) }
+        }
+
+      val lifecycle =
+        remember { MutableRecordLifecycle() }.apply { isActive = lastBackStack.topRecord == record }
+
+      CompositionLocalProvider(LocalCanRetainChecker provides recordInBackStackRetainChecker) {
+        // Now provide a new registry to the content for it to store any retained state in,
+        // along with a retain checker which is always true (as upstream registries will
+        // maintain the lifetime), and the other provided values
+        val recordRetainedStateRegistry =
+          rememberRetained(key = record.registryKey) { RetainedStateRegistry() }
+
+        CompositionLocalProvider(
+          LocalRetainedStateRegistry provides recordRetainedStateRegistry,
+          LocalCanRetainChecker provides CanRetainChecker.Always,
+          LocalRecordLifecycle provides lifecycle,
+        ) {
+          CircuitContent(
+            screen = record.screen,
+            navigator = lastNavigator,
+            circuit = lastCircuit,
+            unavailableContent = lastUnavailableRoute,
+            key = record.key,
+          )
+        }
+      }
+    }
+
+  return lastBackStack
+    .iterator()
     .asSequence()
     .map { record ->
       // Query the previous content providers map, so that we use the same
       // RecordContentProvider instances across calls.
       previousContentProviders.getOrPut(record.key) {
-        RecordContentProvider(
-          record = record,
-          content =
-            movableContentOf { record ->
-              CircuitContent(
-                screen = record.screen,
-                modifier = Modifier,
-                navigator = lastNavigator,
-                circuit = lastCircuit,
-                unavailableContent = lastUnavailableRoute,
-                key = record.key,
-              )
-            },
-        )
+        RecordContentProvider(record = record, content = createRecordContent())
       }
     }
     .toImmutableList()