Skip to content

Commit 48d8da1

Browse files
sdsantossdsantos
authored
Merge pull request #179 from ooni/navigation-bug
Protect navigation from duplicated actions and empty screens
2 parents 9ebfc85 + 2ecadfb commit 48d8da1

File tree

3 files changed

+140
-69
lines changed

3 files changed

+140
-69
lines changed

‎composeApp/src/commonMain/kotlin/org/ooni/probe/ui/navigation/BottomNavigationBar.kt‎

Lines changed: 1 addition & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import androidx.compose.material3.Text
99
import androidx.compose.runtime.Composable
1010
import androidx.compose.runtime.getValue
1111
import androidx.navigation.NavController
12-
import androidx.navigation.NavGraph.Companion.findStartDestination
1312
import androidx.navigation.compose.currentBackStackEntryAsState
1413
import ooniprobe.composeapp.generated.resources.Dashboard_Tab_Label
1514
import ooniprobe.composeapp.generated.resources.Res
@@ -38,7 +37,7 @@ fun BottomNavigationBar(navController: NavController) {
3837
},
3938
label = { Text(stringResource(screen.titleRes)) },
4039
selected = currentRoute == screen.route,
41-
onClick = { navController.navigateToMainScreen(screen) },
40+
onClick = { navController.safeNavigateToMain(screen) },
4241
colors = NavigationBarItemDefaults.colors(
4342
indicatorColor = MaterialTheme.colorScheme.primaryContainer,
4443
selectedIconColor = MaterialTheme.colorScheme.onPrimaryContainer,
@@ -48,24 +47,6 @@ fun BottomNavigationBar(navController: NavController) {
4847
}
4948
}
5049

51-
fun NavController.navigateToMainScreen(screen: Screen) {
52-
navigate(screen.route) {
53-
// Pop up to the start destination of the graph to
54-
// avoid building up a large stack of destinations
55-
// on the back stack as users select items
56-
graph.findStartDestination().route?.let {
57-
popUpTo(it) {
58-
saveState = true
59-
}
60-
}
61-
// Avoid multiple copies of the same destination when
62-
// re-selecting the same item
63-
launchSingleTop = true
64-
// Restore state when re-selecting a previously selected item
65-
restoreState = true
66-
}
67-
}
68-
6950
private val Screen.titleRes
7051
get() =
7152
when (this) {

‎composeApp/src/commonMain/kotlin/org/ooni/probe/ui/navigation/Navigation.kt‎

Lines changed: 96 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,10 @@ import androidx.compose.runtime.collectAsState
77
import androidx.compose.runtime.getValue
88
import androidx.compose.ui.Modifier
99
import androidx.compose.ui.window.DialogProperties
10+
import androidx.lifecycle.Lifecycle
1011
import androidx.lifecycle.viewmodel.compose.viewModel
12+
import androidx.navigation.NavController
13+
import androidx.navigation.NavGraph.Companion.findStartDestination
1114
import androidx.navigation.NavHostController
1215
import androidx.navigation.compose.NavHost
1316
import androidx.navigation.compose.composable
@@ -36,26 +39,26 @@ import org.ooni.probe.ui.settings.category.SettingsCategoryScreen
3639
import org.ooni.probe.ui.settings.proxy.ProxyScreen
3740
import org.ooni.probe.ui.upload.UploadMeasurementsDialog
3841

42+
private val START_SCREEN = Screen.Dashboard
43+
3944
@Composable
4045
fun Navigation(
4146
navController: NavHostController,
4247
dependencies: Dependencies,
4348
) {
4449
NavHost(
4550
navController = navController,
46-
startDestination = Screen.Dashboard.route,
51+
startDestination = START_SCREEN.route,
4752
modifier = Modifier.fillMaxSize(),
4853
) {
4954
composable(route = Screen.Onboarding.route) {
5055
val viewModel = viewModel {
5156
dependencies.onboardingViewModel(
5257
goToDashboard = {
53-
navController.popBackStack()
54-
navController.navigateToMainScreen(Screen.Dashboard)
58+
navController.goBackAndNavigateToMain(Screen.Dashboard)
5559
},
5660
goToSettings = {
57-
navController.popBackStack()
58-
navController.navigateToMainScreen(Screen.Settings)
61+
navController.goBackAndNavigateToMain(Screen.Settings)
5962
},
6063
)
6164
}
@@ -67,17 +70,16 @@ fun Navigation(
6770
val viewModel = viewModel {
6871
dependencies.dashboardViewModel(
6972
goToOnboarding = {
70-
navController.popBackStack()
71-
navController.navigate(Screen.Onboarding.route)
73+
navController.goBackAndNavigate(Screen.Onboarding)
7274
},
7375
goToResults = { navController.navigateToMainScreen(Screen.Results) },
74-
goToRunningTest = { navController.navigate(Screen.RunningTest.route) },
75-
goToRunTests = { navController.navigate(Screen.RunTests.route) },
76+
goToRunningTest = { navController.safeNavigate(Screen.RunningTest) },
77+
goToRunTests = { navController.safeNavigate(Screen.RunTests) },
7678
goToDescriptor = { descriptorKey ->
77-
navController.navigate(Screen.Descriptor(descriptorKey).route)
79+
navController.safeNavigate(Screen.Descriptor(descriptorKey))
7880
},
7981
goToReviewDescriptorUpdates = {
80-
navController.navigate(Screen.ReviewUpdates.route)
82+
navController.safeNavigate(Screen.ReviewUpdates)
8183
},
8284
)
8385
}
@@ -88,8 +90,8 @@ fun Navigation(
8890
composable(route = Screen.Results.route) {
8991
val viewModel = viewModel {
9092
dependencies.resultsViewModel(
91-
goToResult = { navController.navigate(Screen.Result(it).route) },
92-
goToUpload = { navController.navigate(Screen.UploadMeasurements().route) },
93+
goToResult = { navController.safeNavigate(Screen.Result(it)) },
94+
goToUpload = { navController.safeNavigate(Screen.UploadMeasurements()) },
9395
)
9496
}
9597
val state by viewModel.state.collectAsState()
@@ -100,7 +102,7 @@ fun Navigation(
100102
val viewModel = viewModel {
101103
dependencies.settingsViewModel(
102104
goToSettingsForCategory = {
103-
navController.navigate(Screen.SettingsCategory(it).route)
105+
navController.safeNavigate(Screen.SettingsCategory(it))
104106
},
105107
)
106108
}
@@ -118,12 +120,12 @@ fun Navigation(
118120
val viewModel = viewModel {
119121
dependencies.resultViewModel(
120122
resultId = resultId,
121-
onBack = { navController.popBackStack() },
123+
onBack = { navController.goBack() },
122124
goToMeasurement = { reportId, input ->
123-
navController.navigate(Screen.Measurement(reportId, input).route)
125+
navController.safeNavigate(Screen.Measurement(reportId, input))
124126
},
125127
goToUpload = {
126-
navController.navigate(Screen.UploadMeasurements(resultId).route)
128+
navController.safeNavigate(Screen.UploadMeasurements(resultId))
127129
},
128130
)
129131
}
@@ -140,7 +142,7 @@ fun Navigation(
140142
MeasurementScreen(
141143
reportId = MeasurementModel.ReportId(reportId),
142144
input = input,
143-
onBack = { navController.popBackStack() },
145+
onBack = { navController.goBack() },
144146
)
145147
}
146148

@@ -152,7 +154,7 @@ fun Navigation(
152154
when (category) {
153155
PreferenceCategoryKey.ABOUT_OONI.value -> {
154156
val viewModel = viewModel {
155-
dependencies.aboutViewModel(onBack = { navController.navigateUp() })
157+
dependencies.aboutViewModel(onBack = { navController.goBack() })
156158
}
157159
AboutScreen(
158160
onEvent = viewModel::onEvent,
@@ -163,15 +165,15 @@ fun Navigation(
163165

164166
PreferenceCategoryKey.PROXY.value -> {
165167
val viewModel = viewModel {
166-
dependencies.proxyViewModel(onBack = { navController.navigateUp() })
168+
dependencies.proxyViewModel(onBack = { navController.goBack() })
167169
}
168170
val state by viewModel.state.collectAsState()
169171
ProxyScreen(state, viewModel::onEvent)
170172
}
171173

172174
PreferenceCategoryKey.SEE_RECENT_LOGS.value -> {
173175
val viewModel = viewModel {
174-
dependencies.logViewModel(onBack = { navController.popBackStack() })
176+
dependencies.logViewModel(onBack = { navController.goBack() })
175177
}
176178
val state by viewModel.state.collectAsState()
177179
LogScreen(state, viewModel::onEvent)
@@ -182,9 +184,9 @@ fun Navigation(
182184
dependencies.settingsCategoryViewModel(
183185
categoryKey = category,
184186
goToSettingsForCategory = {
185-
navController.navigate(Screen.SettingsCategory(it).route)
187+
navController.safeNavigate(Screen.SettingsCategory(it))
186188
},
187-
onBack = { navController.popBackStack() },
189+
onBack = { navController.goBack() },
188190
)
189191
}
190192
val state by viewModel.state.collectAsState()
@@ -195,7 +197,7 @@ fun Navigation(
195197

196198
composable(route = Screen.RunTests.route) {
197199
val viewModel = viewModel {
198-
dependencies.runViewModel(onBack = { navController.popBackStack() })
200+
dependencies.runViewModel(onBack = { navController.goBack() })
199201
}
200202
val state by viewModel.state.collectAsState()
201203
RunScreen(state, viewModel::onEvent)
@@ -208,7 +210,7 @@ fun Navigation(
208210
entry.arguments?.getLong("runId")?.let { descriptorId ->
209211
val viewModel = viewModel {
210212
dependencies.addDescriptorViewModel(
211-
onBack = { navController.popBackStack() },
213+
onBack = { navController.goBack() },
212214
descriptorId = descriptorId.toString(),
213215
)
214216
}
@@ -219,17 +221,16 @@ fun Navigation(
219221
LaunchedEffect(Unit) {
220222
snackbarHostState?.showSnackbar("Invalid descriptor ID")
221223
}
222-
navController.popBackStack()
224+
navController.goBack()
223225
}
224226
}
225227

226228
composable(route = Screen.RunningTest.route) {
227229
val viewModel = viewModel {
228230
dependencies.runningViewModel(
229-
onBack = { navController.popBackStack() },
231+
onBack = { navController.goBack() },
230232
goToResults = {
231-
navController.popBackStack()
232-
navController.navigateToMainScreen(Screen.Results)
233+
navController.goBackAndNavigateToMain(Screen.Results)
233234
},
234235
)
235236
}
@@ -250,7 +251,7 @@ fun Navigation(
250251
val viewModel = viewModel {
251252
dependencies.uploadMeasurementsViewModel(
252253
resultId = resultId,
253-
onClose = { navController.popBackStack() },
254+
onClose = { navController.goBack() },
254255
)
255256
}
256257
val state by viewModel.state.collectAsState()
@@ -265,9 +266,9 @@ fun Navigation(
265266
val viewModel = viewModel {
266267
dependencies.descriptorViewModel(
267268
descriptorKey = descriptorKey,
268-
onBack = { navController.popBackStack() },
269+
onBack = { navController.goBack() },
269270
goToReviewDescriptorUpdates = {
270-
navController.navigate(Screen.ReviewUpdates.route)
271+
navController.safeNavigate(Screen.ReviewUpdates)
271272
},
272273
goToChooseWebsites = { navController.navigate(Screen.ChooseWebsites.route) },
273274
)
@@ -276,10 +277,10 @@ fun Navigation(
276277
DescriptorScreen(state, viewModel::onEvent)
277278
}
278279

279-
composable(route = Screen.ReviewUpdates.route) { entry ->
280+
composable(route = Screen.ReviewUpdates.route) {
280281
val viewModel = viewModel {
281282
dependencies.reviewUpdatesViewModel(
282-
onBack = { navController.popBackStack() },
283+
onBack = { navController.goBack() },
283284
)
284285
}
285286
val state by viewModel.state.collectAsState()
@@ -300,3 +301,64 @@ fun Navigation(
300301
}
301302
}
302303
}
304+
305+
// Helpers
306+
307+
private fun NavController.goBack() {
308+
if (!isResumed()) return
309+
if (!popBackStack()) {
310+
navigateToMainScreen(START_SCREEN)
311+
}
312+
}
313+
314+
private fun NavController.goBackTo(
315+
screen: Screen,
316+
inclusive: Boolean = false,
317+
) {
318+
if (!isResumed()) return
319+
if (!popBackStack(screen.route, inclusive = inclusive)) {
320+
navigateToMainScreen(START_SCREEN)
321+
}
322+
}
323+
324+
private fun NavController.goBackAndNavigate(screen: Screen) {
325+
if (!isResumed()) return
326+
popBackStack()
327+
navigate(screen.route)
328+
}
329+
330+
private fun NavController.goBackAndNavigateToMain(screen: Screen) {
331+
if (!isResumed()) return
332+
popBackStack()
333+
navigateToMainScreen(screen)
334+
}
335+
336+
private fun NavController.safeNavigate(screen: Screen) {
337+
if (!isResumed()) return
338+
navigate(screen.route)
339+
}
340+
341+
fun NavController.safeNavigateToMain(screen: Screen) {
342+
if (!isResumed()) return
343+
navigateToMainScreen(screen)
344+
}
345+
346+
private fun NavController.isResumed() = currentBackStackEntry?.lifecycle?.currentState == Lifecycle.State.RESUMED
347+
348+
private fun NavController.navigateToMainScreen(screen: Screen) {
349+
navigate(screen.route) {
350+
// Pop up to the start destination of the graph to
351+
// avoid building up a large stack of destinations
352+
// on the back stack as users select items
353+
graph.findStartDestination().route?.let {
354+
popUpTo(it) {
355+
saveState = true
356+
}
357+
}
358+
// Avoid multiple copies of the same destination when
359+
// re-selecting the same item
360+
launchSingleTop = true
361+
// Restore state when re-selecting a previously selected item
362+
restoreState = true
363+
}
364+
}

0 commit comments

Comments
 (0)