refactor(folders): consolidate, externalize, and memoize folder-label resolution

Three code-quality cleanups from the PR #54 review, all in the folder-label
plumbing so they ship as one change (adapted to the post-#108/#117 code):

#69 providerLabel: consolidate provider-brand host matching. Host->brand
knowledge now lives solely in MailProvider: forImapHost matches an entry's
imapHost plus new hostAliases (Gmail gains legacy imap.googlemail.com), and a
new companion brandFor(account) is the single seam that also recognizes
Outlook (by OAuth auth type or a precise office365.com / outlook.office.com
host, not any substring). MailProvider stays the app-password preset registry
(Outlook is not an entry). providerLabel() drops its ad-hoc host substrings.

#68 i18n: move folder-label disambiguation patterns into strings.xml. The
"base - provider", "base (parent)", and "base [path]" grammars become
folder_label_with_provider/parent/path resources, threaded into the pure
resolver as a LabelPatterns bundle whose defaults match the old literals; the
composable resolves the localized strings and passes them down.

#67 FolderDrawer: memoize label resolution, resolver early-return, fail-fast
lookups. resolvedFolderLabels hoists the role->string and pattern lookups out
of a remember() so the resolved map is rebuilt only when folders/accounts/
strings change (not every recomposition of the idle drawer). resolveDrawerLabels
returns baseLabels unchanged when nothing collides, and both map lookups use
getValue so a key miss fails loudly instead of silently un-deduplicating.

Behavior is unchanged: existing FolderLabelsTest and FolderDrawerTest
assertions (from #60/#61/#64/#108) stay green. Adds unit tests for host->brand
matching and its over-match guard, pattern-driven formatting, the early-return
identity, and fail-fast on a missing base label.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
2026-07-02 04:01:27 -05:00
co-authored by Claude Fable 5
parent e68edb4ee3
commit 9484c2a25b
6 changed files with 247 additions and 31 deletions
@@ -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<String> = 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"
}
}
}
@@ -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<Folder>, accounts: List<Account>): Map<String, String> {
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<FolderRole, String> = 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) {
@@ -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<Folder>, accounts: List<Account>): 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<Folder>,
baseLabels: Map<String, String>,
providerLabel: String,
patterns: LabelPatterns = LabelPatterns(),
): Map<String, String> {
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
}
+6
View File
@@ -68,6 +68,12 @@
<string name="folder_archive">Archive</string>
<string name="folder_spam">Spam</string>
<string name="folder_trash">Trash</string>
<!-- Folder-label disambiguation patterns, used only when two folders would otherwise show the
same name. %1$s is the base label; %2$s is the distinguishing detail. Reorder or
re-punctuate per locale (e.g. for RTL). -->
<string name="folder_label_with_provider">%1$s - %2$s</string>
<string name="folder_label_with_parent">%1$s (%2$s)</string>
<string name="folder_label_with_path">%1$s [%2$s]</string>
<!-- Unread badge shown after a folder name; caps very large counts. -->
<string name="folder_unread_overflow">99+</string>
<!-- Screen-reader description for a folder's unread badge, e.g. "12 unread messages". -->
@@ -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),
)
}
@@ -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 <Work>", labels["Work/Reports"])
assertEquals("Reports <Personal>", 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<NoSuchElementException> {
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"))
}
}