From 5b35ef65d4b464b83b001c0c54d478c03944357c Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Wed, 21 Oct 2026 20:09:47 +0200 Subject: [PATCH] fix(alarms): keep the upcoming Dismiss away from the fire A fire broadcast already in flight when a skip was armed on its occurrence is now ignored, and a Dismiss that lands once the occurrence is due is stale. The notices are synced after the ring starts on the fire path, and a swiped-away notice stays gone across a process restart. A switched-off alarm answers NotFound. Fixes #16 --- .../clockula/alarm/AlarmEngine.kt | 39 +++++++++---- .../alarm/android/AndroidAlarmNotifier.kt | 58 ++++++++++++------- .../clockula/alarm/ring/RingNotifications.kt | 3 +- .../alarm/ring/UpcomingNotifications.kt | 17 +----- .../clockula/alarm/AlarmEngineUpcomingTest.kt | 43 ++++++++++++++ docs/ARCHITECTURE.md | 26 +++++---- 6 files changed, 126 insertions(+), 60 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 a81440f..044b2af 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/alarm/AlarmEngine.kt +++ b/app/src/main/java/de/jeanlucmakiola/clockula/alarm/AlarmEngine.kt @@ -128,11 +128,14 @@ class AlarmEngine @Inject constructor( * already one switch away in the editor, unchallenged. */ suspend fun dismissUpcomingOccurrence(alarmId: Long, fireAt: Instant): DismissOutcome = lock.withLock { - val alarm = alarms.find(alarmId) ?: return@withLock DismissOutcome.NotFound + val alarm = alarms.find(alarmId)?.takeIf { it.enabled } ?: return@withLock DismissOutcome.NotFound + val now = wallClock.now() // Read-only: the resolver's state is discarded, and the reschedule below - // persists whatever it should. - val resolution = AlarmResolver.resolve(alarm, states.state(alarmId), wallClock.now(), zones.current()) - if (resolution.source != FireSource.OCCURRENCE || resolution.nextFire != fireAt) { + // persists whatever it should. `fireAt > now` because the resolver keeps + // a just-passed occurrence through the fire grace: one already due is + // the fire's, not the notice's. + val resolution = AlarmResolver.resolve(alarm, states.state(alarmId), now, zones.current()) + if (resolution.source != FireSource.OCCURRENCE || resolution.nextFire != fireAt || fireAt <= now) { // Whatever posted the stale notice is put right on the way out. rescheduleLocked() return@withLock DismissOutcome.Stale @@ -220,7 +223,12 @@ class AlarmEngine @Inject constructor( ) } - private suspend fun rescheduleLocked(): ScheduledFire? { + /** + * [deferUpcomingTo] collects what the upcoming notices would be synced from + * instead of syncing them, for a caller with something more urgent to do + * first (the fire path). + */ + private suspend fun rescheduleLocked(deferUpcomingTo: MutableList? = null): ScheduledFire? { val now = wallClock.now() val zone = zones.current() @@ -250,7 +258,7 @@ class AlarmEngine @Inject constructor( ScheduledFire(winner.first.id, fireAt, winner.second.source, mode) } // After the real registration, never before it. - syncUpcomingLocked(resolved, now) + if (deferUpcomingTo == null) syncUpcomingLocked(resolved, now) else deferUpcomingTo += resolved return scheduled } @@ -260,8 +268,8 @@ class AlarmEngine @Inject constructor( * snooze, a fire, a boot, a clock or zone change — already ends in * [rescheduleLocked], so this is the one place they are kept true. * - * Nothing here may cost an alarm: [onFireLocked] reschedules *before* it - * starts the ring, so a failure on the way is swallowed rather than thrown. + * Nothing here may cost an alarm, so a failure is swallowed rather than + * thrown into whichever entry point is rescheduling. */ private fun syncUpcomingLocked(resolved: List, now: Instant) { runCatching { @@ -297,9 +305,12 @@ class AlarmEngine @Inject constructor( // delivered late (after a reboot, or by Doze) still rings. Rejecting it // here would only re-register the same past instant and fire again. val lateBy = if (isSnoozeFire) AlarmRing.AUTO_SILENCE_AFTER else AlarmRing.FIRE_GRACE - // Deleted, switched off, or a PendingIntent so stale the moment has gone: - // re-resolve the slot and stay quiet. - if (alarm == null || !alarm.enabled || now - scheduledFor > lateBy) { + // A skip armed on this very occurrence after its fire was already on its + // way: the user dismissed it in advance (#16), so it must not ring. + val skipped = !isSnoozeFire && state.skippedOccurrence == scheduledFor + // Deleted, switched off, skipped, or a PendingIntent so stale the moment + // has gone: re-resolve the slot and stay quiet. + if (alarm == null || !alarm.enabled || skipped || now - scheduledFor > lateBy) { rescheduleLocked() return FireOutcome.Ignored } @@ -323,11 +334,15 @@ class AlarmEngine @Inject constructor( // it holds this alarm's *next* occurrence rather than nothing at all. states.save(opened) notifier.clearSnoozed(alarmId) - rescheduleLocked() + // The upcoming notices wait until the ring has started: their binder + // calls must not stand between the fire and the sound. + val upcoming = mutableListOf() + rescheduleLocked(deferUpcomingTo = upcoming) states.save(states.state(alarmId).copy(ringingSince = now)) ring.startRinging(alarmId) scheduler.scheduleAutoSilence(now + AlarmRing.AUTO_SILENCE_AFTER, alarmId) + syncUpcomingLocked(upcoming, now) return FireOutcome.Ringing(alarmId) } diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/alarm/android/AndroidAlarmNotifier.kt b/app/src/main/java/de/jeanlucmakiola/clockula/alarm/android/AndroidAlarmNotifier.kt index 113440d..865aee9 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/alarm/android/AndroidAlarmNotifier.kt +++ b/app/src/main/java/de/jeanlucmakiola/clockula/alarm/android/AndroidAlarmNotifier.kt @@ -53,44 +53,60 @@ class AndroidAlarmNotifier @Inject constructor( } /** - * What this process last posted, per alarm. The engine syncs on every - * reschedule — each app start, each edit — and re-posting an unchanged - * notice would bring back one the user swiped away. Only the engine calls - * this, under its lock, so the map needs no lock of its own. + * The notices already posted for the occurrences still wanted, as + * `id@fireAt` keys. Persisted, because every process start reschedules: a + * notice the user swiped away must stay gone across a restart, and only a + * notice this code cancelled itself (the alarm left its window) may come + * back. The engine calls this under its lock, so it needs none of its own. */ - private val postedUpcoming = mutableMapOf() + private val record by lazy { context.getSharedPreferences(RECORD_FILE, Context.MODE_PRIVATE) } + + /** What this process last posted, per alarm, so an unchanged notice is not re-sent. */ + private val lastPosted = mutableMapOf() override fun showUpcoming(notices: List) { runCatching { - val wanted = notices.associateBy { it.alarm.id } - // What is on screen, including what an earlier process posted and - // this one has no memory of. + val wantedIds = notices.map { UpcomingNotifications.notificationId(it.alarm.id) }.toSet() + // What is on screen, including what an earlier process posted. val onScreen = context.getSystemService(NotificationManager::class.java) .activeNotifications .filter { it.tag == UpcomingNotifications.TAG } .map { it.id } - val wantedIds = wanted.keys.map(UpcomingNotifications::notificationId).toSet() - (onScreen + postedUpcoming.keys.map(UpcomingNotifications::notificationId)) - .filterNot { it in wantedIds } - .forEach { notifications.cancel(UpcomingNotifications.TAG, it) } - postedUpcoming.keys.retainAll(wanted.keys) + .toSet() + onScreen.filterNot { it in wantedIds }.forEach { notifications.cancel(UpcomingNotifications.TAG, it) } + lastPosted.keys.retainAll(notices.map { it.alarm.id }.toSet()) - // Denied, nothing is recorded, so a later grant posts on the next sync. + val wantedKeys = notices.map(::recordKey).toSet() + val posted = record.getStringSet(RECORD_KEY, emptySet()).orEmpty().intersect(wantedKeys).toMutableSet() + + // Denied, nothing new is recorded, so a later grant posts on the next sync. if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU && ContextCompat.checkSelfPermission(context, Manifest.permission.POST_NOTIFICATIONS) != PackageManager.PERMISSION_GRANTED ) { + record.edit().putStringSet(RECORD_KEY, posted).apply() return@runCatching } for (notice in notices) { - if (postedUpcoming[notice.alarm.id] == notice) continue - notifications.notify( - UpcomingNotifications.TAG, - UpcomingNotifications.notificationId(notice.alarm.id), - UpcomingNotifications.build(context, notice), - ) - postedUpcoming[notice.alarm.id] = notice + val id = UpcomingNotifications.notificationId(notice.alarm.id) + val shown = id in onScreen + // Posted before and no longer on screen: the user swiped it away. + if (!shown && recordKey(notice) in posted) continue + // On screen and unchanged: nothing to send. + if (shown && lastPosted[notice.alarm.id] == notice) continue + notifications.notify(UpcomingNotifications.TAG, id, UpcomingNotifications.build(context, notice)) + lastPosted[notice.alarm.id] = notice + posted += recordKey(notice) } + record.edit().putStringSet(RECORD_KEY, posted).apply() } } + + private fun recordKey(notice: UpcomingNotice): String = + "${notice.alarm.id}@${notice.fireAt.toEpochMilliseconds()}" + + private companion object { + const val RECORD_FILE = "alarm_upcoming_notices" + const val RECORD_KEY = "posted" + } } diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/alarm/ring/RingNotifications.kt b/app/src/main/java/de/jeanlucmakiola/clockula/alarm/ring/RingNotifications.kt index dbe1bc9..17a22d1 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/alarm/ring/RingNotifications.kt +++ b/app/src/main/java/de/jeanlucmakiola/clockula/alarm/ring/RingNotifications.kt @@ -216,7 +216,8 @@ object RingNotifications { private fun formatTime(context: Context, hour: Int, minute: Int): String = format(context, LocalTime.of(hour, minute)) - private fun formatInstant(context: Context, instant: Instant): String = format( + /** Also the upcoming notice's (#16): one formatter for every alarm notification. */ + internal fun formatInstant(context: Context, instant: Instant): String = format( context, JavaInstant.ofEpochMilli(instant.toEpochMilliseconds()).atZone(ZoneId.systemDefault()).toLocalTime(), ) diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/alarm/ring/UpcomingNotifications.kt b/app/src/main/java/de/jeanlucmakiola/clockula/alarm/ring/UpcomingNotifications.kt index 981a842..ae3b027 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/alarm/ring/UpcomingNotifications.kt +++ b/app/src/main/java/de/jeanlucmakiola/clockula/alarm/ring/UpcomingNotifications.kt @@ -12,10 +12,6 @@ import de.jeanlucmakiola.clockula.R import de.jeanlucmakiola.clockula.alarm.AlarmIntents import de.jeanlucmakiola.clockula.alarm.receiver.AlarmActionReceiver import de.jeanlucmakiola.clockula.domain.alarm.UpcomingNotice -import java.time.Instant as JavaInstant -import java.time.ZoneId -import java.time.format.DateTimeFormatter -import java.time.format.FormatStyle /** * "Upcoming alarm" with a Dismiss, posted while an alarm is inside its window @@ -46,7 +42,7 @@ object UpcomingNotifications { fun build(context: Context, notice: UpcomingNotice): Notification { val alarm = notice.alarm - val time = formatInstant(context, notice.fireAt) + val time = RingNotifications.formatInstant(context, notice.fireAt) val text = if (alarm.label.isBlank()) { time } else { @@ -98,17 +94,6 @@ object UpcomingNotifications { PendingIntent.FLAG_UPDATE_CURRENT or PendingIntent.FLAG_IMMUTABLE, ) - /** The device's 12/24-hour choice, then the locale's own short form. */ - private fun formatInstant(context: Context, instant: kotlin.time.Instant): String { - val time = JavaInstant.ofEpochMilli(instant.toEpochMilliseconds()).atZone(ZoneId.systemDefault()).toLocalTime() - val pattern = if (android.text.format.DateFormat.is24HourFormat(context)) { - DateTimeFormatter.ofPattern("HH:mm") - } else { - DateTimeFormatter.ofLocalizedTime(FormatStyle.SHORT) - } - return pattern.format(time) - } - /** Matches `RingNotifications`: keeps a derived request code clear of the small ones. */ private const val REQUEST_CODE_STRIDE = 100_000 diff --git a/app/src/test/java/de/jeanlucmakiola/clockula/alarm/AlarmEngineUpcomingTest.kt b/app/src/test/java/de/jeanlucmakiola/clockula/alarm/AlarmEngineUpcomingTest.kt index 82e3bc8..718a488 100644 --- a/app/src/test/java/de/jeanlucmakiola/clockula/alarm/AlarmEngineUpcomingTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/clockula/alarm/AlarmEngineUpcomingTest.kt @@ -12,6 +12,7 @@ import kotlinx.coroutines.test.runTest import org.junit.jupiter.api.Test import org.junit.jupiter.api.io.TempDir import java.nio.file.Path +import kotlin.time.Duration.Companion.minutes /** * The upcoming-alarm notification (#16). The harness starts at 05:00 Berlin, so @@ -164,4 +165,46 @@ class AlarmEngineUpcomingTest { assertThat(harness.ring.events).containsExactly(RingEvent.Started(1L)) assertThat(harness.scheduler.next?.fireAt).isEqualTo(sixThirtyTomorrow) } + + @Test + fun `a fire already on its way when the notice was dismissed does not ring`(@TempDir tempDir: Path) = + runTest { + val harness = alarmEngineHarness(tempDir) + harness.givenAlarm(id = 1L, hour = 6, minute = 30, repeatDays = RepeatDays.EVERY_DAY) + harness.engine.reschedule() + harness.engine.dismissUpcomingOccurrence(1L, sixThirty) + + // The broadcast for 06:30 was sent before the skip was armed. + harness.wallClock.instant = sixThirty + val outcome = harness.engine.onFire(1L, sixThirty) + + assertThat(outcome).isEqualTo(FireOutcome.Ignored) + assertThat(harness.ring.events).isEmpty() + assertThat(harness.scheduler.next?.fireAt).isEqualTo(sixThirtyTomorrow) + // The skip is spent on the one occurrence, not on tomorrow's. + assertThat(harness.alarms.find(1L)!!.skipNextOccurrence).isFalse() + } + + @Test + fun `a Dismiss once the occurrence is due belongs to the fire, not the notice`(@TempDir tempDir: Path) = + runTest { + val harness = alarmEngineHarness(tempDir) + harness.givenAlarm(id = 1L, hour = 6, minute = 30, repeatDays = RepeatDays.EVERY_DAY) + harness.engine.reschedule() + + // Inside the fire grace, with the fire itself running late. + harness.wallClock.instant = sixThirty + 1.minutes + val outcome = harness.engine.dismissUpcomingOccurrence(1L, sixThirty) + + assertThat(outcome).isEqualTo(DismissOutcome.Stale) + assertThat(harness.alarms.find(1L)!!.skipNextOccurrence).isFalse() + } + + @Test + fun `a switched-off alarm has nothing to dismiss`(@TempDir tempDir: Path) = runTest { + val harness = alarmEngineHarness(tempDir) + harness.givenAlarm(id = 1L, hour = 6, minute = 30, enabled = false) + + assertThat(harness.engine.dismissUpcomingOccurrence(1L, sixThirty)).isEqualTo(DismissOutcome.NotFound) + } } diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index e9f8c17..8fa4e3f 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -1237,22 +1237,28 @@ the alarm's row for whoever opens the app instead. - **Kept true in one place.** Every path that moves an alarm already ends in `rescheduleLocked`, so that is where the notices are synced and the third slot is armed for the earliest window still to open, *after* the real - registration. The sync is wrapped: `onFire` reschedules before it starts the - ring, and a notification failure must not cost the ring. The check's - receiver just calls `onUpcomingWindow()`, which is `reschedule()`. + registration. The sync is wrapped so a notification failure costs nothing, + and on the fire path it is deferred until the ring has started, so its binder + calls never stand between the fire and the sound. The check's receiver just + calls `onUpcomingWindow()`, which is `reschedule()`. - **Dismiss is `dismissUpcoming`, pinned to one occurrence.** `dismissUpcomingOccurrence(id, fireAt)` re-resolves under the lock and acts - only if the alarm's next fire is still that natural occurrence; otherwise it - is `DismissOutcome.Stale` and writes nothing. Without the pin a tap landing - as the alarm starts would close the ring and get round its challenge. When it - matches, it takes `dismissUpcoming`'s own branches: a repeating alarm skips - that occurrence, a one-shot is switched off (or deleted, if transient). + only if the alarm's next fire is still that natural occurrence *and still in + the future*; otherwise it is `DismissOutcome.Stale` and dismisses nothing. + Without the pin a tap landing as the alarm starts would close the ring and get + round its challenge. When it matches, it takes `dismissUpcoming`'s own + branches: a repeating alarm skips that occurrence, a one-shot is switched off + (or deleted, if transient). A fire broadcast already in flight when a skip + was armed is `Ignored` by `onFire`, which checks the skip watermark against + the instant it carries. - **No dismiss challenge before the ring**, as in Google Clock. The challenge proves you are awake while an alarm sounds; before it, skipping is already one unchallenged switch away in the editor. - **Denied notifications** cost the notice and nothing else; the row's chip - still works. The notifier remembers what it posted in this process, so a - reschedule does not bring back a notice the user swiped away. + still works. The notifier keeps a small SharedPreferences record of the + `id@fireAt` notices it has posted, so a reschedule, even in a new process, + does not bring back a notice the user swiped away; one the app cancelled + itself (the alarm left its window) is dropped from the record and may return. **The `showIntent` was wrong until M9.** It is what the system opens when the user taps the status-bar alarm icon or the lockscreen's next-alarm line, and it