One reminder notification per event occurrence (#463) (#468)

An event with several reminders posted a separate notification for each one. Each occurrence now has one notification, and every later reminder replaces the earlier one in place.

- The notification tag and the snooze alarm are keyed on the occurrence. Snooze, dismiss and open actions stay per reminder, so tapping them on an outdated notification can't remove the newer one.
- A different reminder alerts again; the same reminder posted twice stays silent.
- If several reminders of one occurrence are due in the same scan (e.g. after the phone was off), only the latest is posted.
- A newly due reminder cancels a pending snooze for its occurrence, and a snoozed reminder doesn't re-show over a newer one.
- Notifications posted before this change still dismiss and snooze, and a newer reminder for the same event replaces them.
- Reminder and occurrence keys share one hash in `ReminderPlan`.

Closes #463

Co-authored-by: Jean-Luc Makiola <business@jeanlucmakiola.de>
Reviewed-on: https://codeberg.org/jlmakiola/calendula/pulls/468

app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderActionReceiver.kt

@@ -38,6 +38,8 @@ class ReminderActionReceiver : BroadcastReceiver() {
             ACTION_DISMISS -> notifier.cancel(alert)
 
             ACTION_SNOOZE -> {
+                // Tapped on a stale view: the reminder that replaced it stays put.
+                if (notifier.isSuperseded(alert)) return
                 // Cancel now so the notification doesn't linger until the alarm;
                 // the snooze delay read is the only async work.
                 notifier.cancel(alert)
@@ -50,6 +52,8 @@ class ReminderActionReceiver : BroadcastReceiver() {
 
             ACTION_SHOW -> {
                 goAsync {
+                    // A later reminder posted while this one was snoozed has taken its place.
+                    if (notifier.isSuperseded(alert)) return@goAsync
                     if (settingsPrefs.remindersEnabled.first() && notifier.canPost()) {
                         notifier.post(alert)
                     }
@@ -82,12 +86,14 @@ class ReminderActionReceiver : BroadcastReceiver() {
         /**
          * An explicit intent to this receiver carrying [alert] as extras. The
          * data URI duplicates no information but is what keeps two reminders'
-         * `PendingIntent`s apart — `filterEquals` never compares extras.
+         * `PendingIntent`s apart — `filterEquals` never compares extras. The
+         * snooze alarm is the exception: one per occurrence, so a later snooze
+         * replaces an earlier one and a newer reminder can cancel it.
          */
         fun intent(context: Context, action: String, alert: ReminderAlert): Intent =
             Intent(context, ReminderActionReceiver::class.java).apply {
                 this.action = action
-                data = "calendula://reminder/${alert.key}".toUri()
+                data = "calendula://reminder/${identity(alert, action)}".toUri()
                 putExtra(EXTRA_ALERT_KEY, alert.key)
                 putExtra(EXTRA_EVENT_ID, alert.eventId)
                 putExtra(EXTRA_CALENDAR_ID, alert.calendarId)
@@ -101,7 +107,7 @@ class ReminderActionReceiver : BroadcastReceiver() {
         /**
          * A stable request code per (alert, action), so one notification's
          * PendingIntents stay distinct. The shift keeps the action slot intact;
-         * the top bits it drops are separated by [intent]'s per-reminder URI.
+         * the top bits it drops are separated by [intent]'s URI.
          */
         fun requestCode(alert: ReminderAlert, action: String): Int {
             val actionOffset = when (action) {
@@ -111,9 +117,12 @@ class ReminderActionReceiver : BroadcastReceiver() {
                 ACTION_OPEN -> 4
                 else -> 0
             }
-            return (alert.key.toInt() shl 3) + actionOffset
+            return (identity(alert, action).toInt() shl 3) + actionOffset
         }
 
+        private fun identity(alert: ReminderAlert, action: String): Long =
+            if (action == ACTION_SHOW) alert.occurrenceKey else alert.key
+
         private fun alertFrom(intent: Intent): ReminderAlert? {
             if (!intent.hasExtra(EXTRA_ALERT_KEY)) return null
             return ReminderAlert(

app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderAlert.kt

@@ -1,6 +1,7 @@
 package de.jeanlucmakiola.calendula.data.reminders
 
 import de.jeanlucmakiola.calendula.domain.reminders.PlannedReminder
+import de.jeanlucmakiola.calendula.domain.reminders.occurrenceKey
 
 /**
  * One reminder as the notification layer needs it: what to show, and the stable
@@ -20,6 +21,13 @@ data class ReminderAlert(
     val isAllDay: Boolean,
 )
 
+/**
+ * The occurrence [ReminderAlert] belongs to. Keys the notification and the
+ * snooze alarm, so a later reminder for the same occurrence replaces the earlier
+ * one instead of stacking beside it (#463).
+ */
+val ReminderAlert.occurrenceKey: Long get() = occurrenceKey(eventId, beginMillis)
+
 fun PlannedReminder.toAlert(): ReminderAlert = ReminderAlert(
     key = key,
     eventId = instance.eventId,

app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderNotifier.kt

@@ -6,6 +6,7 @@ import android.app.NotificationManager
 import android.app.PendingIntent
 import android.content.Context
 import android.os.Build
+import android.os.Bundle
 import android.util.Log
 import androidx.core.app.NotificationCompat
 import androidx.core.app.NotificationManagerCompat
@@ -28,13 +29,13 @@ import javax.inject.Inject
 import javax.inject.Singleton
 
 /**
- * Posts one notification per due reminder on a dedicated channel. Tapping opens
+ * Posts one notification per occurrence on a dedicated channel. Tapping opens
  * the event's detail screen.
  *
- * The tag is the reminder's stable key, so a scan that posts the same reminder
- * again — a catch-up pass overlapping the alarm that already fired — replaces
- * its notification silently ([NotificationCompat.Builder.setOnlyAlertOnce])
- * instead of stacking a second one.
+ * The tag is the [occurrenceKey], so each reminder for an occurrence replaces
+ * the one before it in place (#463). A different reminder alerts afresh; the
+ * same one posted again — a catch-up pass overlapping the alarm that already
+ * fired — replaces its notification silently.
  */
 @Singleton
 class ReminderNotifier @Inject constructor(
@@ -65,7 +66,7 @@ class ReminderNotifier @Inject constructor(
     suspend fun post(alert: ReminderAlert) {
         if (isSilenced(alert.calendarId)) return
         ensureChannel()
-        val title = alert.title.ifBlank { context.getString(R.string.event_untitled) }
+        val title = displayTitle(alert)
         val is24Hour = settingsPrefs.timeFormat.first()
             .is24Hour(android.text.format.DateFormat.is24HourFormat(context))
         val zone = ZoneId.systemDefault()
@@ -96,7 +97,8 @@ class ReminderNotifier @Inject constructor(
             .setCategory(NotificationCompat.CATEGORY_EVENT)
             .setPriority(NotificationCompat.PRIORITY_HIGH)
             .setAutoCancel(true)
-            .setOnlyAlertOnce(true)
+            .setOnlyAlertOnce(shownKey(alert) == alert.key)
+            .addExtras(Bundle().apply { putLong(EXTRA_REMINDER_KEY, alert.key) })
             .setContentIntent(detailIntent(alert))
             .addAction(
                 R.drawable.ic_notification_snooze,
@@ -109,20 +111,58 @@ class ReminderNotifier @Inject constructor(
                 actionIntent(alert, ReminderActionReceiver.ACTION_DISMISS),
             )
             .build()
+        val manager = NotificationManagerCompat.from(context)
+        cancelLegacy(alert)
         try {
-            NotificationManagerCompat.from(context)
-                .notify(alert.key.toString(), NOTIFICATION_ID, notification)
+            manager.notify(alert.occurrenceKey.toString(), NOTIFICATION_ID, notification)
         } catch (e: SecurityException) {
             // POST_NOTIFICATIONS was revoked between canPost() and here.
             Log.w(TAG, "Could not post reminder for event ${alert.eventId}", e)
         }
     }
 
-    /** Remove a posted reminder (snooze re-shows it later; dismiss is final). */
+    /**
+     * Remove [alert]'s notification (snooze re-shows it later; dismiss is final),
+     * unless a later reminder has replaced it — an action tapped on a stale view
+     * mustn't take the newer one with it.
+     */
     fun cancel(alert: ReminderAlert) {
-        NotificationManagerCompat.from(context).cancel(alert.key.toString(), NOTIFICATION_ID)
+        val manager = NotificationManagerCompat.from(context)
+        if (shownKey(alert) == alert.key) manager.cancel(alert.occurrenceKey.toString(), NOTIFICATION_ID)
+        // Posted before reminders shared a notification, under the reminder's own key.
+        manager.cancel(alert.key.toString(), NOTIFICATION_ID)
     }
 
+    /** Whether a different reminder for [alert]'s occurrence is on screen now. */
+    fun isSuperseded(alert: ReminderAlert): Boolean = shownKey(alert).let { it != null && it != alert.key }
+
+    /** The key of the reminder showing for [alert]'s occurrence, if any. */
+    private fun shownKey(alert: ReminderAlert): Long? {
+        val tag = alert.occurrenceKey.toString()
+        return NotificationManagerCompat.from(context).activeNotifications
+            .firstOrNull { it.tag == tag && it.id == NOTIFICATION_ID }
+            ?.notification?.extras
+            ?.takeIf { it.containsKey(EXTRA_REMINDER_KEY) }
+            ?.getLong(EXTRA_REMINDER_KEY)
+    }
+
+    /**
+     * Remove a reminder for the same occurrence posted before reminders shared a
+     * notification. It carries no reminder key; its time and title identify it.
+     */
+    private fun cancelLegacy(alert: ReminderAlert) {
+        val manager = NotificationManagerCompat.from(context)
+        manager.activeNotifications
+            .filter { it.id == NOTIFICATION_ID && it.notification.channelId == CHANNEL_ID }
+            .filter { !it.notification.extras.containsKey(EXTRA_REMINDER_KEY) }
+            .filter { it.notification.`when` == alert.beginMillis }
+            .filter { it.notification.extras.getCharSequence(NotificationCompat.EXTRA_TITLE)?.toString() == displayTitle(alert) }
+            .forEach { manager.cancel(it.tag, it.id) }
+    }
+
+    private fun displayTitle(alert: ReminderAlert): String =
+        alert.title.ifBlank { context.getString(R.string.event_untitled) }
+
     private fun actionIntent(alert: ReminderAlert, action: String): PendingIntent =
         PendingIntent.getBroadcast(
             context,
@@ -157,8 +197,10 @@ class ReminderNotifier @Inject constructor(
         const val TAG = "ReminderNotifier"
         const val CHANNEL_ID = "reminders"
 
-        // One id, distinct tags: the tag (alert id) already keys the
+        // One id, distinct tags: the tag (occurrence) already keys the
         // notification, so a fixed id keeps cancellation/replacement simple.
         const val NOTIFICATION_ID = 1
+
+        const val EXTRA_REMINDER_KEY = "de.jeanlucmakiola.calendula.reminder_key"
     }
 }

app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderScanner.kt

@@ -43,6 +43,7 @@ class ReminderScanner @Inject constructor(
     private val source: ReminderInstanceSource,
     private val calendarDataSource: CalendarDataSource,
     private val notifier: ReminderNotifier,
+    private val snoozes: ReminderSnoozeScheduler,
     private val alarms: ReminderAlarmScheduler,
     private val state: ReminderStatePrefs,
     private val settingsPrefs: SettingsPrefs,
@@ -104,7 +105,11 @@ class ReminderScanner @Inject constructor(
         )
 
         if (notifier.canPost()) {
-            schedule.due.forEach { notifier.post(it.toAlert()) }
+            schedule.due.forEach {
+                val alert = it.toAlert()
+                snoozes.cancel(alert)
+                notifier.post(alert)
+            }
         }
         // Advance even when nothing could be posted, so muting notifications
         // doesn't build a backlog.

app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderSnoozeScheduler.kt

@@ -33,4 +33,16 @@ class ReminderSnoozeScheduler @Inject constructor(
         )
         alarmManager.setWakeup(triggerAtMillis, pendingIntent)
     }
+
+    /** Drop a snooze still pending for [alert]'s occurrence, so it can't re-show over a newer reminder. */
+    fun cancel(alert: ReminderAlert) {
+        val pendingIntent = PendingIntent.getBroadcast(
+            context,
+            ReminderActionReceiver.requestCode(alert, ReminderActionReceiver.ACTION_SHOW),
+            ReminderActionReceiver.intent(context, ReminderActionReceiver.ACTION_SHOW, alert),
+            PendingIntent.FLAG_NO_CREATE or PendingIntent.FLAG_IMMUTABLE,
+        ) ?: return
+        context.getSystemService<AlarmManager>()?.cancel(pendingIntent)
+        pendingIntent.cancel()
+    }
 }

app/src/main/java/de/jeanlucmakiola/calendula/domain/reminders/ReminderPlan.kt

@@ -32,19 +32,26 @@ data class PlannedReminder(
     val minutes: Int,
     val alarmMillis: Long,
 ) {
+    /** The occurrence this reminder belongs to; it shares one notification with its siblings (#463). */
+    val occurrenceKey: Long = occurrenceKey(instance.eventId, instance.beginMillis)
+
     /**
-     * Stable identity, keying the notification tag and the snooze/dismiss
+     * Stable identity of this one reminder, keying the snooze/dismiss
      * `PendingIntent`s. Survives reboot, re-scan and reinstall.
      */
-    val key: Long = key(instance.eventId, instance.beginMillis, minutes)
+    val key: Long = mix64(occurrenceKey + minutes)
+}
 
-    private companion object {
-        fun key(eventId: Long, beginMillis: Long, minutes: Int): Long {
-            var h = eventId * 1_000_003L
-            h = (h xor beginMillis) * 31L
-            return h + minutes
-        }
-    }
+/** Stable identity of one occurrence, shared by every reminder on it. */
+fun occurrenceKey(eventId: Long, beginMillis: Long): Long =
+    mix64(mix64(eventId) + beginMillis)
+
+/** SplitMix64's finaliser: spreads close inputs across all 64 bits. */
+private fun mix64(value: Long): Long {
+    var z = value + -0x61c8864680b583ebL
+    z = (z xor (z ushr 30)) * -0x40a7b892e31b1a47L
+    z = (z xor (z ushr 27)) * -0x6b2fb644ecceee15L
+    return z xor (z ushr 31)
 }
 
 /** What one scan concluded: post these now, and wake up again at [nextAlarmMillis]. */
@@ -146,7 +153,8 @@ private fun allDayAlarmMillis(
  *
  * Due means the fire instant falls in `(lastFiredMillis, nowMillis]`, so a scan
  * running twice cannot post twice while a late one still catches up. Reminders
- * whose event has ended are dropped ([isStillRelevant]). [nextAlarmMillis] is
+ * whose event has ended are dropped ([isStillRelevant]), and of several due on
+ * one occurrence only the latest is kept — they'd replace each other anyway (#463). [nextAlarmMillis] is
  * capped at [horizonMillis] so the lookahead window keeps rolling forward.
  */
 fun scheduleReminders(
@@ -158,7 +166,8 @@ fun scheduleReminders(
     val due = planned
         .filter { it.alarmMillis in (lastFiredMillis + 1)..nowMillis }
         .filter { it.instance.isStillRelevant(nowMillis) }
-        .distinctBy { it.key }
+        .groupBy { it.occurrenceKey }
+        .values.map { siblings -> siblings.maxBy { it.alarmMillis } }
         .sortedWith(compareBy({ it.instance.beginMillis }, { it.key }))
     val nextPending = planned
         .filter { it.alarmMillis > nowMillis }

app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/ReminderAlertTest.kt

@@ -0,0 +1,39 @@
+package de.jeanlucmakiola.calendula.data.reminders
+
+import com.google.common.truth.Truth.assertThat
+import de.jeanlucmakiola.calendula.domain.reminders.PlannedReminder
+import de.jeanlucmakiola.calendula.domain.reminders.ReminderEventInstance
+import org.junit.jupiter.api.Test
+
+class ReminderAlertTest {
+
+    private fun instance(eventId: Long = 1L, beginMillis: Long = 1_800_000_000_000L) = ReminderEventInstance(
+        eventId = eventId,
+        calendarId = 1L,
+        beginMillis = beginMillis,
+        endMillis = beginMillis + 3_600_000L,
+        title = "Dentist",
+        location = null,
+        isAllDay = false,
+    )
+
+    private fun alert(instance: ReminderEventInstance, minutes: Int) =
+        PlannedReminder(instance, minutes, instance.beginMillis - minutes * 60_000L).toAlert()
+
+    @Test
+    fun `two reminders on one occurrence share a notification`() {
+        val dayBefore = alert(instance(), 1_440)
+        val hourBefore = alert(instance(), 60)
+
+        assertThat(dayBefore.key).isNotEqualTo(hourBefore.key)
+        assertThat(dayBefore.occurrenceKey).isEqualTo(hourBefore.occurrenceKey)
+    }
+
+    @Test
+    fun `occurrences of one series keep their own notifications`() {
+        val first = alert(instance(beginMillis = 1_800_000_000_000L), 60)
+        val second = alert(instance(beginMillis = 1_800_086_400_000L), 60)
+
+        assertThat(first.occurrenceKey).isNotEqualTo(second.occurrenceKey)
+    }
+}

app/src/test/java/de/jeanlucmakiola/calendula/domain/reminders/ReminderPlanTest.kt

@@ -419,6 +419,30 @@ class ReminderPlanTest {
         assertThat(schedule.due).isEmpty()
     }
 
+    @Test
+    fun `of several reminders due on one occurrence only the latest is posted`() {
+        val planned = plan(
+            instances = listOf(instance(1L, beginMillis = now + 20 * minute)),
+            minutesByEvent = mapOf(1L to listOf(60, 30)),
+        )
+
+        val schedule = scheduleReminders(
+            planned, lastFiredMillis = now - day, nowMillis = now, horizonMillis = now + day,
+        )
+
+        assertThat(schedule.due.map { it.minutes }).containsExactly(30)
+    }
+
+    @Test
+    fun `two reminders on one occurrence share an occurrence key`() {
+        val planned = plan(
+            instances = listOf(instance(1L)),
+            minutesByEvent = mapOf(1L to listOf(10, 30)),
+        )
+
+        assertThat(planned[0].occurrenceKey).isEqualTo(planned[1].occurrenceKey)
+    }
+
     @Test
     fun `due reminders come out in occurrence order`() {
         val planned = plan(

docs/ARCHITECTURE.md

@@ -333,7 +333,7 @@ sequenceDiagram
     Src-->>Sc: Instances ⋈ Reminders
     Sc->>P: planReminders / scheduleReminders(watermark, now)
     P-->>Sc: due + next alarm
-    Sc->>N: post(alert) — tag = reminder key
+    Sc->>N: post(alert) — tag = occurrence, so a later reminder replaces it
     Sc->>A: scheduleScan(next)
 ```