fix(#145): review round 3 — rescan on mute, serialise channel writes, never drop a reminder for its channel

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<ReminderChannelGroupSpec>,
     val channels: List<ReminderChannelSpec>,
-    /** 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<ReminderChannelSpec>,
+    /** The [groups] holding a channel that exists or is about to be written. */
+    val groupsInUse: List<ReminderChannelGroupSpec>,
     val staleChannelIds: List<String>,
     val staleGroupIds: List<String>,
 )
@@ -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 },
     )

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<CalendarSource>, channels: List<NotificationChannel>): Any = listOf(
-        calendars.map { listOf(it.id, it.displayName, it.accountName, it.accountType, it.isLocal) },
+    private fun syncKey(calendars: List<CalendarSource>, channelIds: Set<String>): 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<String>,
     ): 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 {

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 <T> 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
         }
     }
 

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<CalendarSource>, existing: List<String> = emptyList(), groups: List<String> = 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"
     }

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 —