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