From fa999f8cf06f8cf2acb6f3834104f111480c258c Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Fri, 13 Nov 2026 16:37:11 +0100 Subject: [PATCH] =?UTF-8?q?fix(#145):=20review=20round=203=20=E2=80=94=20r?= =?UTF-8?q?escan=20on=20mute,=20serialise=20channel=20writes,=20never=20dr?= =?UTF-8?q?op=20a=20reminder=20for=20its=20channel?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../data/reminders/ReminderChannelPlan.kt | 33 +++++++++--- .../data/reminders/ReminderChannels.kt | 52 +++++++++++-------- .../ui/settings/SettingsViewModel.kt | 31 ++++++----- .../data/reminders/ReminderChannelPlanTest.kt | 26 ++++++++-- docs/ARCHITECTURE.md | 6 ++- 5 files changed, 100 insertions(+), 48 deletions(-) diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderChannelPlan.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderChannelPlan.kt index bf00c6eb..1f070222 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderChannelPlan.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderChannelPlan.kt @@ -38,16 +38,26 @@ data class ReminderChannelSpec( val name: String, /** Null only for a channel created before its calendar could be looked up. */ val groupId: String?, + /** + * Whether a sync creates it up front. A calendar that syncs no events never + * fires, so its channel only appears once something posts on it. + */ + val upFront: Boolean = true, ) /** What a channel already looks like in the system, as far as the plan compares it. */ -data class ExistingReminderChannel(val id: String, val name: String, val description: String?) +data class ExistingReminderChannel(val id: String, val name: String, val description: String?, val groupId: String?) data class ReminderChannelPlan( val groups: List, val channels: List, - /** The [channels] that are missing, or whose name or (localized) description changed. */ + /** + * The [channels] a sync writes: missing up-front ones, and existing ones + * whose name or (localized) description changed, or that still lack a group. + */ val channelsToWrite: List, + /** The [groups] holding a channel that exists or is about to be written. */ + val groupsInUse: List, val staleChannelIds: List, val staleGroupIds: List, ) @@ -89,18 +99,29 @@ fun planReminderChannels( calendarId = calendar.id, name = calendar.displayName.ifBlank { unnamed }, groupId = if (calendar.isLocal) LOCAL_GROUP_ID else accountGroupId(calendar.accountType, calendar.accountName), + upFront = calendar.isLocal || calendar.syncsEvents, ) } val existingById = existing.associateBy { it.id } val wantedChannels = channels.mapTo(HashSet()) { it.id } val wantedGroups = groups.mapTo(HashSet()) { it.id } + val toWrite = channels.filter { spec -> + val current = existingById[spec.id] + if (current == null) { + spec.upFront + } else { + current.name != spec.name || current.description != description || + (current.groupId == null && spec.groupId != null) + } + } + val usedGroupIds = channels + .filter { it.id in existingById || it in toWrite } + .mapNotNullTo(HashSet()) { it.groupId } return ReminderChannelPlan( groups = groups, channels = channels, - channelsToWrite = channels.filter { spec -> - val current = existingById[spec.id] - current == null || current.name != spec.name || current.description != description - }, + channelsToWrite = toWrite, + groupsInUse = groups.filter { it.id in usedGroupIds }, staleChannelIds = existingById.keys.filter { isReminderChannelId(it) && it !in wantedChannels }, staleGroupIds = existingGroupIds.filter { isReminderGroupId(it) && it !in wantedGroups }, ) diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderChannels.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderChannels.kt index 4fdecf95..2f80a55c 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderChannels.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderChannels.kt @@ -35,56 +35,63 @@ class ReminderChannels @Inject constructor( private var lastSync: Any? = null /** - * Bring the channels in line with the provider's calendars. A calendar that - * syncs no events gets none, and an empty snapshot changes nothing: that is - * a provider hiccup far more often than every calendar gone at once. + * Bring the channels in line with the provider's calendars. An empty + * snapshot changes nothing: that is a provider hiccup far more often than + * every calendar gone at once. */ + @Synchronized fun sync() { - val calendars = calendarDataSource.calendars().filter { it.isLocal || it.syncsEvents } + val calendars = calendarDataSource.calendars() if (calendars.isEmpty()) return val manager = manager val existing = manager.notificationChannels - val key = syncKey(calendars, existing) - if (key == lastSync && existing.none { it.id == LEGACY_REMINDER_CHANNEL_ID }) return + val existingIds = existing.mapTo(HashSet()) { it.id } + if (syncKey(calendars, existingIds) == lastSync && LEGACY_REMINDER_CHANNEL_ID !in existingIds) return val groups = manager.notificationChannelGroups val plan = plan(calendars, existing, groups.map { it.id }) plan.staleChannelIds.forEach(manager::deleteNotificationChannel) plan.staleGroupIds.forEach(manager::deleteNotificationChannelGroup) val groupNames = groups.associate { it.id to it.name?.toString() } write( - groups = plan.groups.filter { groupNames[it.id] != groupLabel(it) }, + groups = plan.groupsInUse.filter { groupNames[it.id] != groupLabel(it) }, channels = plan.channelsToWrite, existing = existing, ) - retireLegacyChannel() - lastSync = syncKey(calendars, manager.notificationChannels) + val ids = existingIds - plan.staleChannelIds.toSet() + plan.channelsToWrite.map { it.id } + lastSync = syncKey(calendars, if (retireLegacyChannel()) ids - LEGACY_REMINDER_CHANNEL_ID else ids) } - private fun syncKey(calendars: List, channels: List): Any = listOf( - calendars.map { listOf(it.id, it.displayName, it.accountName, it.accountType, it.isLocal) }, + private fun syncKey(calendars: List, channelIds: Set): Any = listOf( + calendars.map { listOf(it.id, it.displayName, it.accountName, it.accountType, it.isLocal, it.syncsEvents) }, Locale.getDefault(), - channels.mapTo(HashSet()) { it.id }, + channelIds, ) /** * The channel to post [calendarId]'s reminder on, created first if a sync * has not got to it yet. Null when the calendar no longer exists. Should the - * calendar lookup fail, the channel is still created, plainly named and - * ungrouped, for the next sync to name and file — a reminder is never lost - * to its channel. + * calendar lookup fail or come back empty, the channel is still created, + * plainly named and ungrouped, for the next sync to name and file — a + * reminder is never lost to its channel. */ + @Synchronized fun channelFor(calendarId: Long): String? { val id = reminderChannelId(calendarId) val manager = manager if (manager.getNotificationChannel(id) != null) return id val existing = manager.notificationChannels - val plan = try { - plan(calendarDataSource.calendars(), existing, emptyList()) + val calendars = try { + calendarDataSource.calendars() } catch (e: Exception) { Log.w(TAG, "Could not look up calendar $calendarId for its channel", e) - write(emptyList(), listOf(ReminderChannelSpec(id, calendarId, context.getString(R.string.reminder_channel_name), null)), existing) + emptyList() + } + if (calendars.isEmpty()) { + val fallback = ReminderChannelSpec(id, calendarId, context.getString(R.string.reminder_channel_name), null) + write(emptyList(), listOf(fallback), existing) return id } + val plan = plan(calendars, existing, emptyList()) val spec = plan.channels.firstOrNull { it.calendarId == calendarId } ?: return null val knownGroups = manager.notificationChannelGroups.mapTo(HashSet()) { it.id } write( @@ -101,7 +108,7 @@ class ReminderChannels @Inject constructor( existingGroupIds: List, ): ReminderChannelPlan = planReminderChannels( calendars = calendars, - existing = existing.map { ExistingReminderChannel(it.id, it.name.toString(), it.description) }, + existing = existing.map { ExistingReminderChannel(it.id, it.name.toString(), it.description, it.group) }, existingGroupIds = existingGroupIds, unnamed = context.getString(R.string.calendar_unnamed), description = context.getString(R.string.reminder_channel_description), @@ -157,11 +164,11 @@ class ReminderChannels @Inject constructor( /** * Delete the pre-#145 channel once the per-calendar ones carry its settings, * but not while a reminder posted on it is still on screen — deleting the - * channel would take that notification with it. + * channel would take that notification with it. Returns whether it is gone. */ - private fun retireLegacyChannel() { + private fun retireLegacyChannel(): Boolean { val manager = manager - if (manager.getNotificationChannel(LEGACY_REMINDER_CHANNEL_ID) == null) return + if (manager.getNotificationChannel(LEGACY_REMINDER_CHANNEL_ID) == null) return true val showing = runCatching { manager.activeNotifications.any { it.notification.channelId == LEGACY_REMINDER_CHANNEL_ID } }.getOrElse { e -> @@ -169,6 +176,7 @@ class ReminderChannels @Inject constructor( true } if (!showing) manager.deleteNotificationChannel(LEGACY_REMINDER_CHANNEL_ID) + return !showing } private fun groupLabel(group: ReminderChannelGroupSpec): String { diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/settings/SettingsViewModel.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/settings/SettingsViewModel.kt index af7ff478..5dae73ee 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/settings/SettingsViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/settings/SettingsViewModel.kt @@ -3,6 +3,7 @@ package de.jeanlucmakiola.calendula.ui.settings import android.content.Context import android.net.Uri import android.os.Build +import android.util.Log import androidx.datastore.preferences.core.Preferences import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope @@ -361,28 +362,30 @@ class SettingsViewModel @Inject constructor( } } - fun setCalendarRemindersEnabled(calendarId: Long, enabled: Boolean) = - save { setCalendarRemindersEnabled(calendarId, enabled) } - - /** [calendarId]'s reminder channel, created if needed so its system settings can open (#340). */ - suspend fun reminderChannelFor(calendarId: Long): String? = withContext(io) { - try { - reminderChannels.channelFor(calendarId) - } catch (e: CancellationException) { - throw e - } catch (e: Exception) { - null + /** Re-plans, since a muted calendar armed no wake-up for its next reminder. */ + fun setCalendarRemindersEnabled(calendarId: Long, enabled: Boolean) { + viewModelScope.launch { + prefs.setCalendarRemindersEnabled(calendarId, enabled) + reminderScanner.scan() } } + /** [calendarId]'s reminder channel, created if needed so its system settings can open (#340). */ + suspend fun reminderChannelFor(calendarId: Long): String? = onChannels { channelFor(calendarId) } + /** Create the per-calendar channels before showing the app's channel list, even with reminders off. */ - suspend fun syncReminderChannels() = withContext(io) { + suspend fun syncReminderChannels() { + onChannels { sync() } + } + + private suspend fun onChannels(block: ReminderChannels.() -> T): T? = withContext(io) { try { - reminderChannels.sync() + reminderChannels.block() } catch (e: CancellationException) { throw e } catch (e: Exception) { - Unit + Log.w("SettingsViewModel", "Reminder channel update failed", e) + null } } diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/ReminderChannelPlanTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/ReminderChannelPlanTest.kt index d18fa12d..e32fa3a4 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/ReminderChannelPlanTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/ReminderChannelPlanTest.kt @@ -75,7 +75,7 @@ class ReminderChannelPlanTest { fun `stale reminder channels and groups go, foreign ones stay`() { val current = calendar(1, "me@example.org", "com.google") fun plan(calendars: List, existing: List = emptyList(), groups: List = emptyList()) = - planReminderChannels(calendars, existing.map { ExistingReminderChannel(it, "Cal ${it.last()}", DESC) }, groups, "Unnamed", DESC) + planReminderChannels(calendars, existing.map { ExistingReminderChannel(it, "Cal ${it.last()}", DESC, null) }, groups, "Unnamed", DESC) val keptGroup = plan(listOf(current)).groups.single().id val goneGroup = plan(listOf(calendar(9, "old", "com.google"))).groups.single().id val result = plan( @@ -97,9 +97,9 @@ class ReminderChannelPlanTest { calendar(4, "a", "com.google"), ), existing = listOf( - ExistingReminderChannel(reminderChannelId(1), "Cal 1", DESC), - ExistingReminderChannel(reminderChannelId(4), "Cal 4", "Old description"), - ExistingReminderChannel(reminderChannelId(2), "Old name", DESC), + ExistingReminderChannel(reminderChannelId(1), "Cal 1", DESC, "g"), + ExistingReminderChannel(reminderChannelId(4), "Cal 4", "Old description", "g"), + ExistingReminderChannel(reminderChannelId(2), "Old name", DESC, "g"), ), existingGroupIds = emptyList(), unnamed = "Unnamed", @@ -109,6 +109,24 @@ class ReminderChannelPlanTest { assertThat(plan.channelsToWrite.single { it.calendarId == 3L }.name).isEqualTo("Unnamed") } + @Test + fun `non-syncing calendars keep a channel but get none up front`() { + val quiet = calendar(2, "a", "com.google").copy(syncsEvents = false) + val fresh = planReminderChannels(listOf(quiet), emptyList(), emptyList(), "Unnamed", DESC) + assertThat(fresh.channelsToWrite).isEmpty() + assertThat(fresh.groupsInUse).isEmpty() + val posted = planReminderChannels( + calendars = listOf(quiet), + existing = listOf(ExistingReminderChannel(reminderChannelId(2), "Cal 2", DESC, null)), + existingGroupIds = emptyList(), + unnamed = "Unnamed", + description = DESC, + ) + assertThat(posted.staleChannelIds).isEmpty() + assertThat(posted.channelsToWrite.map { it.calendarId }).containsExactly(2L) + assertThat(posted.groupsInUse).hasSize(1) + } + private companion object { const val DESC = "Reminders" } diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 9e8e4a2a..4822bd47 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -392,7 +392,8 @@ switched off. The drawer's filter sheet (`CalendarPrefs.hiddenCalendarIds`) is a separate in-app declutter that never touches reminders. **A channel per calendar (#145).** `ReminderChannels` keeps one notification -channel per calendar that syncs events, grouped by account, so a calendar can be +channel per calendar, grouped by account (one that syncs no events only gets +its channel once something posts on it), so a calendar can be muted or given its own sound in the system settings before it ever fires. Every scan with reminders on syncs them (skipped when nothing changed): missing channels are created, renamed ones updated, and channels of deleted calendars @@ -405,7 +406,8 @@ nothing showing still sits on it. Settings → Notifications only deep-links int the system's channel screens (#340); the app never sets a sound itself. Separate from both, `SettingsPrefs.mutedReminderCalendarIds` is the in-app per-calendar off switch (#272): the scan plans nothing for those calendars (so they arm no -wake-ups), and `ReminderNotifier.post` checks it again for snoozes. +wake-ups; toggling it re-scans), and `ReminderNotifier.post` checks it again +for snoozes. Deliberately absent: a fallback to the provider's `EVENT_REMINDER` broadcast. Keeping both would double-post wherever the provider works, and Etar's way out —