From 820317f9d90aa7300239934b5e03f0ba049a36d9 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Wed, 21 Oct 2026 19:24:52 +0200 Subject: [PATCH] fix(timers): tell a fresh install by its boot id and repair a garbled timer vibration An empty store was a racy sign of a fresh install: the UI can write the store (the notification-permission flag) before the startup settle runs, and a fresh install would then vibrate its timers. The boot id is written only by the engines' pass, which runs after the settle in the same coroutine, and an Auto Backup restore brings the old device's. Once settled, the startup pass costs one read and no package-manager call. A garbled stored value is replaced rather than left following the alarm default. Refs #22 --- .../de/jeanlucmakiola/clockula/ClockulaApp.kt | 10 ++-- .../clockula/data/prefs/SettingsPrefs.kt | 23 +++++--- .../clockula/data/prefs/ClockPrefsTest.kt | 4 +- .../clockula/data/prefs/SettingsPrefsTest.kt | 54 +++++++++++++++---- docs/ARCHITECTURE.md | 8 +-- 5 files changed, 72 insertions(+), 27 deletions(-) diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/ClockulaApp.kt b/app/src/main/java/de/jeanlucmakiola/clockula/ClockulaApp.kt index 5945f12..30c4d1f 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/ClockulaApp.kt +++ b/app/src/main/java/de/jeanlucmakiola/clockula/ClockulaApp.kt @@ -94,20 +94,20 @@ class ClockulaApp : Application() { } /** - * Ahead of the engines' pass, because the boot gate writes the store and - * "nothing written yet" is half of how a fresh install is told apart. Never + * Ahead of the engines' pass, because the boot gate writes the boot id, and + * its absence is half of how a fresh install is told apart. Never * in the way of that pass: a failure here leaves the setting unsettled, * which reads as the old behaviour (#22). */ private suspend fun settleTimerVibrate() { try { - settings.settleTimerVibrate(freshInstall = isFreshInstall()) + settings.settleTimerVibrate(freshInstall = ::isFreshInstall) } catch (cancelled: CancellationException) { throw cancelled } catch (_: Exception) { // Unsettled reads as the old behaviour. The next launch tries again, - // and by then the store is no longer empty, so even a fresh install - // settles on the alarm default: vibrating, the safe side to err on. + // and by then the boot id is there, so even a fresh install settles + // on the alarm default: vibrating, the safe side to err on. } } diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/data/prefs/SettingsPrefs.kt b/app/src/main/java/de/jeanlucmakiola/clockula/data/prefs/SettingsPrefs.kt index bed61f8..c1f81da 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/data/prefs/SettingsPrefs.kt +++ b/app/src/main/java/de/jeanlucmakiola/clockula/data/prefs/SettingsPrefs.kt @@ -127,22 +127,29 @@ class SettingsPrefs @Inject constructor(private val store: PrefStore) { * Gives the timers' vibration a value of its own, once (#22). Until then a * timer vibrated exactly when the alarm default said so, and an install * from before keeps that: the alarm default is copied, so changing it - * later no longer drags the timers along. Only a fresh install — a - * [freshInstall] package *and* a store nothing has written yet — starts - * with it off. Either signal alone mistakes something for fresh (an Auto - * Backup restore; a store replaced after corruption), and that mistake - * would silently stop someone's timers buzzing. + * later no longer drags the timers along. Only a fresh install starts with + * it off: a [freshInstall] package *and* a store without a boot id, which + * the engines write on every launch after this runs. Either signal alone + * mistakes something for fresh (an Auto Backup restore brings the old + * device's boot id; a store replaced after corruption is not a fresh + * package), and that mistake would silently stop someone's timers buzzing. + * The boot id rather than "empty" because the UI may write the store first. * * Best-effort by design: until it lands, and if it never does, the read * side's fallback keeps the old behaviour, so a failed write costs nothing. + * Once settled it is one read; [freshInstall] is asked only when needed. */ - suspend fun settleTimerVibrate(freshInstall: Boolean) { + suspend fun settleTimerVibrate(freshInstall: () -> Boolean) { + if (store.get(ClockPrefs.timerVibrate) != null) return // One transaction, so nothing written meanwhile can make the snapshot // it decides on stale. Raw keys for the reads: the kit reads a Pref // only from a store, not from a transaction's preferences. store.edit { prefs -> - if (stringPreferencesKey(ClockPrefs.timerVibrate.name) in prefs) return@edit - prefs[ClockPrefs.timerVibrate] = if (prefs.asMap().isEmpty() && freshInstall) { + val stored = prefs[stringPreferencesKey(ClockPrefs.timerVibrate.name)] + // Settled meanwhile; a garbled value falls through and is replaced. + if (stored?.toBooleanStrictOrNull() != null) return@edit + val neverRan = stringPreferencesKey(SystemPrefs.lastBootId.name) !in prefs + prefs[ClockPrefs.timerVibrate] = if (neverRan && freshInstall()) { false } else { prefs[booleanPreferencesKey(ClockPrefs.vibrate.name)] ?: ClockPrefs.vibrate.default diff --git a/app/src/test/java/de/jeanlucmakiola/clockula/data/prefs/ClockPrefsTest.kt b/app/src/test/java/de/jeanlucmakiola/clockula/data/prefs/ClockPrefsTest.kt index 8401734..fc833b0 100644 --- a/app/src/test/java/de/jeanlucmakiola/clockula/data/prefs/ClockPrefsTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/clockula/data/prefs/ClockPrefsTest.kt @@ -38,7 +38,7 @@ class ClockPrefsTest { fun `a fresh install reads the documented defaults`(@TempDir tempDir: Path) = runTest { val prefs = SettingsPrefs(PrefStore(newDataStore(tempDir))) // As ClockulaApp's startup pass does on a fresh install. - prefs.settleTimerVibrate(freshInstall = true) + prefs.settleTimerVibrate(freshInstall = { true }) assertThat(prefs.defaults.first()).isEqualTo(ClockDefaults()) } @@ -77,7 +77,7 @@ class ClockPrefsTest { @Test fun `the defaults do not re-emit when the same value is written again`(@TempDir tempDir: Path) = runTest { val prefs = SettingsPrefs(PrefStore(newDataStore(tempDir))) - prefs.settleTimerVibrate(freshInstall = true) + prefs.settleTimerVibrate(freshInstall = { true }) prefs.defaults.test { assertThat(awaitItem()).isEqualTo(ClockDefaults()) diff --git a/app/src/test/java/de/jeanlucmakiola/clockula/data/prefs/SettingsPrefsTest.kt b/app/src/test/java/de/jeanlucmakiola/clockula/data/prefs/SettingsPrefsTest.kt index bdffca6..c44f501 100644 --- a/app/src/test/java/de/jeanlucmakiola/clockula/data/prefs/SettingsPrefsTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/clockula/data/prefs/SettingsPrefsTest.kt @@ -185,7 +185,7 @@ class SettingsPrefsTest { fun `a fresh install settles timer vibration off`(@TempDir tempDir: Path) = runTest { val prefs = SettingsPrefs(PrefStore(newDataStore(tempDir))) - prefs.settleTimerVibrate(freshInstall = true) + prefs.settleTimerVibrate(freshInstall = { true }) assertThat(prefs.currentDefaults().timerVibrate).isFalse() assertThat(prefs.currentDefaults().vibrate).isTrue() @@ -196,7 +196,7 @@ class SettingsPrefsTest { val prefs = SettingsPrefs(PrefStore(newDataStore(tempDir))) prefs.setVibrate(false) - prefs.settleTimerVibrate(freshInstall = false) + prefs.settleTimerVibrate(freshInstall = { false }) // Settled, so the two now move apart. prefs.setVibrate(true) @@ -210,7 +210,7 @@ class SettingsPrefsTest { val prefs = SettingsPrefs(PrefStore(newDataStore(tempDir))) prefs.setSnoozeMinutes(7) - prefs.settleTimerVibrate(freshInstall = false) + prefs.settleTimerVibrate(freshInstall = { false }) prefs.setVibrate(false) assertThat(prefs.currentDefaults().timerVibrate).isTrue() @@ -221,28 +221,49 @@ class SettingsPrefsTest { // A store replaced after corruption, or never written: empty, but not new. val prefs = SettingsPrefs(PrefStore(newDataStore(tempDir))) - prefs.settleTimerVibrate(freshInstall = false) + prefs.settleTimerVibrate(freshInstall = { false }) assertThat(prefs.currentDefaults().timerVibrate).isTrue() } @Test fun `a new package with a restored store is not taken for fresh`(@TempDir tempDir: Path) = runTest { - // Auto Backup restored the old device's store before the first launch. - val prefs = SettingsPrefs(PrefStore(newDataStore(tempDir))) - prefs.setSnoozeMinutes(7) + // Auto Backup restored the old device's store, boot id and all. + val store = PrefStore(newDataStore(tempDir)) + val prefs = SettingsPrefs(store) + store.set(SystemPrefs.lastBootId, "old-device-boot") - prefs.settleTimerVibrate(freshInstall = true) + prefs.settleTimerVibrate(freshInstall = { true }) assertThat(prefs.currentDefaults().timerVibrate).isTrue() } + @Test + fun `a fresh install is still fresh when the UI wrote the store first`(@TempDir tempDir: Path) = runTest { + val prefs = SettingsPrefs(PrefStore(newDataStore(tempDir))) + prefs.markNotificationPermissionAsked() + + prefs.settleTimerVibrate(freshInstall = { true }) + + assertThat(prefs.currentDefaults().timerVibrate).isFalse() + } + + @Test + fun `once settled, the package is not asked again`(@TempDir tempDir: Path) = runTest { + val prefs = SettingsPrefs(PrefStore(newDataStore(tempDir))) + prefs.settleTimerVibrate(freshInstall = { true }) + + prefs.settleTimerVibrate(freshInstall = { error("asked again") }) + + assertThat(prefs.currentDefaults().timerVibrate).isFalse() + } + @Test fun `settling never overwrites a choice`(@TempDir tempDir: Path) = runTest { val prefs = SettingsPrefs(PrefStore(newDataStore(tempDir))) prefs.setTimerVibrate(true) - prefs.settleTimerVibrate(freshInstall = true) + prefs.settleTimerVibrate(freshInstall = { true }) assertThat(prefs.currentDefaults().timerVibrate).isTrue() } @@ -258,6 +279,21 @@ class SettingsPrefsTest { assertThat(prefs.currentDefaults().timerVibrate).isTrue() } + @Test + fun `a garbled timer vibration is settled like an absent one`(@TempDir tempDir: Path) = runTest { + val dataStore = newDataStore(tempDir) + val prefs = SettingsPrefs(PrefStore(dataStore)) + prefs.setVibrate(false) + dataStore.updateData { stored -> + stored.toMutablePreferences().apply { this[stringPreferencesKey("default_timer_vibrate")] = "maybe" } + } + + prefs.settleTimerVibrate(freshInstall = { false }) + prefs.setVibrate(true) + + assertThat(prefs.currentDefaults().timerVibrate).isFalse() + } + @Test fun `the timer ramp is clamped like the alarm ramp`(@TempDir tempDir: Path) = runTest { val prefs = SettingsPrefs(PrefStore(newDataStore(tempDir))) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index f87ee1d..7f5db0a 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -1913,9 +1913,11 @@ the setting existed had its timers vibrate exactly when the alarm default did, and keeps that. The key is nullable: absent reads as the alarm default, and `SettingsPrefs.settleTimerVibrate` writes a value of its own once, at the head of `ClockulaApp`'s startup pass (before the boot gate writes the store). Only a -package never updated since install *and* an empty store count as fresh and -get `false`; anything else (an update, an Auto Backup restore, a store replaced -after corruption) gets the alarm default copied. It is deliberately not a +package never updated since install *and* a store without `last_boot_id` +(the engines write it on every launch, after the settle) count as fresh and +get `false`; anything else (an update, an Auto Backup restore, which brings the +old boot id, a store replaced after corruption) gets the alarm default copied. +The boot id rather than an empty store, because the UI may write first. It is deliberately not a DataStore migration: a migration's write runs inside the store's initialisation, and a failed one would fail every read, the alarm path's included. A failed settle only leaves the fallback in place.