diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/Nav.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/Nav.kt index ac0adde7bc..53f02d6c7e 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/Nav.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/Nav.kt @@ -30,6 +30,7 @@ import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.setValue import androidx.lifecycle.viewmodel.compose.LocalViewModelStoreOwner import androidx.navigation.NavBackStackEntry +import androidx.navigation.NavDestination.Companion.hasRoute import androidx.navigation.NavGraph.Companion.findStartDestination import androidx.navigation.NavHostController import com.vitorpamplona.amethyst.ui.navigation.BOTTOM_NAV_ROOT_KEY @@ -102,6 +103,22 @@ class Nav( override fun navBottomBar(route: Route) { navigationScope.launch { ime.settle() + + // A nav-bar tap asks for a tab, never for whatever the user pushed on top of one. Drop + // those pushes first, and without saving them, so the restoreState below can never hand + // a deep stack back. On phones this is always a no-op — AppBottomBar hides itself off + // tab roots, so the bar is only ever tapped from one — but the large-screen rail stays + // on screen the whole time and is routinely tapped from three screens deep. + popPushesAboveTabRoot() + + // Dropping those pushes is often the whole job — re-tapping the tab the user is inside, + // and every tap of Home from somewhere inside Home, end here. Same already-there guard + // nav() uses, so a parameterized tab compares by its filled route. + if (getRouteWithArguments(route::class, controller) == route) { + controller.currentBackStackEntry?.savedStateHandle?.set(BOTTOM_NAV_ROOT_KEY, true) + return@launch + } + controller.navigate(route) { // Clear sibling bottom-nav entries but keep Home (the start // destination) below, so back-swipe from any tab returns to @@ -117,7 +134,15 @@ class Nav( saveState = true } launchSingleTop = true - restoreState = true + // ...but never onto Home. A non-inclusive popUpTo files the popped entries under the + // popUpTo TARGET's own destination id as well as the popped tab's — the + // `if (!inclusive)` branch of NavControllerImpl.executePopOperations — so restoring + // here would hand Home the stack that was saved on the way out of some *other* tab. + // That is how tapping Home on the rail came back to a thread instead of the feed. + // Home is the anchor, so it is never popped and has no saved stack of its own to + // miss: launchSingleTop reuses the entry already sitting there, ViewModelStore and + // all. + restoreState = route != Route.Home } // Mark this entry as a tab root: hides the back arrow in canPop // and skips the horizontal slide in composableFromEnd. @@ -146,6 +171,25 @@ class Nav( } } + /** + * Drops every entry the user pushed on top of the tab root they are currently in, discarding + * that state rather than saving it — a nav-bar tap is a request for a tab, and nothing above a + * tab root should survive to be replayed by a later `restoreState`. + * + * Stops at the first entry stamped [BOTTOM_NAV_ROOT_KEY] by [navBottomBar], or at Home, which + * is a tab root the user may never have tapped because the graph starts there. Terminates + * either way: every iteration that does not return removes one entry from a finite back stack, + * and [NavHostController.popBackStack] reports false once there is nothing left to pop. + */ + private fun popPushesAboveTabRoot() { + while (true) { + val top = controller.currentBackStackEntry ?: return + if (top.isBottomNavRoot()) return + if (top.destination.hasRoute(Route.Home::class)) return + if (!controller.popBackStack()) return + } + } + @Composable override fun canPop(): Boolean { // Decide the back arrow / bottom-bar visibility from THIS screen's own diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavBottomBarStackTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavBottomBarStackTest.kt new file mode 100644 index 0000000000..c5cac66bbb --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavBottomBarStackTest.kt @@ -0,0 +1,116 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.ui.navigation + +import androidx.navigation.NavBackStackEntry +import androidx.navigation.NavHostController +import androidx.navigation.NavOptions +import androidx.navigation.NavOptionsBuilder +import androidx.navigation.navOptions +import com.vitorpamplona.amethyst.ui.navigation.navs.Nav +import com.vitorpamplona.amethyst.ui.navigation.routes.Route +import io.mockk.every +import io.mockk.mockk +import io.mockk.slot +import io.mockk.verify +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * A nav-bar tap must land on the tab itself, never on whatever the user had pushed on top of one. + * + * The phone bottom bar gets this for free — it hides itself off tab roots, so it can only ever be + * tapped from one. The large-screen navigation rail stays on screen the whole time, which is where + * the gap showed: tapping Home from a thread three screens deep came back to that thread instead of + * the feed. + * + * Two rules keep that from happening, one per test below: + * + * 1. Pushes above a tab root are dropped, unsaved, on the way out — anything saved is eligible to be + * replayed by a later `restoreState`. + * 2. Home is never restored onto. `popUpTo(Home) { inclusive = false; saveState = true }` files the + * popped entries under the popUpTo target's own destination id as well as the popped tab's — see + * the `if (!inclusive)` branch of `NavControllerImpl.executePopOperations` — so + * `navigate(Home) { restoreState = true }` handed Home back the stack the user had left behind in + * some *other* tab. Every other tab restores as before; that is what keeps its ViewModelStore. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class NavBottomBarStackTest { + /** A back-stack entry that is, or isn't, a tab root as far as [isBottomNavRoot] can tell. */ + private fun entry(isTabRoot: Boolean): NavBackStackEntry = + mockk(relaxed = true) { + every { savedStateHandle.get(BOTTOM_NAV_ROOT_KEY) } returns isTabRoot + } + + /** + * Taps [route] on a controller whose back stack holds none of the nav-bar destinations, and + * returns the options the resulting navigate was built with. + */ + private fun TestScope.optionsForTapping(route: Route): NavOptions { + val options = slot Unit>() + val controller = + mockk(relaxed = true) { + every { currentBackStackEntry } returns entry(isTabRoot = true) + every { navigate(route, capture(options)) } returns Unit + } + + Nav(controller, this).navBottomBar(route) + advanceUntilIdle() + + return navOptions(options.captured) + } + + @Test + fun dropsWhatTheUserPushedOnTopOfTheTabBeforeSwitchingTabs() = + runTest { + // Home > Note > Profile: two pushes sitting on a tab root. + val controller = + mockk(relaxed = true) { + every { currentBackStackEntry } returnsMany + listOf(entry(isTabRoot = false), entry(isTabRoot = false), entry(isTabRoot = true)) + every { popBackStack() } returns true + } + + Nav(controller, this).navBottomBar(Route.Message) + advanceUntilIdle() + + // Exactly the two pushes and no more: the tab root itself stays, so the navigate below + // saves a tab root rather than a branch that could be replayed later. + verify(exactly = 2) { controller.popBackStack() } + } + + @Test + fun neverRestoresASavedStackOntoHome() = + runTest { + assertFalse(optionsForTapping(Route.Home).shouldRestoreState()) + } + + @Test + fun restoresTheSavedStackOfEveryOtherTab() = + runTest { + assertTrue(optionsForTapping(Route.Message).shouldRestoreState()) + } +}