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 6c95c1e..2177204 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/alarm/AlarmEngine.kt +++ b/app/src/main/java/de/jeanlucmakiola/clockula/alarm/AlarmEngine.kt @@ -11,7 +11,6 @@ import de.jeanlucmakiola.clockula.domain.alarm.AlarmRing import de.jeanlucmakiola.clockula.domain.alarm.AlarmRingState import de.jeanlucmakiola.clockula.domain.alarm.DismissOutcome import de.jeanlucmakiola.clockula.domain.alarm.FireOutcome -import de.jeanlucmakiola.clockula.domain.alarm.FireSource import de.jeanlucmakiola.clockula.domain.alarm.RingSession import de.jeanlucmakiola.clockula.domain.alarm.ScheduledFire import de.jeanlucmakiola.clockula.domain.alarm.SnoozeOutcome @@ -139,7 +138,7 @@ class AlarmEngine @Inject constructor( // 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) { + if (resolution.nextFire != fireAt || !UpcomingNotices.isInWindow(fireAt, resolution.source, now)) { // Whatever posted the stale notice is put right on the way out. rescheduleLocked() return@withLock DismissOutcome.Stale @@ -232,17 +231,19 @@ class AlarmEngine @Inject constructor( * 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? { + private suspend fun rescheduleLocked(deferUpcomingTo: MutableList? = null): ScheduledFire? { val now = wallClock.now() val zone = zones.current() var earliest: Pair? = null val resolved = mutableListOf() + val pendingSkips = mutableListOf() for (alarm in alarms.alarms().first()) { val state = states.state(alarm.id) val resolution = AlarmResolver.resolve(alarm, state, now, zone) persist(alarm, state, resolution) resolved += UpcomingAlarm(alarm, resolution.nextFire, resolution.source) + resolution.state.skippedOccurrence?.let { pendingSkips += it } val fireAt = resolution.nextFire ?: continue val best = earliest @@ -262,7 +263,8 @@ class AlarmEngine @Inject constructor( ScheduledFire(winner.first.id, fireAt, winner.second.source, mode) } // After the real registration, never before it. - if (deferUpcomingTo == null) syncUpcomingLocked(resolved, now) else deferUpcomingTo += resolved + val upcoming = UpcomingSync(resolved, pendingSkips) + if (deferUpcomingTo == null) syncUpcomingLocked(upcoming, now) else deferUpcomingTo += upcoming return scheduled } @@ -275,9 +277,9 @@ class AlarmEngine @Inject constructor( * 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) { + private fun syncUpcomingLocked(upcoming: UpcomingSync, now: Instant) { runCatching { - val plan = UpcomingNotices.plan(resolved, now) + val plan = UpcomingNotices.plan(upcoming.resolved, now, upcoming.pendingSkips) notifier.showUpcoming(plan.notices) val next = plan.nextCheckAt if (next == null) scheduler.cancelUpcomingCheck() else scheduler.scheduleUpcomingCheck(next) @@ -313,16 +315,15 @@ 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 - // An occurrence already accounted for: skipped in advance (#16) — armed or, - // once a reschedule spent the skip, recorded as handled — or a duplicate - // of one that already rang. A fire already on its way when that was - // written must not ring it. Equality only: a watermark ahead of a clock - // moved backwards must never silence an earlier occurrence. - val accountedFor = !isSnoozeFire && - (scheduledFor == state.skippedOccurrence || scheduledFor == state.handledOccurrence) - // Deleted, switched off, accounted for, or a PendingIntent so stale the - // moment has gone: re-resolve the slot and stay quiet. - if (alarm == null || !alarm.enabled || accountedFor || 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. + // Only the armed skip, not the handled-occurrence watermark: the + // resolver deliberately re-arms a handled occurrence that a clock moved + // backwards has put back in the future, and that one must ring. + val skipped = !isSnoozeFire && scheduledFor == state.skippedOccurrence + // 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 } @@ -348,7 +349,7 @@ class AlarmEngine @Inject constructor( notifier.clearSnoozed(alarmId) // The upcoming notices wait until the ring has started: their binder // calls must not stand between the fire and the sound. - val upcoming = mutableListOf() + val upcoming = mutableListOf() rescheduleLocked(deferUpcomingTo = upcoming) try { @@ -356,7 +357,7 @@ class AlarmEngine @Inject constructor( ring.startRinging(alarmId) scheduler.scheduleAutoSilence(now + AlarmRing.AUTO_SILENCE_AFTER, alarmId) } finally { - syncUpcomingLocked(upcoming, now) + upcoming.forEach { syncUpcomingLocked(it, now) } } return FireOutcome.Ringing(alarmId) } @@ -450,3 +451,10 @@ class AlarmEngine @Inject constructor( rescheduleLocked() } } + +/** What one reschedule pass hands the upcoming-alarm sync (#16). */ +private class UpcomingSync( + val resolved: List, + /** Armed skips' watermarks, so a check can spend each one on time. */ + val pendingSkips: List, +) diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/domain/alarm/UpcomingNotices.kt b/app/src/main/java/de/jeanlucmakiola/clockula/domain/alarm/UpcomingNotices.kt index 1b366d6..955c81b 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/domain/alarm/UpcomingNotices.kt +++ b/app/src/main/java/de/jeanlucmakiola/clockula/domain/alarm/UpcomingNotices.kt @@ -14,7 +14,7 @@ data class UpcomingNotice( /** * What the upcoming-alarm surface should show now, and when it next has to - * look again. [nextCheckAt] is null when no alarm's window is still to open. + * look again. [nextCheckAt] is null when there is nothing left to look at. */ data class UpcomingNoticePlan( val notices: List, @@ -44,17 +44,37 @@ object UpcomingNotices { fun isInWindow(nextFire: Instant?, source: FireSource, now: Instant, lead: Duration = LEAD): Boolean = source == FireSource.OCCURRENCE && nextFire != null && nextFire > now && nextFire - now <= lead - /** Notices ordered by when they ring, then id; the next check is the earliest window still to open. */ - fun plan(upcoming: List, now: Instant, lead: Duration = LEAD): UpcomingNoticePlan { + /** + * Notices ordered by when they ring, then id. The next check is the + * earliest of three instants still to come: + * + * - a window opening, so its notice goes up; + * - a noticed occurrence's fire grace running out, so a notice whose fire + * came late (Doze, no exact alarms) does not outlive its alarm; + * - an armed skip's ([pendingSkips]) fire grace running out, so a skip + * dismissed in advance is spent on time and the list stops saying + * "skipping next" about an occurrence that has gone by. + * + * The last two wait out the grace rather than land on the alarm's own + * instant, so they never race its fire. + */ + fun plan( + upcoming: List, + now: Instant, + pendingSkips: List = emptyList(), + lead: Duration = LEAD, + fireGrace: Duration = AlarmRing.FIRE_GRACE, + ): UpcomingNoticePlan { val notices = upcoming .filter { isInWindow(it.nextFire, it.source, now, lead) } .map { UpcomingNotice(it.alarm, it.nextFire!!) } .sortedWith(compareBy({ it.fireAt }, { it.alarm.id })) - val nextCheckAt = upcoming + val openings = upcoming .filter { it.source == FireSource.OCCURRENCE } .mapNotNull { entry -> entry.nextFire?.let { it - lead } } - .filter { it > now } - .minOrNull() + val closings = notices.map { it.fireAt + fireGrace } + val skipsSpent = pendingSkips.map { it + fireGrace } + val nextCheckAt = (openings + closings + skipsSpent).filter { it > now }.minOrNull() return UpcomingNoticePlan(notices, nextCheckAt) } } 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 c6e4fba..c2ae9a4 100644 --- a/app/src/test/java/de/jeanlucmakiola/clockula/alarm/AlarmEngineUpcomingTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/clockula/alarm/AlarmEngineUpcomingTest.kt @@ -2,6 +2,7 @@ package de.jeanlucmakiola.clockula.alarm import com.google.common.truth.Truth.assertThat import de.jeanlucmakiola.clockula.domain.RepeatDays +import de.jeanlucmakiola.clockula.domain.alarm.AlarmRing import de.jeanlucmakiola.clockula.domain.alarm.DismissOutcome import de.jeanlucmakiola.clockula.domain.alarm.FireOutcome import de.jeanlucmakiola.clockula.domain.alarm.UpcomingNotices @@ -51,7 +52,8 @@ class AlarmEngineUpcomingTest { harness.engine.onUpcomingWindow() assertThat(harness.notifier.upcoming.map { it.alarm.id }).containsExactly(1L, 2L).inOrder() - assertThat(harness.scheduler.upcomingCheck).isNull() + // Next: the first notice's alarm, gone by. + assertThat(harness.scheduler.upcomingCheck).isEqualTo(sixThirty + AlarmRing.FIRE_GRACE) } @Test @@ -69,7 +71,12 @@ class AlarmEngineUpcomingTest { assertThat(alarm.skipNextOccurrence).isTrue() assertThat(harness.scheduler.next?.fireAt).isEqualTo(sixThirtyTomorrow) assertThat(harness.notifier.upcoming).isEmpty() - assertThat(harness.scheduler.upcomingCheck).isEqualTo(sixThirtyTomorrow - UpcomingNotices.LEAD) + // Back once the skipped occurrence has gone by, to spend the skip. + assertThat(harness.scheduler.upcomingCheck).isEqualTo(sixThirty + AlarmRing.FIRE_GRACE) + harness.wallClock.instant = sixThirty + AlarmRing.FIRE_GRACE + harness.engine.onUpcomingWindow() + assertThat(harness.alarms.find(1L)!!.skipNextOccurrence).isFalse() + assertThat(harness.scheduler.next?.fireAt).isEqualTo(sixThirtyTomorrow) } @Test @@ -210,21 +217,21 @@ class AlarmEngineUpcomingTest { } @Test - fun `a fire that lands after the dismissed skip was spent still 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) + fun `a handled occurrence a clock moved backwards re-armed still rings`(@TempDir tempDir: Path) = runTest { + val harness = alarmEngineHarness(tempDir) + harness.givenAlarm(id = 1L, hour = 6, minute = 30, repeatDays = RepeatDays.EVERY_DAY) + harness.engine.reschedule() + harness.givenRinging(alarmId = 1L, at = sixThirty) + harness.engine.dismiss(1L) - // Another entry point reschedules first and spends the skip. - harness.wallClock.instant = sixThirty - harness.engine.reschedule() - val outcome = harness.engine.onFire(1L, sixThirty) + // The clock is set back under the dismissed alarm; the resolver re-arms it. + harness.wallClock.instant = sixThirty - 15.minutes + harness.engine.onSystemTimeOrZoneChanged() + assertThat(harness.scheduler.next?.fireAt).isEqualTo(sixThirty) - assertThat(outcome).isEqualTo(FireOutcome.Ignored) - assertThat(harness.ring.events).isEmpty() - } + harness.wallClock.instant = sixThirty + assertThat(harness.engine.onFire(1L, sixThirty)).isEqualTo(FireOutcome.Ringing(1L)) + } @Test fun `an old skip gone by but not yet spent does not swallow the new dismissal`(@TempDir tempDir: Path) = diff --git a/app/src/test/java/de/jeanlucmakiola/clockula/domain/alarm/UpcomingNoticesTest.kt b/app/src/test/java/de/jeanlucmakiola/clockula/domain/alarm/UpcomingNoticesTest.kt index f13e755..b7135d1 100644 --- a/app/src/test/java/de/jeanlucmakiola/clockula/domain/alarm/UpcomingNoticesTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/clockula/domain/alarm/UpcomingNoticesTest.kt @@ -4,6 +4,7 @@ import com.google.common.truth.Truth.assertThat import de.jeanlucmakiola.clockula.testing.offsetInstant import de.jeanlucmakiola.clockula.testing.upcomingAlarm import org.junit.jupiter.api.Test +import kotlin.time.Duration.Companion.days import kotlin.time.Duration.Companion.hours import kotlin.time.Duration.Companion.milliseconds import kotlin.time.Duration.Companion.minutes @@ -66,8 +67,6 @@ class UpcomingNoticesTest { fun `the next check is when the earliest window still to open opens`() { val plan = UpcomingNotices.plan( listOf( - // Already open: noticed now, needs no check. - upcomingAlarm(id = 1L, nextFire = now + 30.minutes), upcomingAlarm(id = 2L, nextFire = now + 5.hours), upcomingAlarm(id = 3L, nextFire = now + 3.hours), // A snooze's own notification covers it; no window to wait for. @@ -79,6 +78,34 @@ class UpcomingNoticesTest { assertThat(plan.nextCheckAt).isEqualTo(now + 1.hours) } + @Test + fun `a noticed occurrence is looked at again once its fire grace has run out`() { + val fireAt = now + 30.minutes + val plan = UpcomingNotices.plan( + listOf( + upcomingAlarm(id = 1L, nextFire = fireAt), + upcomingAlarm(id = 2L, nextFire = now + 5.hours), + ), + now, + ) + + // So a notice whose fire came late does not outlive its alarm. + assertThat(plan.nextCheckAt).isEqualTo(fireAt + AlarmRing.FIRE_GRACE) + } + + @Test + fun `an armed skip is looked at again once its occurrence has gone by`() { + val skipped = now + 40.minutes + val plan = UpcomingNotices.plan( + listOf(upcomingAlarm(id = 1L, nextFire = now + 1.days)), + now, + pendingSkips = listOf(skipped), + ) + + // So the skip is spent then, not at tomorrow's window. + assertThat(plan.nextCheckAt).isEqualTo(skipped + AlarmRing.FIRE_GRACE) + } + @Test fun `nothing to come means no notice and no check`() { val plan = UpcomingNotices.plan( diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index c3bd2a0..6ad2880 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -1238,7 +1238,12 @@ 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 so a notification failure costs nothing, + registration. The check also wakes once a noticed occurrence's fire grace has + run out (so a notice cannot outlive a fire that came late) and once an armed + skip's has (so a skip dismissed in advance is spent on time, and the list + stops saying "skipping next" about an occurrence already gone). Both wait out + the grace rather than land on the alarm's own instant, so they never race its + fire. 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()`. @@ -1250,10 +1255,11 @@ the alarm's row for whoever opens the app instead. 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`: a natural fire whose instant *equals* - the skip or the handled-occurrence watermark is already accounted for. - Equality, not order, so a watermark left ahead of a clock moved backwards - never silences an earlier occurrence. A repeating alarm's resolution is + was armed is `Ignored` by `onFire`, which checks the armed skip watermark + against the instant it carries. Only the skip: the handled-occurrence + watermark is left to the resolver, which deliberately re-arms a handled + occurrence that a clock moved backwards has put back in the future. A + repeating alarm's resolution is persisted before the skip is armed, so an old skip that has gone by but was never spent cannot absorb the new one. - **No dismiss challenge before the ring**, as in Google Clock. The challenge