fix(reminders): close both gaps in disabled-calendar suppression (#17)

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 <noreply@anthropic.com>

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<Long>): 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<ReminderAlert>,
     disabledCalendarIds: Set<Long>,
-): List<ReminderAlert> = due.filterNot { it.calendarId in disabledCalendarIds }
+): List<ReminderAlert> = 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()

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()

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<Preferences>,
+) {
+
+    /** Add [alerts] to the stash, replacing any existing entry with the same id. */
+    suspend fun stash(alerts: List<ReminderAlert>, 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<Long>, nowMillis: Long): List<ReminderAlert> {
+        val recovered = mutableListOf<ReminderAlert>()
+        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<ReminderAlert> =
+        prefs[KEY].orEmpty().mapNotNull { decodeStashEntry(it) }
+
+    private fun MutablePreferences.putStash(alerts: List<ReminderAlert>) {
+        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)

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<Long>) {
+        if (reEnabledIds.isEmpty()) return
+        val recovered = suppressedStore.recoverFor(reEnabledIds, System.currentTimeMillis())
+        if (recovered.isNotEmpty() &&
+            settingsPrefs.remindersEnabled.first() &&
+            notifier.canPost()
+        ) {
+            recovered.forEach { notifier.post(it) }
         }
     }
 

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()
+    }
 }

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<Preferences> =
+        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
+}