From 43ef4027f5b36879b7cc8aa3a2df9c72b9c33e59 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Mon, 2 Nov 2026 20:30:43 +0100 Subject: [PATCH] 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 Reviewed-on: https://codeberg.org/jlmakiola/calendula/pulls/468 --- .../data/reminders/ReminderActionReceiver.kt | 17 +++-- .../calendula/data/reminders/ReminderAlert.kt | 8 +++ .../data/reminders/ReminderNotifier.kt | 66 +++++++++++++++---- .../data/reminders/ReminderScanner.kt | 7 +- .../data/reminders/ReminderSnoozeScheduler.kt | 12 ++++ .../domain/reminders/ReminderPlan.kt | 31 +++++---- .../data/reminders/ReminderAlertTest.kt | 39 +++++++++++ .../domain/reminders/ReminderPlanTest.kt | 24 +++++++ docs/ARCHITECTURE.md | 2 +- 9 files changed, 177 insertions(+), 29 deletions(-) create mode 100644 app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/ReminderAlertTest.kt diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderActionReceiver.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderActionReceiver.kt index b4d5e94..c0bd756 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderActionReceiver.kt +++ b/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( diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderAlert.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderAlert.kt index 8a5630a..cb9780d 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderAlert.kt +++ b/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, 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 07a0004..215554b 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 @@ -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" } } diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderScanner.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderScanner.kt index 38ea963..a67c9b6 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderScanner.kt +++ b/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. diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderSnoozeScheduler.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderSnoozeScheduler.kt index 0c9e05c..c92e712 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderSnoozeScheduler.kt +++ b/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()?.cancel(pendingIntent) + pendingIntent.cancel() + } } diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/domain/reminders/ReminderPlan.kt b/app/src/main/java/de/jeanlucmakiola/calendula/domain/reminders/ReminderPlan.kt index 559cc51..c70b87d 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/domain/reminders/ReminderPlan.kt +++ b/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 } diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/ReminderAlertTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/ReminderAlertTest.kt new file mode 100644 index 0000000..3bebbcc --- /dev/null +++ b/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) + } +} diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/domain/reminders/ReminderPlanTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/domain/reminders/ReminderPlanTest.kt index cf17e91..c23f0f2 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/domain/reminders/ReminderPlanTest.kt +++ b/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( diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 77fc57c..5d58523 100644 --- a/docs/ARCHITECTURE.md +++ b/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) ```