From 31fd5a5935e2c3bc9eb6b6108d13bd83356ceaf4 Mon Sep 17 00:00:00 2001 From: Josh Stagg Date: Sat, 6 Apr 2024 14:30:13 -0700 Subject: [PATCH 1/9] Navigator - Guard against a goTo to the current screen --- CHANGELOG.md | 1 + .../slack/circuit/foundation/AnsweringNavigator.kt | 3 ++- .../com/slack/circuit/foundation/CircuitContent.kt | 3 ++- .../com/slack/circuit/foundation/NavigatorImpl.kt | 7 +++++-- .../kotlin/com/slack/circuit/runtime/Navigator.kt | 11 ++++++++--- .../com/slack/circuit/test/FakeNavigatorTest.kt | 14 ++++++++++++++ .../kotlin/com/slack/circuit/test/FakeNavigator.kt | 9 ++++++--- .../android/AndroidScreenAwareNavigator.kt | 10 ++++++---- .../com/slack/circuitx/effects/ImpressionEffect.kt | 4 ++-- .../slack/circuitx/overlays/FullScreenOverlay.kt | 2 +- .../kotlin/com/slack/circuit/star/MainActivity.kt | 3 ++- 11 files changed, 49 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8fd68b0a8..7bc0af891 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ Changelog -------------- - **New**: Add WASM targets. +- **Change**: Navigator.goTo now returns a Bool indicating navigation success - Mark `Presenter.Factory` as `@Stable`. - Mark `Ui.Factory` as `@Stable`. - Mark `CircuitContext` as `@Stable`. diff --git a/circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/AnsweringNavigator.kt b/circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/AnsweringNavigator.kt index 019f88af7..95edff732 100644 --- a/circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/AnsweringNavigator.kt +++ b/circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/AnsweringNavigator.kt @@ -124,9 +124,10 @@ public fun rememberAnsweringNavigator( } val answeringNavigator = remember { object : GoToNavigator { - override fun goTo(screen: Screen) { + override fun goTo(screen: Screen): Boolean { currentBackStack.push(screen, key) launched = true + return true } } } 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 index b3feef71b..0b7b6c251 100644 --- a/circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/CircuitContent.kt +++ b/circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/CircuitContent.kt @@ -45,8 +45,9 @@ public fun CircuitContent( val navigator = remember(onNavEvent) { object : Navigator { - override fun goTo(screen: Screen) { + override fun goTo(screen: Screen): Boolean { onNavEvent(NavEvent.GoTo(screen)) + return true } override fun resetRoot( diff --git a/circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/NavigatorImpl.kt b/circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/NavigatorImpl.kt index 9494527e9..e417345f1 100644 --- a/circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/NavigatorImpl.kt +++ b/circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/NavigatorImpl.kt @@ -54,8 +54,11 @@ internal class NavigatorImpl( check(!backStack.isEmpty) { "Backstack size must not be empty." } } - override fun goTo(screen: Screen) { - backStack.push(screen) + override fun goTo(screen: Screen): Boolean { + return if (peek() != screen) { + backStack.push(screen) + true + } else false } override fun pop(result: PopResult?): Screen? { diff --git a/circuit-runtime/src/commonMain/kotlin/com/slack/circuit/runtime/Navigator.kt b/circuit-runtime/src/commonMain/kotlin/com/slack/circuit/runtime/Navigator.kt index 0b8525985..e9339a9fd 100644 --- a/circuit-runtime/src/commonMain/kotlin/com/slack/circuit/runtime/Navigator.kt +++ b/circuit-runtime/src/commonMain/kotlin/com/slack/circuit/runtime/Navigator.kt @@ -12,13 +12,18 @@ import kotlinx.collections.immutable.persistentListOf /** A Navigator that only supports [goTo]. */ @Stable public interface GoToNavigator { - public fun goTo(screen: Screen) + /** + * Navigate to the [screen]. + * + * @return If the navigator successfully went to the [screen] + */ + public fun goTo(screen: Screen): Boolean } /** A basic navigation interface for navigating between [screens][Screen]. */ @Stable public interface Navigator : GoToNavigator { - public override fun goTo(screen: Screen) + public override fun goTo(screen: Screen): Boolean public fun pop(result: PopResult? = null): Screen? @@ -82,7 +87,7 @@ public interface Navigator : GoToNavigator { ): ImmutableList public object NoOp : Navigator { - override fun goTo(screen: Screen) {} + override fun goTo(screen: Screen): Boolean = true override fun pop(result: PopResult?): Screen? = null diff --git a/circuit-test/src/androidUnitTest/kotlin/com/slack/circuit/test/FakeNavigatorTest.kt b/circuit-test/src/androidUnitTest/kotlin/com/slack/circuit/test/FakeNavigatorTest.kt index 8f5375e26..c71dfd41f 100644 --- a/circuit-test/src/androidUnitTest/kotlin/com/slack/circuit/test/FakeNavigatorTest.kt +++ b/circuit-test/src/androidUnitTest/kotlin/com/slack/circuit/test/FakeNavigatorTest.kt @@ -34,6 +34,20 @@ class FakeNavigatorTest { assertThat(oldScreens).containsExactly(TestScreen1) assertThat(navigator.awaitResetRoot()).isEqualTo(ResetRootEvent(TestScreen1, oldScreens)) } + + @Test + fun goTo() = runTest { + val navigator = FakeNavigator(TestScreen1) + // Go to a second screen + assertThat(navigator.goTo(TestScreen2)).isTrue() + assertThat(navigator.awaitNextScreen()).isEqualTo(TestScreen2) + // Going to the same screen won't navigate + assertThat(navigator.goTo(TestScreen2)).isFalse() + navigator.expectNoEvents() + // Go to a third screen + assertThat(navigator.goTo(TestScreen3)).isTrue() + assertThat(navigator.awaitNextScreen()).isEqualTo(TestScreen3) + } } @Parcelize private data object TestScreen1 : Screen diff --git a/circuit-test/src/commonMain/kotlin/com/slack/circuit/test/FakeNavigator.kt b/circuit-test/src/commonMain/kotlin/com/slack/circuit/test/FakeNavigator.kt index 0fcf5e8b2..1ac354f0a 100644 --- a/circuit-test/src/commonMain/kotlin/com/slack/circuit/test/FakeNavigator.kt +++ b/circuit-test/src/commonMain/kotlin/com/slack/circuit/test/FakeNavigator.kt @@ -52,9 +52,12 @@ public class FakeNavigator internal constructor(private val delegate: Navigator) private val resetRootEvents = Turbine() private val popEvents = Turbine() - override fun goTo(screen: Screen) { - delegate.goTo(screen) - goToEvents.add(screen) + override fun goTo(screen: Screen): Boolean { + val res = delegate.goTo(screen) + if (res) { + goToEvents.add(screen) + } + return res } override fun pop(result: PopResult?): Screen? { diff --git a/circuitx/android/src/main/kotlin/com/slack/circuitx/android/AndroidScreenAwareNavigator.kt b/circuitx/android/src/main/kotlin/com/slack/circuitx/android/AndroidScreenAwareNavigator.kt index 413cb74e4..b7236443d 100644 --- a/circuitx/android/src/main/kotlin/com/slack/circuitx/android/AndroidScreenAwareNavigator.kt +++ b/circuitx/android/src/main/kotlin/com/slack/circuitx/android/AndroidScreenAwareNavigator.kt @@ -35,8 +35,9 @@ public interface AndroidScreen : Screen */ @Parcelize public data class IntentScreen(val intent: Intent, val options: Bundle? = null) : AndroidScreen { - public fun startWith(context: Context) { + public fun startWith(context: Context): Boolean { context.startActivity(intent, options) + return true } } @@ -45,8 +46,8 @@ private class AndroidScreenAwareNavigator( private val delegate: Navigator, private val starter: AndroidScreenStarter, ) : Navigator by delegate { - override fun goTo(screen: Screen) { - when (screen) { + override fun goTo(screen: Screen): Boolean { + return when (screen) { is AndroidScreen -> starter.start(screen) else -> delegate.goTo(screen) } @@ -61,7 +62,7 @@ private class AndroidScreenAwareNavigator( */ public fun interface AndroidScreenStarter { /** Starts the given [screen]. */ - public fun start(screen: AndroidScreen) + public fun start(screen: AndroidScreen): Boolean } /** @@ -78,6 +79,7 @@ public fun rememberAndroidScreenAwareNavigator(delegate: Navigator, context: Con AndroidScreenStarter { screen -> when (screen) { is IntentScreen -> screen.startWith(context) + else -> false } } } diff --git a/circuitx/effects/src/commonMain/kotlin/com/slack/circuitx/effects/ImpressionEffect.kt b/circuitx/effects/src/commonMain/kotlin/com/slack/circuitx/effects/ImpressionEffect.kt index dd82adf11..660cb7e51 100644 --- a/circuitx/effects/src/commonMain/kotlin/com/slack/circuitx/effects/ImpressionEffect.kt +++ b/circuitx/effects/src/commonMain/kotlin/com/slack/circuitx/effects/ImpressionEffect.kt @@ -72,9 +72,9 @@ public fun rememberImpressionNavigator( } private class OnNavEventNavigator(val delegate: Navigator, val onNavEvent: () -> Unit) : Navigator { - override fun goTo(screen: Screen) { + override fun goTo(screen: Screen): Boolean { onNavEvent() - delegate.goTo(screen) + return delegate.goTo(screen) } override fun pop(result: PopResult?): Screen? { diff --git a/circuitx/overlays/src/commonMain/kotlin/com/slack/circuitx/overlays/FullScreenOverlay.kt b/circuitx/overlays/src/commonMain/kotlin/com/slack/circuitx/overlays/FullScreenOverlay.kt index 59e56a9a3..08da881f1 100644 --- a/circuitx/overlays/src/commonMain/kotlin/com/slack/circuitx/overlays/FullScreenOverlay.kt +++ b/circuitx/overlays/src/commonMain/kotlin/com/slack/circuitx/overlays/FullScreenOverlay.kt @@ -67,7 +67,7 @@ internal class DispatchingOverlayNavigator( private val overlayNavigator: OverlayNavigator, private val onPop: () -> Unit, ) : Navigator { - override fun goTo(screen: Screen) { + override fun goTo(screen: Screen): Boolean { error("goTo() is not supported in full screen overlays!") } diff --git a/samples/star/src/androidMain/kotlin/com/slack/circuit/star/MainActivity.kt b/samples/star/src/androidMain/kotlin/com/slack/circuit/star/MainActivity.kt index 9fdfcd9c0..3bb83d3e6 100644 --- a/samples/star/src/androidMain/kotlin/com/slack/circuit/star/MainActivity.kt +++ b/samples/star/src/androidMain/kotlin/com/slack/circuit/star/MainActivity.kt @@ -95,7 +95,7 @@ class MainActivity @Inject constructor(private val circuit: Circuit) : AppCompat else -> error("Unknown AndroidScreen: $screen") } - private fun goTo(screen: OpenUrlScreen) { + private fun goTo(screen: OpenUrlScreen): Boolean { val scheme = CustomTabColorSchemeParams.Builder().setToolbarColor(0x000000).build() CustomTabsIntent.Builder() .setColorSchemeParams(COLOR_SCHEME_LIGHT, scheme) @@ -103,5 +103,6 @@ class MainActivity @Inject constructor(private val circuit: Circuit) : AppCompat .setShowTitle(true) .build() .launchUrl(this, Uri.parse(screen.url)) + return true } } From 699bdb89de12aa7f911d10f605861deb50122998 Mon Sep 17 00:00:00 2001 From: Josh Stagg Date: Sun, 7 Apr 2024 15:05:40 -0700 Subject: [PATCH 2/9] Changelog update --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7bc0af891..321a8d9a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,8 @@ Changelog -------------- - **New**: Add WASM targets. -- **Change**: Navigator.goTo now returns a Bool indicating navigation success +- **Behaviour Change**: `NavigatorImpl.goTo` no longer navigates if the `Screen` is equal to `Navigator.peek()` +- **Change**: `Navigator.goTo` now returns a Bool indicating navigation success - Mark `Presenter.Factory` as `@Stable`. - Mark `Ui.Factory` as `@Stable`. - Mark `CircuitContext` as `@Stable`. From 5c6a7600a7faf22d74f38bc5d31a68f6acf70333 Mon Sep 17 00:00:00 2001 From: Josh Stagg Date: Mon, 8 Apr 2024 10:36:34 -0700 Subject: [PATCH 3/9] Spotless --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 321a8d9a9..431f97329 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ Changelog -------------- - **New**: Add WASM targets. -- **Behaviour Change**: `NavigatorImpl.goTo` no longer navigates if the `Screen` is equal to `Navigator.peek()` +- **Behaviour Change**: `NavigatorImpl.goTo` no longer navigates if the `Screen` is equal to `Navigator.peek()` - **Change**: `Navigator.goTo` now returns a Bool indicating navigation success - Mark `Presenter.Factory` as `@Stable`. - Mark `Ui.Factory` as `@Stable`. From e087ccc3b0124473ee3c98643e41d5ea6b1c2928 Mon Sep 17 00:00:00 2001 From: Josh Stagg Date: Mon, 8 Apr 2024 10:53:18 -0700 Subject: [PATCH 4/9] Missed one --- .../star/src/jvmMain/kotlin/com/slack/circuit/star/main.kt | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/samples/star/src/jvmMain/kotlin/com/slack/circuit/star/main.kt b/samples/star/src/jvmMain/kotlin/com/slack/circuit/star/main.kt index 02eb01787..6c85de30b 100644 --- a/samples/star/src/jvmMain/kotlin/com/slack/circuit/star/main.kt +++ b/samples/star/src/jvmMain/kotlin/com/slack/circuit/star/main.kt @@ -45,8 +45,8 @@ fun main() { val navigator = remember(circuitNavigator) { object : Navigator by circuitNavigator { - override fun goTo(screen: Screen) { - when (screen) { + override fun goTo(screen: Screen) : Boolean { + return when (screen) { is OpenUrlScreen -> openUrl(screen.url) else -> circuitNavigator.goTo(screen) } @@ -103,7 +103,8 @@ fun main() { } } -private fun openUrl(url: String) { +private fun openUrl(url: String) : Boolean { val desktop = Desktop.getDesktop() desktop.browse(URI.create(url)) + return true } From ceb82177be52c592ee3a5b6071f2842bdbeb8e92 Mon Sep 17 00:00:00 2001 From: Josh Stagg Date: Mon, 8 Apr 2024 11:08:55 -0700 Subject: [PATCH 5/9] More spotless --- .../star/src/jvmMain/kotlin/com/slack/circuit/star/main.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/samples/star/src/jvmMain/kotlin/com/slack/circuit/star/main.kt b/samples/star/src/jvmMain/kotlin/com/slack/circuit/star/main.kt index 6c85de30b..ab385e271 100644 --- a/samples/star/src/jvmMain/kotlin/com/slack/circuit/star/main.kt +++ b/samples/star/src/jvmMain/kotlin/com/slack/circuit/star/main.kt @@ -45,7 +45,7 @@ fun main() { val navigator = remember(circuitNavigator) { object : Navigator by circuitNavigator { - override fun goTo(screen: Screen) : Boolean { + override fun goTo(screen: Screen): Boolean { return when (screen) { is OpenUrlScreen -> openUrl(screen.url) else -> circuitNavigator.goTo(screen) @@ -103,7 +103,7 @@ fun main() { } } -private fun openUrl(url: String) : Boolean { +private fun openUrl(url: String): Boolean { val desktop = Desktop.getDesktop() desktop.browse(URI.create(url)) return true From 5f2a8d4cf870f8b317d7ab33decfe667968accf9 Mon Sep 17 00:00:00 2001 From: Josh Stagg Date: Fri, 12 Apr 2024 11:03:44 -0700 Subject: [PATCH 6/9] Move the same screen guard into the BackStack --- .../com/slack/circuit/backstack/BackStack.kt | 6 +++-- .../circuit/backstack/SaveableBackStack.kt | 20 +++++++++----- .../backstack/SaveableBackStackTest.kt | 10 +++++++ .../slack/circuit/foundation/NavigatorImpl.kt | 5 +--- .../slack/circuit/test/FakeNavigatorTest.kt | 3 ++- .../com/slack/circuit/test/FakeNavigator.kt | 27 +++++++++++++------ 6 files changed, 49 insertions(+), 22 deletions(-) diff --git a/backstack/src/commonMain/kotlin/com/slack/circuit/backstack/BackStack.kt b/backstack/src/commonMain/kotlin/com/slack/circuit/backstack/BackStack.kt index a5438d640..1a28c6b08 100644 --- a/backstack/src/commonMain/kotlin/com/slack/circuit/backstack/BackStack.kt +++ b/backstack/src/commonMain/kotlin/com/slack/circuit/backstack/BackStack.kt @@ -41,8 +41,9 @@ public interface BackStack : Iterable { * @param record The record to push onto the stack. * @param resultKey An optional key that would be used to tag a result produced by this record. * The previous record on the stack will receive this key. + * @return If the [record] was successfully pushed onto the back stack */ - public fun push(record: R, resultKey: String? = null) + public fun push(record: R, resultKey: String? = null): Boolean /** * Push a new [Screen] onto the back stack. This will be enveloped in a [Record] and the new @@ -51,8 +52,9 @@ public interface BackStack : Iterable { * @param screen The screen to push onto the stack. * @param resultKey An optional key that would be used to tag a result produced by this record. * The previous record on the stack will receive this key. + * @return If the [screen] was successfully pushed onto the back stack */ - public fun push(screen: Screen, resultKey: String? = null) + public fun push(screen: Screen, resultKey: String? = null): Boolean /** * Attempt to pop the top item off of the back stack, returning the popped [Record] if popping was diff --git a/backstack/src/commonMain/kotlin/com/slack/circuit/backstack/SaveableBackStack.kt b/backstack/src/commonMain/kotlin/com/slack/circuit/backstack/SaveableBackStack.kt index a05967b49..266f8cead 100644 --- a/backstack/src/commonMain/kotlin/com/slack/circuit/backstack/SaveableBackStack.kt +++ b/backstack/src/commonMain/kotlin/com/slack/circuit/backstack/SaveableBackStack.kt @@ -90,18 +90,24 @@ internal constructor( public override val topRecord: Record? get() = entryList.firstOrNull() - public override fun push(screen: Screen, resultKey: String?) { + public override fun push(screen: Screen, resultKey: String?): Boolean { return push(screen, emptyMap(), resultKey) } - public fun push(screen: Screen, args: Map, resultKey: String?) { - push(Record(screen, args), resultKey) + public fun push(screen: Screen, args: Map, resultKey: String?): Boolean { + return push(Record(screen, args), resultKey) } - public override fun push(record: Record, resultKey: String?) { - entryList.add(0, record) - // Clear the cached pending result from the previous top record - entryList.getOrNull(1)?.apply { resultKey?.let(::prepareForResult) } + public override fun push(record: Record, resultKey: String?): Boolean { + val topRecord = entryList.firstOrNull() + // Guard pushing the exact same record value to the top, records.key is always unique so verify + // the parameters individually. + return if (topRecord?.screen != record.screen || topRecord.args != record.args) { + entryList.add(0, record) + // Clear the cached pending result from the previous top record + entryList.getOrNull(1)?.apply { resultKey?.let(::prepareForResult) } + true + } else false } override fun pop(result: PopResult?): Record? { diff --git a/backstack/src/commonTest/kotlin/com/slack/circuit/backstack/SaveableBackStackTest.kt b/backstack/src/commonTest/kotlin/com/slack/circuit/backstack/SaveableBackStackTest.kt index c6f169ecc..4298d3e78 100644 --- a/backstack/src/commonTest/kotlin/com/slack/circuit/backstack/SaveableBackStackTest.kt +++ b/backstack/src/commonTest/kotlin/com/slack/circuit/backstack/SaveableBackStackTest.kt @@ -11,6 +11,8 @@ import assertk.assertions.isEqualTo import assertk.assertions.isNotNull import com.slack.circuit.internal.test.TestScreen import kotlin.test.Test +import kotlin.test.assertFalse +import kotlin.test.assertTrue class SaveableBackStackTest { @@ -100,6 +102,14 @@ class SaveableBackStackTest { assertThat(backStack.entryList.toList()).isEqualTo(restored.entryList.toList()) assertThat(backStack.stateStore.toMap()).isEqualTo(restored.stateStore.toMap()) } + + @Test + fun test_guard_pushing_same_top_record() { + val backStack = SaveableBackStack(TestScreen.RootAlpha) + assertTrue(backStack.push(TestScreen.ScreenA)) + assertTrue(backStack.push(TestScreen.ScreenB)) + assertFalse(backStack.push(TestScreen.ScreenB)) + } } private fun save(backStack: SaveableBackStack) = diff --git a/circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/NavigatorImpl.kt b/circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/NavigatorImpl.kt index e417345f1..2ca81a92f 100644 --- a/circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/NavigatorImpl.kt +++ b/circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/NavigatorImpl.kt @@ -55,10 +55,7 @@ internal class NavigatorImpl( } override fun goTo(screen: Screen): Boolean { - return if (peek() != screen) { - backStack.push(screen) - true - } else false + return backStack.push(screen) } override fun pop(result: PopResult?): Screen? { diff --git a/circuit-test/src/androidUnitTest/kotlin/com/slack/circuit/test/FakeNavigatorTest.kt b/circuit-test/src/androidUnitTest/kotlin/com/slack/circuit/test/FakeNavigatorTest.kt index c71dfd41f..36388a8fe 100644 --- a/circuit-test/src/androidUnitTest/kotlin/com/slack/circuit/test/FakeNavigatorTest.kt +++ b/circuit-test/src/androidUnitTest/kotlin/com/slack/circuit/test/FakeNavigatorTest.kt @@ -4,6 +4,7 @@ package com.slack.circuit.test import com.google.common.truth.Truth.assertThat import com.slack.circuit.runtime.screen.Screen +import com.slack.circuit.test.FakeNavigator.GoToEvent import com.slack.circuit.test.FakeNavigator.ResetRootEvent import kotlinx.coroutines.test.runTest import kotlinx.parcelize.Parcelize @@ -43,7 +44,7 @@ class FakeNavigatorTest { assertThat(navigator.awaitNextScreen()).isEqualTo(TestScreen2) // Going to the same screen won't navigate assertThat(navigator.goTo(TestScreen2)).isFalse() - navigator.expectNoEvents() + assertThat(navigator.awaitNextGoTo()).isEqualTo(GoToEvent(TestScreen2, success = false)) // Go to a third screen assertThat(navigator.goTo(TestScreen3)).isTrue() assertThat(navigator.awaitNextScreen()).isEqualTo(TestScreen3) diff --git a/circuit-test/src/commonMain/kotlin/com/slack/circuit/test/FakeNavigator.kt b/circuit-test/src/commonMain/kotlin/com/slack/circuit/test/FakeNavigator.kt index 1ac354f0a..f654a1024 100644 --- a/circuit-test/src/commonMain/kotlin/com/slack/circuit/test/FakeNavigator.kt +++ b/circuit-test/src/commonMain/kotlin/com/slack/circuit/test/FakeNavigator.kt @@ -48,16 +48,14 @@ public class FakeNavigator internal constructor(private val delegate: Navigator) SaveableBackStack(root) ) - private val goToEvents = Turbine() + private val goToEvents = Turbine() private val resetRootEvents = Turbine() private val popEvents = Turbine() override fun goTo(screen: Screen): Boolean { - val res = delegate.goTo(screen) - if (res) { - goToEvents.add(screen) - } - return res + val success = delegate.goTo(screen) + goToEvents.add(GoToEvent(screen, success)) + return success } override fun pop(result: PopResult?): Screen? { @@ -81,10 +79,13 @@ public class FakeNavigator internal constructor(private val delegate: Navigator) * * For non-coroutines users only. */ - public fun takeNextScreen(): Screen = goToEvents.takeItem() + public fun takeNextScreen(): Screen = goToEvents.takeItem().assertSuccessfulScreen() /** Awaits the next [Screen] that was navigated to or throws if no screens were navigated to. */ - public suspend fun awaitNextScreen(): Screen = goToEvents.awaitItem() + public suspend fun awaitNextScreen(): Screen = goToEvents.awaitItem().assertSuccessfulScreen() + + /** Awaits the next navigation [goTo] or throws if no goTo are performed. */ + public suspend fun awaitNextGoTo(): GoToEvent = goToEvents.awaitItem() /** Awaits the next navigation [resetRoot] or throws if no resets were performed. */ public suspend fun awaitResetRoot(): ResetRootEvent = resetRootEvents.awaitItem() @@ -102,6 +103,9 @@ public class FakeNavigator internal constructor(private val delegate: Navigator) goToEvents.expectNoEvents() } + /** Represents a recorded [Navigator.goTo] event. */ + public data class GoToEvent(val screen: Screen, val success: Boolean) + /** Represents a recorded [Navigator.pop] event. */ public data class PopEvent(val poppedScreen: Screen?, val result: PopResult? = null) @@ -112,4 +116,11 @@ public class FakeNavigator internal constructor(private val delegate: Navigator) val saveState: Boolean = false, val restoreState: Boolean = false, ) + + private fun GoToEvent.assertSuccessfulScreen(): Screen { + if (!success) { + throw AssertionError("Screen navigation was not successful") + } + return screen + } } From 6c6481f1d04a4b091addbbfa674f235e2bb5d745 Mon Sep 17 00:00:00 2001 From: Josh Stagg Date: Fri, 12 Apr 2024 18:34:51 -0700 Subject: [PATCH 7/9] Odd double emit --- .../slack/circuit/backstack/RememberSaveableBackstackTest.kt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/backstack/src/commonJvmTest/kotlin/com/slack/circuit/backstack/RememberSaveableBackstackTest.kt b/backstack/src/commonJvmTest/kotlin/com/slack/circuit/backstack/RememberSaveableBackstackTest.kt index a32022573..75e0aaf4b 100644 --- a/backstack/src/commonJvmTest/kotlin/com/slack/circuit/backstack/RememberSaveableBackstackTest.kt +++ b/backstack/src/commonJvmTest/kotlin/com/slack/circuit/backstack/RememberSaveableBackstackTest.kt @@ -50,6 +50,8 @@ class RememberSaveableBackstackTest { val firstStack = awaitItem() rootScreen = TestScreen.ScreenB val secondStack = awaitItem() + val thirdStack = awaitItem() // Why is this emitting twice?? + assertSame(secondStack, thirdStack) assertNotSame(firstStack, secondStack) assertEquals(firstStack.entryList.first().screen, TestScreen.ScreenA) @@ -65,6 +67,8 @@ class RememberSaveableBackstackTest { val firstStack = awaitItem() rootScreens = listOf(TestScreen.ScreenB) val secondStack = awaitItem() + val thirdStack = awaitItem() // Why is this emitting twice?? + assertSame(secondStack, thirdStack) assertNotSame(firstStack, secondStack) assertEquals(firstStack.entryList.first().screen, TestScreen.ScreenA) From ad032897a0282e67c3931009108c00938b610bc2 Mon Sep 17 00:00:00 2001 From: Josh Stagg Date: Fri, 12 Apr 2024 18:55:48 -0700 Subject: [PATCH 8/9] Remove recursive state read --- .../slack/circuit/backstack/RememberSaveableBackstackTest.kt | 4 ---- .../kotlin/com/slack/circuit/backstack/SaveableBackStack.kt | 3 ++- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/backstack/src/commonJvmTest/kotlin/com/slack/circuit/backstack/RememberSaveableBackstackTest.kt b/backstack/src/commonJvmTest/kotlin/com/slack/circuit/backstack/RememberSaveableBackstackTest.kt index 75e0aaf4b..a32022573 100644 --- a/backstack/src/commonJvmTest/kotlin/com/slack/circuit/backstack/RememberSaveableBackstackTest.kt +++ b/backstack/src/commonJvmTest/kotlin/com/slack/circuit/backstack/RememberSaveableBackstackTest.kt @@ -50,8 +50,6 @@ class RememberSaveableBackstackTest { val firstStack = awaitItem() rootScreen = TestScreen.ScreenB val secondStack = awaitItem() - val thirdStack = awaitItem() // Why is this emitting twice?? - assertSame(secondStack, thirdStack) assertNotSame(firstStack, secondStack) assertEquals(firstStack.entryList.first().screen, TestScreen.ScreenA) @@ -67,8 +65,6 @@ class RememberSaveableBackstackTest { val firstStack = awaitItem() rootScreens = listOf(TestScreen.ScreenB) val secondStack = awaitItem() - val thirdStack = awaitItem() // Why is this emitting twice?? - assertSame(secondStack, thirdStack) assertNotSame(firstStack, secondStack) assertEquals(firstStack.entryList.first().screen, TestScreen.ScreenA) diff --git a/backstack/src/commonMain/kotlin/com/slack/circuit/backstack/SaveableBackStack.kt b/backstack/src/commonMain/kotlin/com/slack/circuit/backstack/SaveableBackStack.kt index 266f8cead..5273184ec 100644 --- a/backstack/src/commonMain/kotlin/com/slack/circuit/backstack/SaveableBackStack.kt +++ b/backstack/src/commonMain/kotlin/com/slack/circuit/backstack/SaveableBackStack.kt @@ -21,6 +21,7 @@ import androidx.compose.runtime.saveable.Saver import androidx.compose.runtime.saveable.listSaver import androidx.compose.runtime.saveable.mapSaver import androidx.compose.runtime.saveable.rememberSaveable +import androidx.compose.runtime.snapshots.Snapshot import com.benasher44.uuid.uuid4 import com.slack.circuit.runtime.screen.PopResult import com.slack.circuit.runtime.screen.Screen @@ -99,7 +100,7 @@ internal constructor( } public override fun push(record: Record, resultKey: String?): Boolean { - val topRecord = entryList.firstOrNull() + val topRecord = Snapshot.withoutReadObservation { entryList.firstOrNull() } // Guard pushing the exact same record value to the top, records.key is always unique so verify // the parameters individually. return if (topRecord?.screen != record.screen || topRecord.args != record.args) { From fefe02e72c114d01b8be88f5963667f5f5482a92 Mon Sep 17 00:00:00 2001 From: Josh Stagg Date: Fri, 12 Apr 2024 20:45:24 -0700 Subject: [PATCH 9/9] Fix a test --- .../kotlin/com/slack/circuit/foundation/NavigatorTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/circuit-foundation/src/commonJvmTest/kotlin/com/slack/circuit/foundation/NavigatorTest.kt b/circuit-foundation/src/commonJvmTest/kotlin/com/slack/circuit/foundation/NavigatorTest.kt index 91ae164fb..71267f61c 100644 --- a/circuit-foundation/src/commonJvmTest/kotlin/com/slack/circuit/foundation/NavigatorTest.kt +++ b/circuit-foundation/src/commonJvmTest/kotlin/com/slack/circuit/foundation/NavigatorTest.kt @@ -29,7 +29,7 @@ class NavigatorTest { @Test fun popAtRoot() { val backStack = SaveableBackStack(TestScreen) - backStack.push(TestScreen) + backStack.push(TestScreen2) var onRootPop = 0 val navigator = NavigatorImpl(backStack) { onRootPop++ }