From edbeadfa3084b24832228f398a74b9c5d2b77b86 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Sat, 25 Jul 2026 21:33:58 +0200 Subject: [PATCH] fix(reminders): stop marking a silenced reminder handled and losing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "With VISIBLE=0 the provider creates no alert rows" justified deleting the suppression stash, but it only holds where the flag was actually written. Where the switch lives in pendingDisabledCalendarIds — a read-only install, or an upgrade whose flush hasn't landed — the provider still holds VISIBLE=1 and keeps creating and broadcasting rows. The receiver silenced those in ReminderNotifier.post and then marked the whole due batch STATE_FIRED, and dueAlerts only ever returns STATE_SCHEDULED, so switching the calendar back on before the event could no longer surface the reminder: it was gone. post() now reports whether it put a notification up, and the receiver marks what it posted plus what it silenced for an event already over (handledAlertIds). A silenced alert for an event still ahead stays scheduled, which makes the provider's own table the stash SuppressedReminderStore used to be — no local mirror, no serialization. ReminderRecovery re-posts those rows when the calendar is switched back on, so recovery doesn't wait for the next unrelated broadcast. Co-Authored-By: Claude Opus 5 (1M context) --- .../calendula/data/reminders/AlertHandling.kt | 30 +++++++++ .../data/reminders/EventReminderReceiver.kt | 10 ++- .../data/reminders/ReminderNotifier.kt | 11 +++- .../data/reminders/ReminderRecovery.kt | 46 +++++++++++++ .../ui/calendars/CalendarsViewModel.kt | 8 +++ .../data/reminders/AlertHandlingTest.kt | 66 +++++++++++++++++++ 6 files changed, 166 insertions(+), 5 deletions(-) create mode 100644 app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/AlertHandling.kt create mode 100644 app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderRecovery.kt create mode 100644 app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/AlertHandlingTest.kt diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/AlertHandling.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/AlertHandling.kt new file mode 100644 index 0000000..9d6cf57 --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/AlertHandling.kt @@ -0,0 +1,30 @@ +package de.jeanlucmakiola.calendula.data.reminders + +/** + * Still relevant while the event has not ended: a reminder for an event that is + * already over is pointless to re-surface. Falls back to the begin time when the + * end is unknown (0L). + */ +internal fun ReminderAlert.isRelevantAt(nowMillis: Long): Boolean = + (endMillis.takeIf { it > 0L } ?: beginMillis) >= nowMillis + +/** + * The alerts [EventReminderReceiver] may mark handled (`STATE_FIRED`): the ones + * it posted, plus the ones it silenced whose event is already over. + * + * A silenced alert for an event still ahead is deliberately left + * `STATE_SCHEDULED`. Silencing is not handling — the calendar is switched off in + * Calendula while the provider still holds `VISIBLE = 1` (a read-only install, + * or an upgrade whose flush hasn't landed), so switching it back on before the + * event must still be able to surface the reminder. [ReminderAlertStore.dueAlerts] + * only ever returns scheduled rows, so marking them here would lose them for + * good; leaving them makes the provider's own table the stash + * ([ReminderRecovery]). + */ +internal fun handledAlertIds( + due: List, + postedIds: Set, + nowMillis: Long, +): List = due + .filter { it.alertId in postedIds || !it.isRelevantAt(nowMillis) } + .map { it.alertId } diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/EventReminderReceiver.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/EventReminderReceiver.kt index e61dbfb..95067e3 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/EventReminderReceiver.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/EventReminderReceiver.kt @@ -30,7 +30,9 @@ import javax.inject.Inject * Settings → Calendars has `Calendars.VISIBLE = 0`, and the provider creates no * alert rows for it in the first place (#75). The one case that flag can't * cover — a read-only install, which keeps its switches app-side — is gated in - * [ReminderNotifier.post], where the snoozed re-show passes too. + * [ReminderNotifier.post], where the snoozed re-show passes too. What that gate + * silences is *not* marked fired while the event is still ahead, so switching + * the calendar back on can still surface it (see [handledAlertIds]). */ @AndroidEntryPoint class EventReminderReceiver : BroadcastReceiver() { @@ -52,8 +54,10 @@ class EventReminderReceiver : BroadcastReceiver() { if (settingsPrefs.remindersEnabled.first()) { val now = System.currentTimeMillis() val due = alertStore.dueAlerts(now) - due.forEach { notifier.post(it) } - alertStore.markFired(due.map { it.alertId }, now) + val postedIds = due + .filter { notifier.post(it) } + .mapTo(mutableSetOf()) { it.alertId } + alertStore.markFired(handledAlertIds(due, postedIds, now), now) } } finally { pendingResult.finish() diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderNotifier.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderNotifier.kt index 4d89253..0b16714 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderNotifier.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderNotifier.kt @@ -64,8 +64,13 @@ class ReminderNotifier @Inject constructor( calendarId in calendarPrefs.pendingDisabledCalendarIds.first() || calendarDataSource.isCalendarVisible(calendarId) == false - suspend fun post(alert: ReminderAlert) { - if (isSilenced(alert.calendarId)) return + /** + * Post [alert], unless its calendar is switched off. Returns whether the + * notification was put up: a silenced alert must stay unhandled so that + * switching the calendar back on can still surface it (see [handledAlertIds]). + */ + suspend fun post(alert: ReminderAlert): Boolean { + if (isSilenced(alert.calendarId)) return false ensureChannel() val title = alert.title.ifBlank { context.getString(R.string.event_untitled) } val is24Hour = settingsPrefs.timeFormat.first() @@ -117,6 +122,8 @@ class ReminderNotifier @Inject constructor( // POST_NOTIFICATIONS was revoked between canPost() and here. Log.w(TAG, "Could not post reminder for event ${alert.eventId}", e) } + // Handled either way: re-running it would hit the same revoked permission. + return true } /** Remove a posted reminder (snooze re-shows it later; dismiss is final). */ diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderRecovery.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderRecovery.kt new file mode 100644 index 0000000..93bf846 --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderRecovery.kt @@ -0,0 +1,46 @@ +package de.jeanlucmakiola.calendula.data.reminders + +import de.jeanlucmakiola.calendula.data.di.IoDispatcher +import de.jeanlucmakiola.calendula.data.prefs.SettingsPrefs +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.withContext +import javax.inject.Inject +import javax.inject.Singleton + +/** + * Re-posts the reminders a switched-off calendar silenced, when it is switched + * back on while they still matter. + * + * Only the app-side switch needs this — a read-only install, or an upgrade the + * reconciler hasn't flushed yet. With `Calendars.VISIBLE = 0` the provider + * deletes the calendar's alert rows itself and re-creates them on the way back; + * app-side the rows stay, still `STATE_SCHEDULED`, because + * [EventReminderReceiver] deliberately leaves the ones it silenced unhandled + * (see [handledAlertIds]). So the provider's own table is the stash, and nothing + * is mirrored locally. + * + * Best effort at switch-on time: it mirrors the receiver's gates (reminders on, + * notifications postable) and there is no later re-scan, so an alert left + * unposted because those are closed is simply released. + */ +@Singleton +class ReminderRecovery @Inject constructor( + private val alertStore: ReminderAlertStore, + private val notifier: ReminderNotifier, + private val settingsPrefs: SettingsPrefs, + @IoDispatcher private val io: CoroutineDispatcher, +) { + + suspend fun rePostFor(calendarIds: Collection) = withContext(io) { + if (calendarIds.isEmpty()) return@withContext + if (!settingsPrefs.remindersEnabled.first() || !notifier.canPost()) return@withContext + val now = System.currentTimeMillis() + val ids = calendarIds.toSet() + val recovered = alertStore.dueAlerts(now) + .filter { it.calendarId in ids && it.isRelevantAt(now) } + if (recovered.isEmpty()) return@withContext + val postedIds = recovered.filter { notifier.post(it) }.map { it.alertId } + alertStore.markFired(postedIds, now) + } +} diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsViewModel.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsViewModel.kt index 78423c0..1635054 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsViewModel.kt @@ -13,6 +13,7 @@ import de.jeanlucmakiola.calendula.data.di.IoDispatcher import de.jeanlucmakiola.calendula.data.ics.IcsExporter import de.jeanlucmakiola.calendula.data.prefs.BackupStatus import de.jeanlucmakiola.calendula.data.prefs.SettingsPrefs +import de.jeanlucmakiola.calendula.data.reminders.ReminderRecovery import de.jeanlucmakiola.calendula.domain.CalendarSource import de.jeanlucmakiola.calendula.domain.ics.IcsWriter import kotlinx.coroutines.CoroutineDispatcher @@ -43,6 +44,7 @@ class CalendarsViewModel @Inject constructor( private val repository: CalendarRepository, private val icsExporter: IcsExporter, private val settingsPrefs: SettingsPrefs, + private val reminderRecovery: ReminderRecovery, @IoDispatcher private val io: CoroutineDispatcher, ) : ViewModel() { @@ -125,9 +127,14 @@ class CalendarsViewModel @Inject constructor( * every surface *and* the provider stops (or resumes) scheduling its * reminders. Nothing is patched by hand — the provider notifies and the * observer re-queries. + * + * Switching one back on also re-posts the reminders it silenced while it was + * off and that are still relevant — those the app kept app-side because it + * may not write the flag ([ReminderRecovery]). */ fun setCalendarVisible(id: Long, visible: Boolean) = write { repository.setCalendarsVisible(listOf(id), visible) + if (visible) reminderRecovery.rePostFor(listOf(id)) } /** @@ -138,6 +145,7 @@ class CalendarsViewModel @Inject constructor( */ fun setAccountVisible(ids: Collection, visible: Boolean) = write { repository.setCalendarsVisible(ids, visible) + if (visible) reminderRecovery.rePostFor(ids) } // --- Automatic backup (issue #8) ------------------------------------ diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/AlertHandlingTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/AlertHandlingTest.kt new file mode 100644 index 0000000..9b377e2 --- /dev/null +++ b/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/AlertHandlingTest.kt @@ -0,0 +1,66 @@ +package de.jeanlucmakiola.calendula.data.reminders + +import com.google.common.truth.Truth.assertThat +import org.junit.jupiter.api.Test + +/** + * What the reminder receiver may mark handled. The silenced-but-still-ahead case + * is the one that matters: those rows are the only copy of the reminder (#75). + */ +class AlertHandlingTest { + + private val now = 1_700_000_000_000L + + private fun alert( + id: Long, + calendarId: Long = 1L, + beginMillis: Long = now + 60_000L, + endMillis: Long = now + 3_600_000L, + ) = ReminderAlert( + alertId = id, + eventId = id * 10, + calendarId = calendarId, + beginMillis = beginMillis, + endMillis = endMillis, + title = "E $id", + location = null, + isAllDay = false, + ) + + @Test + fun `posted alerts are handled`() { + val due = listOf(alert(1L), alert(2L)) + + assertThat(handledAlertIds(due, postedIds = setOf(1L, 2L), nowMillis = now)) + .containsExactly(1L, 2L) + } + + @Test + fun `a silenced alert whose event is still ahead stays unhandled`() { + // Switching its calendar back on before the event has to bring it back, + // and dueAlerts only ever returns STATE_SCHEDULED rows. + val due = listOf(alert(1L), alert(2L)) + + assertThat(handledAlertIds(due, postedIds = setOf(1L), nowMillis = now)) + .containsExactly(1L) + } + + @Test + fun `a silenced alert whose event is over is handled`() { + // Nothing left to re-surface, so it must not linger as scheduled. + val over = alert(2L, beginMillis = now - 7_200_000L, endMillis = now - 3_600_000L) + val due = listOf(alert(1L), over) + + assertThat(handledAlertIds(due, postedIds = setOf(1L), nowMillis = now)) + .containsExactly(1L, 2L) + } + + @Test + fun `an unknown end time falls back to the begin time`() { + val started = alert(1L, beginMillis = now - 1L, endMillis = 0L) + val notYet = alert(2L, beginMillis = now + 1L, endMillis = 0L) + + assertThat(handledAlertIds(listOf(started, notYet), postedIds = emptySet(), nowMillis = now)) + .containsExactly(1L) + } +}