From 350393c35268b994dccdbef60470bfd5e1a3a2e4 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Mon, 10 Aug 2026 16:49:08 +0200 Subject: [PATCH] Arm and retire the onboarding wizard from stored state (#163) Arming hung off the in-app grant callback, so a fresh install that granted on Android's settings screen and came back to a restarted process never got the wizard. It now arms on any launch holding the permission, before the grant reaches the plan, and clears again on the closing screen so a later re-grant is only the permission step. Also: retire the visibility notice once read, keep the backup step when the calendar read fails, and collect the view choice for every step. --- .../calendula/data/prefs/SettingsPrefs.kt | 22 +++++-- .../jeanlucmakiola/calendula/ui/RootScreen.kt | 6 +- .../calendula/ui/onboarding/OnboardingPlan.kt | 13 ++-- .../ui/onboarding/OnboardingSteps.kt | 47 +++++++------ .../ui/onboarding/OnboardingViewModel.kt | 66 ++++++++++++++++--- .../calendula/data/prefs/SettingsPrefsTest.kt | 26 ++++++++ .../ui/onboarding/OnboardingPlanTest.kt | 19 ++++++ 7 files changed, 156 insertions(+), 43 deletions(-) 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 16a2de3..b0f2110 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 @@ -556,10 +556,11 @@ class SettingsPrefs @Inject constructor( } /** - * Whether this install went through the calendar grant in-app, i.e. is a - * fresh one that owes the wizard's optional steps (#163). Set at the grant - * and only for an install that had not finished the reminder step, so an - * existing user who revokes and re-grants the permission isn't re-onboarded. + * Whether this install is a fresh one still owing the wizard's optional + * steps (#163). Armed on any launch that holds the calendar permission and + * has not finished the reminder step, and cleared again by + * [finishOnboardingWizard], so an existing user who revokes and re-grants + * the permission isn't re-onboarded. */ val onboardingWizardArmed: Flow = store.data.map { prefs -> prefs[ONBOARDING_WIZARD_ARMED_KEY] ?: false @@ -620,6 +621,19 @@ class SettingsPrefs @Inject constructor( store.edit { it[ONBOARDING_DONE_KEY] = shown } } + /** + * 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. + */ + suspend fun finishOnboardingWizard() { + store.edit { prefs -> + prefs[ONBOARDING_DONE_KEY] = true + prefs[ONBOARDING_WIZARD_ARMED_KEY] = false + } + } + /** * The default reminder lead times (minutes before start) prefilled on new * **timed** events. The empty list = no default reminder — the prior 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 51fcb10..f5bd5cc 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/RootScreen.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/RootScreen.kt @@ -90,8 +90,10 @@ fun RootScreen( val onboarding: OnboardingViewModel = hiltViewModel() val plan by onboarding.plan.collectAsStateWithLifecycle() - // Runs however the permission was granted, including via Android's - // app-settings screen (caught by the ON_RESUME above). + // 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. LaunchedEffect(hasPermission) { onboarding.setHasPermission(hasPermission) if (hasPermission && !grantedAtLaunch) onboarding.onPermissionGranted() 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 6fd893c..9846c35 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,11 +55,14 @@ data class OnboardingPlan( /** * Work out the flow from what is stored. * - * The optional steps are only for installs that went through the grant in-app — - * 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 [de.jeanlucmakiola.calendula.data.prefs.SettingsPrefs.armOnboardingWizard] - * commits to storage a moment later. + * 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 + * [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. * * [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 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 0ec1e2b..91d25f4 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 @@ -25,6 +25,11 @@ fun OnboardingSteps( onPermissionGranted: () -> Unit, 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. + val choice by viewModel.viewChoice.collectAsStateWithLifecycle() + // Coerced because the outgoing half of a transition may be a step the live // plan has since dropped — the backup step goes once the calendars say it // does not apply — and a "Step 0 of 3" flash is worse than a stale number. @@ -78,31 +83,25 @@ fun OnboardingSteps( ) // The choice is null only until DataStore's first emission; rendering // nothing for that frame beats a preview built on the wrong defaults. - OnboardingStep.View -> { - val choice by viewModel.viewChoice.collectAsStateWithLifecycle() - choice?.let { - ViewStep( - choice = it, - onSelect = viewModel::setDefaultView, - onFinished = viewModel::finishView, - modifier = modifier, - progress = progress, - navigationIcon = navigationIcon, - ) - } + OnboardingStep.View -> choice?.let { + ViewStep( + choice = it, + onSelect = viewModel::setDefaultView, + onFinished = viewModel::finishView, + modifier = modifier, + progress = progress, + navigationIcon = navigationIcon, + ) } - OnboardingStep.MonthStyle -> { - val choice by viewModel.viewChoice.collectAsStateWithLifecycle() - choice?.let { - MonthStyleStep( - choice = it, - onSelect = viewModel::setMonthViewStyle, - onFinished = viewModel::finishMonthStyle, - modifier = modifier, - progress = progress, - navigationIcon = navigationIcon, - ) - } + OnboardingStep.MonthStyle -> choice?.let { + MonthStyleStep( + choice = it, + onSelect = viewModel::setMonthViewStyle, + onFinished = viewModel::finishMonthStyle, + modifier = modifier, + progress = progress, + navigationIcon = navigationIcon, + ) } OnboardingStep.Visibility -> VisibilityStep( onFinished = viewModel::finishVisibility, 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 1c9f531..9c9cefb 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 @@ -31,6 +31,7 @@ import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.flowOn import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.mapLatest import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.launch import javax.inject.Inject @@ -55,20 +56,39 @@ class OnboardingViewModel @Inject constructor( ) : ViewModel() { /** Null until the host reports it — assuming either way would flash a screen. */ - private val hasPermission = MutableStateFlow(null) + private val permissionReported = MutableStateFlow(null) + + /** + * 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. + */ + private val hasPermission: StateFlow = permissionReported + .mapLatest { granted -> + if (granted == true) armWizardIfFresh() + granted + } + .stateIn( + scope = viewModelScope, + started = SharingStarted.Eagerly, + initialValue = null, + ) /** * 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. + * 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. */ private val backupApplies: Flow = hasPermission.flatMapLatest { granted -> if (granted != true) { flowOf(null) } else { repository.calendars() - .catch { emit(emptyList()) } - .map { calendars -> calendars.backupApplies() } + .map { calendars -> if (calendars.isEmpty()) null else calendars.backupApplies() } + .catch { emit(null) } .flowOn(io) } } @@ -125,7 +145,18 @@ class OnboardingViewModel @Inject constructor( ) fun setHasPermission(granted: Boolean) { - hasPermission.value = granted + permissionReported.value = granted + } + + /** + * 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. + */ + private suspend fun armWizardIfFresh() { + if (!prefs.reminderOnboardingDone.first()) prefs.armOnboardingWizard() } /** @@ -134,7 +165,6 @@ class OnboardingViewModel @Inject constructor( * daily worker. */ fun onPermissionGranted() { - viewModelScope.launch { prefs.armOnboardingWizard() } scanner.scanInBackground() } @@ -220,12 +250,32 @@ class OnboardingViewModel @Inject constructor( /** Acknowledge the calendar-visibility notice (#75). */ fun finishVisibility() { - viewModelScope.launch { prefs.setOnboardingVisibilityDone() } + 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. + if (plan.value?.steps?.last() == OnboardingStep.Visibility) retireVisibilityNotice() + } } /** Close the wizard from its last screen. */ fun finishOnboarding() { - viewModelScope.launch { prefs.setOnboardingDoneShown() } + viewModelScope.launch { + prefs.finishOnboardingWizard() + retireVisibilityNotice() + } + } + + /** + * 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. + */ + private suspend fun retireVisibilityNotice() { + if (calendarPrefs.visibilityNoticePending.first() == true) { + calendarPrefs.setVisibilityNoticePending(false) + } } /** diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/data/prefs/SettingsPrefsTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/data/prefs/SettingsPrefsTest.kt index 76eafa9..abe5922 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/data/prefs/SettingsPrefsTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/calendula/data/prefs/SettingsPrefsTest.kt @@ -337,6 +337,32 @@ class SettingsPrefsTest { assertThat(prefs.reminderOnboardingDone.first()).isTrue() } + @Test + fun `the wizard arms on a first run`(@TempDir tempDir: Path) = runTest { + val prefs = SettingsPrefs(newDataStore(tempDir)) + prefs.armOnboardingWizard() + assertThat(prefs.onboardingWizardArmed.first()).isTrue() + } + + @Test + fun `the wizard does not arm once the reminder step is answered`(@TempDir tempDir: Path) = runTest { + // An existing install re-granting the permission is not re-onboarded. + val prefs = SettingsPrefs(newDataStore(tempDir)) + prefs.setReminderOnboardingDone() + prefs.armOnboardingWizard() + assertThat(prefs.onboardingWizardArmed.first()).isFalse() + } + + @Test + fun `closing the wizard disarms it`(@TempDir tempDir: Path) = runTest { + val prefs = SettingsPrefs(newDataStore(tempDir)) + prefs.armOnboardingWizard() + + prefs.finishOnboardingWizard() + assertThat(prefs.onboardingDoneShown.first()).isTrue() + assertThat(prefs.onboardingWizardArmed.first()).isFalse() + } + @Test fun `default reminder is empty until set`(@TempDir tempDir: Path) = runTest { val prefs = SettingsPrefs(newDataStore(tempDir)) 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 d6475c2..0226ee8 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 @@ -103,6 +103,25 @@ class OnboardingPlanTest { assertThat(revoked.showsProgress).isFalse() } + @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. + val revoked = plan(remindersDone = true, wizardArmed = false, doneShown = true) + assertThat(revoked.steps).containsExactly(OnboardingStep.Permission) + assertThat(revoked.showsProgress).isFalse() + } + + @Test + fun `a read notice leaves nothing behind in a later plan`() { + // The notice is retired once read, so its step stops padding the count + // on a later re-grant. + val later = plan(remindersDone = true, visibilityArmed = false, visibilityDone = true) + assertThat(later.steps).containsExactly(OnboardingStep.Permission) + assertThat(later.showsProgress).isFalse() + } + @Test fun `answering every step ends the flow`() { val done = plan(