From d2bf4349c54d5f889ac62a22cdf005ed52465059 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Wed, 21 Oct 2026 19:18:19 +0200 Subject: [PATCH] fix(alarms): never announce an alarm that has since stopped ringing A notice is now re-checked against the engine before a screen shows it, so an alarm switched off or deleted by any path - the ring screen, a notification, an intent, a restore - is never announced as set. A retract takes the publish lock, so a publish already past its read cannot put the notice back. Settling is per alarm: switching one alarm off in the list no longer cancels the repeat days still settling on another. The identical-words dedupe is gone, since it could swallow a real confirmation. The editor's writes that move a fire time, with their reschedule and announcement, run on the application scope. Pick, OK, back could otherwise cancel the reschedule half-way and leave a stored time the system had not been given. upcomingFor() reads alarm and state under the engine lock, so a fire cannot land between the two reads. Refs #19 --- .../clockula/alarm/AlarmEngine.kt | 17 +++-- .../ui/alarms/AlarmEditorViewModel.kt | 16 +++-- .../clockula/ui/alarms/AlarmSetSnackbar.kt | 65 ++++++++++++------- .../ui/alarms/AlarmEditorViewModelTest.kt | 12 ++-- .../clockula/ui/alarms/AlarmsViewModelTest.kt | 27 +++++++- docs/ARCHITECTURE.md | 16 +++-- 6 files changed, 100 insertions(+), 53 deletions(-) diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/alarm/AlarmEngine.kt b/app/src/main/java/de/jeanlucmakiola/clockula/alarm/AlarmEngine.kt index 070b660..20b849f 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/alarm/AlarmEngine.kt +++ b/app/src/main/java/de/jeanlucmakiola/clockula/alarm/AlarmEngine.kt @@ -197,13 +197,18 @@ class AlarmEngine @Inject constructor( /** * One alarm's [upcoming] entry, resolved against [now]. Read-only like - * [upcoming], and for the same reason it takes no lock: it writes nothing. - * Null for an id no alarm has. A caller that measures a countdown passes - * the `now` it measures from, so both ends of it are the same instant. + * [upcoming]; null for an id no alarm has. A caller that measures a + * countdown passes the `now` it measures from, so both ends of it are the + * same instant. + * + * It writes nothing, but it does take [lock]: the alarm and its ring state + * are two reads, and a fire landing between them would pair an alarm with + * a state that is not its own. `upcoming()` gets the same consistency from + * combining the two flows. */ - suspend fun upcomingFor(alarmId: Long, now: Instant = wallClock.now()): UpcomingAlarm? { - val alarm = alarms.find(alarmId) ?: return null - return preview(alarm, states.state(alarmId), now, zones.current()) + suspend fun upcomingFor(alarmId: Long, now: Instant = wallClock.now()): UpcomingAlarm? = lock.withLock { + val alarm = alarms.find(alarmId) ?: return@withLock null + preview(alarm, states.state(alarmId), now, zones.current()) } /** diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/ui/alarms/AlarmEditorViewModel.kt b/app/src/main/java/de/jeanlucmakiola/clockula/ui/alarms/AlarmEditorViewModel.kt index 3f495eb..c525fff 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/ui/alarms/AlarmEditorViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/clockula/ui/alarms/AlarmEditorViewModel.kt @@ -146,7 +146,9 @@ class AlarmEditorViewModel @Inject internal constructor( } fun onSkipNextChanged(skip: Boolean) { - viewModelScope.launch { + // On the application scope, like every write that moves a fire time: + // see [edit]. + appScope.launch { alarms.setSkipNextOccurrence(alarmId, skip) engine.reschedule() // Skipping moves the next ring a day or more out, which is exactly @@ -265,9 +267,13 @@ class AlarmEditorViewModel @Inject internal constructor( /** * Every editor write. [reschedule] is on only for a change that can move a * fire time — so fiddling with sound settings costs no AlarmManager traffic - * (M5 D12). [announces] is on for the same changes, after [settle] — on - * the announcer's application scope, so leaving the editor while the - * repeat days settle still has them said, for the list to show (#19). + * (M5 D12). [announces] is on for the same changes, after [settle] (#19). + * + * A change that moves a fire time runs on the application scope. Pick → + * OK → back is the usual way to set an alarm, and back clears this + * ViewModel: on its own scope the reschedule — and the "Alarm set for …" + * the list is meant to pick up — could be cancelled half-way, leaving a + * stored time the system had not been given yet. */ private fun edit( reschedule: Boolean = false, @@ -275,7 +281,7 @@ class AlarmEditorViewModel @Inject internal constructor( settle: Duration = Duration.ZERO, transform: (Alarm) -> Alarm, ) { - viewModelScope.launch { + (if (reschedule) appScope else viewModelScope).launch { alarms.edit(alarmId, transform) if (reschedule) engine.reschedule() if (announces) announcer.announce(alarmId, settle) diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/ui/alarms/AlarmSetSnackbar.kt b/app/src/main/java/de/jeanlucmakiola/clockula/ui/alarms/AlarmSetSnackbar.kt index f9a8a55..bfb3df4 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/ui/alarms/AlarmSetSnackbar.kt +++ b/app/src/main/java/de/jeanlucmakiola/clockula/ui/alarms/AlarmSetSnackbar.kt @@ -74,17 +74,23 @@ internal class AlarmSetAnnouncer @Inject constructor( */ private val lock = Mutex() - /** The request still settling, and the alarm it is about. Guarded by `this`. */ - private var pending: Job? = null - private var pendingFor: Long? = null + /** + * Requests still settling, one per alarm: a request about one alarm must + * not cancel another's — switching B off in the list would otherwise + * swallow the repeat days just set on A. Guarded by `this`. + */ + private val pending = mutableMapOf() /** * The notice to show, or null. Past [FRESH_FOR] it is dropped unshown: a * countdown that surfaced when the user came back to the app a while later - * would be wrong, and about something they did long ago. + * would be wrong, and about something they did long ago. It is checked + * against the engine again on the way out, so an alarm switched off or + * deleted by any path — the ring screen, a notification, an intent, a + * restore — is never announced as set. */ val notice: Flow = current.map { notice -> - notice?.takeIf { elapsed.elapsedRealtime() - it.announcedAt < FRESH_FOR } + notice?.takeIf { isFresh(it) && stillRings(it) } } /** @@ -96,25 +102,26 @@ internal class AlarmSetAnnouncer @Inject constructor( */ fun announce(alarmId: Long, settle: Duration = Duration.ZERO) { synchronized(this) { - pending?.cancel() - pendingFor = alarmId - pending = appScope.launch { + pending.remove(alarmId)?.cancel() + val job = appScope.launch { delay(settle) publish(alarmId) } + pending[alarmId] = job + job.invokeOnCompletion { synchronized(this) { pending.remove(alarmId, job) } } } } - /** Takes back anything said or about to be said about [alarmId]: it was deleted or dismissed. */ - fun retract(alarmId: Long) { - synchronized(this) { - if (pendingFor == alarmId) { - pending?.cancel() - pending = null - pendingFor = null - } + /** + * Takes back anything said or about to be said about [alarmId]: it was + * deleted or dismissed. Under the same lock as [publish], so a publish + * already past its read cannot put the notice back afterwards. + */ + suspend fun retract(alarmId: Long) { + synchronized(this) { pending.remove(alarmId)?.cancel() } + lock.withLock { + current.update { notice -> notice?.takeUnless { it.alarmId == alarmId } } } - current.update { notice -> notice?.takeUnless { it.alarmId == alarmId } } } /** [notice] was shown until it timed out or was swiped away. */ @@ -123,15 +130,9 @@ internal class AlarmSetAnnouncer @Inject constructor( } private suspend fun publish(alarmId: Long) = lock.withLock { - // One `now` for both ends of the countdown, or a resolve that lands a - // hair past the occurrence would read as a day and an hour. - val now = wall.now() - val countdown = AlarmSetCountdown.of(engine.upcomingFor(alarmId, now), now) + val countdown = countdownOf(alarmId) current.update { standing -> when { - // The same words about the same alarm stay as they are, rather - // than taking the snackbar down and putting it straight back. - countdown != null && standing?.alarmId == alarmId && standing.countdown == countdown -> standing countdown != null -> AlarmSetNotice(alarmId, countdown, elapsed.elapsedRealtime()) standing?.alarmId == alarmId -> null else -> standing @@ -139,6 +140,18 @@ internal class AlarmSetAnnouncer @Inject constructor( } } + private fun isFresh(notice: AlarmSetNotice): Boolean = + elapsed.elapsedRealtime() - notice.announcedAt < FRESH_FOR + + private suspend fun stillRings(notice: AlarmSetNotice): Boolean = countdownOf(notice.alarmId) != null + + private suspend fun countdownOf(alarmId: Long): NextFireLabel? { + // One `now` for both ends of the countdown, or a resolve that lands a + // hair past the occurrence would read as a day and an hour. + val now = wall.now() + return AlarmSetCountdown.of(engine.upcomingFor(alarmId, now), now) + } + private companion object { /** Comfortably longer than a short snackbar, so a hand-over between screens fits. */ val FRESH_FOR = 10.seconds @@ -169,7 +182,9 @@ internal fun AlarmSetSnackbarEffect( lifecycle.repeatOnLifecycle(Lifecycle.State.RESUMED) { notice.collectLatest { current -> if (current == null) return@collectLatest - host.showSnackbar(alarmSetMessage(resources, current.countdown) ?: return@collectLatest) + // A notice with nothing to say is used up all the same, so it + // cannot sit pending for ever. + alarmSetMessage(resources, current.countdown)?.let { host.showSnackbar(it) } shown(current) } } diff --git a/app/src/test/java/de/jeanlucmakiola/clockula/ui/alarms/AlarmEditorViewModelTest.kt b/app/src/test/java/de/jeanlucmakiola/clockula/ui/alarms/AlarmEditorViewModelTest.kt index 48d5f32..f245c7a 100644 --- a/app/src/test/java/de/jeanlucmakiola/clockula/ui/alarms/AlarmEditorViewModelTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/clockula/ui/alarms/AlarmEditorViewModelTest.kt @@ -39,7 +39,6 @@ import org.junit.jupiter.api.io.TempDir import java.nio.file.Path import java.time.DayOfWeek import kotlin.time.Duration.Companion.hours -import kotlin.time.Duration.Companion.seconds /** * The editor over the **real** alarm engine: every control writes at once @@ -475,7 +474,7 @@ class AlarmEditorViewModelTest { } @Test - fun `the same countdown again leaves the standing notice alone`(@TempDir tempDir: Path) = + fun `an alarm switched off by another path is not announced as set`(@TempDir tempDir: Path) = runTest(main.dispatcher) { val harness = alarmEngineHarness(tempDir, now = fiveToday) harness.givenAlarmWith(hour = 7) @@ -483,14 +482,11 @@ class AlarmEditorViewModelTest { advanceUntilIdle() viewModel.onTimePicked(6, 45) advanceUntilIdle() - val first = viewModel.alarmSetNotice.first() - // A snackbar taken down and put straight back would flicker. - elapsed.advance(1.seconds) - viewModel.onTimePicked(6, 45) - advanceUntilIdle() + // The ring screen, a notification or an intent: none of them retracts. + harness.alarms.setEnabled(1L, enabled = false) - assertThat(viewModel.alarmSetNotice.first()).isSameInstanceAs(first) + assertThat(viewModel.alarmSetNotice.first()).isNull() } // --- label --- diff --git a/app/src/test/java/de/jeanlucmakiola/clockula/ui/alarms/AlarmsViewModelTest.kt b/app/src/test/java/de/jeanlucmakiola/clockula/ui/alarms/AlarmsViewModelTest.kt index 64530dd..b3465e2 100644 --- a/app/src/test/java/de/jeanlucmakiola/clockula/ui/alarms/AlarmsViewModelTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/clockula/ui/alarms/AlarmsViewModelTest.kt @@ -26,6 +26,7 @@ import org.junit.jupiter.api.extension.RegisterExtension import org.junit.jupiter.api.io.TempDir import java.nio.file.Path import kotlin.time.Duration.Companion.minutes +import kotlin.time.Duration.Companion.seconds /** * The list over the **real** alarm engine: what the rows say, that the readout @@ -48,13 +49,16 @@ class AlarmsViewModelTest { * `collectAsStateWithLifecycle`; without one there is no subscriber and * nothing upstream would run. */ - private fun TestScope.viewModel(harness: AlarmEngineHarness) = AlarmsViewModel( + private fun TestScope.viewModel( + harness: AlarmEngineHarness, + announcer: AlarmSetAnnouncer = AlarmSetAnnouncer(harness.engine, harness.wallClock, FakeElapsedRealtimeClock(), this), + ) = AlarmsViewModel( alarms = harness.alarms, engine = harness.engine, wall = harness.wallClock, zones = harness.zones, source = AlarmListSource(harness.engine, harness.wallClock, harness.ticker), - announcer = AlarmSetAnnouncer(harness.engine, harness.wallClock, FakeElapsedRealtimeClock(), this), + announcer = announcer, ).also { viewModel -> backgroundScope.launch { viewModel.state.collect {} } } private fun AlarmsUiState.rows(): List = (this as AlarmsUiState.Loaded).rows @@ -258,6 +262,25 @@ class AlarmsViewModelTest { assertThat(viewModel.alarmSetNotice.first()).isNull() } + @Test + fun `switching another alarm off keeps one still settling`(@TempDir tempDir: Path) = + runTest(main.dispatcher) { + val harness = alarmEngineHarness(tempDir) + harness.givenAlarm(id = 1L, hour = 7) + harness.givenAlarm(id = 2L, hour = 9) + val announcer = AlarmSetAnnouncer(harness.engine, harness.wallClock, FakeElapsedRealtimeClock(), this) + val viewModel = viewModel(harness, announcer) + advanceUntilIdle() + + // The editor's repeat days on alarm 1, still settling... + announcer.announce(1L, settle = 1.seconds) + // ...when alarm 2 is switched off in the list. + viewModel.onToggleEnabled(2L, enabled = false) + advanceUntilIdle() + + assertThat(viewModel.alarmSetNotice.first()?.alarmId).isEqualTo(1L) + } + @Test fun `adding an alarm leaves the announcement to the editor`(@TempDir tempDir: Path) = runTest(main.dispatcher) { diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index f63c3bc..cfa5939 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -1126,13 +1126,15 @@ at once, but waits for repeat-day taps to settle for a second, so Mon–Fri is one snackbar rather than five. `AlarmSetAnnouncer` is an app-wide singleton holding the notice as *state* until a screen has shown it to the end: pick → OK → back disposes the editor's snackbar a moment after it appears, and the -list underneath picks the same notice up. The settle runs on the application -scope, so leaving the editor mid-settle still announces; a screen shows the -notice only while resumed, so a cross-fade never shows it twice; the editor -shows only notices about its own alarm; deleting an alarm, or the `AlarmClock` -chooser dismissing it, takes its notice back. A notice older than ten seconds -on the monotonic clock is dropped unshown rather than shown with a stale -countdown. +list underneath picks the same notice up. The editor's fire-time-moving +writes, their reschedule and the per-alarm settle all run on the application +scope, so leaving the editor at once neither loses the announcement nor leaves +a stored time unscheduled. A screen shows the notice only while resumed, so a +cross-fade never shows it twice; the editor shows only notices about its own +alarm; a notice is re-checked against the engine before it is shown, so an +alarm switched off or deleted by any path is never announced as set. A notice +older than ten seconds on the monotonic clock is dropped unshown rather than +shown with a stale countdown. `upcoming(refresh: Flow)` takes its **re-read cadence as a parameter**. The resolver needs a `now`, and `upcoming` reads one inside a `combine` of the