From 3f8a0cedd0b7f72ac88c928889162e695c87f514 Mon Sep 17 00:00:00 2001 From: Jeremy Woods Date: Tue, 12 Jul 2022 17:24:36 -0700 Subject: [PATCH 1/2] Mark transitionsInProgress complete in BottomSheetNavigator We should be marking markTransitionComplete on the transitionsInProgress, not the backStack. Fixes #978 --- .../BottomSheetNavigatorTest.kt | 63 +++++++++++++++++++ .../material/BottomSheetNavigator.kt | 2 +- 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/navigation-material/src/androidTest/java/com/google/accompanist/navigation.material/BottomSheetNavigatorTest.kt b/navigation-material/src/androidTest/java/com/google/accompanist/navigation.material/BottomSheetNavigatorTest.kt index d6a4d0c65..fe5516d40 100644 --- a/navigation-material/src/androidTest/java/com/google/accompanist/navigation.material/BottomSheetNavigatorTest.kt +++ b/navigation-material/src/androidTest/java/com/google/accompanist/navigation.material/BottomSheetNavigatorTest.kt @@ -17,10 +17,13 @@ package com.google.accompanist.navigation.material import android.os.Bundle +import androidx.activity.OnBackPressedDispatcher +import androidx.activity.compose.LocalOnBackPressedDispatcherOwner import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.foundation.layout.size +import androidx.compose.material.Button import androidx.compose.material.ExperimentalMaterialApi import androidx.compose.material.ModalBottomSheetState import androidx.compose.material.ModalBottomSheetValue @@ -34,10 +37,14 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.platform.testTag import androidx.compose.ui.test.junit4.createComposeRule import androidx.compose.ui.test.onNodeWithTag +import androidx.compose.ui.test.onNodeWithText import androidx.compose.ui.test.performClick import androidx.compose.ui.unit.dp +import androidx.lifecycle.Lifecycle +import androidx.navigation.NavBackStackEntry import androidx.navigation.NavHostController import androidx.navigation.compose.NavHost +import androidx.navigation.compose.composable import androidx.navigation.compose.rememberNavController import androidx.navigation.plusAssign import androidx.navigation.testing.TestNavigatorState @@ -305,6 +312,62 @@ internal class BottomSheetNavigatorTest { .containsExactly(entry) } + @Test + fun testBackPressedDestroysEntry() { + lateinit var onBackPressedDispatcher: OnBackPressedDispatcher + lateinit var navController: NavHostController + + composeTestRule.setContent { + val bottomSheetNavigator = rememberBottomSheetNavigator() + navController = rememberNavController(bottomSheetNavigator) + onBackPressedDispatcher = + LocalOnBackPressedDispatcherOwner.current?.onBackPressedDispatcher!! + + ModalBottomSheetLayout(bottomSheetNavigator) { + Box(modifier = Modifier.fillMaxSize()) { + NavHost( + navController = navController, + startDestination = "mainScreen" + ) { + + composable( + route = "mainScreen", + content = { + Button(onClick = { navController.navigate("bottomSheet") }) { + Text(text = "open drawer") + } + } + ) + + bottomSheet( + route = "bottomSheet", + content = { + Box(modifier = Modifier.fillMaxSize()) { + Text( + text = "bottomSheet" + ) + } + } + ) + } + } + } + } + + composeTestRule.onNodeWithText("open drawer").performClick() + + lateinit var bottomSheetEntry: NavBackStackEntry + + composeTestRule.runOnIdle { + bottomSheetEntry = navController.currentBackStackEntry!! + onBackPressedDispatcher.onBackPressed() + } + + composeTestRule.runOnIdle { + assertThat(bottomSheetEntry.lifecycle.currentState).isEqualTo(Lifecycle.State.DESTROYED) + } + } + /** * Create a [BottomSheetNavigator.Destination] with some fake content * Having an empty Composable will result in the sheet's height being 0 which crashes diff --git a/navigation-material/src/main/java/com/google/accompanist/navigation/material/BottomSheetNavigator.kt b/navigation-material/src/main/java/com/google/accompanist/navigation/material/BottomSheetNavigator.kt index 05af1416a..1a65781f1 100644 --- a/navigation-material/src/main/java/com/google/accompanist/navigation/material/BottomSheetNavigator.kt +++ b/navigation-material/src/main/java/com/google/accompanist/navigation/material/BottomSheetNavigator.kt @@ -177,7 +177,7 @@ public class BottomSheetNavigator( // currently displaying because it will have its transition completed when the sheet's // animation has completed DisposableEffect(backStackEntries) { - backStackEntries.forEach { + state.transitionsInProgress.value.forEach { if (it != latestEntry) state.markTransitionComplete(it) } onDispose { } From e32a4e297e5f1b7b9b77779b19a5fe54e3ac7926 Mon Sep 17 00:00:00 2001 From: Jeremy Woods Date: Wed, 13 Jul 2022 14:20:35 -0700 Subject: [PATCH 2/2] Only get transitionsInProgress when attached Make sure that the Navigator is attached before attempting to get its state. --- .../navigation/material/BottomSheetNavigator.kt | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/navigation-material/src/main/java/com/google/accompanist/navigation/material/BottomSheetNavigator.kt b/navigation-material/src/main/java/com/google/accompanist/navigation/material/BottomSheetNavigator.kt index 1a65781f1..ca94762e8 100644 --- a/navigation-material/src/main/java/com/google/accompanist/navigation/material/BottomSheetNavigator.kt +++ b/navigation-material/src/main/java/com/google/accompanist/navigation/material/BottomSheetNavigator.kt @@ -150,6 +150,18 @@ public class BottomSheetNavigator( MutableStateFlow(emptyList()) } + /** + * Get the transitionsInProgress from the [state]. In some cases, the [sheetContent] might be + * composed before the Navigator is attached, so we specifically return an empty flow if we + * aren't attached yet. + */ + private val transitionsInProgress: StateFlow> + get() = if (attached) { + state.transitionsInProgress + } else { + MutableStateFlow(emptySet()) + } + /** * Access properties of the [ModalBottomSheetLayout]'s [ModalBottomSheetState] */ @@ -163,6 +175,7 @@ public class BottomSheetNavigator( val columnScope = this val saveableStateHolder = rememberSaveableStateHolder() val backStackEntries by backStack.collectAsState() + val transitionsInProgressEntries by transitionsInProgress.collectAsState() // We always replace the sheet's content instead of overlaying and nesting floating // window destinations. That means that only *one* concurrent destination is supported by @@ -177,7 +190,7 @@ public class BottomSheetNavigator( // currently displaying because it will have its transition completed when the sheet's // animation has completed DisposableEffect(backStackEntries) { - state.transitionsInProgress.value.forEach { + transitionsInProgressEntries.forEach { if (it != latestEntry) state.markTransitionComplete(it) } onDispose { } @@ -194,7 +207,7 @@ public class BottomSheetNavigator( onSheetDismissed = { backStackEntry -> // Sheet dismissal can be started through popBackStack in which case we have a // transition that we'll want to complete - if (state.transitionsInProgress.value.contains(backStackEntry)) { + if (transitionsInProgressEntries.contains(backStackEntry)) { state.markTransitionComplete(backStackEntry) } else { state.pop(popUpTo = backStackEntry, saveState = false)