diff --git a/app/src/main/kotlin/org/libremail/domain/model/MailProvider.kt b/app/src/main/kotlin/org/libremail/domain/model/MailProvider.kt index 927b32e..858a6bb 100644 --- a/app/src/main/kotlin/org/libremail/domain/model/MailProvider.kt +++ b/app/src/main/kotlin/org/libremail/domain/model/MailProvider.kt @@ -33,6 +33,12 @@ enum class MailProvider( private val smtpHost: String, private val smtpPort: Int, private val smtpSecurity: MailSecurity, + /** + * Extra IMAP hosts that also identify this provider, besides [imapHost] — e.g. Gmail's legacy + * `imap.googlemail.com`. Matched case-insensitively by [matchesHost] so a manually configured + * account on a legacy host still resolves to the right brand. + */ + private val hostAliases: List = emptyList(), ) { GMAIL( key = "gmail", @@ -43,6 +49,8 @@ enum class MailProvider( // Google documents smtp.gmail.com:587 with STARTTLS as the standard submission endpoint. smtpPort = SMTP_SUBMISSION_PORT, smtpSecurity = MailSecurity.STARTTLS, + // Legacy Gmail IMAP host still seen on older, manually configured accounts. + hostAliases = listOf("imap.googlemail.com"), ), YAHOO( key = "yahoo", @@ -83,12 +91,45 @@ enum class MailProvider( ) } + /** + * True when [host] is this provider's [imapHost] or one of its [hostAliases] (case-insensitive). + * The single place that knows which IMAP hosts belong to this provider. + */ + fun matchesHost(host: String): Boolean = + imapHost.equals(host, ignoreCase = true) || hostAliases.any { it.equals(host, ignoreCase = true) } + companion object { + /** The folder-label brand shown for Microsoft Outlook / Office 365 accounts. */ + const val OUTLOOK_BRAND = "Outlook" + /** Resolves a provider by its [key], or null if none matches (case-insensitive). */ fun fromKey(key: String): MailProvider? = entries.firstOrNull { it.key.equals(key, ignoreCase = true) } - /** Resolves a provider by its IMAP host, or null if none matches (case-insensitive). */ - fun forImapHost(host: String): MailProvider? = - entries.firstOrNull { it.imapHost.equals(host, ignoreCase = true) } + /** Resolves an app-password provider by its IMAP host (or alias), or null if none matches. */ + fun forImapHost(host: String): MailProvider? = entries.firstOrNull { it.matchesHost(host) } + + /** + * The recognized brand name for [account], or null when its host maps to no known brand. The + * single source of truth for host→brand matching: Outlook is identified by its OAuth auth + * type or a Microsoft mail host (it is deliberately not an app-password [MailProvider] entry, + * as it uses interactive OAuth), and every other brand resolves through [forImapHost]. + */ + fun brandFor(account: Account): String? = when { + account.authType == AuthType.OAUTH_OUTLOOK -> OUTLOOK_BRAND + isOutlookHost(account.imap.host) -> OUTLOOK_BRAND + else -> forImapHost(account.imap.host)?.displayName + } + + /** + * True for Microsoft's Outlook / Office 365 mail hosts (e.g. `outlook.office365.com`). Matches + * the `office365.com` domain and `outlook.office.com` precisely, rather than any host merely + * containing "outlook", so an unrelated host can't be mislabeled as Outlook. + */ + private fun isOutlookHost(host: String): Boolean { + val normalized = host.lowercase() + return normalized == "office365.com" || + normalized.endsWith(".office365.com") || + normalized == "outlook.office.com" + } } } diff --git a/app/src/main/kotlin/org/libremail/ui/mailbox/FolderDrawer.kt b/app/src/main/kotlin/org/libremail/ui/mailbox/FolderDrawer.kt index 8695334..9481e43 100644 --- a/app/src/main/kotlin/org/libremail/ui/mailbox/FolderDrawer.kt +++ b/app/src/main/kotlin/org/libremail/ui/mailbox/FolderDrawer.kt @@ -97,7 +97,9 @@ fun FolderDrawer( } val unread = folderUnreadCounts[folder.fullName] ?: 0 NavigationDrawerItem( - label = { Text(resolvedLabels[folder.fullName] ?: folderDisplayLabel(folder)) }, + // getValue: resolvedLabels is keyed by every folder in this same list, so a miss is + // a bug to surface, not to paper over with a re-derived (un-deduplicated) label. + label = { Text(resolvedLabels.getValue(folder.fullName)) }, icon = iconContent, badge = if (unread > 0) { { UnreadBadgeLabel(unread) } @@ -183,24 +185,43 @@ private fun UnreadBadgeLabel(count: Int) { * The provider suffix derives from the account owning the listed folders (see [providerLabelFor]), * not from whichever account is currently selected, so a folder list that briefly lags an account * switch never borrows the incoming account's brand. + * + * The Compose-only lookups (the role→name strings and the localized disambiguation [LabelPatterns]) + * are hoisted out of the [remember] calculation — `stringResource` can't run inside its + * `@DisallowComposableCalls` body — which then memoizes the resolved map, rebuilding it only when + * the folders, accounts, or those strings change. Callers that compose while idle (the closed + * navigation drawer) and taps that change only the selection then reuse the same map instance. */ @Composable fun resolvedFolderLabels(folders: List, accounts: List): Map { - val baseLabels = folders.associate { it.fullName to folderDisplayLabel(it) } - return resolveDrawerLabels(folders, baseLabels, providerLabelFor(folders, accounts)) + val roleLabels = folderRoleLabels() + val patterns = LabelPatterns( + withProvider = stringResource(R.string.folder_label_with_provider), + withParent = stringResource(R.string.folder_label_with_parent), + withPath = stringResource(R.string.folder_label_with_path), + ) + return remember(folders, accounts, roleLabels, patterns) { + val baseLabels = folders.associate { folder -> + folder.fullName to (roleLabels[folder.role] ?: folder.displayName) + } + resolveDrawerLabels(folders, baseLabels, providerLabelFor(folders, accounts), patterns) + } } +/** The localized friendly names for the standard folder roles; [FolderRole.NORMAL] has none. */ +@Composable +private fun folderRoleLabels(): Map = mapOf( + FolderRole.INBOX to stringResource(R.string.folder_inbox), + FolderRole.SENT to stringResource(R.string.folder_sent), + FolderRole.DRAFTS to stringResource(R.string.folder_drafts), + FolderRole.ARCHIVE to stringResource(R.string.folder_archive), + FolderRole.SPAM to stringResource(R.string.folder_spam), + FolderRole.TRASH to stringResource(R.string.folder_trash), +) + /** The user-facing label for a folder: a friendly name for standard roles, else the server name. */ @Composable -fun folderDisplayLabel(folder: Folder): String = when (folder.role) { - FolderRole.INBOX -> stringResource(R.string.folder_inbox) - FolderRole.SENT -> stringResource(R.string.folder_sent) - FolderRole.DRAFTS -> stringResource(R.string.folder_drafts) - FolderRole.ARCHIVE -> stringResource(R.string.folder_archive) - FolderRole.SPAM -> stringResource(R.string.folder_spam) - FolderRole.TRASH -> stringResource(R.string.folder_trash) - FolderRole.NORMAL -> folder.displayName -} +fun folderDisplayLabel(folder: Folder): String = folderRoleLabels()[folder.role] ?: folder.displayName /** A leading icon for standard folders, limited to the material-icons-core set (null = no icon). */ private fun folderIcon(role: FolderRole): ImageVector? = when (role) { diff --git a/app/src/main/kotlin/org/libremail/ui/mailbox/FolderLabels.kt b/app/src/main/kotlin/org/libremail/ui/mailbox/FolderLabels.kt index db2d928..94005e4 100644 --- a/app/src/main/kotlin/org/libremail/ui/mailbox/FolderLabels.kt +++ b/app/src/main/kotlin/org/libremail/ui/mailbox/FolderLabels.kt @@ -2,22 +2,16 @@ package org.libremail.ui.mailbox import org.libremail.domain.model.Account -import org.libremail.domain.model.AuthType import org.libremail.domain.model.Folder import org.libremail.domain.model.MailProvider /** * The provider name appended when de-duplicating a folder label (the "Gmail" in "Drafts - Gmail"). - * A recognized brand where possible, else the account's email domain so any account has a suffix. + * A recognized brand where possible (resolved through [MailProvider.brandFor], the single source of + * host→brand knowledge), else the account's email domain so any account still gets a suffix. */ -fun providerLabel(account: Account): String = when { - account.authType == AuthType.OAUTH_OUTLOOK || - account.imap.host.contains("outlook", ignoreCase = true) || - account.imap.host.contains("office365", ignoreCase = true) -> "Outlook" - - else -> MailProvider.forImapHost(account.imap.host)?.displayName - ?: account.email.substringAfterLast('@', account.imap.host) -} +fun providerLabel(account: Account): String = + MailProvider.brandFor(account) ?: account.email.substringAfterLast('@', account.imap.host) /** * The provider suffix used when de-duplicating [folders]' labels, derived from the account that owns @@ -29,6 +23,24 @@ fun providerLabel(account: Account): String = when { fun providerLabelFor(folders: List, accounts: List): String = accounts.firstOrNull { it.id == folders.firstOrNull()?.accountId }?.let(::providerLabel).orEmpty() +// Default disambiguation patterns. These mirror the strings.xml resources of the same names, and the +// literals used before the patterns were externalized (#68); the localized resources are supplied by +// the composable call site (see resolvedFolderLabels). "%1$s" is the base label, "%2$s" the detail. +private const val PROVIDER_LABEL_PATTERN = "%1\$s - %2\$s" +private const val PARENT_LABEL_PATTERN = "%1\$s (%2\$s)" +private const val PATH_LABEL_PATTERN = "%1\$s [%2\$s]" + +/** + * The locale-specific patterns for joining a folder's base label with its disambiguating detail. + * Defaults match the pre-i18n literals so [resolveDrawerLabels] stays a pure, JVM-testable function; + * the drawer supplies the localized strings.xml values (the folder_label_with_* strings). + */ +data class LabelPatterns( + val withProvider: String = PROVIDER_LABEL_PATTERN, + val withParent: String = PARENT_LABEL_PATTERN, + val withPath: String = PATH_LABEL_PATTERN, +) + /** * Resolves each folder's user-facing label so folders that would render the same name are told apart * (used by the drawer, the move-to picker, and the app-bar title). [baseLabels] maps a folder's @@ -46,16 +58,22 @@ fun resolveDrawerLabels( folders: List, baseLabels: Map, providerLabel: String, + patterns: LabelPatterns = LabelPatterns(), ): Map { - fun base(folder: Folder) = baseLabels[folder.fullName] ?: folder.displayName + // getValue, not a fallback: baseLabels is built from this same folder list, so a miss is a + // programming error that should fail loudly rather than silently revert to an un-deduped label. + fun base(folder: Folder) = baseLabels.getValue(folder.fullName) val duplicated = folders.groupingBy(::base).eachCount().filterValues { it > 1 }.keys + // Dominant case: nothing collides, so every label is already its base. Return the base labels + // as-is rather than rebuilding a value-identical map (and re-running the counting pass below). + if (duplicated.isEmpty()) return baseLabels val resolved = folders.associate { folder -> val base = base(folder) val label = when { base !in duplicated -> base - folder.specialUse -> "$base - $providerLabel" - else -> userFolderLabel(folder, base) + folder.specialUse -> patterns.withProvider.format(base, providerLabel) + else -> userFolderLabel(folder, base, patterns.withParent) } folder.fullName to label } @@ -64,7 +82,9 @@ fun resolveDrawerLabels( // unambiguous full path so the tied entries stay apart. val stillTied = resolved.values.groupingBy { it }.eachCount().filterValues { it > 1 }.keys if (stillTied.isEmpty()) return resolved - return resolved.mapValues { (fullName, label) -> if (label in stillTied) "$label [$fullName]" else label } + return resolved.mapValues { (fullName, label) -> + if (label in stillTied) patterns.withPath.format(label, fullName) else label + } } /** @@ -74,10 +94,10 @@ fun resolveDrawerLabels( * named like the base keeps it — the others show their real server name rather than falling through * to the safety net as a self-referential "Sent [Sent]". */ -private fun userFolderLabel(folder: Folder, base: String): String { +private fun userFolderLabel(folder: Folder, base: String, parentPattern: String): String { val parent = parentOf(folder.fullName, folder.displayName) return when { - parent != null -> "$base ($parent)" + parent != null -> parentPattern.format(base, parent) folder.displayName.equals(base, ignoreCase = true) -> base else -> folder.displayName } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 75f6c41..f3869d0 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -68,6 +68,12 @@ Archive Spam Trash + + %1$s - %2$s + %1$s (%2$s) + %1$s [%2$s] 99+ diff --git a/app/src/test/kotlin/org/libremail/domain/model/MailProviderTest.kt b/app/src/test/kotlin/org/libremail/domain/model/MailProviderTest.kt index 52a8519..2a8a108 100644 --- a/app/src/test/kotlin/org/libremail/domain/model/MailProviderTest.kt +++ b/app/src/test/kotlin/org/libremail/domain/model/MailProviderTest.kt @@ -3,6 +3,7 @@ package org.libremail.domain.model import org.junit.Test import kotlin.test.assertEquals +import kotlin.test.assertFalse import kotlin.test.assertNotNull import kotlin.test.assertNull import kotlin.test.assertTrue @@ -119,4 +120,45 @@ class MailProviderTest { assertTrue(provider.displayName.isNotBlank()) } } + + // #69: host→brand matching is consolidated here. forImapHost matches primary hosts and aliases; + // brandFor is the single seam that adds Outlook (deliberately not a preset entry) to the mix. + @Test + fun `forImapHost matches primary hosts and Gmail's legacy alias, case-insensitively`() { + assertEquals(MailProvider.GMAIL, MailProvider.forImapHost("imap.gmail.com")) + assertEquals(MailProvider.GMAIL, MailProvider.forImapHost("imap.googlemail.com")) + assertEquals(MailProvider.GMAIL, MailProvider.forImapHost("IMAP.GMAIL.COM")) + assertEquals(MailProvider.ICLOUD, MailProvider.forImapHost("imap.mail.me.com")) + assertNull(MailProvider.forImapHost("imap.example.org")) + } + + @Test + fun `matchesHost recognizes a provider's aliases but not unrelated hosts`() { + assertTrue(MailProvider.GMAIL.matchesHost("imap.googlemail.com")) + assertFalse(MailProvider.GMAIL.matchesHost("imap.mail.me.com")) + } + + @Test + fun `brandFor centralizes host to brand matching, including Outlook`() { + assertEquals("Gmail", MailProvider.brandFor(account("imap.googlemail.com"))) + assertEquals("iCloud Mail", MailProvider.brandFor(account("imap.mail.me.com"))) + // A manually configured Office 365 host is Outlook even without the OAuth auth type. + assertEquals(MailProvider.OUTLOOK_BRAND, MailProvider.brandFor(account("outlook.office365.com"))) + // The OAuth auth type brands as Outlook regardless of host. + assertEquals( + MailProvider.OUTLOOK_BRAND, + MailProvider.brandFor(account("example.com", AuthType.OAUTH_OUTLOOK)), + ) + // Over-match guard: a host merely containing "outlook" maps to no brand. + assertNull(MailProvider.brandFor(account("outlook.example.com"))) + } + + private fun account(imapHost: String, authType: AuthType = AuthType.PASSWORD_IMAP) = Account( + id = "acct", + email = "user@example.com", + displayName = "user", + authType = authType, + imap = ServerConfig(imapHost, 993, MailSecurity.SSL_TLS), + smtp = ServerConfig("smtp.example.com", 587, MailSecurity.STARTTLS), + ) } diff --git a/app/src/test/kotlin/org/libremail/ui/mailbox/FolderLabelsTest.kt b/app/src/test/kotlin/org/libremail/ui/mailbox/FolderLabelsTest.kt index 43b3493..0e876ac 100644 --- a/app/src/test/kotlin/org/libremail/ui/mailbox/FolderLabelsTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/mailbox/FolderLabelsTest.kt @@ -9,6 +9,8 @@ import org.libremail.domain.model.FolderRole import org.libremail.domain.model.MailSecurity import org.libremail.domain.model.ServerConfig import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertSame class FolderLabelsTest { @@ -214,4 +216,88 @@ class FolderLabelsTest { ) assertEquals("example.org", providerLabel(account("alice@example.org", "imap.example.org"))) } + + // #69: host→brand matching is consolidated in MailProvider.brandFor and reached via providerLabel. + @Test + fun `providerLabel matches Gmail's legacy googlemail host via a provider alias`() { + assertEquals("Gmail", providerLabel(account("a@gmail.com", "imap.googlemail.com"))) + } + + @Test + fun `providerLabel brands a manually configured Office 365 host as Outlook without OAuth`() { + assertEquals("Outlook", providerLabel(account("a@contoso.com", "outlook.office365.com"))) + } + + @Test + fun `providerLabel does not brand an unrelated host that merely contains outlook`() { + // Over-match guard: the old contains("outlook") check would have branded this as Outlook. + assertEquals("example.com", providerLabel(account("a@example.com", "outlook.example.com"))) + } + + // #68: the resolver formats disambiguated labels through the supplied patterns (externalized to + // strings.xml), rather than hardcoded string interpolation. + @Test + fun `resolveDrawerLabels formats disambiguated labels using the supplied patterns`() { + val folders = listOf( + folder("[Gmail]/Drafts", "Drafts", FolderRole.DRAFTS, specialUse = true), + folder("Drafts", "Drafts", FolderRole.DRAFTS), + folder("Work/Reports"), + folder("Personal/Reports"), + ) + val labels = resolveDrawerLabels( + folders, + baseLabelsOf(folders, friendlyNames), + "Gmail", + LabelPatterns(withProvider = "%1\$s @ %2\$s", withParent = "%1\$s <%2\$s>"), + ) + assertEquals("Drafts @ Gmail", labels["[Gmail]/Drafts"]) + assertEquals("Reports ", labels["Work/Reports"]) + assertEquals("Reports ", labels["Personal/Reports"]) + } + + @Test + fun `resolveDrawerLabels uses the supplied path pattern for the full-path safety net`() { + val folders = listOf( + folder("[Gmail]/All Mail", "All Mail", FolderRole.ARCHIVE, specialUse = true), + folder("Archives", "Archives", FolderRole.ARCHIVE, specialUse = true), + ) + val labels = resolveDrawerLabels( + folders, + baseLabelsOf(folders, friendlyNames), + "Gmail", + LabelPatterns(withPath = "%1\$s {%2\$s}"), + ) + assertEquals("Archive - Gmail {[Gmail]/All Mail}", labels["[Gmail]/All Mail"]) + assertEquals("Archive - Gmail {Archives}", labels["Archives"]) + } + + // #67: the resolver short-circuits when nothing collides, returning the base labels unchanged... + @Test + fun `resolveDrawerLabels returns the base labels unchanged when nothing collides`() { + val folders = listOf(folder("Receipts"), folder("Work/Reports")) + val baseLabels = baseLabelsOf(folders, friendlyNames) + assertSame(baseLabels, resolveDrawerLabels(folders, baseLabels, "Gmail")) + } + + // ...and fails loudly rather than silently reverting to un-deduplicated labels on a missing key. + @Test + fun `resolveDrawerLabels fails fast when a folder has no base label`() { + val folders = listOf( + folder("Inbox", "Inbox", FolderRole.INBOX), + folder("Orphan"), + ) + val incompleteBaseLabels = mapOf("Inbox" to "Inbox") + assertFailsWith { + resolveDrawerLabels(folders, incompleteBaseLabels, "Gmail") + } + } + + @Test + fun `resolveDrawerLabels is deterministic for equal inputs`() { + val folders = listOf( + folder("[Gmail]/Drafts", "Drafts", FolderRole.DRAFTS, specialUse = true), + folder("Drafts", "Drafts", FolderRole.DRAFTS), + ) + assertEquals(resolve(folders, "Gmail"), resolve(folders, "Gmail")) + } }