From 05c84c476101f1caf0de8d92f5fb0c8ee332f560 Mon Sep 17 00:00:00 2001 From: Nicolas Presseault Date: Fri, 17 May 2024 11:59:05 -0400 Subject: [PATCH 1/3] Fix an issue on android where popping in a nested navigation (through Back Handler) would not pop the nested navigation --- gradle/libs.versions.toml | 6 +- navigation/common/build.gradle.kts | 8 ++ .../navigation/compose/PilotBackHandler.kt | 2 +- .../DefaultPilotNavigationManager.kt | 16 ++-- .../DefaultPilotNavigationManagerTest.kt | 84 +++++++++++++++++++ 5 files changed, 105 insertions(+), 11 deletions(-) create mode 100644 navigation/common/src/commonTest/kotlin/com/mirego/pilot/navigation/DefaultPilotNavigationManagerTest.kt diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index e03a43ee..114519bb 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -13,16 +13,20 @@ kotlinx-coroutines = "1.8.0" mirego-publish = "1.5" coil = "2.5.0" dokka = "1.9.20" +assertk = "0.28.1" [libraries] kotlin-stdlib-jdk8 = { module = "org.jetbrains.kotlin:kotlin-stdlib-jdk8", version.ref = "kotlin" } +kotlin-test = { module = "org.jetbrains.kotlin:kotlin-test", version.ref = "kotlin" } +kotlin-test-junit = { module = "org.jetbrains.kotlin:kotlin-test-junit", version.ref = "kotlin" } kotlinx-coroutines-core = { module = "org.jetbrains.kotlinx:kotlinx-coroutines-core", version.ref = "kotlinx-coroutines" } +kotlinx-coroutines-test = { module = "org.jetbrains.kotlinx:kotlinx-coroutines-test", version.ref = "kotlinx-coroutines" } +assertk = { module = "com.willowtreeapps.assertk:assertk", version.ref = "assertk" } androidx-compose-foundation = { module = "androidx.compose.foundation:foundation", version.ref = "androidx-compose-runtime" } androidx-compose-ui = { module = "androidx.compose.ui:ui", version.ref = "androidx-compose-runtime" } androidx-compose-material3 = { module = "androidx.compose.material3:material3", version.ref = "androidx-compose-material3" } - androidx-lifecycle-common = { module = "androidx.lifecycle:lifecycle-common", version.ref = "androidx-lifecycle" } androidx-lifecycle-viewmodel = { module = "androidx.lifecycle:lifecycle-viewmodel", version.ref = "androidx-lifecycle" } androidx-lifecycle-viewmodel-ktx = { module = "androidx.lifecycle:lifecycle-viewmodel-ktx", version.ref = "androidx-lifecycle" } diff --git a/navigation/common/build.gradle.kts b/navigation/common/build.gradle.kts index 89c183de..dc90887e 100644 --- a/navigation/common/build.gradle.kts +++ b/navigation/common/build.gradle.kts @@ -32,6 +32,14 @@ kotlin { implementation(libs.kotlinx.coroutines.core) } } + val commonTest by getting { + dependencies { + implementation(libs.kotlin.test) + implementation(libs.kotlin.test.junit) + implementation(libs.kotlinx.coroutines.test) + implementation(libs.assertk) + } + } val androidMain by getting { dependencies { implementation(libs.kotlin.stdlib.jdk8) diff --git a/navigation/common/src/androidMain/kotlin/com/mirego/pilot/navigation/compose/PilotBackHandler.kt b/navigation/common/src/androidMain/kotlin/com/mirego/pilot/navigation/compose/PilotBackHandler.kt index a8bc4b14..6aa0ad77 100644 --- a/navigation/common/src/androidMain/kotlin/com/mirego/pilot/navigation/compose/PilotBackHandler.kt +++ b/navigation/common/src/androidMain/kotlin/com/mirego/pilot/navigation/compose/PilotBackHandler.kt @@ -11,6 +11,6 @@ import com.mirego.pilot.navigation.PilotNavigationManager public fun PilotBackHandler(navController: NavHostController, navigationManager: PilotNavigationManager<*, *>, rootName: String) { val backStackEntry by navController.currentBackStackEntryFlow.collectAsState(initial = null) BackHandler(enabled = backStackEntry?.destination?.route != rootName) { - navigationManager.pop() + navigationManager.pop(locally = true) } } diff --git a/navigation/common/src/commonMain/kotlin/com/mirego/pilot/navigation/DefaultPilotNavigationManager.kt b/navigation/common/src/commonMain/kotlin/com/mirego/pilot/navigation/DefaultPilotNavigationManager.kt index eb19924d..cd86ef37 100644 --- a/navigation/common/src/commonMain/kotlin/com/mirego/pilot/navigation/DefaultPilotNavigationManager.kt +++ b/navigation/common/src/commonMain/kotlin/com/mirego/pilot/navigation/DefaultPilotNavigationManager.kt @@ -32,21 +32,19 @@ public open class DefaultPilotNavigationManager(testScope).apply { + listener = parentListener + } + private val childNavigationManager = DefaultPilotNavigationManager(testScope, parentNavigationManager = parentNavigationManager).apply { + listener = childListener + } + + @Test + fun `when pushing and popping on the child not locally it should push and pop to the parent`() { + childNavigationManager.push(TestNavigationRoute.Route1, locally = false) + childNavigationManager.push(TestNavigationRoute.Route2, locally = false) + assertThat(parentListener.routes).containsExactly(TestNavigationRoute.Route1, TestNavigationRoute.Route2) + assertThat(childListener.routes).isEmpty() + + childNavigationManager.pop(locally = false) + assertThat(parentListener.routes).containsExactly(TestNavigationRoute.Route1) + assertThat(childListener.routes).isEmpty() + + childNavigationManager.pop(locally = false) + assertThat(parentListener.routes).isEmpty() + assertThat(childListener.routes).isEmpty() + } + + @Test + fun `when popping locally and there is nothing to pop it should pop the parent`() { + parentNavigationManager.push(TestNavigationRoute.Route1, locally = true) + childNavigationManager.push(TestNavigationRoute.Route2, locally = true) + assertThat(parentListener.routes).containsExactly(TestNavigationRoute.Route1) + assertThat(childListener.routes).containsExactly(TestNavigationRoute.Route2) + + childNavigationManager.pop(locally = true) + assertThat(parentListener.routes).containsExactly(TestNavigationRoute.Route1) + assertThat(childListener.routes).isEmpty() + + childNavigationManager.pop(locally = true) + assertThat(parentListener.routes).isEmpty() + assertThat(childListener.routes).isEmpty() + } + + private enum class TestNavigationRouteName { + ROUTE1, + ROUTE2, + } + + private sealed class TestNavigationRoute(routeName: TestNavigationRouteName) : EnumPilotNavigationRoute(routeName) { + data object Route1 : TestNavigationRoute(TestNavigationRouteName.ROUTE1) + data object Route2 : TestNavigationRoute(TestNavigationRouteName.ROUTE2) + } + + private class TestNavigationListener : PilotNavigationListener() { + val routes = mutableListOf() + + override fun push(route: TestNavigationRoute): Boolean { + routes.add(route) + return true + } + + override fun pop() { + routes.removeLastOrNull() + } + + override fun popTo(route: TestNavigationRoute, inclusive: Boolean) { + while (routes.last() != route) { + routes.removeLast() + } + } + } +} From d6e421d7f8e966da379f397c82fb0a3c18008349 Mon Sep 17 00:00:00 2001 From: Nicolas Presseault Date: Fri, 17 May 2024 12:16:30 -0400 Subject: [PATCH 2/3] Add more test, while at it --- .../DefaultPilotNavigationManagerTest.kt | 33 ++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/navigation/common/src/commonTest/kotlin/com/mirego/pilot/navigation/DefaultPilotNavigationManagerTest.kt b/navigation/common/src/commonTest/kotlin/com/mirego/pilot/navigation/DefaultPilotNavigationManagerTest.kt index 79737639..e76af478 100644 --- a/navigation/common/src/commonTest/kotlin/com/mirego/pilot/navigation/DefaultPilotNavigationManagerTest.kt +++ b/navigation/common/src/commonTest/kotlin/com/mirego/pilot/navigation/DefaultPilotNavigationManagerTest.kt @@ -53,14 +53,42 @@ class DefaultPilotNavigationManagerTest { assertThat(childListener.routes).isEmpty() } + @Test + fun `when pop inclusive it should pop the given route`() { + parentNavigationManager.push(TestNavigationRoute.Route1) + parentNavigationManager.push(TestNavigationRoute.Route2) + parentNavigationManager.push(TestNavigationRoute.Route3) + + parentNavigationManager.popToId(TestNavigationRoute.Route2.uniqueId, inclusive = true) + assertThat(parentListener.routes).containsExactly(TestNavigationRoute.Route1) + + parentNavigationManager.popToName(TestNavigationRouteName.ROUTE1.name, inclusive = true) + assertThat(parentListener.routes).isEmpty() + } + + @Test + fun `when pop exclusive it should not pop the given route`() { + parentNavigationManager.push(TestNavigationRoute.Route1) + parentNavigationManager.push(TestNavigationRoute.Route2) + parentNavigationManager.push(TestNavigationRoute.Route3) + + parentNavigationManager.popToId(TestNavigationRoute.Route2.uniqueId, inclusive = false) + assertThat(parentListener.routes).containsExactly(TestNavigationRoute.Route1, TestNavigationRoute.Route2) + + parentNavigationManager.popToName(TestNavigationRoute.Route1.name, inclusive = false) + assertThat(parentListener.routes).containsExactly(TestNavigationRoute.Route1) + } + private enum class TestNavigationRouteName { ROUTE1, ROUTE2, + ROUTE3, } private sealed class TestNavigationRoute(routeName: TestNavigationRouteName) : EnumPilotNavigationRoute(routeName) { data object Route1 : TestNavigationRoute(TestNavigationRouteName.ROUTE1) data object Route2 : TestNavigationRoute(TestNavigationRouteName.ROUTE2) + data object Route3: TestNavigationRoute(TestNavigationRouteName.ROUTE3) } private class TestNavigationListener : PilotNavigationListener() { @@ -77,7 +105,10 @@ class DefaultPilotNavigationManagerTest { override fun popTo(route: TestNavigationRoute, inclusive: Boolean) { while (routes.last() != route) { - routes.removeLast() + routes.removeLastOrNull() + } + if (inclusive) { + routes.removeLastOrNull() } } } From 4bb068719c8693d46070fccafe3f4e3684bb8666 Mon Sep 17 00:00:00 2001 From: Nicolas Presseault Date: Fri, 17 May 2024 12:44:26 -0400 Subject: [PATCH 3/3] ktlint --- .../pilot/navigation/DefaultPilotNavigationManagerTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/navigation/common/src/commonTest/kotlin/com/mirego/pilot/navigation/DefaultPilotNavigationManagerTest.kt b/navigation/common/src/commonTest/kotlin/com/mirego/pilot/navigation/DefaultPilotNavigationManagerTest.kt index e76af478..79efaf9e 100644 --- a/navigation/common/src/commonTest/kotlin/com/mirego/pilot/navigation/DefaultPilotNavigationManagerTest.kt +++ b/navigation/common/src/commonTest/kotlin/com/mirego/pilot/navigation/DefaultPilotNavigationManagerTest.kt @@ -88,7 +88,7 @@ class DefaultPilotNavigationManagerTest { private sealed class TestNavigationRoute(routeName: TestNavigationRouteName) : EnumPilotNavigationRoute(routeName) { data object Route1 : TestNavigationRoute(TestNavigationRouteName.ROUTE1) data object Route2 : TestNavigationRoute(TestNavigationRouteName.ROUTE2) - data object Route3: TestNavigationRoute(TestNavigationRouteName.ROUTE3) + data object Route3 : TestNavigationRoute(TestNavigationRouteName.ROUTE3) } private class TestNavigationListener : PilotNavigationListener() {