From 8910ab7fbf12f89683cb99e409d3825de40cd447 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Wed, 21 Oct 2026 18:56:07 +0200 Subject: [PATCH] fix(timers): keep a ramp from restarting mid-ring and a migration from guessing A timer ring that hands over from one expired timer to another now starts at full volume instead of ramping again from near silence. The timer-vibrate migration only treats an install as fresh when the package was never updated and the store is empty. An updated install whose store was replaced after corruption, or an Auto Backup restore onto a new device, keeps the alarm default as before. The Vibrate row now says whether it is on, and only when it is off adds that a silent timer still vibrates. Refs #22 --- .../clockula/data/di/DataModule.kt | 14 +++++++- .../data/prefs/TimerVibrateMigration.kt | 22 +++++++----- .../clockula/domain/timer/TimerSettings.kt | 9 +++++ .../clockula/timer/service/TimerService.kt | 4 ++- .../ui/settings/TimerDefaultsScreen.kt | 11 ++++-- app/src/main/res/values/strings.xml | 2 +- .../clockula/data/di/DataModuleTest.kt | 1 + .../data/prefs/TimerVibrateMigrationTest.kt | 34 +++++++++++++++---- .../domain/timer/TimerRingPolicyTest.kt | 8 +++++ docs/ARCHITECTURE.md | 8 +++-- 10 files changed, 90 insertions(+), 23 deletions(-) diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/data/di/DataModule.kt b/app/src/main/java/de/jeanlucmakiola/clockula/data/di/DataModule.kt index 707addc..2fb730b 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/data/di/DataModule.kt +++ b/app/src/main/java/de/jeanlucmakiola/clockula/data/di/DataModule.kt @@ -46,8 +46,18 @@ object DataModule { ): DataStore = createDataStore( scope = CoroutineScope(io + SupervisorJob()), produceFile = { context.preferencesDataStoreFile(PREFS_NAME) }, + isFreshInstall = { isFreshInstall(context) }, ) + /** + * Never updated since it was installed. Unknown reads as not fresh, which + * is the migrations' safe side: it keeps what an install already did. + */ + private fun isFreshInstall(context: Context): Boolean = runCatching { + val info = context.packageManager.getPackageInfo(context.packageName, 0) + info.firstInstallTime == info.lastUpdateTime + }.getOrDefault(false) + @Provides @Singleton fun providePrefStore(dataStore: DataStore): PrefStore = PrefStore(dataStore) @@ -65,13 +75,15 @@ object DataModule { * the alternative is an app that can only be fixed by clearing its data. * * Migrations run before the first read, so nothing ever sees a key they fill. + * [isFreshInstall] is asked only by them, and only once. */ internal fun createDataStore( scope: CoroutineScope, produceFile: () -> File, + isFreshInstall: () -> Boolean, ): DataStore = PreferenceDataStoreFactory.create( corruptionHandler = ReplaceFileCorruptionHandler { emptyPreferences() }, - migrations = listOf(TimerVibrateMigration), + migrations = listOf(TimerVibrateMigration(isFreshInstall)), scope = scope, produceFile = produceFile, ) diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/data/prefs/TimerVibrateMigration.kt b/app/src/main/java/de/jeanlucmakiola/clockula/data/prefs/TimerVibrateMigration.kt index b31291c..0c82ba8 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/data/prefs/TimerVibrateMigration.kt +++ b/app/src/main/java/de/jeanlucmakiola/clockula/data/prefs/TimerVibrateMigration.kt @@ -8,15 +8,21 @@ import de.jeanlucmakiola.floret.prefs.set /** * Gives an install from before #22 a timer-vibrate setting that keeps its * timers doing what they did: until then a timer vibrated exactly when the - * alarm default did, so that is the value written. A fresh install — an empty - * store, since anything that has run once has at least written its boot id — - * gets the setting's own default, off. + * alarm default did, so that is the value written. Only a fresh install gets + * the setting's own default, off. * - * The value is always written, the default included, so this runs once: left - * absent on a fresh install, the next launch would see a non-empty store and - * copy the alarm default after all. + * Fresh means both: the package was never updated ([isFreshInstall], from the + * package manager) **and** the store is empty. The first alone would take an + * Auto Backup restore onto a new device for fresh; the second alone would take + * an updated install whose store was replaced after corruption, or never + * written, for fresh. Either mistake silently stops someone's timers buzzing, + * so when in doubt this keeps the old behaviour. + * + * The value is always written, the default included, so this runs once. */ -internal object TimerVibrateMigration : DataMigration { +internal class TimerVibrateMigration( + private val isFreshInstall: () -> Boolean, +) : DataMigration { private val timerVibrateKey = booleanPreferencesKey(ClockPrefs.timerVibrate.name) private val alarmVibrateKey = booleanPreferencesKey(ClockPrefs.vibrate.name) @@ -24,7 +30,7 @@ internal object TimerVibrateMigration : DataMigration { override suspend fun shouldMigrate(currentData: Preferences): Boolean = timerVibrateKey !in currentData override suspend fun migrate(currentData: Preferences): Preferences { - val inherited = if (currentData.asMap().isEmpty()) { + val inherited = if (currentData.asMap().isEmpty() && isFreshInstall()) { ClockPrefs.timerVibrate.default } else { currentData[alarmVibrateKey] ?: ClockPrefs.vibrate.default diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/domain/timer/TimerSettings.kt b/app/src/main/java/de/jeanlucmakiola/clockula/domain/timer/TimerSettings.kt index 91833b2..43cd22d 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/domain/timer/TimerSettings.kt +++ b/app/src/main/java/de/jeanlucmakiola/clockula/domain/timer/TimerSettings.kt @@ -20,3 +20,12 @@ fun Timer.resolveSettings(defaults: ClockDefaults): TimerSettings = TimerSetting vibrate = defaults.timerVibrate, rampSeconds = defaults.timerVolumeRampSeconds, ) + +/** + * The ramp a ring actually starts with. A [handover] — the sounding timer + * changes while the ring goes on, because a second one expired and sorts first + * or the first was stopped with another still expired — starts at full volume: + * the user has already heard the ring, and starting the ramp over would drop it + * to a whisper partway through. + */ +fun TimerSettings.rampSecondsFor(handover: Boolean): Int = if (handover) 0 else rampSeconds diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/timer/service/TimerService.kt b/app/src/main/java/de/jeanlucmakiola/clockula/timer/service/TimerService.kt index 5010fbf..ea103e6 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/timer/service/TimerService.kt +++ b/app/src/main/java/de/jeanlucmakiola/clockula/timer/service/TimerService.kt @@ -17,6 +17,7 @@ import de.jeanlucmakiola.clockula.domain.ring.RingFallbackPolicy import de.jeanlucmakiola.clockula.domain.timer.TimerNotificationState import de.jeanlucmakiola.clockula.domain.timer.TimerMode import de.jeanlucmakiola.clockula.domain.timer.TimerRingDecision +import de.jeanlucmakiola.clockula.domain.timer.rampSecondsFor import de.jeanlucmakiola.clockula.ring.RingAudioPlayer import de.jeanlucmakiola.clockula.ring.RingVibrator import de.jeanlucmakiola.clockula.timer.AlarmRingStatus @@ -228,6 +229,7 @@ class TimerService : Service() { // An unchanged decision does nothing: restarting the player on // every emission would stutter the ring. if (sounding == decision.subjectId) return + val handover = sounding != null releaseRing() sounding = decision.subjectId acquireWakeLock() @@ -240,7 +242,7 @@ class TimerService : Service() { audio.start( scope = scope, ringtoneUri = decision.settings.ringtoneUri, - rampSeconds = decision.settings.rampSeconds, + rampSeconds = decision.settings.rampSecondsFor(handover), // Silence is not one of the outcomes: with no audio source at // all the timer vibrates even when vibration is switched off. onSilenced = vibrator::start, diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/TimerDefaultsScreen.kt b/app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/TimerDefaultsScreen.kt index b3f25d9..9130302 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/TimerDefaultsScreen.kt +++ b/app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/TimerDefaultsScreen.kt @@ -13,6 +13,7 @@ import androidx.compose.ui.res.stringResource import androidx.compose.ui.unit.dp import de.jeanlucmakiola.clockula.R import de.jeanlucmakiola.clockula.ui.alarms.VolumeRampPicker +import de.jeanlucmakiola.clockula.ui.alarms.vibrateText import de.jeanlucmakiola.clockula.ui.alarms.volumeRampText import de.jeanlucmakiola.floret.components.CollapsingScaffold import de.jeanlucmakiola.floret.components.GroupedRow @@ -44,11 +45,15 @@ internal fun TimerDefaultsScreen( onClick = { viewModel.onOpenRingtonePicker(DefaultRingtoneKind.TIMER) }, ) // A switch, not the alarm screen's picker: there is no per-timer - // override for it to pick "app default" against. The summary says what - // the ring does with Silent and vibration both off — vibrates anyway. + // override for it to pick "app default" against. Off, the summary says + // what the ring does with Silent and vibration both off: vibrates anyway. GroupedRow( title = stringResource(R.string.editor_vibrate), - summary = stringResource(R.string.settings_timer_vibrate_hint), + summary = if (defaults.timerVibrate) { + vibrateText(on = true) + } else { + stringResource(R.string.settings_timer_vibrate_off) + }, position = Position.Middle, trailing = { Switch( diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index c9117ae..f1c70a3 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -300,7 +300,7 @@ Timers Sound, vibration and gradual volume How timers ring. A timer can still pick its own sound in its editor. - A timer set to silent vibrates either way + Off. A timer set to Silent still vibrates Clock Home time zone World clocks show how far each city is from your home time zone. diff --git a/app/src/test/java/de/jeanlucmakiola/clockula/data/di/DataModuleTest.kt b/app/src/test/java/de/jeanlucmakiola/clockula/data/di/DataModuleTest.kt index 46a4cdc..df5d4a9 100644 --- a/app/src/test/java/de/jeanlucmakiola/clockula/data/di/DataModuleTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/clockula/data/di/DataModuleTest.kt @@ -25,6 +25,7 @@ class DataModuleTest { private fun TestScope.newStore(file: File) = createDataStore( scope = CoroutineScope(UnconfinedTestDispatcher(testScheduler) + Job()), produceFile = { file }, + isFreshInstall = { false }, ) @Test diff --git a/app/src/test/java/de/jeanlucmakiola/clockula/data/prefs/TimerVibrateMigrationTest.kt b/app/src/test/java/de/jeanlucmakiola/clockula/data/prefs/TimerVibrateMigrationTest.kt index ba2832e..dd55e95 100644 --- a/app/src/test/java/de/jeanlucmakiola/clockula/data/prefs/TimerVibrateMigrationTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/clockula/data/prefs/TimerVibrateMigrationTest.kt @@ -42,12 +42,18 @@ class TimerVibrateMigrationTest { scope.coroutineContext.job.cancelAndJoin() } - private suspend fun TestScope.openPrefs(tempDir: Path, scope: CoroutineScope = newScope()): SettingsPrefs = - SettingsPrefs(PrefStore(createDataStore(scope = scope, produceFile = { file(tempDir) }))) + /** [freshInstall] is the package manager's half: never updated since installed. */ + private fun TestScope.openPrefs( + tempDir: Path, + freshInstall: Boolean = false, + scope: CoroutineScope = newScope(), + ): SettingsPrefs = SettingsPrefs( + PrefStore(createDataStore(scope = scope, produceFile = { file(tempDir) }, isFreshInstall = { freshInstall })), + ) @Test fun `a fresh install does not vibrate its timers`(@TempDir tempDir: Path) = runTest { - val prefs = openPrefs(tempDir) + val prefs = openPrefs(tempDir, freshInstall = true) assertThat(prefs.currentDefaults().timerVibrate).isFalse() assertThat(prefs.currentDefaults().vibrate).isTrue() @@ -58,10 +64,11 @@ class TimerVibrateMigrationTest { @TempDir tempDir: Path, ) = runTest { val firstLaunch = newScope() - openPrefs(tempDir, firstLaunch).setSnoozeMinutes(7) + openPrefs(tempDir, freshInstall = true, scope = firstLaunch).setSnoozeMinutes(7) firstLaunch.coroutineContext.job.cancelAndJoin() - val prefs = openPrefs(tempDir) + // Updated since, too: by now the key is there, and nothing asks again. + val prefs = openPrefs(tempDir, freshInstall = false) assertThat(prefs.currentDefaults().timerVibrate).isFalse() } @@ -89,6 +96,21 @@ class TimerVibrateMigrationTest { assertThat(openPrefs(tempDir).currentDefaults().timerVibrate).isFalse() } + @Test + fun `an updated install with an empty store keeps vibrating its timers`(@TempDir tempDir: Path) = runTest { + // A store replaced after corruption, or never written: empty, but not new. + val prefs = openPrefs(tempDir, freshInstall = false) + + assertThat(prefs.currentDefaults().timerVibrate).isTrue() + } + + @Test + fun `a new install restored from Auto Backup keeps what the old device did`(@TempDir tempDir: Path) = runTest { + writeOldInstall(tempDir, alarmVibrate = true) + + assertThat(openPrefs(tempDir, freshInstall = true).currentDefaults().timerVibrate).isTrue() + } + @Test fun `once migrated, the two settings move independently`(@TempDir tempDir: Path) = runTest { writeOldInstall(tempDir, alarmVibrate = true) @@ -102,7 +124,7 @@ class TimerVibrateMigrationTest { @Test fun `a choice the user made is never overwritten`(@TempDir tempDir: Path) = runTest { val firstLaunch = newScope() - openPrefs(tempDir, firstLaunch).setTimerVibrate(true) + openPrefs(tempDir, freshInstall = true, scope = firstLaunch).setTimerVibrate(true) firstLaunch.coroutineContext.job.cancelAndJoin() assertThat(openPrefs(tempDir).currentDefaults().timerVibrate).isTrue() diff --git a/app/src/test/java/de/jeanlucmakiola/clockula/domain/timer/TimerRingPolicyTest.kt b/app/src/test/java/de/jeanlucmakiola/clockula/domain/timer/TimerRingPolicyTest.kt index 43ae2ab..d033b1e 100644 --- a/app/src/test/java/de/jeanlucmakiola/clockula/domain/timer/TimerRingPolicyTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/clockula/domain/timer/TimerRingPolicyTest.kt @@ -102,6 +102,14 @@ class TimerRingPolicyTest { assertThat(ramped.sound().settings.rampSeconds).isEqualTo(20) } + @Test + fun `a new ring ramps, a handover between timers mid-ring does not`() { + val settings = decide(defaults = defaults.copy(timerVolumeRampSeconds = 20)).sound().settings + + assertThat(settings.rampSecondsFor(handover = false)).isEqualTo(20) + assertThat(settings.rampSecondsFor(handover = true)).isEqualTo(0) + } + /** §5.4 #8 */ @Test fun `a session already sounding is not restarted`() { diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 0366fa3..c2ec184 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -1911,9 +1911,11 @@ alarm that should buzz the bed and a kitchen timer that should only beep are different questions. A fresh install has it **off**. An install from before the setting existed had its timers vibrate exactly when the alarm default did, so `TimerVibrateMigration` (a DataStore migration, run before the first read) -writes that value once rather than silently changing what its timers do; it -tells a fresh install by an empty store, since anything that has run has -written its boot id. Silent with vibration off is still not silence: +writes that value once rather than silently changing what its timers do. Only +a package never updated since install *and* an empty store count as fresh; +anything else (an update, an Auto Backup restore, a store replaced after +corruption) keeps the old behaviour. A ring that hands over from one expired +timer to another mid-ring does not start the ramp again. Silent with vibration off is still not silence: `AudioSourcePolicy` gives Silent no sources, and the player's `onSilenced` starts the vibrator regardless of the setting.