From 037d05b171e052a5823b72b0595bb7afd89724c0 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Thu, 2 Jul 2026 23:01:41 +0200 Subject: [PATCH] fix(reminders): close both gaps in disabled-calendar suppression (#17) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two holes in the v2.13.0 'disabled calendars no longer notify' fix: - A snoozed reminder bypassed the filter: ReminderActionReceiver's ACTION_SHOW posted unconditionally. The gate now lives once in ReminderNotifier.post — the single choke point both receivers use — built on a shared ReminderAlert.isForDisabledCalendar predicate that never treats calendarId 0L (pre-upgrade snooze intents without EXTRA_CALENDAR_ID) as disabled. - A reminder firing while its calendar was disabled was lost forever: the receiver marks the full due set STATE_FIRED (deliberately, to stop provider re-broadcasts) and nothing re-scans. Suppressed alerts are now stashed in a DataStore-backed SuppressedReminderStore and re-posted when the calendar is re-enabled, while the event hasn't ended yet; expired entries are purged opportunistically. Co-Authored-By: Claude Opus 4.8 --- .../data/reminders/EventReminderReceiver.kt | 30 +++- .../data/reminders/ReminderNotifier.kt | 7 + .../data/reminders/SuppressedReminderStore.kt | 130 ++++++++++++++++++ .../ui/calendars/CalendarsViewModel.kt | 33 ++++- .../data/reminders/PostableAlertsTest.kt | 16 +++ .../reminders/SuppressedReminderStoreTest.kt | 129 +++++++++++++++++ 6 files changed, 337 insertions(+), 8 deletions(-) create mode 100644 app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/SuppressedReminderStore.kt create mode 100644 app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/SuppressedReminderStoreTest.kt 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 dc6871f..22fc5cf 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 @@ -17,16 +17,28 @@ import kotlinx.coroutines.flow.first import kotlinx.coroutines.launch import javax.inject.Inject +/** + * True when [this] alert belongs to a calendar the user disabled in-app, so its + * reminder must be suppressed (mirroring the event filtering in + * CalendarRepositoryImpl). Alerts whose calendar is unknown (id 0L — e.g. a + * pre-upgrade snooze PendingIntent minted before EXTRA_CALENDAR_ID existed) are + * never treated as disabled. This is the one predicate the disabled-calendar + * gate is built from: [postableAlerts] here and the choke point in + * [ReminderNotifier.post] both use it. + */ +internal fun ReminderAlert.isForDisabledCalendar(disabledCalendarIds: Set): Boolean = + calendarId != 0L && calendarId in disabledCalendarIds + /** * The due alerts that should actually surface as notifications: everything - * except alerts whose calendar the user has disabled in-app (mirroring the - * event filtering in CalendarRepositoryImpl). The caller still marks the full - * due set fired, so suppressed alerts are not re-broadcast by the provider. + * except alerts whose calendar the user has disabled in-app. The caller still + * marks the full due set fired, so suppressed alerts are not re-broadcast by the + * provider. */ internal fun postableAlerts( due: List, disabledCalendarIds: Set, -): List = due.filterNot { it.calendarId in disabledCalendarIds } +): List = due.filterNot { it.isForDisabledCalendar(disabledCalendarIds) } /** * Becomes the app that turns the calendar provider's reminder alarms into @@ -45,6 +57,7 @@ class EventReminderReceiver : BroadcastReceiver() { @Inject lateinit var notifier: ReminderNotifier @Inject lateinit var settingsPrefs: SettingsPrefs @Inject lateinit var calendarPrefs: CalendarPrefs + @Inject lateinit var suppressedStore: SuppressedReminderStore override fun onReceive(context: Context, intent: Intent) { if (intent.action != CalendarContract.ACTION_EVENT_REMINDER) return @@ -60,11 +73,16 @@ class EventReminderReceiver : BroadcastReceiver() { val now = System.currentTimeMillis() val due = alertStore.dueAlerts(now) val disabled = calendarPrefs.disabledCalendarIds.first() + val postable = postableAlerts(due, disabled) // Suppress reminders for disabled calendars, but still mark // every due alert fired so the provider stops re-broadcasting - // the suppressed ones. - postableAlerts(due, disabled).forEach { notifier.post(it) } + // the suppressed ones. Stash those suppressed alerts so + // re-enabling their calendar can recover them (they would + // otherwise stay STATE_FIRED forever with no re-scan). + postable.forEach { notifier.post(it) } alertStore.markFired(due.map { it.alertId }, now) + suppressedStore.stash(due - postable.toSet(), now) + suppressedStore.purgeExpired(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 7b92a9d..9bd0dde 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 @@ -14,6 +14,7 @@ import androidx.core.content.ContextCompat import dagger.hilt.android.qualifiers.ApplicationContext import de.jeanlucmakiola.calendula.MainActivity import de.jeanlucmakiola.calendula.R +import de.jeanlucmakiola.calendula.data.prefs.CalendarPrefs import de.jeanlucmakiola.calendula.data.prefs.SettingsPrefs import de.jeanlucmakiola.calendula.data.prefs.is24Hour import kotlinx.coroutines.flow.first @@ -33,6 +34,7 @@ import javax.inject.Singleton class ReminderNotifier @Inject constructor( @ApplicationContext private val context: Context, private val settingsPrefs: SettingsPrefs, + private val calendarPrefs: CalendarPrefs, ) { /** False when the user declined `POST_NOTIFICATIONS` or muted the app. */ @@ -44,6 +46,11 @@ class ReminderNotifier @Inject constructor( } suspend fun post(alert: ReminderAlert) { + // The single choke point for the disabled-calendar gate: it covers both + // the provider broadcast (EventReminderReceiver) and a snoozed re-show + // (ReminderActionReceiver), so a calendar disabled after a snooze no + // longer notifies — without either receiver duplicating the check. + if (alert.isForDisabledCalendar(calendarPrefs.disabledCalendarIds.first())) return ensureChannel() val title = alert.title.ifBlank { context.getString(R.string.event_untitled) } val is24Hour = settingsPrefs.timeFormat.first() diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/SuppressedReminderStore.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/SuppressedReminderStore.kt new file mode 100644 index 0000000..eb5b6e0 --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/SuppressedReminderStore.kt @@ -0,0 +1,130 @@ +package de.jeanlucmakiola.calendula.data.reminders + +import androidx.datastore.core.DataStore +import androidx.datastore.preferences.core.MutablePreferences +import androidx.datastore.preferences.core.Preferences +import androidx.datastore.preferences.core.edit +import androidx.datastore.preferences.core.stringSetPreferencesKey +import java.util.Base64 +import javax.inject.Inject +import javax.inject.Singleton + +/** + * 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). Used both to decide what to re-post and to purge the stash. + */ +internal fun ReminderAlert.isRelevantAt(nowMillis: Long): Boolean = + (endMillis.takeIf { it > 0L } ?: beginMillis) >= nowMillis + +/** + * Local stash of reminder alerts that fired while their calendar was disabled + * in-app. [EventReminderReceiver] marks every due alert `STATE_FIRED` regardless + * (so the provider stops re-broadcasting the suppressed ones), which would + * otherwise lose those reminders forever — there is no re-scan. Stashing lets + * [de.jeanlucmakiola.calendula.ui.calendars.CalendarsViewModel] re-post them if + * the user re-enables the calendar before the event is over. + * + * Persisted in the shared preferences DataStore as a set of self-describing + * strings (one per alert); the stash never reaches a screen, so there is no + * domain model. Entries whose event has already ended are dropped on the next + * stash/recover/purge, so the stash only ever holds a handful of pending alerts. + */ +@Singleton +class SuppressedReminderStore @Inject constructor( + private val store: DataStore, +) { + + /** Add [alerts] to the stash, replacing any existing entry with the same id. */ + suspend fun stash(alerts: List, nowMillis: Long) { + if (alerts.isEmpty()) return + store.edit { prefs -> + val byId = decodeAll(prefs).associateByTo(mutableMapOf()) { it.alertId } + alerts.forEach { byId[it.alertId] = it } + prefs.putStash(byId.values.filter { it.isRelevantAt(nowMillis) }) + } + } + + /** + * Remove and return the still-relevant stashed alerts belonging to any of + * [calendarIds]; drops expired entries for every calendar in passing. + */ + suspend fun recoverFor(calendarIds: Set, nowMillis: Long): List { + val recovered = mutableListOf() + store.edit { prefs -> + val kept = decodeAll(prefs).filter { alert -> + when { + !alert.isRelevantAt(nowMillis) -> false // expired: drop + alert.calendarId in calendarIds -> { recovered += alert; false } + else -> true + } + } + prefs.putStash(kept) + } + return recovered + } + + /** Drop entries whose event has already ended — cheap opportunistic cleanup. */ + suspend fun purgeExpired(nowMillis: Long) { + store.edit { prefs -> + prefs.putStash(decodeAll(prefs).filter { it.isRelevantAt(nowMillis) }) + } + } + + private fun decodeAll(prefs: Preferences): List = + prefs[KEY].orEmpty().mapNotNull { decodeStashEntry(it) } + + private fun MutablePreferences.putStash(alerts: List) { + val encoded = alerts.map { encodeStashEntry(it) }.toSet() + if (encoded.isEmpty()) remove(KEY) else set(KEY, encoded) + } + + private companion object { + val KEY = stringSetPreferencesKey("suppressed_reminders") + } +} + +// One stash entry as a delimited string. The '|' separator is safe because every +// free-text field is Base64-encoded first (that alphabet never contains '|'), and +// a null location is stored as a distinct sentinel that Base64 also never yields. +private const val FIELD_SEP = "|" +private const val NULL_LOCATION = "-" + +internal fun encodeStashEntry(alert: ReminderAlert): String = listOf( + alert.alertId.toString(), + alert.eventId.toString(), + alert.calendarId.toString(), + alert.beginMillis.toString(), + alert.endMillis.toString(), + if (alert.isAllDay) "1" else "0", + alert.title.toBase64(), + alert.location?.toBase64() ?: NULL_LOCATION, +).joinToString(FIELD_SEP) + +/** Reverse of [encodeStashEntry]; returns null for a malformed entry (dropped). */ +internal fun decodeStashEntry(raw: String): ReminderAlert? { + val parts = raw.split(FIELD_SEP) + if (parts.size != 8) return null + return try { + ReminderAlert( + alertId = parts[0].toLong(), + eventId = parts[1].toLong(), + calendarId = parts[2].toLong(), + beginMillis = parts[3].toLong(), + endMillis = parts[4].toLong(), + title = parts[6].fromBase64(), + location = parts[7].takeIf { it != NULL_LOCATION }?.fromBase64(), + isAllDay = parts[5] == "1", + ) + } catch (e: NumberFormatException) { + null + } catch (e: IllegalArgumentException) { // bad Base64 + null + } +} + +private fun String.toBase64(): String = + Base64.getEncoder().encodeToString(toByteArray(Charsets.UTF_8)) + +private fun String.fromBase64(): String = + String(Base64.getDecoder().decode(this), Charsets.UTF_8) 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 d56ee9a..bfadaf6 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 @@ -14,6 +14,8 @@ import de.jeanlucmakiola.calendula.data.ics.IcsExporter import de.jeanlucmakiola.calendula.data.prefs.BackupStatus import de.jeanlucmakiola.calendula.data.prefs.CalendarPrefs import de.jeanlucmakiola.calendula.data.prefs.SettingsPrefs +import de.jeanlucmakiola.calendula.data.reminders.ReminderNotifier +import de.jeanlucmakiola.calendula.data.reminders.SuppressedReminderStore import de.jeanlucmakiola.calendula.domain.CalendarSource import de.jeanlucmakiola.calendula.domain.ics.IcsWriter import kotlinx.coroutines.CoroutineDispatcher @@ -45,6 +47,8 @@ class CalendarsViewModel @Inject constructor( private val icsExporter: IcsExporter, private val prefs: CalendarPrefs, private val settingsPrefs: SettingsPrefs, + private val suppressedStore: SuppressedReminderStore, + private val notifier: ReminderNotifier, @IoDispatcher private val io: CoroutineDispatcher, ) : ViewModel() { @@ -144,7 +148,10 @@ class CalendarsViewModel @Inject constructor( viewModelScope.launch { val current = prefs.disabledCalendarIds.first() val next = if (disabled) current + id else current - id - if (next != current) prefs.setDisabledCalendarIds(next) + if (next != current) { + prefs.setDisabledCalendarIds(next) + if (!disabled) recoverReminders(setOf(id)) + } } } @@ -157,7 +164,29 @@ class CalendarsViewModel @Inject constructor( viewModelScope.launch { val current = prefs.disabledCalendarIds.first() val next = if (disabled) current + ids else current - ids.toSet() - if (next != current) prefs.setDisabledCalendarIds(next) + if (next != current) { + prefs.setDisabledCalendarIds(next) + if (!disabled) recoverReminders(current intersect ids.toSet()) + } + } + } + + /** + * Re-post the reminders that fired while [reEnabledIds] were disabled and are + * still relevant (event not yet over), then drop them from the stash. Runs + * after the disabled set is written, so the notifier's own disabled gate lets + * them through. Best-effort at re-enable time: it mirrors the receiver gates + * (reminders on + postable), and there is no later re-scan, so alerts left + * unposted because those gates are closed are simply released. + */ + private suspend fun recoverReminders(reEnabledIds: Set) { + if (reEnabledIds.isEmpty()) return + val recovered = suppressedStore.recoverFor(reEnabledIds, System.currentTimeMillis()) + if (recovered.isNotEmpty() && + settingsPrefs.remindersEnabled.first() && + notifier.canPost() + ) { + recovered.forEach { notifier.post(it) } } } diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/PostableAlertsTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/PostableAlertsTest.kt index 222281e..5fedc7d 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/PostableAlertsTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/PostableAlertsTest.kt @@ -52,4 +52,20 @@ class PostableAlertsTest { assertThat(postable).isEqualTo(due) } + + @Test + fun `alert with unknown calendar id 0 is never treated as disabled`() { + // Pre-upgrade snooze PendingIntents carry no calendar id (defaults to 0L). + val preUpgrade = alert(1, calendarId = 0L) + + assertThat(preUpgrade.isForDisabledCalendar(disabledCalendarIds = setOf(0L))).isFalse() + assertThat(postableAlerts(listOf(preUpgrade), disabledCalendarIds = setOf(0L))) + .containsExactly(preUpgrade) + } + + @Test + fun `isForDisabledCalendar matches only the disabled ids`() { + assertThat(alert(1, calendarId = 200).isForDisabledCalendar(setOf(200))).isTrue() + assertThat(alert(1, calendarId = 100).isForDisabledCalendar(setOf(200))).isFalse() + } } diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/SuppressedReminderStoreTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/SuppressedReminderStoreTest.kt new file mode 100644 index 0000000..74c5eda --- /dev/null +++ b/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/SuppressedReminderStoreTest.kt @@ -0,0 +1,129 @@ +package de.jeanlucmakiola.calendula.data.reminders + +import androidx.datastore.core.DataStore +import androidx.datastore.preferences.core.PreferenceDataStoreFactory +import androidx.datastore.preferences.core.Preferences +import com.google.common.truth.Truth.assertThat +import kotlinx.coroutines.test.runTest +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.io.TempDir +import java.nio.file.Path + +class SuppressedReminderStoreTest { + + private fun newDataStore(tempDir: Path): DataStore = + PreferenceDataStoreFactory.create( + produceFile = { tempDir.resolve("test_prefs.preferences_pb").toFile() }, + ) + + private fun alert( + alertId: Long, + calendarId: Long, + endMillis: Long = Long.MAX_VALUE, + title: String = "Event $alertId", + location: String? = null, + ) = ReminderAlert( + alertId = alertId, + eventId = alertId * 10, + calendarId = calendarId, + beginMillis = 0L, + endMillis = endMillis, + title = title, + location = location, + isAllDay = false, + ) + + @Test + fun `encode then decode round-trips every field including delimiters`() { + val original = alert( + alertId = 7, + calendarId = 42, + endMillis = 123_456_789L, + // Free-text with the field separator and other awkward characters. + title = "Lunch | with | Alice", + location = "Café, 3rd floor | room B", + ).copy(beginMillis = 100L, isAllDay = true) + + val decoded = decodeStashEntry(encodeStashEntry(original)) + + assertThat(decoded).isEqualTo(original) + } + + @Test + fun `decode returns null for a malformed entry`() { + assertThat(decodeStashEntry("not-a-valid-entry")).isNull() + } + + @Test + fun `null location round-trips`() { + val original = alert(1, calendarId = 1, location = null) + assertThat(decodeStashEntry(encodeStashEntry(original))).isEqualTo(original) + } + + @Test + fun `recoverFor returns and removes only the re-enabled calendars`() = runTest { + val store = SuppressedReminderStore(newDataStore(tempDir)) + val keep = alert(1, calendarId = 100) + val recoverA = alert(2, calendarId = 200) + val recoverB = alert(3, calendarId = 200) + store.stash(listOf(keep, recoverA, recoverB), nowMillis = 0L) + + val recovered = store.recoverFor(setOf(200L), nowMillis = 0L) + + assertThat(recovered).containsExactly(recoverA, recoverB) + // The still-disabled calendar's alert stays stashed; the recovered ones are gone. + assertThat(store.recoverFor(setOf(100L, 200L), nowMillis = 0L)).containsExactly(keep) + } + + @Test + fun `stash drops alerts whose event already ended`() = runTest { + val store = SuppressedReminderStore(newDataStore(tempDir)) + val past = alert(1, calendarId = 100, endMillis = 500L) + val future = alert(2, calendarId = 100, endMillis = 2_000L) + + store.stash(listOf(past, future), nowMillis = 1_000L) + + assertThat(store.recoverFor(setOf(100L), nowMillis = 1_000L)).containsExactly(future) + } + + @Test + fun `purgeExpired removes only entries past their event end`() = runTest { + val store = SuppressedReminderStore(newDataStore(tempDir)) + // Stash both before "now" so neither is dropped on write, then advance time. + store.stash( + listOf( + alert(1, calendarId = 100, endMillis = 500L), + alert(2, calendarId = 100, endMillis = 2_000L), + ), + nowMillis = 0L, + ) + + store.purgeExpired(nowMillis = 1_000L) + + assertThat(store.recoverFor(setOf(100L), nowMillis = 0L).map { it.alertId }) + .containsExactly(2L) + } + + @Test + fun `stash replaces an existing entry with the same alert id`() = runTest { + val store = SuppressedReminderStore(newDataStore(tempDir)) + store.stash(listOf(alert(1, calendarId = 100, title = "old")), nowMillis = 0L) + store.stash(listOf(alert(1, calendarId = 100, title = "new")), nowMillis = 0L) + + val recovered = store.recoverFor(setOf(100L), nowMillis = 0L) + + assertThat(recovered).hasSize(1) + assertThat(recovered.single().title).isEqualTo("new") + } + + @Test + fun `isRelevantAt is true up to the event end and false after`() { + val a = alert(1, calendarId = 1, endMillis = 1_000L) + assertThat(a.isRelevantAt(999L)).isTrue() + assertThat(a.isRelevantAt(1_000L)).isTrue() + assertThat(a.isRelevantAt(1_001L)).isFalse() + } + + @TempDir + lateinit var tempDir: Path +}