diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 75a3a82..40cda40 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -109,6 +109,9 @@ dependencies { testImplementation(libs.kotlinx.coroutines.test) testImplementation(libs.robolectric) testImplementation(libs.androidx.test.core) + testImplementation(libs.compose.ui.test.junit4) + // Supplies the ComponentActivity the compose test rule launches into. + debugImplementation(libs.compose.ui.test.manifest) androidTestImplementation(libs.androidx.test.junit) androidTestImplementation(libs.androidx.espresso.core) } diff --git a/app/src/main/kotlin/dev/privacyllc/period/feature/common/SettingsSubpage.kt b/app/src/main/kotlin/dev/privacyllc/period/feature/common/SettingsSubpage.kt new file mode 100644 index 0000000..957e17b --- /dev/null +++ b/app/src/main/kotlin/dev/privacyllc/period/feature/common/SettingsSubpage.kt @@ -0,0 +1,91 @@ +package dev.privacyllc.period.feature.common + +import androidx.compose.foundation.layout.Box +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.WindowInsets +import androidx.compose.foundation.layout.fillMaxSize +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.material.icons.Icons +import androidx.compose.material.icons.automirrored.filled.ArrowBack +import androidx.compose.material3.ExperimentalMaterial3Api +import androidx.compose.material3.Icon +import androidx.compose.material3.IconButton +import androidx.compose.material3.Surface +import androidx.compose.material3.Text +import androidx.compose.material3.TopAppBar +import androidx.compose.runtime.Composable +import androidx.compose.ui.Modifier +import androidx.compose.ui.res.stringResource +import androidx.compose.ui.semantics.heading +import androidx.compose.ui.semantics.semantics +import androidx.compose.ui.text.style.TextOverflow +import dev.privacyllc.period.R + +/** + * The chrome every screen below a tab shares: a bar naming the row the user + * tapped, and an arrow back to it. + * + * ## Why this exists as a component rather than three copies + * + * Until this file, no screen in the app had a top bar, a back arrow, or any + * navigation-controller call at all — a grep for `TopAppBar`, `navigationIcon` + * and `popBackStack` across the whole tree returned nothing. Every settings + * child relied on the system gesture, and App lock *looked* like it had a bar + * (a full-bleed Surface with a headline `Text` at the top) without having one. + * A user who reaches for the affordance she can see finds it is a label. + * + * Three copies of a bar would drift: one would get the ellipsis for long titles + * at font scale 2.0 and the others would wrap mid-word, which is the defect the + * tab labels already shipped once. One component cannot drift. + * + * ## Insets + * + * The bar declines window insets deliberately. `PeriodApp`'s `Scaffold` already + * pads its content region for the status bar and hands that down as + * `innerPadding`, so a bar that added `TopAppBarDefaults.windowInsets` here + * would pad it a second time and float below a gap. That makes this composable + * dependent on its host, which is worth saying out loud: it belongs inside that + * Scaffold, and putting it on a full-screen route would tuck the arrow under + * the status bar. + */ +@OptIn(ExperimentalMaterial3Api::class) +@Composable +fun SettingsSubpage( + title: String, + onBack: () -> Unit, + modifier: Modifier = Modifier, + content: @Composable () -> Unit, +) { + Surface(modifier = modifier.fillMaxSize()) { + Column(Modifier.fillMaxSize()) { + TopAppBar( + title = { + Text( + title, + maxLines = 1, + // Same reasoning as the tab labels: at the largest + // accessibility font scale Compose wraps mid-word, and + // "App loc / k" is worse than "App lock". + overflow = TextOverflow.Ellipsis, + // A screen reader can then jump to it, which is how + // somebody using one finds out where they are. + modifier = Modifier.semantics { heading() }, + ) + }, + navigationIcon = { + IconButton(onClick = onBack) { + Icon( + Icons.AutoMirrored.Filled.ArrowBack, + // AutoMirrored so the arrow turns around in a + // right-to-left layout rather than pointing the + // wrong way. + contentDescription = stringResource(R.string.action_back), + ) + } + }, + windowInsets = WindowInsets(0), + ) + Box(Modifier.weight(1f).fillMaxWidth()) { content() } + } + } +} diff --git a/app/src/main/kotlin/dev/privacyllc/period/feature/export/ExportHost.kt b/app/src/main/kotlin/dev/privacyllc/period/feature/export/ExportHost.kt index 86f4227..01d914e 100644 --- a/app/src/main/kotlin/dev/privacyllc/period/feature/export/ExportHost.kt +++ b/app/src/main/kotlin/dev/privacyllc/period/feature/export/ExportHost.kt @@ -2,6 +2,7 @@ package dev.privacyllc.period.feature.export import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.statusBarsPadding import androidx.compose.foundation.layout.padding import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Surface @@ -58,7 +59,15 @@ fun ExportHost(viewModel: ExportViewModel = hiltViewModel()) { } ?: return Surface(color = MaterialTheme.colorScheme.surfaceVariant) { - Column(Modifier.fillMaxWidth().padding(horizontal = 24.dp, vertical = 12.dp)) { + // The banner is the Scaffold's top bar, so nothing above it pads for the + // status bar: the Surface paints edge to edge and the text clears the + // clock itself. + Column( + Modifier + .fillMaxWidth() + .statusBarsPadding() + .padding(horizontal = 24.dp, vertical = 12.dp), + ) { Text( message, style = MaterialTheme.typography.bodyMedium, diff --git a/app/src/main/kotlin/dev/privacyllc/period/feature/export/ExportScreen.kt b/app/src/main/kotlin/dev/privacyllc/period/feature/export/ExportScreen.kt index 8996681..00803ec 100644 --- a/app/src/main/kotlin/dev/privacyllc/period/feature/export/ExportScreen.kt +++ b/app/src/main/kotlin/dev/privacyllc/period/feature/export/ExportScreen.kt @@ -23,6 +23,7 @@ import androidx.compose.ui.unit.dp import androidx.hilt.lifecycle.viewmodel.compose.hiltViewModel import androidx.lifecycle.compose.collectAsStateWithLifecycle import dev.privacyllc.period.designsystem.PeriodTheme +import dev.privacyllc.period.feature.common.SettingsSubpage /** * The disclosure, and one button. @@ -39,29 +40,30 @@ import dev.privacyllc.period.designsystem.PeriodTheme * result above the nav host instead, where they actually land. */ @Composable -fun ExportScreen(viewModel: ExportViewModel = hiltViewModel()) { +fun ExportScreen(onNavigateBack: () -> Unit, viewModel: ExportViewModel = hiltViewModel()) { val outcome by viewModel.outcome.collectAsStateWithLifecycle() ExportScreenContent( working = outcome == ExportOutcome.WRITING, onExport = viewModel::begin, + onNavigateBack = onNavigateBack, ) } @Composable -internal fun ExportScreenContent(working: Boolean, onExport: () -> Unit) { - Surface(modifier = Modifier.fillMaxSize()) { +internal fun ExportScreenContent( + working: Boolean, + onExport: () -> Unit, + onNavigateBack: () -> Unit = {}, +) { + // The title moves into the bar, which carries the heading semantics now — + // two headings saying the same thing is one for a screen reader to skip. + SettingsSubpage(title = ExportCopy.TITLE, onBack = onNavigateBack) { Column( modifier = Modifier .fillMaxSize() .verticalScroll(rememberScrollState()) .padding(24.dp), ) { - Text( - ExportCopy.TITLE, - style = MaterialTheme.typography.headlineMedium, - modifier = Modifier.semantics { heading() }, - ) - Section(ExportCopy.WHAT_HEADING, ExportCopy.WHAT_BODY) Section(ExportCopy.WHERE_HEADING, ExportCopy.WHERE_BODY) Section( diff --git a/app/src/main/kotlin/dev/privacyllc/period/feature/lock/LockSettingsScreen.kt b/app/src/main/kotlin/dev/privacyllc/period/feature/lock/LockSettingsScreen.kt index 27e74da..faa7d19 100644 --- a/app/src/main/kotlin/dev/privacyllc/period/feature/lock/LockSettingsScreen.kt +++ b/app/src/main/kotlin/dev/privacyllc/period/feature/lock/LockSettingsScreen.kt @@ -20,6 +20,7 @@ import androidx.compose.material3.Surface import androidx.compose.material3.Switch import androidx.compose.material3.Text import androidx.compose.material3.TextButton +import androidx.activity.compose.BackHandler import androidx.compose.runtime.Composable import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf @@ -39,6 +40,7 @@ import androidx.compose.ui.unit.dp import androidx.hilt.lifecycle.viewmodel.compose.hiltViewModel import androidx.lifecycle.compose.collectAsStateWithLifecycle import dev.privacyllc.period.designsystem.PeriodTheme +import dev.privacyllc.period.feature.common.SettingsSubpage private enum class Mode { OVERVIEW, SET_FIRST, CONFIRM_TO_REMOVE, CONFIRM_TO_CHANGE, SET_REPLACEMENT } @@ -52,25 +54,106 @@ private enum class Mode { OVERVIEW, SET_FIRST, CONFIRM_TO_REMOVE, CONFIRM_TO_CHA * nothing yet to authenticate against. */ @Composable -fun LockSettingsScreen(viewModel: LockSettingsViewModel = hiltViewModel()) { +fun LockSettingsScreen( + onNavigateBack: () -> Unit, + viewModel: LockSettingsViewModel = hiltViewModel(), +) { val state by viewModel.state.collectAsStateWithLifecycle() + LockSettingsContent( + state = state, + onNavigateBack = onNavigateBack, + onSetPin = viewModel::setPin, + onAuthoriseChange = viewModel::authoriseChange, + onCancelChange = viewModel::cancelChange, + onRemovePin = viewModel::removePin, + onBiometric = viewModel::setBiometricEnabled, + onDismissMessage = viewModel::clearMessage, + ) +} + +/** + * The screen without Hilt, so its back behaviour can be asserted. + * + * The steps below are remembered state rather than destinations, which is + * exactly why they need a test: nothing in the navigation graph knows they + * exist, so nothing but this file makes the arrow and the system gesture agree + * about them. + */ +@Composable +internal fun LockSettingsContent( + state: LockSettings, + onNavigateBack: () -> Unit, + onSetPin: (CharArray) -> Unit = {}, + onAuthoriseChange: (CharArray, () -> Unit) -> Unit = { _, _ -> }, + onCancelChange: () -> Unit = {}, + onRemovePin: (CharArray, () -> Unit) -> Unit = { _, _ -> }, + onBiometric: (Boolean) -> Unit = {}, + onDismissMessage: () -> Unit = {}, +) { var mode by remember { mutableStateOf(Mode.OVERVIEW) } + // One definition of "back", used by the arrow and by the system gesture. + // + // They have to agree. This screen has steps the navigation graph knows + // nothing about — `mode` is remembered state, not a destination — so system + // back from "choose a PIN" would otherwise leave App lock entirely while + // the arrow beside it stepped back one. Two controls a hand's width apart, + // doing different things. + val toOverview: () -> Unit = { + // Abandoning a step abandons the permission it was granted, too. + onCancelChange() + onDismissMessage() + mode = Mode.OVERVIEW + } + val stepBack: () -> Unit = { if (mode == Mode.OVERVIEW) onNavigateBack() else toOverview() } + + // Enabled only inside a step: at the Overview the gesture falls through to + // the NavHost, which pops the destination — exactly what the arrow does + // there. Nothing is swallowed. + BackHandler(enabled = mode != Mode.OVERVIEW, onBack = toOverview) + + SettingsSubpage(title = "App lock", onBack = stepBack) { + LockSettingsSteps( + mode = mode, + state = state, + onSetPin = onSetPin, + onAuthoriseChange = onAuthoriseChange, + onCancelChange = onCancelChange, + onRemovePin = onRemovePin, + onBiometric = onBiometric, + onDismissMessage = onDismissMessage, + setMode = { mode = it }, + ) + } +} + +@Composable +private fun LockSettingsSteps( + mode: Mode, + state: LockSettings, + onSetPin: (CharArray) -> Unit, + onAuthoriseChange: (CharArray, () -> Unit) -> Unit, + onCancelChange: () -> Unit, + onRemovePin: (CharArray, () -> Unit) -> Unit, + onBiometric: (Boolean) -> Unit, + onDismissMessage: () -> Unit, + setMode: (Mode) -> Unit, +) { when (mode) { Mode.OVERVIEW -> Overview( state = state, - onSetPin = { mode = Mode.SET_FIRST }, - onChangePin = { mode = Mode.CONFIRM_TO_CHANGE }, - onRemovePin = { mode = Mode.CONFIRM_TO_REMOVE }, - onBiometric = viewModel::setBiometricEnabled, - onDismissMessage = viewModel::clearMessage, + onSetPin = { setMode(Mode.SET_FIRST) }, + onChangePin = { setMode(Mode.CONFIRM_TO_CHANGE) }, + onRemovePin = { setMode(Mode.CONFIRM_TO_REMOVE) }, + onBiometric = onBiometric, + onDismissMessage = onDismissMessage, ) Mode.SET_FIRST, Mode.SET_REPLACEMENT -> PinSetupScreen( busy = state.busy, failed = state.message == LockSettings.Message.COULD_NOT_SET, - onCancel = { mode = Mode.OVERVIEW; viewModel.cancelChange(); viewModel.clearMessage() }, - onConfirmed = { pin -> viewModel.setPin(pin); mode = Mode.OVERVIEW }, + onCancel = { onCancelChange(); onDismissMessage(); setMode(Mode.OVERVIEW) }, + onConfirmed = { pin -> onSetPin(pin); setMode(Mode.OVERVIEW) }, ) // Both confirmations stay on screen until the PIN is actually right. @@ -86,16 +169,16 @@ fun LockSettingsScreen(viewModel: LockSettingsViewModel = hiltViewModel()) { title = "Enter your PIN to turn the lock off", busy = state.busy, wrong = state.message == LockSettings.Message.WRONG_PIN, - onCancel = { mode = Mode.OVERVIEW; viewModel.clearMessage() }, - onSubmit = { pin -> viewModel.removePin(pin) { mode = Mode.OVERVIEW } }, + onCancel = { onDismissMessage(); setMode(Mode.OVERVIEW) }, + onSubmit = { pin -> onRemovePin(pin) { setMode(Mode.OVERVIEW) } }, ) Mode.CONFIRM_TO_CHANGE -> ConfirmPin( title = "Enter your current PIN", busy = state.busy, wrong = state.message == LockSettings.Message.WRONG_PIN, - onCancel = { mode = Mode.OVERVIEW; viewModel.cancelChange(); viewModel.clearMessage() }, - onSubmit = { pin -> viewModel.authoriseChange(pin) { mode = Mode.SET_REPLACEMENT } }, + onCancel = { onCancelChange(); onDismissMessage(); setMode(Mode.OVERVIEW) }, + onSubmit = { pin -> onAuthoriseChange(pin) { setMode(Mode.SET_REPLACEMENT) } }, ) } } @@ -116,13 +199,9 @@ private fun Overview( .verticalScroll(rememberScrollState()) .padding(vertical = 16.dp), ) { - Text( - "App lock", - style = MaterialTheme.typography.headlineSmall, - modifier = Modifier.padding(horizontal = 24.dp), - ) - - Spacer(Modifier.height(8.dp)) + // No heading here: the bar above carries it, and it carries the + // heading semantics too. Two of them is one for a screen reader to + // wade through and one more thing to keep in step. Text( if (state.hasPin) { diff --git a/app/src/main/kotlin/dev/privacyllc/period/feature/settings/NotificationSettingsScreen.kt b/app/src/main/kotlin/dev/privacyllc/period/feature/settings/NotificationSettingsScreen.kt index b3bae52..c373353 100644 --- a/app/src/main/kotlin/dev/privacyllc/period/feature/settings/NotificationSettingsScreen.kt +++ b/app/src/main/kotlin/dev/privacyllc/period/feature/settings/NotificationSettingsScreen.kt @@ -36,6 +36,7 @@ import androidx.hilt.lifecycle.viewmodel.compose.hiltViewModel import androidx.lifecycle.compose.collectAsStateWithLifecycle import dev.privacyllc.period.core.datastore.NotificationPrivacy import dev.privacyllc.period.core.datastore.UserPreferences +import dev.privacyllc.period.feature.common.SettingsSubpage import dev.privacyllc.period.designsystem.PeriodTheme import java.time.LocalTime import java.time.format.DateTimeFormatter @@ -49,7 +50,10 @@ import java.time.format.DateTimeFormatter * the screen changed. */ @Composable -fun NotificationSettingsScreen(viewModel: NotificationSettingsViewModel = hiltViewModel()) { +fun NotificationSettingsScreen( + onNavigateBack: () -> Unit, + viewModel: NotificationSettingsViewModel = hiltViewModel(), +) { val prefs by viewModel.state.collectAsStateWithLifecycle() val needsPermission by viewModel.needsPermission.collectAsStateWithLifecycle() @@ -68,7 +72,11 @@ fun NotificationSettingsScreen(viewModel: NotificationSettingsViewModel = hiltVi } } - NotificationSettingsContent(prefs, viewModel) + // The title is the Settings row she tapped to get here, so the bar names + // where she is in the words she chose it by. + SettingsSubpage(title = "Reminders and privacy", onBack = onNavigateBack) { + NotificationSettingsContent(prefs, viewModel) + } } @Composable diff --git a/app/src/main/kotlin/dev/privacyllc/period/navigation/PeriodApp.kt b/app/src/main/kotlin/dev/privacyllc/period/navigation/PeriodApp.kt index 61345fe..3fe90e4 100644 --- a/app/src/main/kotlin/dev/privacyllc/period/navigation/PeriodApp.kt +++ b/app/src/main/kotlin/dev/privacyllc/period/navigation/PeriodApp.kt @@ -29,7 +29,9 @@ import androidx.compose.ui.tooling.preview.Preview import androidx.navigation.NavDestination.Companion.hierarchy import androidx.navigation.NavGraph.Companion.findStartDestination import androidx.navigation.compose.NavHost +import androidx.navigation.NavDestination import androidx.navigation.compose.composable +import androidx.navigation.compose.navigation import androidx.navigation.compose.currentBackStackEntryAsState import androidx.hilt.lifecycle.viewmodel.compose.hiltViewModel import androidx.lifecycle.compose.collectAsStateWithLifecycle @@ -93,20 +95,46 @@ fun PeriodApp() { val backStackEntry by navController.currentBackStackEntryAsState() val currentDestination = backStackEntry?.destination - ExportHost() + val inSettings = currentDestination.isIn(PeriodDestination.SETTINGS) Scaffold( + // The export result belongs above everything, including a sub-page's own + // bar. It used to be a sibling emitted BEFORE the Scaffold — and + // PeriodTheme's Surface is a Box, where later siblings draw on top of + // earlier ones, so an opaque Scaffold was painted over it. It has + // almost certainly never been seen; the export's on-device checks were + // never run either. + topBar = { ExportHost() }, bottomBar = { NavigationBar { PeriodDestination.entries.forEach { destination -> - val selected = currentDestination?.hierarchy?.any { it.route == destination.route } == true + val selected = currentDestination.isIn(destination) NavigationBarItem( selected = selected, onClick = { - navController.navigate(destination.route) { - popUpTo(navController.graph.findStartDestination().id) { saveState = true } - launchSingleTop = true - restoreState = true + if (selected) { + // Re-tapping the tab you are already on returns + // it to its root. For three tabs that is a + // no-op; for Settings it is the way out of a + // child, and the thing a user reaches for when + // a screen has no other exit. + navController.popBackStack(destination.rootRoute, inclusive = false) + } else { + navController.navigate(destination.route) { + popUpTo(navController.graph.findStartDestination().id) { + // Today, Calendar and Insights keep their + // place across a tab switch. Settings must + // not: its children are one-shot tasks, and + // a saved stack is restored on the way back + // in — which is how tapping Settings from + // inside App lock landed on App lock again, + // for the rest of the process. Nothing is + // saved, so there is nothing to restore. + saveState = !inSettings + } + launchSingleTop = true + restoreState = true + } } }, icon = { Icon(destination.icon, contentDescription = null) }, @@ -139,21 +167,32 @@ fun PeriodApp() { composable(PeriodDestination.CALENDAR.route) { CalendarScreen() } composable(PeriodDestination.INSIGHTS.route) { InsightsScreen() } - // Settings is a root with children, so the tab lands on the §36 - // tree and reminders live one level down. The tab item's own - // navigate() pops back to this start destination, which is what - // makes tapping Settings from inside reminders return here rather - // than doing nothing. - composable(PeriodDestination.SETTINGS.route) { - SettingsScreen( - onOpenNotifications = { navController.navigate(SETTINGS_NOTIFICATIONS) }, - onOpenAppLock = { navController.navigate(SETTINGS_LOCK) }, - onOpenExport = { navController.navigate(SETTINGS_EXPORT) }, - ) + // Settings is a nested graph, not a single destination. + // + // Two things fall out of that, and both were broken without it. The + // tab's route now names the graph, so every child is inside the + // tab's hierarchy and the tab renders as selected while you are on + // App lock — before, no tab looked selected on any settings child, + // and the user tapped an unselected Settings tab to escape. And the + // graph gives the re-tap above a root to pop back to. + navigation(route = PeriodDestination.SETTINGS.route, startDestination = SETTINGS_ROOT) { + composable(SETTINGS_ROOT) { + SettingsScreen( + onOpenNotifications = { navController.navigate(SETTINGS_NOTIFICATIONS) }, + onOpenAppLock = { navController.navigate(SETTINGS_LOCK) }, + onOpenExport = { navController.navigate(SETTINGS_EXPORT) }, + ) + } + composable(SETTINGS_NOTIFICATIONS) { + NotificationSettingsScreen(onNavigateBack = { navController.popBackStack() }) + } + composable(SETTINGS_LOCK) { + LockSettingsScreen(onNavigateBack = { navController.popBackStack() }) + } + composable(SETTINGS_EXPORT) { + ExportScreen(onNavigateBack = { navController.popBackStack() }) + } } - composable(SETTINGS_NOTIFICATIONS) { NotificationSettingsScreen() } - composable(SETTINGS_LOCK) { LockSettingsScreen() } - composable(SETTINGS_EXPORT) { ExportScreen() } } } } @@ -203,6 +242,12 @@ private fun PlaceholderPreview() { * screen itself may be long gone, because returning from the save dialog can * re-lock the app and the user unlocks onto Today. */ +/** + * The §36 tree itself. Distinct from the Settings *graph*, whose route is the + * tab's — one names where the tab points, the other what it shows first. + */ +private const val SETTINGS_ROOT = "settings/root" + /** Reminder settings, a child of the Settings tab rather than a fifth tab. */ private const val SETTINGS_NOTIFICATIONS = "settings/notifications" @@ -211,3 +256,16 @@ private const val SETTINGS_LOCK = "settings/lock" /** Export, above Delete in §36's order: the offer of a copy comes before the erase. */ private const val SETTINGS_EXPORT = "settings/export" + +/** True while [tab] is anywhere in this destination's hierarchy, children included. */ +private fun NavDestination?.isIn(tab: PeriodDestination) = + this?.hierarchy?.any { it.route == tab.route } == true + +/** + * Where re-selecting a tab returns to. + * + * Only Settings has children, so only Settings distinguishes the two; for the + * others the graph route and the screen route are the same thing. + */ +private val PeriodDestination.rootRoute + get() = if (this == PeriodDestination.SETTINGS) SETTINGS_ROOT else route diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 5115e13..22746c5 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -19,6 +19,9 @@ Insights Settings + + Back +