Address review feedback from Manu

Author: dturner

app/src/androidTest/java/com/google/samples/apps/nowinandroid/ui/NavigationTest.kt

@@ -206,7 +206,6 @@ class NavigationTest {
 
     @Test
     fun whenSettingsDialogDismissed_previousScreenIsDisplayed() {
-
         composeTestRule.apply {
 
             // Navigate to the saved screen, open the settings dialog, then close it.

app/src/androidTest/java/com/google/samples/apps/nowinandroid/ui/NiaAppStateTest.kt

@@ -30,19 +30,14 @@ import androidx.navigation.compose.ComposeNavigator
 import androidx.navigation.compose.composable
 import androidx.navigation.createGraph
 import androidx.navigation.testing.TestNavHostController
-import com.google.samples.apps.nowinandroid.core.testing.util.MainDispatcherRule
 import com.google.samples.apps.nowinandroid.core.testing.util.TestNetworkMonitor
-import kotlinx.coroutines.cancel
 import kotlinx.coroutines.flow.collect
 import kotlinx.coroutines.launch
-import kotlinx.coroutines.test.TestScope
 import kotlinx.coroutines.test.UnconfinedTestDispatcher
 import kotlinx.coroutines.test.runTest
-import org.junit.After
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
 import org.junit.Assert.assertTrue
-import org.junit.Before
 import org.junit.Rule
 import org.junit.Test
 
@@ -55,31 +50,15 @@ import org.junit.Test
 @OptIn(ExperimentalMaterial3WindowSizeClassApi::class)
 class NiaAppStateTest {
 
-    @get:Rule
-    val mainDispatcherRule = MainDispatcherRule()
-
     @get:Rule
     val composeTestRule = createComposeRule()
 
     // Create the test dependencies.
-    private lateinit var testScope: TestScope
     private val networkMonitor = TestNetworkMonitor()
 
     // Subject under test.
     private lateinit var state: NiaAppState
 
-    @Before
-    fun setup() {
-        // We use the Unconfined dispatcher to ensure that coroutines are executed sequentially in
-        // tests.
-        testScope = TestScope(UnconfinedTestDispatcher())
-    }
-
-    @After
-    fun cleanup() {
-        testScope.cancel()
-    }
-
     @Test
     fun niaAppState_currentDestination() = runTest {
         var currentDestination: String? = null
@@ -91,7 +70,7 @@ class NiaAppStateTest {
                     windowSizeClass = getCompactWindowClass(),
                     navController = navController,
                     networkMonitor = networkMonitor,
-                    coroutineScope = testScope
+                    coroutineScope = backgroundScope
                 )
             }
 
@@ -122,28 +101,14 @@ class NiaAppStateTest {
         assertTrue(state.topLevelDestinations[2].name.contains("interests", true))
     }
 
-    @Test
-    fun niaAppState_showTopBarForTopLevelDestinations() {
-        composeTestRule.setContent {
-            val navController = rememberTestNavController()
-            state = rememberNiaAppState(
-                windowSizeClass = getCompactWindowClass(),
-                navController = navController,
-                networkMonitor = networkMonitor
-            )
-
-            // Do nothing - we should already be
-        }
-    }
-
     @Test
     fun niaAppState_showBottomBar_compact() = runTest {
         composeTestRule.setContent {
             state = NiaAppState(
                 windowSizeClass = getCompactWindowClass(),
                 navController = NavHostController(LocalContext.current),
                 networkMonitor = networkMonitor,
-                coroutineScope = testScope
+                coroutineScope = backgroundScope
             )
         }
 
@@ -158,7 +123,7 @@ class NiaAppStateTest {
                 windowSizeClass = WindowSizeClass.calculateFromSize(DpSize(800.dp, 800.dp)),
                 navController = NavHostController(LocalContext.current),
                 networkMonitor = networkMonitor,
-                coroutineScope = testScope
+                coroutineScope = backgroundScope
             )
         }
 
@@ -174,7 +139,7 @@ class NiaAppStateTest {
                 windowSizeClass = WindowSizeClass.calculateFromSize(DpSize(900.dp, 1200.dp)),
                 navController = NavHostController(LocalContext.current),
                 networkMonitor = networkMonitor,
-                coroutineScope = testScope
+                coroutineScope = backgroundScope
             )
         }
 
@@ -183,27 +148,23 @@ class NiaAppStateTest {
     }
 
     @Test
-    fun stateIsOfflineWhenNetworkMonitorIsOffline() = runTest {
+    fun stateIsOfflineWhenNetworkMonitorIsOffline() = runTest(UnconfinedTestDispatcher()) {
 
         composeTestRule.setContent {
             state = NiaAppState(
                 windowSizeClass = WindowSizeClass.calculateFromSize(DpSize(900.dp, 1200.dp)),
                 navController = NavHostController(LocalContext.current),
                 networkMonitor = networkMonitor,
-                coroutineScope = testScope
+                coroutineScope = backgroundScope
             )
         }
 
-        val collectJob = testScope.launch { state.isOffline.collect() }
-
+        backgroundScope.launch { state.isOffline.collect() }
         networkMonitor.setConnected(false)
-
         assertEquals(
             true,
             state.isOffline.value
         )
-
-        collectJob.cancel()
     }
 
     private fun getCompactWindowClass() = WindowSizeClass.calculateFromSize(DpSize(500.dp, 300.dp))

app/src/main/java/com/google/samples/apps/nowinandroid/ui/NiaApp.kt

@@ -70,7 +70,8 @@ import com.google.samples.apps.nowinandroid.navigation.TopLevelDestination
 @OptIn(
     ExperimentalMaterial3Api::class,
     ExperimentalLayoutApi::class,
-    ExperimentalComposeUiApi::class, ExperimentalLifecycleComposeApi::class
+    ExperimentalComposeUiApi::class,
+    ExperimentalLifecycleComposeApi::class
 )
 @Composable
 fun NiaApp(
@@ -101,19 +102,18 @@ fun NiaApp(
             snackbarHost = { SnackbarHost(snackbarHostState) },
             topBar = {
                 // Show the top app bar on top level destinations.
-                val topLevelDestination =
-                    appState.topLevelDestinations[appState.currentDestination?.route]
-                if (topLevelDestination != null) {
+                val destination = appState.currentTopLevelDestination
+                if (destination != null) {
                     NiaTopAppBar(
-                        titleRes = topLevelDestination.titleTextId,
+                        titleRes = destination.titleTextId,
                         actionIcon = NiaIcons.Settings,
                         actionIconContentDescription = stringResource(
                             id = settingsR.string.top_app_bar_action_icon_description
                         ),
                         colors = TopAppBarDefaults.centerAlignedTopAppBarColors(
                             containerColor = Color.Transparent
                         ),
-                        onActionClick = { appState.toggleSettingsDialog(true) }
+                        onActionClick = { appState.setShowSettingsDialog(true) }
                     )
                 }
             },
@@ -141,7 +141,7 @@ fun NiaApp(
 
             if (appState.shouldShowSettingsDialog) {
                 SettingsDialog(
-                    onDismiss = { appState.toggleSettingsDialog(false) }
+                    onDismiss = { appState.setShowSettingsDialog(false) }
                 )
             }
 

app/src/main/java/com/google/samples/apps/nowinandroid/ui/NiaAppState.kt

@@ -76,9 +76,11 @@ class NiaAppState(
         @Composable get() = navController
             .currentBackStackEntryAsState().value?.destination
 
-    private var _shouldShowSettingsDialog by mutableStateOf(false)
-    val shouldShowSettingsDialog
-        get() = _shouldShowSettingsDialog
+    val currentTopLevelDestination: TopLevelDestination?
+        @Composable get() = topLevelDestinations[currentDestination?.route]
+
+    var shouldShowSettingsDialog by mutableStateOf(false)
+        private set
 
     val shouldShowBottomBar: Boolean
         get() = windowSizeClass.widthSizeClass == WindowWidthSizeClass.Compact ||
@@ -136,8 +138,8 @@ class NiaAppState(
         navController.popBackStack()
     }
 
-    fun toggleSettingsDialog(shouldShow: Boolean) {
-        _shouldShowSettingsDialog = shouldShow
+    fun setShowSettingsDialog(shouldShow: Boolean) {
+        shouldShowSettingsDialog = shouldShow
     }
 }
 

core/designsystem/src/main/java/com/google/samples/apps/nowinandroid/core/designsystem/component/TopAppBar.kt

@@ -32,9 +32,7 @@ import androidx.compose.runtime.Composable
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.graphics.vector.ImageVector
 import androidx.compose.ui.res.stringResource
-import androidx.compose.ui.semantics.contentDescription
 import androidx.compose.ui.tooling.preview.Preview
-import com.google.samples.apps.nowinandroid.core.designsystem.R
 
 @OptIn(ExperimentalMaterial3Api::class)
 @Composable

core/designsystem/src/main/res/values/strings.xml

@@ -18,5 +18,4 @@
     <string name="follow">Follow</string>
     <string name="unfollow">Unfollow</string>
     <string name="browse_topic">Browse topic</string>
-    <string name="screen_title">Screen title</string>
 </resources>

feature/settings/src/main/java/com/google/samples/apps/nowinandroid/feature/settings/SettingsViewModel.kt

@@ -33,7 +33,7 @@ import kotlinx.coroutines.launch
 
 @HiltViewModel
 class SettingsViewModel @Inject constructor(
-    private val userDataRepository: UserDataRepository
+    private val userDataRepository: UserDataRepository,
 ) : ViewModel() {
     val settingsUiState: StateFlow<SettingsUiState> =
         userDataRepository.userDataStream