fix(alarms): re-arm a skip when the pause changes, and clear spent dates in place
A skip armed under one pause kept its watermark when the pause changed, so removing the pause left the alarm silent until the old pause's end. The engine now re-arms a pending skip after a pause edit. Spent dates and pauses are cleared with targeted, conditional updates instead of a whole-row rewrite. SET_ALARM ignores a date or pause that has already gone by when looking for an alarm to re-use, the editor re-reads the clock when a date picker opens, and the next-alarm widget shows the year for a date in another year.
This commit is contained in:
@@ -127,6 +127,20 @@ class AlarmEngine @Inject constructor(
|
||||
DismissOutcome.Disabled(alarmId)
|
||||
}
|
||||
|
||||
/**
|
||||
* The alarm's pause just changed: a skip armed on the old shape of its
|
||||
* sequence is re-armed on the new one, so "skip next" still means the
|
||||
* next ring — not one the old pause jumped to, nor one the new pause
|
||||
* already silences. A watermark already due is left for the resolver to
|
||||
* consume as usual.
|
||||
*/
|
||||
suspend fun onPauseChanged(alarmId: Long): ScheduledFire? = lock.withLock {
|
||||
val state = states.state(alarmId)
|
||||
val armed = state.skippedOccurrence
|
||||
if (armed != null && armed > wallClock.now()) states.save(state.copy(skippedOccurrence = null))
|
||||
rescheduleLocked()
|
||||
}
|
||||
|
||||
suspend fun dismiss(alarmId: Long): Unit =
|
||||
lock.withLock { closeCycleLocked(alarmId, states.state(alarmId)) }
|
||||
|
||||
@@ -325,18 +339,12 @@ class AlarmEngine @Inject constructor(
|
||||
// A transient alarm is deleted wherever a permanent one would be
|
||||
// disabled: the cycle has closed either way (M9 D10).
|
||||
if (resolution.disableAlarm) disableOrDeleteLocked(alarm)
|
||||
if (resolution.clearScheduledDate || resolution.clearPause) {
|
||||
// Conditional on the stored value still being the one resolved, so
|
||||
// an editor that has just picked a new date or pause keeps it.
|
||||
alarms.edit(alarm.id) { current ->
|
||||
current.copy(
|
||||
scheduledDate = current.scheduledDate.takeUnless {
|
||||
resolution.clearScheduledDate && it == alarm.scheduledDate
|
||||
},
|
||||
pause = current.pause.takeUnless { resolution.clearPause && it == alarm.pause },
|
||||
)
|
||||
}
|
||||
}
|
||||
// Conditional on the stored value still being the one resolved, so an
|
||||
// editor that has just picked a new date or pause keeps it.
|
||||
val date = alarm.scheduledDate
|
||||
if (resolution.clearScheduledDate && date != null) alarms.clearScheduledDate(alarm.id, date)
|
||||
val pause = alarm.pause
|
||||
if (resolution.clearPause && pause != null) alarms.clearPause(alarm.id, pause)
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -36,6 +36,20 @@ abstract class AlarmDao {
|
||||
@Query("UPDATE alarms SET skip_next_occurrence = :skip, updated_at = :updatedAt WHERE id = :id")
|
||||
abstract suspend fun setSkipNextOccurrence(id: Long, skip: Boolean, updatedAt: Long)
|
||||
|
||||
/** Clears the date only while it is still [expected], so a newly picked one survives. */
|
||||
@Query(
|
||||
"UPDATE alarms SET scheduled_date = NULL, updated_at = :updatedAt " +
|
||||
"WHERE id = :id AND scheduled_date = :expected",
|
||||
)
|
||||
abstract suspend fun clearScheduledDate(id: Long, expected: Long, updatedAt: Long)
|
||||
|
||||
/** Clears the pause only while it is still [expectedStart]..[expectedEnd]. */
|
||||
@Query(
|
||||
"UPDATE alarms SET pause_start = NULL, pause_end = NULL, updated_at = :updatedAt " +
|
||||
"WHERE id = :id AND pause_start = :expectedStart AND pause_end = :expectedEnd",
|
||||
)
|
||||
abstract suspend fun clearPause(id: Long, expectedStart: Long, expectedEnd: Long, updatedAt: Long)
|
||||
|
||||
/**
|
||||
* Reads the row, applies [transform] and writes the result back inside one
|
||||
* transaction. A missing row, or a null result, writes nothing. [transform]
|
||||
|
||||
@@ -2,7 +2,9 @@ package de.jeanlucmakiola.clockula.data.alarms
|
||||
|
||||
import de.jeanlucmakiola.clockula.domain.Alarm
|
||||
import de.jeanlucmakiola.clockula.domain.AlarmDraft
|
||||
import de.jeanlucmakiola.clockula.domain.PauseRange
|
||||
import kotlinx.coroutines.flow.Flow
|
||||
import java.time.LocalDate
|
||||
|
||||
interface AlarmRepository {
|
||||
fun alarms(): Flow<List<Alarm>>
|
||||
@@ -15,6 +17,14 @@ interface AlarmRepository {
|
||||
suspend fun setEnabled(id: Long, enabled: Boolean)
|
||||
suspend fun setSkipNextOccurrence(id: Long, skip: Boolean)
|
||||
|
||||
/**
|
||||
* Drops a spent date or pause, but only while the stored one is still
|
||||
* [expected]: a column-targeted write, so no other field is rewritten
|
||||
* and a value the editor has just picked is never cleared.
|
||||
*/
|
||||
suspend fun clearScheduledDate(id: Long, expected: LocalDate)
|
||||
suspend fun clearPause(id: Long, expected: PauseRange)
|
||||
|
||||
/**
|
||||
* Transactional read-modify-write. Stamps `updated_at` from the wall clock
|
||||
* and keeps the id, so a concurrent `setEnabled` from the engine cannot be
|
||||
|
||||
@@ -2,9 +2,11 @@ package de.jeanlucmakiola.clockula.data.alarms
|
||||
|
||||
import de.jeanlucmakiola.clockula.domain.Alarm
|
||||
import de.jeanlucmakiola.clockula.domain.AlarmDraft
|
||||
import de.jeanlucmakiola.clockula.domain.PauseRange
|
||||
import de.jeanlucmakiola.clockula.domain.time.WallClock
|
||||
import kotlinx.coroutines.flow.Flow
|
||||
import kotlinx.coroutines.flow.map
|
||||
import java.time.LocalDate
|
||||
import javax.inject.Inject
|
||||
import javax.inject.Singleton
|
||||
|
||||
@@ -42,6 +44,12 @@ class AlarmRepositoryImpl @Inject constructor(
|
||||
override suspend fun setSkipNextOccurrence(id: Long, skip: Boolean) =
|
||||
dao.setSkipNextOccurrence(id, skip, wallClock.now().toEpochMilliseconds())
|
||||
|
||||
override suspend fun clearScheduledDate(id: Long, expected: LocalDate) =
|
||||
dao.clearScheduledDate(id, expected.toEpochDay(), wallClock.now().toEpochMilliseconds())
|
||||
|
||||
override suspend fun clearPause(id: Long, expected: PauseRange) =
|
||||
dao.clearPause(id, expected.start.toEpochDay(), expected.end.toEpochDay(), wallClock.now().toEpochMilliseconds())
|
||||
|
||||
override suspend fun edit(id: Long, transform: (Alarm) -> Alarm) {
|
||||
val updatedAt = wallClock.now().toEpochMilliseconds()
|
||||
dao.updateWithin(id) { entity ->
|
||||
|
||||
@@ -26,6 +26,8 @@ data class NextAlarmWidgetModel(
|
||||
* the coming week, where "Thursday" can only mean one day.
|
||||
*/
|
||||
val date: LocalDate? = null,
|
||||
/** [date] falls in another year than today, so it needs the year to be read right. */
|
||||
val dateNeedsYear: Boolean = false,
|
||||
)
|
||||
|
||||
object NextAlarmWidgetPresenter {
|
||||
@@ -48,6 +50,7 @@ object NextAlarmWidgetPresenter {
|
||||
label = next.alarm.label,
|
||||
date = zonedFire.toLocalDate()
|
||||
.takeUnless { it.isBefore(zonedNow.toLocalDate().plusDays(DAYS_A_WEEKDAY_NAMES)) },
|
||||
dateNeedsYear = zonedFire.year != zonedNow.year,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -9,6 +9,7 @@ import de.jeanlucmakiola.clockula.domain.AlarmDraft
|
||||
import de.jeanlucmakiola.clockula.domain.Timer
|
||||
import de.jeanlucmakiola.clockula.domain.TimerDraft
|
||||
import de.jeanlucmakiola.clockula.domain.TimerState
|
||||
import de.jeanlucmakiola.clockula.domain.alarm.AlarmSchedule
|
||||
import de.jeanlucmakiola.clockula.domain.alarm.FireSource
|
||||
import de.jeanlucmakiola.clockula.domain.interop.AlarmClockRequest
|
||||
import de.jeanlucmakiola.clockula.domain.interop.AlarmMatching
|
||||
@@ -21,7 +22,9 @@ import de.jeanlucmakiola.clockula.domain.time.WallClock
|
||||
import de.jeanlucmakiola.clockula.domain.time.ZoneProvider
|
||||
import de.jeanlucmakiola.clockula.timer.TimerEngine
|
||||
import kotlinx.coroutines.flow.first
|
||||
import java.time.ZoneId
|
||||
import kotlin.time.Duration
|
||||
import kotlin.time.Instant
|
||||
import javax.inject.Inject
|
||||
import javax.inject.Singleton
|
||||
|
||||
@@ -73,7 +76,9 @@ class AlarmClockHandler @Inject constructor(
|
||||
return InteropOutcome.OpenAlarmEditor(id)
|
||||
}
|
||||
|
||||
val existing = alarms.alarms().first().filter { it.matches(spec) }.minByOrNull { it.id }
|
||||
val now = wallClock.now()
|
||||
val zone = zones.current()
|
||||
val existing = alarms.alarms().first().filter { it.matches(spec, now, zone) }.minByOrNull { it.id }
|
||||
if (existing != null) {
|
||||
// "The alarm is always enabled", and a pending skip would silently
|
||||
// eat the very occurrence the caller just asked for (D11). The
|
||||
@@ -105,11 +110,12 @@ class AlarmClockHandler @Inject constructor(
|
||||
* on an alarm with no date and no pause. The contract cannot express
|
||||
* either, so an alarm carrying one is not "identical": re-using it would
|
||||
* either leave the caller's alarm silent until the pause ends, or throw
|
||||
* away a holiday the user set. A new alarm costs neither.
|
||||
* away a holiday the user set. A new alarm costs neither. One that has
|
||||
* gone by, and only waits to be cleared, no longer counts.
|
||||
*/
|
||||
private fun Alarm.matches(spec: AlarmSpec): Boolean =
|
||||
scheduledDate == null &&
|
||||
pause == null &&
|
||||
private fun Alarm.matches(spec: AlarmSpec, now: Instant, zone: ZoneId): Boolean =
|
||||
AlarmSchedule.upcomingDate(this, now, zone) == null &&
|
||||
AlarmSchedule.currentPause(this, now, zone) == null &&
|
||||
time == spec.time &&
|
||||
repeatDays == spec.repeatDays &&
|
||||
label == spec.label &&
|
||||
|
||||
@@ -165,7 +165,10 @@ private fun AlarmEditorContent(
|
||||
summary = editing.pause?.let { pauseRangeText(it) }
|
||||
?: stringResource(R.string.editor_pause_none),
|
||||
position = positionOf(1, repeatRows),
|
||||
onClick = { openPicker = OpenPicker.PAUSE },
|
||||
onClick = {
|
||||
viewModel.onOpenDatePicker()
|
||||
openPicker = OpenPicker.PAUSE
|
||||
},
|
||||
)
|
||||
GroupedRow(
|
||||
title = stringResource(R.string.editor_skip_next),
|
||||
@@ -185,7 +188,10 @@ private fun AlarmEditorContent(
|
||||
summary = editing.scheduledDate?.let { scheduledDateText(it) }
|
||||
?: stringResource(R.string.editor_schedule_none),
|
||||
position = positionOf(1, repeatRows),
|
||||
onClick = { openPicker = OpenPicker.SCHEDULE_DATE },
|
||||
onClick = {
|
||||
viewModel.onOpenDatePicker()
|
||||
openPicker = OpenPicker.SCHEDULE_DATE
|
||||
},
|
||||
)
|
||||
}
|
||||
|
||||
|
||||
@@ -66,6 +66,9 @@ class AlarmEditorViewModel @Inject internal constructor(
|
||||
/** A picked document whose read grant could not be made persistent (D20). */
|
||||
private val accessMayLapse = MutableStateFlow(false)
|
||||
|
||||
/** Bumped to re-render against a fresh `now` — see [onOpenDatePicker]. */
|
||||
private val refreshes = MutableStateFlow(0)
|
||||
|
||||
private val _state = MutableStateFlow<AlarmEditorUiState>(AlarmEditorUiState.Loading)
|
||||
|
||||
val state: StateFlow<AlarmEditorUiState> = _state.asStateFlow()
|
||||
@@ -91,7 +94,8 @@ class AlarmEditorViewModel @Inject internal constructor(
|
||||
settings.defaults,
|
||||
labelWrites,
|
||||
accessMayLapse,
|
||||
) { alarm, defaults, typedLabel, mayLapse ->
|
||||
refreshes,
|
||||
) { alarm, defaults, typedLabel, mayLapse, _ ->
|
||||
Inputs(alarm, defaults, typedLabel, mayLapse)
|
||||
}.collectLatest { inputs ->
|
||||
_state.value = render(inputs)
|
||||
@@ -159,15 +163,27 @@ class AlarmEditorViewModel @Inject internal constructor(
|
||||
/** null ends the pause. Only a repeating alarm can pause; a pause wholly in the past is refused. */
|
||||
fun onPauseChosen(pause: PauseRange?) {
|
||||
val today = AlarmSchedule.today(wallClock.now(), zones.current())
|
||||
edit(reschedule = true) { alarm ->
|
||||
when {
|
||||
!alarm.repeatDays.isRepeating -> alarm
|
||||
pause != null && pause.end.isBefore(today) -> alarm
|
||||
else -> alarm.copy(pause = pause)
|
||||
viewModelScope.launch {
|
||||
alarms.edit(alarmId) { alarm ->
|
||||
when {
|
||||
!alarm.repeatDays.isRepeating -> alarm
|
||||
pause != null && pause.end.isBefore(today) -> alarm
|
||||
else -> alarm.copy(pause = pause)
|
||||
}
|
||||
}
|
||||
engine.onPauseChanged(alarmId)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Re-reads the clock before a date picker opens. The state is otherwise
|
||||
* rendered only when the alarm changes, so an editor left open past the
|
||||
* alarm's time, or past midnight, would offer a day it then refuses.
|
||||
*/
|
||||
fun onOpenDatePicker() {
|
||||
refreshes.value += 1
|
||||
}
|
||||
|
||||
/** Verbatim, conflated (M5 D24). */
|
||||
fun onLabelTyped(text: String) {
|
||||
// Untrimmed: `Alarm.label` is verbatim everywhere else, and trimming as
|
||||
|
||||
@@ -140,7 +140,9 @@ private fun timeLabel(context: Context, model: NextAlarmWidgetModel): String {
|
||||
private fun dayLabel(context: Context, model: NextAlarmWidgetModel): String = when {
|
||||
model.isToday -> context.getString(R.string.widget_next_alarm_today)
|
||||
model.date != null -> Locale.getDefault().let { locale ->
|
||||
DateTimeFormatter.ofPattern(DateFormat.getBestDateTimePattern(locale, "EEEMMMd"), locale).format(model.date)
|
||||
// The year only when it is not this one, as the alarm list does.
|
||||
val skeleton = if (model.dateNeedsYear) "yEEEMMMd" else "EEEMMMd"
|
||||
DateTimeFormatter.ofPattern(DateFormat.getBestDateTimePattern(locale, skeleton), locale).format(model.date)
|
||||
}
|
||||
model.dayOfWeek != null -> model.dayOfWeek.getDisplayName(JavaTextStyle.FULL, Locale.getDefault())
|
||||
else -> ""
|
||||
|
||||
+11
@@ -95,6 +95,17 @@ class NextAlarmWidgetModelTest {
|
||||
val model = NextAlarmWidgetPresenter.present(rows, T0, zone)
|
||||
|
||||
assertThat(model.date).isEqualTo(fireAt.toJavaTime(zone).toLocalDate())
|
||||
assertThat(model.dateNeedsYear).isFalse()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a date in another year says so`() {
|
||||
// T0 is 2023-11-14; seven weeks on is 2024.
|
||||
val rows = listOf(upcomingAlarm(nextFire = T0 + 49.days))
|
||||
|
||||
val model = NextAlarmWidgetPresenter.present(rows, T0, zone)
|
||||
|
||||
assertThat(model.dateNeedsYear).isTrue()
|
||||
}
|
||||
|
||||
private fun Instant.toJavaTime(zone: ZoneId): ZonedDateTime =
|
||||
|
||||
@@ -180,6 +180,17 @@ class AlarmClockHandlerTest {
|
||||
assertThat(harness.alarmScheduler.next?.fireAt).isEqualTo(sevenThirtyToday)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a date that has gone by no longer stops an alarm being re-used`(@TempDir tempDir: Path) = runTest {
|
||||
val harness = interopHarness(tempDir, now = fiveAm)
|
||||
harness.givenAlarm(harness.alarmFor(id = 2L).copy(enabled = false, scheduledDate = LocalDate.of(2026, 6, 9)))
|
||||
|
||||
harness.handler.handle(setAlarm())
|
||||
|
||||
val alarm = harness.storedAlarms().single()
|
||||
assertThat(listOf(alarm.id, alarm.enabled)).containsExactly(2L, true).inOrder()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a re-used alarm never becomes transient`(@TempDir tempDir: Path) = runTest {
|
||||
val harness = interopHarness(tempDir, now = fiveAm)
|
||||
|
||||
@@ -69,6 +69,26 @@ open class FakeAlarmDao(
|
||||
onDeleted?.invoke(id)
|
||||
}
|
||||
|
||||
override suspend fun clearScheduledDate(id: Long, expected: Long, updatedAt: Long) {
|
||||
rows.update { rows ->
|
||||
rows.map {
|
||||
if (it.id == id && it.scheduledDate == expected) it.copy(scheduledDate = null, updatedAt = updatedAt) else it
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
override suspend fun clearPause(id: Long, expectedStart: Long, expectedEnd: Long, updatedAt: Long) {
|
||||
rows.update { rows ->
|
||||
rows.map {
|
||||
if (it.id == id && it.pauseStart == expectedStart && it.pauseEnd == expectedEnd) {
|
||||
it.copy(pauseStart = null, pauseEnd = null, updatedAt = updatedAt)
|
||||
} else {
|
||||
it
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
override suspend fun setEnabled(id: Long, enabled: Boolean, updatedAt: Long) {
|
||||
rows.update { rows ->
|
||||
rows.map { if (it.id == id) it.copy(enabled = enabled, updatedAt = updatedAt) else it }
|
||||
|
||||
@@ -225,4 +225,42 @@ class AlarmEditorScheduleTest {
|
||||
|
||||
assertThat(harness.stored().scheduledDate).isNull()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `removing a pause re-arms a pending skip on the next ring, not the one after the old pause`(
|
||||
@TempDir tempDir: Path,
|
||||
) = runTest(main.dispatcher) {
|
||||
val harness = alarmEngineHarness(tempDir, now = fiveToday)
|
||||
harness.given(
|
||||
alarmAt(hour = 7, repeatDays = RepeatDays.EVERY_DAY, skipNextOccurrence = true)
|
||||
.copy(pause = PauseRange(today, LocalDate.of(2026, 6, 14))),
|
||||
)
|
||||
harness.engine.reschedule()
|
||||
assertThat(harness.scheduler.next?.fireAt).isEqualTo(offsetInstant("2026-06-16T07:00+02:00"))
|
||||
val viewModel = editor(harness)
|
||||
advanceUntilIdle()
|
||||
|
||||
viewModel.onPauseChosen(null)
|
||||
advanceUntilIdle()
|
||||
|
||||
// Today's 07:00 is the one skipped; tomorrow's rings.
|
||||
assertThat(harness.scheduler.next?.fireAt).isEqualTo(offsetInstant("2026-06-11T07:00+02:00"))
|
||||
assertThat(harness.stored().skipNextOccurrence).isTrue()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `adding a pause over the skipped day moves the skip to the first ring after the pause`(
|
||||
@TempDir tempDir: Path,
|
||||
) = runTest(main.dispatcher) {
|
||||
val harness = alarmEngineHarness(tempDir, now = fiveToday)
|
||||
harness.given(alarmAt(hour = 7, repeatDays = RepeatDays.EVERY_DAY, skipNextOccurrence = true))
|
||||
harness.engine.reschedule()
|
||||
val viewModel = editor(harness)
|
||||
advanceUntilIdle()
|
||||
|
||||
viewModel.onPauseChosen(PauseRange(today, LocalDate.of(2026, 6, 12)))
|
||||
advanceUntilIdle()
|
||||
|
||||
assertThat(harness.scheduler.next?.fireAt).isEqualTo(offsetInstant("2026-06-14T07:00+02:00"))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1326,9 +1326,14 @@ single candidate, so clearing it changes nothing but the row. Measuring from
|
||||
midnight would otherwise be cleared at 00:00 and let its last day's 23:59
|
||||
occurrence ring late through the grace window. The resolver reports
|
||||
`clearScheduledDate` / `clearPause` on every branch, a disabled alarm included,
|
||||
and the engine writes them through `AlarmRepository.edit`, conditional on the
|
||||
and the engine writes them as column-targeted `UPDATE`s conditional on the
|
||||
stored value still being the one it resolved, so a pause the editor has just
|
||||
picked is never cleared by a resolution of the old one.
|
||||
picked is never cleared by a resolution of the old one, and no other column is
|
||||
rewritten. Changing a pause calls `AlarmEngine.onPauseChanged`, which drops a
|
||||
skip watermark still in the future so the resolver re-arms it on the new
|
||||
sequence: removing a pause must not leave the skip on the first ring after the
|
||||
old one, and adding a pause over the skipped day must not spend the skip on a
|
||||
day already silent.
|
||||
|
||||
The surfaces never show a stale one in the meantime: `AlarmSchedule` shows a
|
||||
date only while its occurrence is still ahead and a pause only while its last
|
||||
|
||||
Reference in New Issue
Block a user