diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/SettingsPrefs.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/SettingsPrefs.kt index b0f2110..5fa676d 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/SettingsPrefs.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/SettingsPrefs.kt @@ -623,9 +623,8 @@ class SettingsPrefs @Inject constructor( /** * Close the wizard for good: the closing screen has been seen and the - * install stops counting as fresh, so a later revoke-and-re-grant of the - * calendar permission is only ever the permission screen. One edit, so the - * plan can never observe a half-closed wizard. + * install stops counting as fresh, so a later revoke-and-re-grant is only + * the permission screen. One edit, so the plan can't see it half-closed. */ suspend fun finishOnboardingWizard() { store.edit { prefs -> diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/RootScreen.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/RootScreen.kt index f5bd5cc..9b24e6b 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/RootScreen.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/RootScreen.kt @@ -91,9 +91,8 @@ fun RootScreen( val onboarding: OnboardingViewModel = hiltViewModel() val plan by onboarding.plan.collectAsStateWithLifecycle() // The grant is reported however it was taken, including via Android's - // app-settings screen (caught by the ON_RESUME above) — arming the wizard - // hangs off that, not off the callback. Only a grant made during this - // session owes the re-scan. + // app-settings screen (caught by the ON_RESUME above); arming the wizard + // hangs off that, not off the in-app callback. LaunchedEffect(hasPermission) { onboarding.setHasPermission(hasPermission) if (hasPermission && !grantedAtLaunch) onboarding.onPermissionGranted() @@ -112,9 +111,8 @@ fun RootScreen( else -> RootTarget.Step(current.current) } - // Which way the flow is moving, so a step-back slides back. The app sits - // after every step and the blank first frame before them, so finishing the - // wizard reads as one more move forward. + // Which way the flow is moving, so a step-back slides back. The app sorts + // after every step, so finishing the wizard reads as one more move forward. val ordinal = when (target) { RootTarget.Loading -> Int.MIN_VALUE RootTarget.App -> Int.MAX_VALUE @@ -124,17 +122,13 @@ fun RootScreen( val slideDir = if (ordinal < lastOrdinal) -1 else 1 SideEffect { lastOrdinal = ordinal } - // The wizard swipes: the steps sit side by side and the whole width travels, - // so the flow reads as moving through a stack of screens rather than as one - // screen redrawing itself. val slideSpec = rememberOnboardingSlideSpec() val fadeSpec = rememberCalendarFadeSpec() val reduceMotion = rememberReduceMotion() AnimatedContent( targetState = target, transitionSpec = { - // Coming off the blank first frame is not a step change — the app - // has only just opened, so the first screen simply appears. + // Coming off the blank first frame is not a step change. if (initialState == RootTarget.Loading) { fadeIn(fadeSpec).togetherWith(fadeOut(fadeSpec)) } else { @@ -164,9 +158,8 @@ fun RootScreen( requestedEditKey = requestedEditKey, onEditKeyConsumed = onEditKeyConsumed, ) - // The plan the outgoing step was drawn with is gone by the time the - // transition ends, so the live one carries both halves; each reads - // its own position out of it. + // Both halves of a transition read their position out of the live + // plan; the one the outgoing step was drawn with is already gone. is RootTarget.Step -> current?.let { plan -> OnboardingSteps( step = shown.step, diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/common/ViewPreview.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/common/ViewPreview.kt index 6ca3a1f..670734e 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/common/ViewPreview.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/common/ViewPreview.kt @@ -39,18 +39,14 @@ internal val PREVIEW_VIEWPORT_HEIGHT = 440.dp /** * Renders [content] at a full phone viewport and shrinks the finished layout - * into a box [height] tall, for the settings and onboarding choosers. + * into a box [height] tall, for the settings and onboarding choosers. Callers + * pass the *real* view composables, so a preview cannot drift from what it + * depicts. * - * Previews render the *real* view composables rather than a drawing of them, so - * they cannot drift from the thing they depict: change a cell's shape or an - * event block's colour and every preview follows. The trick is the [layout] - * below — it ignores the incoming constraints, so the view lays itself out at a - * plausible phone viewport, and a [graphicsLayer] scale shrinks the result. - * - * `Modifier.requiredSize` would be the obvious way to force the larger - * measurement, but it *centres* content that overflows the incoming constraints - * — which pushes the view to a negative offset and leaves only its bottom-right - * corner inside the clip. + * The [layout] below ignores the incoming constraints so the view measures at a + * phone viewport, and a [graphicsLayer] scale shrinks the result. + * `Modifier.requiredSize` is the obvious alternative but *centres* content that + * overflows its constraints, leaving only the bottom-right corner in the clip. */ @Composable internal fun ScaledViewPreview( @@ -99,13 +95,9 @@ internal fun ScaledViewPreview( /** * The stage every preview picker sets its preview on, cross-fading as [selected] - * changes. - * - * A preview draws the app's own surface, so on a chooser screen — which draws the - * same surface — it had no edge of its own and dissolved into the page. A tonal - * plate gives it one: the preview reads as a screen being shown rather than as - * part of the screen showing it. The plate takes the width and the outer radius - * of the grouped rows underneath, so the two read as one stack. + * changes. A preview draws the app's own surface, so a tonal plate gives it the + * edge it would otherwise lack on a chooser screen; the plate takes the width + * and outer radius of the grouped rows underneath so the two read as one stack. * * Only vertical spacing is the caller's to pass — the horizontal inset is the * rows' own, and the alignment is the point. diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/BackupStep.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/BackupStep.kt index ae6fa23..87627f1 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/BackupStep.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/BackupStep.kt @@ -28,9 +28,8 @@ import de.jeanlucmakiola.floret.components.OnboardingSpace * Wizard step shown only when everything you write to is a device-only calendar * (#163): the events exist nowhere but this phone, so losing it loses them. * - * The primary action picks a folder and switches automatic backup on in one go - * — the step changes something rather than pointing at Settings. Everything it - * sets is a normal Backup setting, so it stays adjustable afterwards. + * The primary action picks a folder and switches automatic backup on in one go. + * Everything it sets is a normal Backup setting, adjustable afterwards. */ @Composable internal fun BackupStep( @@ -43,8 +42,7 @@ internal fun BackupStep( val pickFolder = rememberLauncherForActivityResult( contract = ActivityResultContracts.OpenDocumentTree(), ) { uri -> - // Cancelling the picker leaves the step open: nothing was decided, and - // "Not now" is right there for the user who meant to decline. + // Cancelling the picker decides nothing and leaves the step open. uri?.let(onEnable) } diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/OnboardingMotion.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/OnboardingMotion.kt index cdeb156..eb26e04 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/OnboardingMotion.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/OnboardingMotion.kt @@ -28,12 +28,9 @@ internal fun rememberOnboardingSlideSpec(): FiniteAnimationSpec = * width swipes across, position on a spring so the arriving step settles with a * little give rather than stopping dead. * - * No cross-fade, unlike the calendar's shared-axis paging. Adjacent months are - * near-identical grids, so there the fade does the swapping and a short slide - * only hints the direction; here the two steps have nothing in common and fading - * one through the other read as a smear. Both steps are opaque and travel in - * lockstep, so a full-width slide shows one screen pushing the other off with no - * seam between them. + * No cross-fade, unlike the calendar's shared-axis paging: two steps have + * nothing in common, so fading one through the other smears. Both are opaque and + * travel in lockstep, showing one screen pushing the other off. * * @param slideDir +1 = forward (incoming from the right), -1 = back. * @param spec spatial spec, typically [rememberOnboardingSlideSpec]. diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/OnboardingPlan.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/OnboardingPlan.kt index 9846c35..ae820fe 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/OnboardingPlan.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/OnboardingPlan.kt @@ -55,31 +55,27 @@ data class OnboardingPlan( /** * Work out the flow from what is stored. * - * The optional steps are only for installs that are still working through their - * first run — an existing user sees nothing new. Before the grant that can only - * be guessed, so an install that has not answered the reminder step either is - * treated as fresh, which is the same condition + * The optional steps belong to installs still working through their first run. + * Before the grant that can only be guessed, so one that has not answered the + * reminder step either counts as fresh — the same condition * [de.jeanlucmakiola.calendula.data.prefs.SettingsPrefs.armOnboardingWizard] - * commits to storage a moment later. The flag is cleared again on the closing - * screen, so revoking and re-granting the permission years later is only ever - * the permission step. + * commits to storage a moment later, and clears again on the closing screen so + * a later re-grant is only the permission step. * * [backupApplies] is null until the calendar list can be read, i.e. for the - * whole permission step. The backup step is assumed to apply until proven + * whole permission step; the backup step is assumed to apply until proven * otherwise, so the flow can only ever get shorter — never sprout a step the * counter had not accounted for. * * The month-style step is *not* conditional on Month being the chosen view: - * Month is reachable from the drawer whatever opens first, and making the step - * appear and disappear as the view is picked would move the counter under the - * user on the step right before it. + * Month is reachable from the drawer whatever opens first, and a step that came + * and went as the view is picked would move the counter under the user on the + * step right before it. * * [visibilityArmed] carries the one-time notice that the device is hiding some - * calendars (#75). It is a step rather than a dialog over the app so that an - * install owing both meets it inside the flow instead of straight after it — - * and it is the only optional step an *existing* install can be given, which is - * why it is not gated on `fresh`. The closing screen is: there is nothing to - * conclude for someone who was only shown one notice. + * calendars (#75) — the only optional step an *existing* install can be given, + * which is why it is not gated on `fresh`, and on its own it earns no closing + * screen. */ fun onboardingPlan( hasPermission: Boolean, diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/OnboardingSteps.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/OnboardingSteps.kt index 91d25f4..2fecbb9 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/OnboardingSteps.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/OnboardingSteps.kt @@ -26,8 +26,7 @@ fun OnboardingSteps( modifier: Modifier = Modifier, ) { // Collected for every step, not just the two that draw a preview: starting - // it when the picker composes would leave the step blank for the frames the - // first stored emission takes to arrive — right as it slides in. + // it when the picker composes would leave that step blank as it slides in. val choice by viewModel.viewChoice.collectAsStateWithLifecycle() // Coerced because the outgoing half of a transition may be a step the live diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/OnboardingViewModel.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/OnboardingViewModel.kt index 9c9cefb..06e6472 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/OnboardingViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/OnboardingViewModel.kt @@ -60,9 +60,8 @@ class OnboardingViewModel @Inject constructor( /** * The grant as the rest of the flow sees it: published only once the wizard - * has been armed for it. Arming is a stored write, so announcing the grant - * first would leave a few frames in which the install looks neither fresh - * nor permission-less and the plan collapses to its one-step form. + * has been armed for it, since arming is a stored write and the frames + * before it lands would collapse the plan to its one-step form. */ private val hasPermission: StateFlow = permissionReported .mapLatest { granted -> @@ -78,9 +77,9 @@ class OnboardingViewModel @Inject constructor( /** * Whether the backup step applies: something of yours is worth exporting and * nothing you write to is synced anywhere. Null while the calendar list - * cannot be read, which is every frame before the grant — and any frame the - * provider read fails, since an empty list is also how a null cursor - * arrives. Guessing false there would drop the step for the whole session. + * cannot be read: every frame before the grant, and any read that fails — + * an empty list is also how a null cursor arrives, and guessing false there + * would drop the step for the whole session. */ private val backupApplies: Flow = hasPermission.flatMapLatest { granted -> if (granted != true) { @@ -152,8 +151,7 @@ class OnboardingViewModel @Inject constructor( * Arm the wizard for an install that has yet to answer the reminder step, * whatever route the grant took. Gating this on the in-app grant would * strand anyone who granted on Android's own settings screen and came back - * to a restarted process: they hold the permission at launch, so nothing - * would ever arm and the flow would silently shrink to the reminder step. + * to a restarted process, leaving them the reminder step alone. */ private suspend fun armWizardIfFresh() { if (!prefs.reminderOnboardingDone.first()) prefs.armOnboardingWizard() @@ -252,9 +250,8 @@ class OnboardingViewModel @Inject constructor( fun finishVisibility() { viewModelScope.launch { prefs.setOnboardingVisibilityDone() - // Retiring the notice drops the step from the plan, so it waits - // until nothing follows it — otherwise the count would fall from - // under the user on the way to the closing screen. + // Retiring the notice drops its step, so it waits until nothing + // follows — the count must not fall on the way to a later screen. if (plan.value?.steps?.last() == OnboardingStep.Visibility) retireVisibilityNotice() } } @@ -268,9 +265,9 @@ class OnboardingViewModel @Inject constructor( } /** - * Put the one-time visibility notice away for good. Nothing else clears it - * now that the wizard has taken the notice over, and left standing it would - * keep its step in every later plan — padding the counter on a re-grant. + * Put the one-time visibility notice away for good — nothing else clears it + * now that the wizard owns it, and left standing it would keep its step in + * every later plan. */ private suspend fun retireVisibilityNotice() { if (calendarPrefs.visibilityNoticePending.first() == true) { diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/ViewStep.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/ViewStep.kt index 4fd42cd..caf14e4 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/ViewStep.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/ViewStep.kt @@ -37,13 +37,9 @@ import de.jeanlucmakiola.floret.components.positionOf import de.jeanlucmakiola.floret.locale.currentLocale /** - * Wizard step: which view the app opens on (#163). - * - * Built like the Settings pickers it mirrors — a live preview over connected - * grouped rows — so the choice is made by looking rather than by reading four - * names and guessing, and so the same choice looks the same wherever it is - * made. Selecting applies immediately and leaves the step open; the preview - * above is the confirmation. + * Wizard step: which view the app opens on (#163). Built like the Settings + * picker it mirrors — a live preview over connected grouped rows. Selecting + * applies immediately and leaves the step open; the preview is the confirmation. */ @Composable internal fun ViewStep( @@ -163,8 +159,8 @@ private fun StepPickerScaffold( navigationIcon = navigationIcon, topSpacing = OnboardingSpace.xs, contentPadding = 0.dp, - // The preview is worth more than a screenful of certainty: it gets the - // room it needs and Continue scrolls in under the options with it. + // The preview gets the room it needs; Continue scrolls in under the + // options rather than pinning to the bottom. scrollingActions = true, hero = { Text( @@ -178,8 +174,8 @@ private fun StepPickerScaffold( actions = { Button( onClick = onFinished, - // The step is full-bleed, so the button takes the grouped rows' - // own inset and ends up as wide as the options above it. + // Full-bleed step: the button takes the grouped rows' own inset + // and ends up as wide as the options above it. modifier = Modifier .padding(horizontal = GroupedListInset) .fillMaxWidth() @@ -196,9 +192,8 @@ private fun StepPickerScaffold( } /** - * The live preview, framed like the picker's. The gap under it is wider than the - * one above: the rows below are the things to tap, and they need to look like a - * group of their own rather than the preview's caption. + * The live preview, framed like the picker's. The wider gap below keeps the rows + * reading as a group of their own rather than as the preview's caption. */ @Composable private fun StepPreview(selected: T, content: @Composable (T) -> Unit) { @@ -210,9 +205,5 @@ private fun StepPreview(selected: T, content: @Composable (T) -> Unit) { ) } -/** - * The same size the Settings chooser gives it. The step no longer tries to fit a - * screen: showing the view properly is what the step is for, and a picture too - * small to read would cost more than the scroll does. - */ +/** The same size the Settings chooser gives it. */ private val PREVIEW_HEIGHT: Dp = 280.dp diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/VisibilityStep.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/VisibilityStep.kt index b6cf7ee..d509766 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/VisibilityStep.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/onboarding/VisibilityStep.kt @@ -21,10 +21,8 @@ import de.jeanlucmakiola.floret.components.OnboardingSpace * Wizard step for the one-time notice that Calendula now follows the device's * own per-calendar visibility and some of yours are switched off (#75, #163). * - * It used to be a dialog thrown over the app once onboarding had finished, which - * put the one thing that explains a changed behaviour outside the flow that - * explains everything else. Nothing to decide here — the app does not switch - * those calendars back on, since that would un-hide them everywhere else too. + * Nothing to decide here — the app does not switch those calendars back on, + * since that would un-hide them everywhere else too. */ @Composable internal fun VisibilityStep( diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/ui/onboarding/OnboardingPlanTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/ui/onboarding/OnboardingPlanTest.kt index 0226ee8..fe0a521 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/ui/onboarding/OnboardingPlanTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/calendula/ui/onboarding/OnboardingPlanTest.kt @@ -106,8 +106,7 @@ class OnboardingPlanTest { @Test fun `re-granting after the wizard itself ran does not re-enter it`() { // The closing screen clears the armed flag, so someone the wizard did - // onboard is in the same position as anyone else who revokes the - // permission: one screen, no counter. + // onboard ends up where any other revoker does: one screen, no counter. val revoked = plan(remindersDone = true, wizardArmed = false, doneShown = true) assertThat(revoked.steps).containsExactly(OnboardingStep.Permission) assertThat(revoked.showsProgress).isFalse() diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 7b6e17a..d013789 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -97,10 +97,11 @@ step it is on, and `RootScreen` renders that step. Two rules make it work: counter under the user, and stepping *back* is just clearing the previous step's answer — there is no separate back stack. The calendar grant is the one step with nothing to return to, so back stops there. -- The optional steps (backup, view) only exist for an install that went - through the grant in-app — recorded by `armOnboardingWizard()`, which - refuses to arm if the reminder step was already answered, so an existing - user who revokes and re-grants the permission is not re-onboarded. +- The optional steps (backup, view) only exist for an install still on its + first run — `armOnboardingWizard()` arms on any launch holding the calendar + permission but refuses if the reminder step was already answered, and the + closing screen clears the flag again, so revoking and re-granting the + permission later never re-onboards anyone. Whether the backup step applies (nothing writable is synced anywhere) cannot be known before the grant, so it is assumed until the calendar list can be