fix(calendars): tell two same-named accounts apart (#77)

Calendars were grouped by account name alone, so a Google account and a DAVx5 account for the same address became one group. That is more than a mis-grouping: the header's source logo and "manage in app" came off cals.first(), so it could send you to the wrong app; "toggle all" spanned both accounts; and the collapsed state, keyed on the name, folded both at once. Grouping now keys on (name, type), in one shared place — Settings → Calendars, the calendar picker and the drawer's filter each had their own copy with slightly different fallbacks. The header takes its logo and manage action from its own group's type, and the collapse set holds the key rather than the name. Where a name really is shared, each group names the app it comes from, resolved the same way the source icon is.
2026-07-30 15:23:33 +02:00
parent 34f23ee14e
commit 05b64804bb
9 changed files with 224 additions and 52 deletions
--- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsScreen.kt
+++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsScreen.kt
@@ -99,7 +99,10 @@ import de.jeanlucmakiola.calendula.domain.isNotSynced
 import de.jeanlucmakiola.calendula.domain.orderedForManager
 import de.jeanlucmakiola.calendula.domain.stateLabels
 import de.jeanlucmakiola.calendula.data.calendar.CalendarColorPalette
+import de.jeanlucmakiola.calendula.ui.common.AccountKey
 import de.jeanlucmakiola.calendula.ui.common.CalendarColorChip
+import de.jeanlucmakiola.calendula.ui.common.accountGroupTitle
+import de.jeanlucmakiola.calendula.ui.common.groupByAccount
 import de.jeanlucmakiola.calendula.ui.common.LocalSoftenColors
 import de.jeanlucmakiola.calendula.ui.common.eventFill
 import de.jeanlucmakiola.calendula.ui.common.LeadingAvatar
@@ -228,7 +231,7 @@ private fun CalendarsList(
    val snackbarHostState = remember { SnackbarHostState() }
    // Accounts the user has folded shut; empty = all expanded (keeps every
    // calendar visible by default, the section is collapsible for tidiness).
-    var collapsedAccounts by remember { mutableStateOf(emptySet<String>()) }
+    var collapsedAccounts by remember { mutableStateOf(emptySet<AccountKey>()) }
    var localExpanded by remember { mutableStateOf(true) }
    val writeErrorText = stringResource(R.string.calendars_write_error)

@@ -417,10 +420,11 @@ private fun CalendarsList(
        SectionHeader(stringResource(R.string.calendars_synced_header))
        HintText(stringResource(R.string.calendars_synced_hint))
        synced
-            .groupBy { it.accountName.ifBlank { it.accountType } }
-            .forEach { (account, cals) ->
-                val expanded = account !in collapsedAccounts
-                val accountType = cals.first().accountType
+            .groupByAccount()
+            .forEach { group ->
+                val cals = group.calendars
+                val expanded = group.key !in collapsedAccounts
+                val accountType = group.accountType
                // A non-syncing calendar has no switch, so it neither counts
                // towards "the whole account is off" nor moves with toggle-all.
                val switchable = cals.filter { it.hasVisibilitySwitch }
@@ -428,7 +432,7 @@ private fun CalendarsList(
                    switchable.none { it.isVisibleInSystem }
                Spacer(Modifier.height(16.dp))
                CalendarGroup(
-                    title = account,
+                    title = accountGroupTitle(group),
                    expanded = expanded,
                    bodyHasRows = true,
                    headerDisabled = accountDisabled,
@@ -440,9 +444,9 @@ private fun CalendarsList(
                    },
                    onToggleExpand = {
                        collapsedAccounts = if (expanded) {
-                            collapsedAccounts + account
+                            collapsedAccounts + group.key
                        } else {
-                            collapsedAccounts - account
+                            collapsedAccounts - group.key
                        }
                    },
                    showToggleAll = switchable.isNotEmpty(),
--- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/common/AccountGroups.kt
+++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/common/AccountGroups.kt
@@ -0,0 +1,86 @@
+package de.jeanlucmakiola.calendula.ui.common
+
+import androidx.compose.runtime.Composable
+import androidx.compose.runtime.remember
+import androidx.compose.ui.platform.LocalContext
+import androidx.compose.ui.res.stringResource
+import de.jeanlucmakiola.calendula.R
+import de.jeanlucmakiola.calendula.domain.CalendarSource
+
+/**
+ * One account's calendars, as every surface that lists calendars by account
+ * shows them.
+ *
+ * An account is identified by name **and** type (#77). A Google account and a
+ * DAVx5 account can carry the same address and still be two separate accounts,
+ * from separate apps: merging them mixed their calendars into one group whose
+ * header — source logo, "manage in app", toggle-all, collapsed state — was
+ * derived from whichever calendar happened to sort first.
+ */
+data class CalendarAccountGroup(
+    /** Stable identity: what makes two calendars belong to the same account. */
+    val key: AccountKey,
+    /** The account's own name, as shown when it is unambiguous. */
+    val label: String,
+    /** True when another group shows the same [label] under a different type. */
+    val ambiguous: Boolean,
+    val calendars: List<CalendarSource>,
+) {
+    val accountType: String get() = key.type
+}
+
+/** The pair a group is keyed on. */
+data class AccountKey(val name: String, val type: String)
+
+/**
+ * Group [calendars] under their owning account, preserving the provider's order
+ * within each group and ordering groups by first appearance.
+ *
+ * The label falls back through name → type → the first calendar's own name, so
+ * a calendar with no account still lands somewhere sensible.
+ */
+fun List<CalendarSource>.groupByAccount(): List<CalendarAccountGroup> {
+    val grouped = groupBy { AccountKey(it.accountName, it.accountType) }
+    val labels = grouped.mapValues { (key, cals) ->
+        key.name.ifBlank { key.type }.ifBlank { cals.first().displayName }
+    }
+    val shared = labels.values.groupingBy { it }.eachCount()
+    return grouped.map { (key, cals) ->
+        val label = labels.getValue(key)
+        CalendarAccountGroup(
+            key = key,
+            label = label,
+            ambiguous = shared.getValue(label) > 1,
+            calendars = cals,
+        )
+    }
+}
+
+/**
+ * What to write above a group: its account name, qualified with the app the
+ * account comes from when another account shares the name (#77).
+ */
+@Composable
+fun accountGroupTitle(group: CalendarAccountGroup): String =
+    if (!group.ambiguous) {
+        group.label
+    } else {
+        stringResource(R.string.calendars_account_from_source, group.label, sourceAppName(group.accountType))
+    }
+
+/**
+ * The human name of the app backing [accountType] — the same app whose icon
+ * [SourceLogo] draws. Falls back to the raw account type, which is at least
+ * unique, when no installed app resolves for it.
+ */
+@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
+    }
+}
--- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/common/CalendarPickerGroups.kt
+++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/common/CalendarPickerGroups.kt
@@ -60,9 +60,7 @@ fun ColumnScope.CalendarPickerGroups(
 ) {
    val local = remember(calendars) { calendars.filter { it.isLocal } }
    val syncedGroups = remember(calendars) {
-        calendars.filterNot { it.isLocal }
-            .groupBy { it.accountName.ifBlank { it.accountType }.ifBlank { it.displayName } }
-            .toList()
+        calendars.filterNot { it.isLocal }.groupByAccount()
    }

    if (local.isNotEmpty()) {
@@ -74,12 +72,12 @@ fun ColumnScope.CalendarPickerGroups(
            onSelect = onSelect,
        )
    }
-    syncedGroups.forEachIndexed { index, (account, cals) ->
+    syncedGroups.forEachIndexed { index, group ->
        if (local.isNotEmpty() || index > 0) Spacer(Modifier.height(16.dp))
        CalendarPickerGroup(
-            title = account,
-            leading = { SourceLogo(cals.first().accountType) },
-            calendars = cals,
+            title = accountGroupTitle(group),
+            leading = { SourceLogo(group.accountType) },
+            calendars = group.calendars,
            selectedId = selectedId,
            onSelect = onSelect,
        )
@@ -181,20 +179,22 @@ fun LeadingAvatar(icon: ImageVector) {
 /** The launcher icon of the app backing [accountType], preferring the human-facing app. */
 private fun sourceAppLogo(context: Context, accountType: String): ImageBitmap? {
    val pm = context.packageManager
-    val candidates = buildList {
-        curatedSourcePackage(accountType)?.let { add(it) }
-        AccountManager.get(context).authenticatorTypes
-            .firstOrNull { it.type.equals(accountType, ignoreCase = true) }
-            ?.packageName
-            ?.let { add(it) }
-    }
-    for (pkg in candidates) {
+    for (pkg in sourceAppPackages(context, accountType)) {
        val bitmap = runCatching { pm.getApplicationIcon(pkg).toBitmap() }.getOrNull()
        if (bitmap != null) return bitmap.asImageBitmap()
    }
    return null
 }

+/** Apps that could stand for [accountType], best candidate first. */
+internal fun sourceAppPackages(context: Context, accountType: String): List<String> = buildList {
+    curatedSourcePackage(accountType)?.let { add(it) }
+    AccountManager.get(context).authenticatorTypes
+        .firstOrNull { it.type.equals(accountType, ignoreCase = true) }
+        ?.packageName
+        ?.let { add(it) }
+}
+
 /** Preferred app for account types whose authenticator isn't the app to open. */
 internal fun curatedSourcePackage(accountType: String): String? = when {
    accountType.equals("com.google", ignoreCase = true) -> "com.google.android.calendar"
--- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/filter/CalendarFilterList.kt
+++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/filter/CalendarFilterList.kt
@@ -21,6 +21,7 @@ import androidx.lifecycle.compose.collectAsStateWithLifecycle
 import de.jeanlucmakiola.calendula.R
 import de.jeanlucmakiola.calendula.domain.FailureReason
 import de.jeanlucmakiola.calendula.ui.common.CalendarColorChip
+import de.jeanlucmakiola.calendula.ui.common.sourceAppName
 import de.jeanlucmakiola.floret.components.GroupedRow
 import de.jeanlucmakiola.floret.components.positionOf

@@ -61,7 +62,15 @@ private fun FilterList(
    Column(modifier = modifier.fillMaxWidth()) {
        groups.forEach { group ->
            Text(
-                text = group.account,
+                text = if (group.ambiguous) {
+                    stringResource(
+                        R.string.calendars_account_from_source,
+                        group.account,
+                        sourceAppName(group.accountType),
+                    )
+                } else {
+                    group.account
+                },
                style = MaterialTheme.typography.labelMedium,
                color = MaterialTheme.colorScheme.onSurfaceVariant,
                modifier = Modifier.padding(start = 28.dp, end = 28.dp, top = 12.dp, bottom = 4.dp),
--- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/filter/FilterUiState.kt
+++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/filter/FilterUiState.kt
@@ -13,9 +13,17 @@ sealed interface FilterUiState {
    data class Success(val groups: List<AccountGroup>) : FilterUiState
 }

-/** Calendars grouped under the account that owns them (Nextcloud / Local / …). */
+/**
+ * Calendars grouped under the account that owns them (Nextcloud / Local / …).
+ *
+ * [accountType] and [ambiguous] carry what the header needs to tell two
+ * same-named accounts from different apps apart (#77); the label itself is
+ * built in the UI layer, which is where the source app's name can be looked up.
+ */
 data class AccountGroup(
    val account: String,
+    val accountType: String,
+    val ambiguous: Boolean,
    val calendars: List<CalendarRow>,
 )

--- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/filter/FilterViewModel.kt
+++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/filter/FilterViewModel.kt
@@ -8,6 +8,7 @@ import de.jeanlucmakiola.calendula.data.di.IoDispatcher
 import de.jeanlucmakiola.calendula.data.prefs.CalendarPrefs
 import de.jeanlucmakiola.calendula.domain.CalendarSource
 import de.jeanlucmakiola.calendula.domain.FailureReason
+import de.jeanlucmakiola.calendula.ui.common.groupByAccount
 import kotlinx.coroutines.CoroutineDispatcher
 import kotlinx.coroutines.flow.SharingStarted
 import kotlinx.coroutines.flow.StateFlow
@@ -38,7 +39,7 @@ class FilterViewModel @Inject constructor(
            if (enabled.isEmpty()) {
                FilterUiState.Failure(FailureReason.NoCalendarsConfigured)
            } else {
-                FilterUiState.Success(groupByAccount(enabled, hidden))
+                FilterUiState.Success(groupCalendarsForFilter(enabled, hidden))
            }
        }
            .catch { emit(FilterUiState.Failure(FailureReason.ProviderUnavailable)) }
@@ -60,30 +61,27 @@ class FilterViewModel @Inject constructor(
 }

 /**
- * Group calendars under their owning account, preserving the provider's order
- * within each group and ordering groups by first appearance. A calendar is
- * "visible" when its id is *not* in [hidden].
+ * Group calendars under their owning account — by name *and* type, so two
+ * accounts that merely share a name stay apart (#77) — preserving the
+ * provider's order within each group and ordering groups by first appearance.
+ * A calendar is "visible" when its id is *not* in [hidden].
 */
-internal fun groupByAccount(
+internal fun groupCalendarsForFilter(
    calendars: List<CalendarSource>,
    hidden: Set<Long>,
 ): List<AccountGroup> =
-    calendars
-        .groupBy { it.accountLabel() }
-        .map { (account, cals) ->
-            AccountGroup(
-                account = account,
-                calendars = cals.map { c ->
-                    CalendarRow(
-                        id = c.id,
-                        displayName = c.displayName,
-                        color = c.color,
-                        visible = c.id !in hidden,
-                    )
-                },
-            )
-        }
-
-/** Account header text: the account name, falling back to its type. */
-private fun CalendarSource.accountLabel(): String =
-    accountName.takeIf { it.isNotBlank() } ?: accountType.takeIf { it.isNotBlank() } ?: displayName
+    calendars.groupByAccount().map { group ->
+        AccountGroup(
+            account = group.label,
+            accountType = group.accountType,
+            ambiguous = group.ambiguous,
+            calendars = group.calendars.map { c ->
+                CalendarRow(
+                    id = c.id,
+                    displayName = c.displayName,
+                    color = c.color,
+                    visible = c.id !in hidden,
+                )
+            },
+        )
+    }
--- a/app/src/main/res/values/strings.xml
+++ b/app/src/main/res/values/strings.xml
@@ -488,6 +488,8 @@
    <string name="calendars_synced_header">Synced calendars</string>
    <string name="calendars_synced_hint">These come from accounts on your device. Create and edit them in their own app.</string>
    <string name="calendars_manage_in_app">Manage in app</string>
+    <!-- Account header when two accounts share a name: %1$s is the account, %2$s the app it comes from. -->
+    <string name="calendars_account_from_source">%1$s (%2$s)</string>
    <string name="calendars_account_menu_a11y">More options for %1$s</string>
    <string name="calendars_enable_all">Enable all</string>
    <string name="calendars_disable_all">Disable all</string>
--- a/app/src/test/java/de/jeanlucmakiola/calendula/ui/filter/FilterGroupingTest.kt
+++ b/app/src/test/java/de/jeanlucmakiola/calendula/ui/filter/FilterGroupingTest.kt
@@ -28,7 +28,7 @@ class FilterGroupingTest {
            cal(3, "Shared", "team@dav"),
        )

-        val groups = groupByAccount(calendars, hidden = emptySet())
+        val groups = groupCalendarsForFilter(calendars, hidden = emptySet())

        assertThat(groups.map { it.account }).containsExactly("alice@dav", "team@dav").inOrder()
        assertThat(groups[0].calendars.map { it.displayName })
@@ -43,7 +43,7 @@ class FilterGroupingTest {
            cal(2, "Work", "alice@dav"),
        )

-        val groups = groupByAccount(calendars, hidden = setOf(2L))
+        val groups = groupCalendarsForFilter(calendars, hidden = setOf(2L))
        val rows = groups.single().calendars.associateBy { it.id }

        assertThat(rows.getValue(1L).visible).isTrue()
@@ -52,10 +52,52 @@ class FilterGroupingTest {

    @Test
    fun `blank account name falls back to type`() {
-        val groups = groupByAccount(
+        val groups = groupCalendarsForFilter(
            listOf(cal(1, "Birthdays", account = "", type = "LOCAL")),
            hidden = emptySet(),
        )
        assertThat(groups.single().account).isEqualTo("LOCAL")
    }
+
+    /** #77: same address, two apps — two accounts, and they must read as two. */
+    @Test
+    fun `one name under two account types stays two groups`() {
+        val calendars = listOf(
+            cal(1, "Personal", "me@example.com", type = "com.google"),
+            cal(2, "Shared", "me@example.com", type = "bitfire.at.davdroid"),
+        )
+
+        val groups = groupCalendarsForFilter(calendars, hidden = emptySet())
+
+        assertThat(groups).hasSize(2)
+        assertThat(groups.map { it.accountType })
+            .containsExactly("com.google", "bitfire.at.davdroid").inOrder()
+        assertThat(groups.map { it.calendars.single().id }).containsExactly(1L, 2L).inOrder()
+        assertThat(groups.all { it.ambiguous }).isTrue()
+    }
+
+    @Test
+    fun `one name under one type is not ambiguous`() {
+        val groups = groupCalendarsForFilter(
+            listOf(cal(1, "Personal", "me@example.com")),
+            hidden = emptySet(),
+        )
+
+        assertThat(groups.single().ambiguous).isFalse()
+    }
+
+    /** Two different names from the same app are still two plain groups. */
+    @Test
+    fun `different names under one type are not ambiguous`() {
+        val groups = groupCalendarsForFilter(
+            listOf(
+                cal(1, "Personal", "alice@example.com"),
+                cal(2, "Shared", "bob@example.com"),
+            ),
+            hidden = emptySet(),
+        )
+
+        assertThat(groups).hasSize(2)
+        assertThat(groups.none { it.ambiguous }).isTrue()
+    }
 }