fix(reminders): stop marking a silenced reminder handled and losing it
"With VISIBLE=0 the provider creates no alert rows" justified deleting the suppression stash, but it only holds where the flag was actually written. Where the switch lives in pendingDisabledCalendarIds — a read-only install, or an upgrade whose flush hasn't landed — the provider still holds VISIBLE=1 and keeps creating and broadcasting rows. The receiver silenced those in ReminderNotifier.post and then marked the whole due batch STATE_FIRED, and dueAlerts only ever returns STATE_SCHEDULED, so switching the calendar back on before the event could no longer surface the reminder: it was gone. post() now reports whether it put a notification up, and the receiver marks what it posted plus what it silenced for an event already over (handledAlertIds). A silenced alert for an event still ahead stays scheduled, which makes the provider's own table the stash SuppressedReminderStore used to be — no local mirror, no serialization. ReminderRecovery re-posts those rows when the calendar is switched back on, so recovery doesn't wait for the next unrelated broadcast. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,30 @@
|
|||||||
|
package de.jeanlucmakiola.calendula.data.reminders
|
||||||
|
|
||||||
|
/**
|
||||||
|
* 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).
|
||||||
|
*/
|
||||||
|
internal fun ReminderAlert.isRelevantAt(nowMillis: Long): Boolean =
|
||||||
|
(endMillis.takeIf { it > 0L } ?: beginMillis) >= nowMillis
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The alerts [EventReminderReceiver] may mark handled (`STATE_FIRED`): the ones
|
||||||
|
* it posted, plus the ones it silenced whose event is already over.
|
||||||
|
*
|
||||||
|
* A silenced alert for an event still ahead is deliberately left
|
||||||
|
* `STATE_SCHEDULED`. Silencing is not handling — the calendar is switched off in
|
||||||
|
* Calendula while the provider still holds `VISIBLE = 1` (a read-only install,
|
||||||
|
* or an upgrade whose flush hasn't landed), so switching it back on before the
|
||||||
|
* event must still be able to surface the reminder. [ReminderAlertStore.dueAlerts]
|
||||||
|
* only ever returns scheduled rows, so marking them here would lose them for
|
||||||
|
* good; leaving them makes the provider's own table the stash
|
||||||
|
* ([ReminderRecovery]).
|
||||||
|
*/
|
||||||
|
internal fun handledAlertIds(
|
||||||
|
due: List<ReminderAlert>,
|
||||||
|
postedIds: Set<Long>,
|
||||||
|
nowMillis: Long,
|
||||||
|
): List<Long> = due
|
||||||
|
.filter { it.alertId in postedIds || !it.isRelevantAt(nowMillis) }
|
||||||
|
.map { it.alertId }
|
||||||
@@ -30,7 +30,9 @@ import javax.inject.Inject
|
|||||||
* Settings → Calendars has `Calendars.VISIBLE = 0`, and the provider creates no
|
* Settings → Calendars has `Calendars.VISIBLE = 0`, and the provider creates no
|
||||||
* alert rows for it in the first place (#75). The one case that flag can't
|
* alert rows for it in the first place (#75). The one case that flag can't
|
||||||
* cover — a read-only install, which keeps its switches app-side — is gated in
|
* cover — a read-only install, which keeps its switches app-side — is gated in
|
||||||
* [ReminderNotifier.post], where the snoozed re-show passes too.
|
* [ReminderNotifier.post], where the snoozed re-show passes too. What that gate
|
||||||
|
* silences is *not* marked fired while the event is still ahead, so switching
|
||||||
|
* the calendar back on can still surface it (see [handledAlertIds]).
|
||||||
*/
|
*/
|
||||||
@AndroidEntryPoint
|
@AndroidEntryPoint
|
||||||
class EventReminderReceiver : BroadcastReceiver() {
|
class EventReminderReceiver : BroadcastReceiver() {
|
||||||
@@ -52,8 +54,10 @@ class EventReminderReceiver : BroadcastReceiver() {
|
|||||||
if (settingsPrefs.remindersEnabled.first()) {
|
if (settingsPrefs.remindersEnabled.first()) {
|
||||||
val now = System.currentTimeMillis()
|
val now = System.currentTimeMillis()
|
||||||
val due = alertStore.dueAlerts(now)
|
val due = alertStore.dueAlerts(now)
|
||||||
due.forEach { notifier.post(it) }
|
val postedIds = due
|
||||||
alertStore.markFired(due.map { it.alertId }, now)
|
.filter { notifier.post(it) }
|
||||||
|
.mapTo(mutableSetOf()) { it.alertId }
|
||||||
|
alertStore.markFired(handledAlertIds(due, postedIds, now), now)
|
||||||
}
|
}
|
||||||
} finally {
|
} finally {
|
||||||
pendingResult.finish()
|
pendingResult.finish()
|
||||||
|
|||||||
@@ -64,8 +64,13 @@ class ReminderNotifier @Inject constructor(
|
|||||||
calendarId in calendarPrefs.pendingDisabledCalendarIds.first() ||
|
calendarId in calendarPrefs.pendingDisabledCalendarIds.first() ||
|
||||||
calendarDataSource.isCalendarVisible(calendarId) == false
|
calendarDataSource.isCalendarVisible(calendarId) == false
|
||||||
|
|
||||||
suspend fun post(alert: ReminderAlert) {
|
/**
|
||||||
if (isSilenced(alert.calendarId)) return
|
* Post [alert], unless its calendar is switched off. Returns whether the
|
||||||
|
* notification was put up: a silenced alert must stay unhandled so that
|
||||||
|
* switching the calendar back on can still surface it (see [handledAlertIds]).
|
||||||
|
*/
|
||||||
|
suspend fun post(alert: ReminderAlert): Boolean {
|
||||||
|
if (isSilenced(alert.calendarId)) return false
|
||||||
ensureChannel()
|
ensureChannel()
|
||||||
val title = alert.title.ifBlank { context.getString(R.string.event_untitled) }
|
val title = alert.title.ifBlank { context.getString(R.string.event_untitled) }
|
||||||
val is24Hour = settingsPrefs.timeFormat.first()
|
val is24Hour = settingsPrefs.timeFormat.first()
|
||||||
@@ -117,6 +122,8 @@ class ReminderNotifier @Inject constructor(
|
|||||||
// POST_NOTIFICATIONS was revoked between canPost() and here.
|
// POST_NOTIFICATIONS was revoked between canPost() and here.
|
||||||
Log.w(TAG, "Could not post reminder for event ${alert.eventId}", e)
|
Log.w(TAG, "Could not post reminder for event ${alert.eventId}", e)
|
||||||
}
|
}
|
||||||
|
// Handled either way: re-running it would hit the same revoked permission.
|
||||||
|
return true
|
||||||
}
|
}
|
||||||
|
|
||||||
/** Remove a posted reminder (snooze re-shows it later; dismiss is final). */
|
/** Remove a posted reminder (snooze re-shows it later; dismiss is final). */
|
||||||
|
|||||||
@@ -0,0 +1,46 @@
|
|||||||
|
package de.jeanlucmakiola.calendula.data.reminders
|
||||||
|
|
||||||
|
import de.jeanlucmakiola.calendula.data.di.IoDispatcher
|
||||||
|
import de.jeanlucmakiola.calendula.data.prefs.SettingsPrefs
|
||||||
|
import kotlinx.coroutines.CoroutineDispatcher
|
||||||
|
import kotlinx.coroutines.flow.first
|
||||||
|
import kotlinx.coroutines.withContext
|
||||||
|
import javax.inject.Inject
|
||||||
|
import javax.inject.Singleton
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Re-posts the reminders a switched-off calendar silenced, when it is switched
|
||||||
|
* back on while they still matter.
|
||||||
|
*
|
||||||
|
* Only the app-side switch needs this — a read-only install, or an upgrade the
|
||||||
|
* reconciler hasn't flushed yet. With `Calendars.VISIBLE = 0` the provider
|
||||||
|
* deletes the calendar's alert rows itself and re-creates them on the way back;
|
||||||
|
* app-side the rows stay, still `STATE_SCHEDULED`, because
|
||||||
|
* [EventReminderReceiver] deliberately leaves the ones it silenced unhandled
|
||||||
|
* (see [handledAlertIds]). So the provider's own table is the stash, and nothing
|
||||||
|
* is mirrored locally.
|
||||||
|
*
|
||||||
|
* Best effort at switch-on time: it mirrors the receiver's gates (reminders on,
|
||||||
|
* notifications postable) and there is no later re-scan, so an alert left
|
||||||
|
* unposted because those are closed is simply released.
|
||||||
|
*/
|
||||||
|
@Singleton
|
||||||
|
class ReminderRecovery @Inject constructor(
|
||||||
|
private val alertStore: ReminderAlertStore,
|
||||||
|
private val notifier: ReminderNotifier,
|
||||||
|
private val settingsPrefs: SettingsPrefs,
|
||||||
|
@IoDispatcher private val io: CoroutineDispatcher,
|
||||||
|
) {
|
||||||
|
|
||||||
|
suspend fun rePostFor(calendarIds: Collection<Long>) = withContext(io) {
|
||||||
|
if (calendarIds.isEmpty()) return@withContext
|
||||||
|
if (!settingsPrefs.remindersEnabled.first() || !notifier.canPost()) return@withContext
|
||||||
|
val now = System.currentTimeMillis()
|
||||||
|
val ids = calendarIds.toSet()
|
||||||
|
val recovered = alertStore.dueAlerts(now)
|
||||||
|
.filter { it.calendarId in ids && it.isRelevantAt(now) }
|
||||||
|
if (recovered.isEmpty()) return@withContext
|
||||||
|
val postedIds = recovered.filter { notifier.post(it) }.map { it.alertId }
|
||||||
|
alertStore.markFired(postedIds, now)
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -13,6 +13,7 @@ import de.jeanlucmakiola.calendula.data.di.IoDispatcher
|
|||||||
import de.jeanlucmakiola.calendula.data.ics.IcsExporter
|
import de.jeanlucmakiola.calendula.data.ics.IcsExporter
|
||||||
import de.jeanlucmakiola.calendula.data.prefs.BackupStatus
|
import de.jeanlucmakiola.calendula.data.prefs.BackupStatus
|
||||||
import de.jeanlucmakiola.calendula.data.prefs.SettingsPrefs
|
import de.jeanlucmakiola.calendula.data.prefs.SettingsPrefs
|
||||||
|
import de.jeanlucmakiola.calendula.data.reminders.ReminderRecovery
|
||||||
import de.jeanlucmakiola.calendula.domain.CalendarSource
|
import de.jeanlucmakiola.calendula.domain.CalendarSource
|
||||||
import de.jeanlucmakiola.calendula.domain.ics.IcsWriter
|
import de.jeanlucmakiola.calendula.domain.ics.IcsWriter
|
||||||
import kotlinx.coroutines.CoroutineDispatcher
|
import kotlinx.coroutines.CoroutineDispatcher
|
||||||
@@ -43,6 +44,7 @@ class CalendarsViewModel @Inject constructor(
|
|||||||
private val repository: CalendarRepository,
|
private val repository: CalendarRepository,
|
||||||
private val icsExporter: IcsExporter,
|
private val icsExporter: IcsExporter,
|
||||||
private val settingsPrefs: SettingsPrefs,
|
private val settingsPrefs: SettingsPrefs,
|
||||||
|
private val reminderRecovery: ReminderRecovery,
|
||||||
@IoDispatcher private val io: CoroutineDispatcher,
|
@IoDispatcher private val io: CoroutineDispatcher,
|
||||||
) : ViewModel() {
|
) : ViewModel() {
|
||||||
|
|
||||||
@@ -125,9 +127,14 @@ class CalendarsViewModel @Inject constructor(
|
|||||||
* every surface *and* the provider stops (or resumes) scheduling its
|
* every surface *and* the provider stops (or resumes) scheduling its
|
||||||
* reminders. Nothing is patched by hand — the provider notifies and the
|
* reminders. Nothing is patched by hand — the provider notifies and the
|
||||||
* observer re-queries.
|
* observer re-queries.
|
||||||
|
*
|
||||||
|
* Switching one back on also re-posts the reminders it silenced while it was
|
||||||
|
* off and that are still relevant — those the app kept app-side because it
|
||||||
|
* may not write the flag ([ReminderRecovery]).
|
||||||
*/
|
*/
|
||||||
fun setCalendarVisible(id: Long, visible: Boolean) = write {
|
fun setCalendarVisible(id: Long, visible: Boolean) = write {
|
||||||
repository.setCalendarsVisible(listOf(id), visible)
|
repository.setCalendarsVisible(listOf(id), visible)
|
||||||
|
if (visible) reminderRecovery.rePostFor(listOf(id))
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -138,6 +145,7 @@ class CalendarsViewModel @Inject constructor(
|
|||||||
*/
|
*/
|
||||||
fun setAccountVisible(ids: Collection<Long>, visible: Boolean) = write {
|
fun setAccountVisible(ids: Collection<Long>, visible: Boolean) = write {
|
||||||
repository.setCalendarsVisible(ids, visible)
|
repository.setCalendarsVisible(ids, visible)
|
||||||
|
if (visible) reminderRecovery.rePostFor(ids)
|
||||||
}
|
}
|
||||||
|
|
||||||
// --- Automatic backup (issue #8) ------------------------------------
|
// --- Automatic backup (issue #8) ------------------------------------
|
||||||
|
|||||||
@@ -0,0 +1,66 @@
|
|||||||
|
package de.jeanlucmakiola.calendula.data.reminders
|
||||||
|
|
||||||
|
import com.google.common.truth.Truth.assertThat
|
||||||
|
import org.junit.jupiter.api.Test
|
||||||
|
|
||||||
|
/**
|
||||||
|
* What the reminder receiver may mark handled. The silenced-but-still-ahead case
|
||||||
|
* is the one that matters: those rows are the only copy of the reminder (#75).
|
||||||
|
*/
|
||||||
|
class AlertHandlingTest {
|
||||||
|
|
||||||
|
private val now = 1_700_000_000_000L
|
||||||
|
|
||||||
|
private fun alert(
|
||||||
|
id: Long,
|
||||||
|
calendarId: Long = 1L,
|
||||||
|
beginMillis: Long = now + 60_000L,
|
||||||
|
endMillis: Long = now + 3_600_000L,
|
||||||
|
) = ReminderAlert(
|
||||||
|
alertId = id,
|
||||||
|
eventId = id * 10,
|
||||||
|
calendarId = calendarId,
|
||||||
|
beginMillis = beginMillis,
|
||||||
|
endMillis = endMillis,
|
||||||
|
title = "E $id",
|
||||||
|
location = null,
|
||||||
|
isAllDay = false,
|
||||||
|
)
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `posted alerts are handled`() {
|
||||||
|
val due = listOf(alert(1L), alert(2L))
|
||||||
|
|
||||||
|
assertThat(handledAlertIds(due, postedIds = setOf(1L, 2L), nowMillis = now))
|
||||||
|
.containsExactly(1L, 2L)
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `a silenced alert whose event is still ahead stays unhandled`() {
|
||||||
|
// Switching its calendar back on before the event has to bring it back,
|
||||||
|
// and dueAlerts only ever returns STATE_SCHEDULED rows.
|
||||||
|
val due = listOf(alert(1L), alert(2L))
|
||||||
|
|
||||||
|
assertThat(handledAlertIds(due, postedIds = setOf(1L), nowMillis = now))
|
||||||
|
.containsExactly(1L)
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `a silenced alert whose event is over is handled`() {
|
||||||
|
// Nothing left to re-surface, so it must not linger as scheduled.
|
||||||
|
val over = alert(2L, beginMillis = now - 7_200_000L, endMillis = now - 3_600_000L)
|
||||||
|
val due = listOf(alert(1L), over)
|
||||||
|
|
||||||
|
assertThat(handledAlertIds(due, postedIds = setOf(1L), nowMillis = now))
|
||||||
|
.containsExactly(1L, 2L)
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `an unknown end time falls back to the begin time`() {
|
||||||
|
val started = alert(1L, beginMillis = now - 1L, endMillis = 0L)
|
||||||
|
val notYet = alert(2L, beginMillis = now + 1L, endMillis = 0L)
|
||||||
|
|
||||||
|
assertThat(handledAlertIds(listOf(started, notYet), postedIds = emptySet(), nowMillis = now))
|
||||||
|
.containsExactly(1L)
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user