fix(alarms): refuse a list delete of a ringing alarm in the engine
The list's delete re-checks for a ring under the engine lock, so a swipe released as the alarm fires cannot silence it round its dismiss challenge. An Undo whose snackbar is already up no longer goes stale on a rotation after a long accessibility timeout, and a deleted card is thrown clear of the screen edge.
This commit is contained in:
1 parent
697678b226
commit
5f3b97b32a
12 files changed
+72
-20
No files matched your search
@@ -145,14 +145,20 @@ class AlarmEngine @Inject constructor(
|
||||
* otherwise outlive the row it belongs to. The ring state goes with the row
|
||||
* (the foreign key cascades). Non-cancellable, like [restore]: a caller
|
||||
* cancelled half-way must not leave the slot registered for a deleted row.
|
||||
*
|
||||
* [unlessRinging] is the list's delete: a swipe released just as the alarm
|
||||
* fired, before the list knew, must not silence it round its dismiss
|
||||
* challenge, so it writes nothing and returns null. Checked here, under the
|
||||
* lock, because only here is the answer not already stale.
|
||||
*/
|
||||
suspend fun delete(alarmId: Long): DeletedAlarm? = lock.withLock {
|
||||
withContext(NonCancellable) { deleteLocked(alarmId) }
|
||||
suspend fun delete(alarmId: Long, unlessRinging: Boolean = false): DeletedAlarm? = lock.withLock {
|
||||
withContext(NonCancellable) { deleteLocked(alarmId, unlessRinging) }
|
||||
}
|
||||
|
||||
private suspend fun deleteLocked(alarmId: Long): DeletedAlarm? {
|
||||
private suspend fun deleteLocked(alarmId: Long, unlessRinging: Boolean): DeletedAlarm? {
|
||||
val alarm = alarms.find(alarmId) ?: return null
|
||||
val state = states.state(alarmId)
|
||||
if (unlessRinging && state.isRinging) return null
|
||||
silenceLocked(alarmId, state)
|
||||
alarms.delete(alarmId)
|
||||
rescheduleLocked()
|
||||
|
||||
@@ -84,6 +84,7 @@ fun AlarmsScreen(
|
||||
onUndo = viewModel::onUndoDelete,
|
||||
onExpired = viewModel::onUndoExpired,
|
||||
isStale = viewModel::isUndoStale,
|
||||
onShown = viewModel::onUndoShown,
|
||||
)
|
||||
|
||||
CollapsingScaffold(
|
||||
|
||||
@@ -91,7 +91,9 @@ class AlarmsViewModel @Inject internal constructor(
|
||||
* delete the user meant.
|
||||
*/
|
||||
fun onDelete(alarmId: Long) {
|
||||
viewModelScope.launch { engine.delete(alarmId)?.let(undo::offer) }
|
||||
// Never a ringing alarm: the card leads to its ring screen then, and the
|
||||
// engine re-checks under its lock in case the ring began mid-swipe.
|
||||
viewModelScope.launch { engine.delete(alarmId, unlessRinging = true)?.let(undo::offer) }
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -112,6 +114,9 @@ class AlarmsViewModel @Inject internal constructor(
|
||||
/** Offered too long ago to show a snackbar for now ([AlarmUndo.isStale]). */
|
||||
fun isUndoStale(deleted: DeletedAlarm): Boolean = undo.isStale(deleted)
|
||||
|
||||
/** The snackbar is up; only its answer settles the deletion now. */
|
||||
fun onUndoShown(deleted: DeletedAlarm) = undo.markShown(deleted)
|
||||
|
||||
/** Dismisses a ring first when switching off (M5 D11), then re-resolves. */
|
||||
fun onToggleEnabled(alarmId: Long, enabled: Boolean) {
|
||||
viewModelScope.launch {
|
||||
|
||||
@@ -138,7 +138,9 @@ internal fun SwipeToDelete(
|
||||
}
|
||||
scope.launch {
|
||||
if (width > 0 && delete) {
|
||||
travel.animateTo(width.toFloat(), settleSpec)
|
||||
// Past its own width: the card sits inset from the
|
||||
// edge, and stopping at its width would leave a sliver.
|
||||
travel.animateTo(width * ThrowFraction, settleSpec)
|
||||
// Re-read after the animation: the card may have been
|
||||
// disabled meanwhile (its alarm started ringing).
|
||||
if (currentEnabled) currentOnDelete()
|
||||
@@ -179,6 +181,9 @@ internal fun SwipeToDelete(
|
||||
}
|
||||
}
|
||||
|
||||
/** How far a deleted card is thrown, as a share of its width, to clear the screen edge. */
|
||||
private const val ThrowFraction = 1.25f
|
||||
|
||||
/** Past this share of the card's width, letting go deletes. */
|
||||
private const val DeleteFraction = 0.4f
|
||||
|
||||
|
||||
@@ -21,12 +21,17 @@ open class UndoSlot<T : Any>(private val clock: ElapsedRealtimeClock) {
|
||||
@Volatile
|
||||
private var offeredAt: Duration = Duration.ZERO
|
||||
|
||||
/** The offer a snackbar has already been put up for, which can no longer go stale. */
|
||||
@Volatile
|
||||
private var shown: T? = null
|
||||
|
||||
/** Null when there is nothing to undo. */
|
||||
val pending: StateFlow<T?> = slot.asStateFlow()
|
||||
|
||||
/** A newer deletion replaces an older one: only the latest snackbar is on screen. */
|
||||
fun offer(deleted: T) {
|
||||
offeredAt = clock.elapsedRealtime()
|
||||
shown = null
|
||||
slot.value = deleted
|
||||
}
|
||||
|
||||
@@ -41,10 +46,19 @@ open class UndoSlot<T : Any>(private val clock: ElapsedRealtimeClock) {
|
||||
* Whether [deleted] was offered too long ago to put a snackbar up for now.
|
||||
* A delete can land while no list is on screen to show it (the app sent
|
||||
* to the background the moment the editor's Delete was tapped); offered
|
||||
* hours later, its Undo would restore a snapshot out of any context.
|
||||
* hours later, its Undo would restore a snapshot out of any context. Never
|
||||
* true once [markShown].
|
||||
*/
|
||||
fun isStale(deleted: T): Boolean =
|
||||
slot.value == deleted && clock.elapsedRealtime() - offeredAt > SHOW_WITHIN
|
||||
slot.value == deleted && shown != deleted && clock.elapsedRealtime() - offeredAt > SHOW_WITHIN
|
||||
|
||||
/**
|
||||
* A snackbar is up for [deleted]. From now on only its answer settles it, so
|
||||
* a rotation after a long accessibility timeout still shows it again.
|
||||
*/
|
||||
fun markShown(deleted: T) {
|
||||
if (slot.value == deleted) shown = deleted
|
||||
}
|
||||
|
||||
private companion object {
|
||||
/** Comfortably longer than a rotation, far shorter than a forgotten app. */
|
||||
|
||||
@@ -32,7 +32,8 @@ internal fun <T : Any> UndoSnackbarEffect(
|
||||
message: String,
|
||||
onUndo: (T) -> Unit,
|
||||
onExpired: (T) -> Unit,
|
||||
isStale: (T) -> Boolean = { false },
|
||||
isStale: (T) -> Boolean,
|
||||
onShown: (T) -> Unit,
|
||||
) {
|
||||
val activity = LocalActivity.current
|
||||
val undoLabel = stringResource(R.string.action_undo)
|
||||
@@ -44,6 +45,7 @@ internal fun <T : Any> UndoSnackbarEffect(
|
||||
currentOnExpired(deleted)
|
||||
return@LaunchedEffect
|
||||
}
|
||||
onShown(deleted)
|
||||
val result = try {
|
||||
host.showSnackbar(message = message, actionLabel = undoLabel, duration = SnackbarDuration.Long)
|
||||
} catch (cancelled: CancellationException) {
|
||||
|
||||
@@ -65,6 +65,7 @@ fun TimersScreen(
|
||||
onUndo = viewModel::onUndoDelete,
|
||||
onExpired = viewModel::onUndoExpired,
|
||||
isStale = viewModel::isUndoStale,
|
||||
onShown = viewModel::onUndoShown,
|
||||
)
|
||||
|
||||
val loaded = state as? TimersUiState.Loaded
|
||||
|
||||
@@ -122,6 +122,9 @@ class TimersViewModel @Inject internal constructor(
|
||||
/** Offered too long ago to show a snackbar for now ([UndoSlot.isStale]). */
|
||||
fun isUndoStale(timer: Timer): Boolean = undo.isStale(timer)
|
||||
|
||||
/** The snackbar is up; only its answer settles the deletion now. */
|
||||
fun onUndoShown(timer: Timer) = undo.markShown(timer)
|
||||
|
||||
fun onSavePreset(duration: Duration) {
|
||||
viewModelScope.launch { settings.addTimerPreset(duration) }
|
||||
}
|
||||
|
||||
@@ -89,6 +89,18 @@ class AlarmEngineDeleteTest {
|
||||
.isEqualTo(listOf(RingEvent.Stopped(1L), null, null))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `the list's delete leaves a ringing alarm ringing`(@TempDir dir: Path) = runTest {
|
||||
val harness = alarmEngineHarness(dir)
|
||||
harness.givenAlarm(id = 1L, hour = 7)
|
||||
harness.givenRinging(alarmId = 1L, at = sevenToday)
|
||||
|
||||
val deleted = harness.engine.delete(1L, unlessRinging = true)
|
||||
|
||||
assertThat(listOf(deleted, harness.ring.ringing.value, harness.alarms.find(1L)?.id))
|
||||
.isEqualTo(listOf(null, 1L, 1L))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a ringing one-shot comes back switched off and silent`(@TempDir dir: Path) = runTest {
|
||||
val harness = alarmEngineHarness(dir)
|
||||
|
||||
@@ -424,7 +424,7 @@ class AlarmsViewModelTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `swiping away the ringing alarm silences it, and undo does not ring it again`(@TempDir tempDir: Path) =
|
||||
fun `a ringing alarm cannot be swiped away round its ring screen`(@TempDir tempDir: Path) =
|
||||
runTest(main.dispatcher) {
|
||||
val harness = alarmEngineHarness(tempDir)
|
||||
harness.givenAlarm(id = 1L, hour = 7, repeatDays = RepeatDays.EVERY_DAY)
|
||||
@@ -434,17 +434,9 @@ class AlarmsViewModelTest {
|
||||
|
||||
viewModel.onDelete(1L)
|
||||
advanceUntilIdle()
|
||||
viewModel.onUndoDelete(checkNotNull(viewModel.pendingUndo.value))
|
||||
advanceUntilIdle()
|
||||
|
||||
assertThat(
|
||||
listOf(
|
||||
harness.ring.events.last(),
|
||||
harness.ring.ringing.value,
|
||||
harness.scheduler.autoSilence,
|
||||
harness.scheduler.next?.fireAt,
|
||||
),
|
||||
).isEqualTo(listOf(RingEvent.Stopped(1L), null, null, offsetInstant("2026-06-11T07:00+02:00")))
|
||||
assertThat(listOf(harness.ring.ringing.value, harness.alarms.find(1L)?.id, viewModel.pendingUndo.value))
|
||||
.isEqualTo(listOf(1L, 1L, null))
|
||||
}
|
||||
|
||||
// --- M9: the AlarmClock contract's dismissal, and the chooser dialog's ---
|
||||
|
||||
@@ -37,4 +37,13 @@ class UndoSlotTest {
|
||||
|
||||
assertThat(fresh to slot.isStale("07:00")).isEqualTo(false to true)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `an offer already on screen never goes stale`() {
|
||||
slot.offer("07:00")
|
||||
slot.markShown("07:00")
|
||||
clock.advance(10.minutes)
|
||||
|
||||
assertThat(slot.isStale("07:00")).isFalse()
|
||||
}
|
||||
}
|
||||
@@ -1720,7 +1720,9 @@ An alarm is deleted by swiping its card towards the end edge, by the card's
|
||||
access, where there is no swipe), or from the editor's top bar. None of them
|
||||
asks first. A *ringing* card offers neither the swipe nor the action: it leads
|
||||
to the ring screen, and a swipe must not become a way round its dismiss
|
||||
challenge. Each is followed by a snackbar on the list ("07:00 alarm deleted",
|
||||
challenge. The engine re-checks under its lock (`delete(id, unlessRinging = true)`),
|
||||
so a swipe released just as the alarm fires, before the list has caught up,
|
||||
deletes nothing and the card slides back. Each is followed by a snackbar on the list ("07:00 alarm deleted",
|
||||
**Undo**), as in Google Clock. v1 asked before instead, on the argument that a
|
||||
mistaken delete is found at 07:00 rather than now. But a dialog catches no more
|
||||
than the snackbar does: both only catch the slip noticed in the moment, and a
|
||||
|
||||
Reference in new issue
Block a user