fix(#145): review round 1 — vibration carry-over, cheaper channel sync, shared account grouping

app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderChannelPlan.kt

@@ -1,6 +1,7 @@
 package de.jeanlucmakiola.calendula.data.reminders
 
 import de.jeanlucmakiola.calendula.domain.CalendarSource
+import de.jeanlucmakiola.calendula.ui.common.groupByAccount
 
 /** The single channel every reminder posted on before they moved per calendar (#145). */
 const val LEGACY_REMINDER_CHANNEL_ID = "reminders"
@@ -19,13 +20,13 @@ fun isReminderChannelId(channelId: String): Boolean = channelId.startsWith(CHANN
 fun isReminderGroupId(groupId: String): Boolean = groupId.startsWith(GROUP_PREFIX)
 
 /**
- * One account's channel group. The app's own device calendars share a single
- * group; [ambiguous] marks a name another group shows under a different type,
- * so the label can name the source app as well.
+ * One account's channel group, labelled as the calendar manager labels it. The
+ * app's own device calendars share a single group ([isLocal]); [ambiguous]
+ * marks a [label] another group shows too, so it should name the source app.
  */
 data class ReminderChannelGroupSpec(
     val id: String,
-    val accountName: String,
+    val label: String,
     val accountType: String,
     val isLocal: Boolean,
     val ambiguous: Boolean,
@@ -39,67 +40,69 @@ data class ReminderChannelSpec(
     val groupId: String,
 )
 
+/** 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)
+
 data class ReminderChannelPlan(
     val groups: List<ReminderChannelGroupSpec>,
-    /** Every channel that should exist; creating an existing one only refreshes its name. */
     val channels: List<ReminderChannelSpec>,
+    /** The [channels] that are missing or whose name changed. */
+    val channelsToWrite: List<ReminderChannelSpec>,
     val staleChannelIds: List<String>,
     val staleGroupIds: List<String>,
 )
 
 /**
  * What the reminder channels should look like for [calendars]: one channel per
- * calendar inside a group per account, and the reminder channels and groups in
- * [existingChannelIds] / [existingGroupIds] that no calendar claims any more.
- * Ids outside the reminder namespace (the backup channel, the legacy channel)
- * are never touched.
+ * calendar inside a group per account, which of them need writing, and the
+ * reminder channels and groups in [existing] / [existingGroupIds] no calendar
+ * claims any more. Ids outside the reminder namespace (the backup channel, the
+ * legacy channel) are never touched.
  */
 fun planReminderChannels(
     calendars: List<CalendarSource>,
-    existingChannelIds: Collection<String>,
+    existing: Collection<ExistingReminderChannel>,
     existingGroupIds: Collection<String>,
+    unnamed: String,
 ): ReminderChannelPlan {
-    val groupsByKey = calendars.groupBy(::accountKey)
-    val nameCounts = groupsByKey.keys.filterNotNull().groupingBy { it.first }.eachCount()
-    val groups = groupsByKey.map { (key, members) ->
-        if (key == null) {
-            ReminderChannelGroupSpec(
-                id = LOCAL_GROUP_ID,
-                accountName = members.first().accountName,
-                accountType = members.first().accountType,
-                isLocal = true,
-                ambiguous = false,
-            )
-        } else {
-            val (name, type) = key
-            ReminderChannelGroupSpec(
-                id = "$GROUP_PREFIX$type/$name",
-                accountName = name,
-                accountType = type,
-                isLocal = false,
-                ambiguous = nameCounts.getValue(name) > 1,
+    val (local, synced) = calendars.partition { it.isLocal }
+    val groups = buildList {
+        if (local.isNotEmpty()) {
+            add(ReminderChannelGroupSpec(LOCAL_GROUP_ID, "", local.first().accountType, isLocal = true, ambiguous = false))
+        }
+        synced.groupByAccount().forEach { group ->
+            add(
+                ReminderChannelGroupSpec(
+                    id = accountGroupId(group.key.type, group.key.name),
+                    label = group.label,
+                    accountType = group.accountType,
+                    isLocal = false,
+                    ambiguous = group.ambiguous,
+                ),
             )
         }
     }
-    val groupIdByKey = groupsByKey.keys.zip(groups.map { it.id }).toMap()
     val channels = calendars.map { calendar ->
         ReminderChannelSpec(
             id = reminderChannelId(calendar.id),
             calendarId = calendar.id,
-            name = calendar.displayName,
-            groupId = groupIdByKey.getValue(accountKey(calendar)),
+            name = calendar.displayName.ifBlank { unnamed },
+            groupId = if (calendar.isLocal) LOCAL_GROUP_ID else accountGroupId(calendar.accountType, calendar.accountName),
         )
     }
+    val existingById = existing.associateBy { it.id }
     val wantedChannels = channels.mapTo(HashSet()) { it.id }
     val wantedGroups = groups.mapTo(HashSet()) { it.id }
     return ReminderChannelPlan(
         groups = groups,
         channels = channels,
-        staleChannelIds = existingChannelIds.filter { isReminderChannelId(it) && it !in wantedChannels },
+        channelsToWrite = channels.filter { spec ->
+            val current = existingById[spec.id]
+            current == null || current.name != spec.name
+        },
+        staleChannelIds = existingById.keys.filter { isReminderChannelId(it) && it !in wantedChannels },
         staleGroupIds = existingGroupIds.filter { isReminderGroupId(it) && it !in wantedGroups },
     )
 }
 
-/** The app's own device calendars share one group (null); every other account has its own. */
-private fun accountKey(calendar: CalendarSource): Pair<String, String>? =
-    if (calendar.isLocal) null else calendar.accountName to calendar.accountType
+private fun accountGroupId(type: String, name: String): String = "$GROUP_PREFIX$type/$name"

app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderChannels.kt

@@ -8,44 +8,39 @@ import android.util.Log
 import dagger.hilt.android.qualifiers.ApplicationContext
 import de.jeanlucmakiola.calendula.R
 import de.jeanlucmakiola.calendula.data.calendar.CalendarDataSource
-import de.jeanlucmakiola.calendula.data.prefs.CalendarPrefs
-import de.jeanlucmakiola.calendula.ui.common.sourceAppPackages
-import kotlinx.coroutines.flow.first
+import de.jeanlucmakiola.calendula.ui.common.sourceAppLabel
 import javax.inject.Inject
 import javax.inject.Singleton
 
 /**
- * The reminder notification channels: one per switched-on calendar, grouped
- * by account (#145), so each calendar can be muted or given its own sound in
- * the system settings before it ever fires.
+ * The reminder notification channels: one per calendar, grouped by account
+ * (#145), created up front so each calendar can be muted or given its own sound
+ * in the system settings before it ever fires.
  *
  * A channel the user has touched keeps their settings; [sync] only creates the
- * missing ones, refreshes names, and deletes channels whose calendar is gone or
- * switched off — the system restores a deleted channel's settings if its id
- * comes back.
+ * missing ones, renames changed ones, and deletes those whose calendar is gone.
  */
 @Singleton
 class ReminderChannels @Inject constructor(
     @ApplicationContext private val context: Context,
     private val calendarDataSource: CalendarDataSource,
-    private val calendarPrefs: CalendarPrefs,
 ) {
     private val manager: NotificationManager
         get() = context.getSystemService(NotificationManager::class.java)
 
-    /** Bring the channels in line with the provider's switched-on calendars. */
-    suspend fun sync() {
-        val pending = calendarPrefs.pendingDisabledCalendarIds.first()
-        val calendars = calendarDataSource.calendars().filter { it.isVisibleInSystem && it.id !in pending }
+    /** Bring the channels in line with the provider's calendars. */
+    fun sync() {
         val manager = manager
-        val plan = planReminderChannels(
-            calendars = calendars,
-            existingChannelIds = manager.notificationChannels.map { it.id },
-            existingGroupIds = manager.notificationChannelGroups.map { it.id },
-        )
+        val existing = manager.notificationChannels
+        val plan = plan(existing)
         plan.staleChannelIds.forEach(manager::deleteNotificationChannel)
         plan.staleGroupIds.forEach(manager::deleteNotificationChannelGroup)
-        create(plan)
+        val groupNames = manager.notificationChannelGroups.associate { it.id to it.name?.toString() }
+        write(
+            groups = plan.groups.filter { groupNames[it.id] != groupLabel(it) },
+            channels = plan.channelsToWrite,
+            existing = existing,
+        )
         if (plan.channels.isNotEmpty()) retireLegacyChannel()
     }
 
@@ -55,32 +50,52 @@ class ReminderChannels @Inject constructor(
      */
     fun channelFor(calendarId: Long): String? {
         val id = reminderChannelId(calendarId)
+        val manager = manager
         if (manager.getNotificationChannel(id) != null) return id
-        val calendar = calendarDataSource.calendars().firstOrNull { it.id == calendarId } ?: return null
-        create(planReminderChannels(listOf(calendar), emptyList(), emptyList()))
+        val existing = manager.notificationChannels
+        val plan = plan(existing)
+        val spec = plan.channels.firstOrNull { it.calendarId == calendarId } ?: return null
+        val knownGroups = manager.notificationChannelGroups.mapTo(HashSet()) { it.id }
+        write(
+            groups = plan.groups.filter { it.id == spec.groupId && it.id !in knownGroups },
+            channels = listOf(spec),
+            existing = existing,
+        )
         return id
     }
 
-    private fun create(plan: ReminderChannelPlan) {
+    private fun plan(existing: List<NotificationChannel>): ReminderChannelPlan = planReminderChannels(
+        calendars = calendarDataSource.calendars(),
+        existing = existing.map { ExistingReminderChannel(it.id, it.name.toString()) },
+        existingGroupIds = manager.notificationChannelGroups.map { it.id },
+        unnamed = context.getString(R.string.calendar_unnamed),
+    )
+
+    private fun write(
+        groups: List<ReminderChannelGroupSpec>,
+        channels: List<ReminderChannelSpec>,
+        existing: List<NotificationChannel>,
+    ) {
         val manager = manager
-        manager.createNotificationChannelGroups(
-            plan.groups.map { NotificationChannelGroup(it.id, groupLabel(it)) },
-        )
+        if (groups.isNotEmpty()) {
+            manager.createNotificationChannelGroups(groups.map { NotificationChannelGroup(it.id, groupLabel(it)) })
+        }
+        if (channels.isEmpty()) return
+        val byId = existing.associateBy { it.id }
+        val template = byId[LEGACY_REMINDER_CHANNEL_ID]
         // Only a channel's first creation sets its behaviour; later calls just
         // rename it, so an existing one passes its own importance back.
-        val template = manager.getNotificationChannel(LEGACY_REMINDER_CHANNEL_ID)
-        val unnamed = context.getString(R.string.calendar_unnamed)
         manager.createNotificationChannels(
-            plan.channels.map { spec ->
-                val existing = manager.getNotificationChannel(spec.id)
+            channels.map { spec ->
+                val current = byId[spec.id]
                 NotificationChannel(
                     spec.id,
-                    spec.name.ifBlank { unnamed },
-                    existing?.importance ?: template?.importance ?: NotificationManager.IMPORTANCE_HIGH,
+                    spec.name,
+                    current?.importance ?: template?.importance ?: NotificationManager.IMPORTANCE_HIGH,
                 ).apply {
                     description = context.getString(R.string.reminder_channel_description)
                     group = spec.groupId
-                    if (existing == null && template != null) inheritFrom(template)
+                    if (current == null && template != null) inheritFrom(template)
                 }
             },
         )
@@ -92,8 +107,9 @@ class ReminderChannels @Inject constructor(
      */
     private fun NotificationChannel.inheritFrom(template: NotificationChannel) {
         setSound(template.sound, template.audioAttributes)
-        enableVibration(template.shouldVibrate())
+        // The pattern setter also flips vibration on or off, so it goes first.
         vibrationPattern = template.vibrationPattern
+        enableVibration(template.shouldVibrate())
         enableLights(template.shouldShowLights())
         lightColor = template.lightColor
         lockscreenVisibility = template.lockscreenVisibility
@@ -119,17 +135,9 @@ class ReminderChannels @Inject constructor(
 
     private fun groupLabel(group: ReminderChannelGroupSpec): String {
         if (group.isLocal) return context.getString(R.string.calendars_local_header)
-        val name = group.accountName.ifBlank { group.accountType }
-            .ifBlank { context.getString(R.string.calendar_unnamed) }
+        val name = group.label.ifBlank { context.getString(R.string.calendar_unnamed) }
         if (!group.ambiguous) return name
-        return context.getString(R.string.calendars_account_from_source, name, sourceAppLabel(group.accountType))
-    }
-
-    private fun sourceAppLabel(accountType: String): String {
-        val pm = context.packageManager
-        return sourceAppPackages(context, accountType).firstNotNullOfOrNull { pkg ->
-            runCatching { pm.getApplicationLabel(pm.getApplicationInfo(pkg, 0)).toString() }.getOrNull()
-        } ?: accountType
+        return context.getString(R.string.calendars_account_from_source, name, sourceAppLabel(context, group.accountType))
     }
 
     private companion object {

app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderScanner.kt

@@ -14,7 +14,6 @@ import de.jeanlucmakiola.calendula.domain.reminders.reminderWatermark
 import de.jeanlucmakiola.calendula.domain.reminders.scheduleReminders
 import de.jeanlucmakiola.floret.android.hasPermission
 import de.jeanlucmakiola.floret.di.IoDispatcher
-import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.CoroutineStart
 import kotlinx.coroutines.SupervisorJob
@@ -78,7 +77,6 @@ class ReminderScanner @Inject constructor(
     private suspend fun runScan() {
         val now = System.currentTimeMillis()
         if (!hasReadCalendar()) return
-        syncChannels()
 
         if (!settingsPrefs.remindersEnabled.first()) {
             // Reminders off: drop the wake-up, but keep the watermark moving
@@ -88,6 +86,7 @@ class ReminderScanner @Inject constructor(
             return
         }
 
+        syncChannels()
         val lookahead = reminderQueryHorizon(LOOKAHEAD_MILLIS, source.longestReminderMinutes())
         // Reach into the past too: an all-day "at time of event" encodes to a
         // negative offset, and a catch-up pass needs the occurrences it missed.
@@ -121,14 +120,13 @@ class ReminderScanner @Inject constructor(
     }
 
     /**
-     * Every trigger that can add, rename or drop a calendar reaches a scan, so
-     * the channels are kept in step here. A failure never costs the reminders.
+     * A provider change re-scans, so a calendar added, renamed or removed keeps
+     * its channel in step here. Skipped while reminders are off, when the
+     * channels have nothing to do. A failure never costs the reminders.
      */
-    private suspend fun syncChannels() {
+    private fun syncChannels() {
         try {
             channels.sync()
-        } catch (e: CancellationException) {
-            throw e
         } catch (e: SecurityException) {
             throw e
         } catch (e: Exception) {

app/src/main/java/de/jeanlucmakiola/calendula/ui/common/AccountGroups.kt

@@ -1,5 +1,6 @@
 package de.jeanlucmakiola.calendula.ui.common
 
+import android.content.Context
 import androidx.compose.runtime.Composable
 import androidx.compose.runtime.remember
 import androidx.compose.ui.platform.LocalContext
@@ -72,11 +73,13 @@ fun accountGroupTitle(group: CalendarAccountGroup): String {
 @Composable
 fun sourceAppName(accountType: String): String {
     val context = LocalContext.current
-    return remember(accountType) {
-        val pm = context.packageManager
-        val packages = sourceAppPackages(context, accountType)
-        packages.firstNotNullOfOrNull { pkg ->
-            runCatching { pm.getApplicationLabel(pm.getApplicationInfo(pkg, 0)).toString() }.getOrNull()
-        } ?: accountType
-    }
+    return remember(accountType) { sourceAppLabel(context, accountType) }
+}
+
+/** [sourceAppName] outside composition, e.g. for notification channel groups. */
+fun sourceAppLabel(context: Context, accountType: String): String {
+    val pm = context.packageManager
+    return sourceAppPackages(context, accountType).firstNotNullOfOrNull { pkg ->
+        runCatching { pm.getApplicationLabel(pm.getApplicationInfo(pkg, 0)).toString() }.getOrNull()
+    } ?: accountType
 }

app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/ReminderChannelPlanTest.kt

@@ -24,8 +24,9 @@ class ReminderChannelPlanTest {
                 calendar(2, "me@example.org", "com.google"),
                 calendar(3, "dav", "bitfire.at.davdroid"),
             ),
-            existingChannelIds = emptyList(),
+            existing = emptyList(),
             existingGroupIds = emptyList(),
+            unnamed = "Unnamed",
         )
         assertThat(plan.groups).hasSize(2)
         assertThat(plan.channels.map { it.id })
@@ -42,8 +43,9 @@ class ReminderChannelPlanTest {
                 calendar(1, "Calendula", "LOCAL", local = true),
                 calendar(2, "Other", "LOCAL", local = true),
             ),
-            existingChannelIds = emptyList(),
+            existing = emptyList(),
             existingGroupIds = emptyList(),
+            unnamed = "Unnamed",
         )
         assertThat(plan.groups).hasSize(1)
         assertThat(plan.groups.single().isLocal).isTrue()
@@ -57,27 +59,47 @@ class ReminderChannelPlanTest {
                 calendar(2, "me@example.org", "bitfire.at.davdroid"),
                 calendar(3, "work", "bitfire.at.davdroid"),
             ),
-            existingChannelIds = emptyList(),
+            existing = emptyList(),
             existingGroupIds = emptyList(),
+            unnamed = "Unnamed",
         )
         assertThat(plan.groups.filter { it.ambiguous }.map { it.accountType })
             .containsExactly("com.google", "bitfire.at.davdroid")
-        assertThat(plan.groups.single { it.accountName == "work" }.ambiguous).isFalse()
+        assertThat(plan.groups.single { it.label == "work" }.ambiguous).isFalse()
     }
 
     @Test
     fun `stale reminder channels and groups go, foreign ones stay`() {
         val current = calendar(1, "me@example.org", "com.google")
-        val keptGroup = planReminderChannels(listOf(current), emptyList(), emptyList()).groups.single().id
-        val gone = planReminderChannels(listOf(calendar(9, "old", "com.google")), emptyList(), emptyList())
-        val plan = planReminderChannels(
+        fun plan(calendars: List<CalendarSource>, existing: List<String> = emptyList(), groups: List<String> = emptyList()) =
+            planReminderChannels(calendars, existing.map { ExistingReminderChannel(it, "Cal ${it.last()}") }, groups, "Unnamed")
+        val keptGroup = plan(listOf(current)).groups.single().id
+        val goneGroup = plan(listOf(calendar(9, "old", "com.google"))).groups.single().id
+        val result = plan(
             calendars = listOf(current),
-            existingChannelIds = listOf(
-                reminderChannelId(1), reminderChannelId(9), LEGACY_REMINDER_CHANNEL_ID, "backup",
-            ),
-            existingGroupIds = listOf(keptGroup, gone.groups.single().id, "other"),
+            existing = listOf(reminderChannelId(1), reminderChannelId(9), LEGACY_REMINDER_CHANNEL_ID, "backup"),
+            groups = listOf(keptGroup, goneGroup, "other"),
         )
-        assertThat(plan.staleChannelIds).containsExactly(reminderChannelId(9))
-        assertThat(plan.staleGroupIds).containsExactly(gone.groups.single().id)
+        assertThat(result.staleChannelIds).containsExactly(reminderChannelId(9))
+        assertThat(result.staleGroupIds).containsExactly(goneGroup)
+    }
+
+    @Test
+    fun `only missing or renamed channels are written`() {
+        val plan = planReminderChannels(
+            calendars = listOf(
+                calendar(1, "a", "com.google"),
+                calendar(2, "a", "com.google"),
+                calendar(3, "a", "com.google").copy(displayName = ""),
+            ),
+            existing = listOf(
+                ExistingReminderChannel(reminderChannelId(1), "Cal 1"),
+                ExistingReminderChannel(reminderChannelId(2), "Old name"),
+            ),
+            existingGroupIds = emptyList(),
+            unnamed = "Unnamed",
+        )
+        assertThat(plan.channelsToWrite.map { it.calendarId }).containsExactly(2L, 3L)
+        assertThat(plan.channelsToWrite.last().name).isEqualTo("Unnamed")
     }
 }

docs/ARCHITECTURE.md

@@ -392,11 +392,11 @@ 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 switched-on calendar, grouped by account, so a calendar can be muted
-or given its own sound in the system settings before it ever fires. Every scan
-syncs them first: missing channels are created, names refreshed, and channels of
-gone or switched-off calendars deleted (the system restores a deleted channel's
-settings if the id returns). A new channel copies the sound, vibration and
+channel per calendar, grouped by account, 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: missing channels are created, renamed ones updated, and
+channels of deleted calendars removed. Switched-off calendars keep theirs, so
+toggling one never cancels a showing reminder or loses its settings. A new channel copies the sound, vibration and
 importance of the pre-#145 single `reminders` channel, which is deleted once
 nothing showing still sits on it. Settings → Notifications only deep-links into
 the system's channel screens (#340); the app never sets a sound itself. Separate