JMR-dev/LibreMail
1
0
Fork 0
Code Issues 40 Pull Requests 2 Actions Packages Projects Releases 2 Wiki Activity

fix(folders): apply label disambiguation to picker and app bar; fix self-referential and transient labels

Browse Source 
Three display bugs from the PR #54 code review, all in the shared
label-resolution/presentation path:

- #59: resolveDrawerLabels was wired only into the drawer, so the
  move-to picker and the app-bar title still rendered the bare
  folderDisplayLabel — two identical "Drafts" rows in the picker could
  move mail to different folders. Both surfaces now consume the same
  resolution via a shared resolvedFolderLabels helper; picker rows
  resolve against the unfiltered target list so a row keeps its
  disambiguation even when its colliding twin is filtered out.

- #60: two top-level folders sharing a role-derived base label (e.g.
  "Sent" and "Sent Items" both classifying SENT on servers without
  SPECIAL-USE) fell through to the full-path safety net as a
  self-referential "Sent [Sent]". Colliding top-level user folders now
  tie-break on the display name: the folder actually named like the
  base keeps it, the others show their real server name. Corrected the
  resolver KDoc's overclaimed uniqueness sketch.

- #61: the drawer derived the de-dup provider suffix from drawerAccount,
  which updates before the lagging folders StateFlow during an account
  switch, so stale Gmail folders briefly rendered as "Drafts - Outlook".
  The suffix now derives from the rendered folder list's own accountId
  (providerLabelFor), keeping a stale list under its own account's brand.

Tests: FolderLabelsTest covers the role tie-break and providerLabelFor;
MailboxViewModelTest pins the switch gap with Turbine; MailboxScreenTest
drives the disambiguated move picker (moving via "Drafts - Gmail" lands
in [Gmail]/Drafts) and the app-bar title; FolderDrawerTest renders the
transient switch frame.

Closes #59, closes #60, closes #61.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
JMR-dev
2026-07-01 23:45:15 -05:00
co-authored by Claude Fable 5
parent 8d503abd7e
commit 7bddc2eb58
7 changed files with 266 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/src/androidTest/kotlin/org/libremail/ui/mailbox/FolderDrawerTest.kt
+26
View File
@@ -79,6 +79,32 @@ class FolderDrawerTest {
composeTestRule.onNodeWithText("Drafts").assertIsDisplayed()
}
@Test
fun accountSwitchGap_staleFolderListKeepsItsOwnProviderSuffix() {
val gmail = account("imap:g", "user@gmail.com").copy(
imap = ServerConfig("imap.gmail.com", 993, MailSecurity.SSL_TLS),
)
val outlook = account("imap:o", "user@outlook.com").copy(
authType = AuthType.OAUTH_OUTLOOK,
imap = ServerConfig("outlook.office365.com", 993, MailSecurity.SSL_TLS),
)
// The transient frame from issue #61: the drawer account has already switched to Outlook,
// but the folder list still holds the Gmail account's folders until its query emits.
setContent(
accounts = listOf(gmail, outlook),
drawerAccount = outlook,
folders = listOf(
folder("imap:g", "INBOX", "INBOX", FolderRole.INBOX),
folder("imap:g", "[Gmail]/Drafts", "Drafts", FolderRole.DRAFTS, specialUse = true),
folder("imap:g", "Drafts", "Drafts", FolderRole.DRAFTS),
),
)
// The de-dup suffix derives from the folders' own account — never the incoming account.
composeTestRule.onNodeWithText("Drafts - Gmail").assertIsDisplayed()
composeTestRule.onNodeWithText("Drafts - Outlook").assertDoesNotExist()
}
@Test
fun tappingAFolder_reportsItsAccountAndFullName() {
var picked: Pair<String, String>? = null
app/src/androidTest/kotlin/org/libremail/ui/mailbox/MailboxScreenTest.kt
+57 -2
View File
@@ -54,6 +54,18 @@ class MailboxScreenTest {
smtp = ServerConfig("smtp.example.org", 465, MailSecurity.SSL_TLS),
)
private val gmailAccount = account.copy(
email = "a@gmail.com",
imap = ServerConfig("imap.gmail.com", 993, MailSecurity.SSL_TLS),
)
/** A Gmail-style tree where the built-in Drafts collides with a same-named user folder. */
private val duplicateDraftsFolders = listOf(
Folder("imap:a", "INBOX", "INBOX", FolderRole.INBOX, selectable = true),
Folder("imap:a", "[Gmail]/Drafts", "Drafts", FolderRole.DRAFTS, selectable = true, specialUse = true),
Folder("imap:a", "Drafts", "Drafts", FolderRole.DRAFTS, selectable = true),
)
private fun message(uid: String, subject: String, bodyFetched: Boolean = false, folder: String = "INBOX") = Message(
id = "imap:a:$folder:$uid",
accountId = "imap:a",
@@ -71,10 +83,10 @@ class MailboxScreenTest {
bodyFetched = bodyFetched,
)
private fun setContent(repo: FakeMailRepository): MailboxViewModel {
private fun setContent(repo: FakeMailRepository, activeAccount: Account = account): MailboxViewModel {
val viewModel = MailboxViewModel(
repo,
FakeAccountRepository(accounts = listOf(account)),
FakeAccountRepository(accounts = listOf(activeAccount)),
FakeMailSyncer(),
SavedStateHandle(),
)
@@ -222,6 +234,49 @@ class MailboxScreenTest {
assertEquals("Receipts", repo.movedToFolder.first().second)
}
@Test
fun movePicker_disambiguatesDuplicateNames_andMovesToTheChosenFolder() {
val repo = FakeMailRepository(
messages = listOf(message("1", "First")),
folders = duplicateDraftsFolders,
)
setContent(repo, activeAccount = gmailAccount)
waitForText("First")
composeTestRule.onNodeWithText("First").performTouchInput { longClick() }
composeTestRule.onNodeWithContentDescription(string(R.string.action_more)).performClick()
composeTestRule.onNodeWithText(string(R.string.action_move)).performClick()
// Issue #59: the two same-named Drafts folders are told apart in the picker, and choosing
// the suffixed row files into the provider's built-in folder — not the user folder.
composeTestRule.onNode(hasText("Drafts") and hasAnyAncestor(isDialog())).assertIsDisplayed()
composeTestRule.onNode(hasText("Drafts - Gmail") and hasAnyAncestor(isDialog())).performClick()
composeTestRule.waitUntil(5_000) { repo.movedToFolder.isNotEmpty() }
assertEquals("[Gmail]/Drafts", repo.movedToFolder.first().second)
}
@Test
fun appBarTitle_keepsTheDisambiguatedFolderLabel() {
val repo = FakeMailRepository(
messages = listOf(message("1", "First")),
folders = duplicateDraftsFolders,
)
setContent(repo, activeAccount = gmailAccount)
waitForText("First")
composeTestRule.onNodeWithContentDescription(string(R.string.drawer_open)).performClick()
composeTestRule.onNodeWithText("Drafts - Gmail").assertIsDisplayed()
composeTestRule.onNodeWithText("Drafts - Gmail").performClick()
// Issue #59: the app-bar title keeps the drawer's de-duplicated label instead of collapsing
// to an ambiguous "Drafts". Once selected, the label renders twice — the app-bar title plus
// the (composed but closed) drawer's entry.
composeTestRule.waitUntil(5_000) {
composeTestRule.onAllNodesWithText("Drafts - Gmail").fetchSemanticsNodes().size == 2
}
}
@Test
fun offlineIndicator_showsForCachedMessages() {
setContent(FakeMailRepository(messages = listOf(message("1", "Cached", bodyFetched = true))))
app/src/main/kotlin/org/libremail/ui/mailbox/FolderDrawer.kt
+14 -2
View File
@@ -79,8 +79,7 @@ fun FolderDrawer(
}
// De-duplicate labels: two distinct folders that would render the same name (e.g. a
// provider's built-in Drafts and a same-named user folder) get disambiguated.
val baseLabels = sorted.associate { it.fullName to folderDisplayLabel(it) }
val resolvedLabels = resolveDrawerLabels(sorted, baseLabels, drawerAccount?.let(::providerLabel).orEmpty())
val resolvedLabels = resolvedFolderLabels(sorted, accounts)
sorted.forEach { folder ->
val isSelected = selectedAccountId != null &&
selectedAccountId == drawerAccount?.id &&
@@ -126,6 +125,19 @@ private fun AccountSwitcher(accounts: List<Account>, current: Account, onSelect:
}
}
/**
* De-duplicated display labels for [folders], keyed by [Folder.fullName] — the one resolution used
* by the drawer, the move-to picker, and the app-bar title, so a folder reads the same everywhere.
* 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.
*/
@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))
}
/** 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) {
app/src/main/kotlin/org/libremail/ui/mailbox/FolderLabels.kt
+35 -6
View File
@@ -20,14 +20,27 @@ fun providerLabel(account: Account): String = when {
}
/**
* Resolves each folder's drawer label so no two entries collide. [baseLabels] maps a folder's
* The provider suffix used when de-duplicating [folders]' labels, derived from the account that owns
* the listed folders themselves. While switching accounts, the drawer's account updates before its
* folder list catches up, so a still-rendered stale list must keep its own account's suffix instead
* of borrowing the incoming account's brand. Empty when [folders] is empty or its owner is not in
* [accounts] (no suffix is better than a wrong one).
*/
fun providerLabelFor(folders: List<Folder>, accounts: List<Account>): String =
accounts.firstOrNull { it.id == folders.firstOrNull()?.accountId }?.let(::providerLabel).orEmpty()
/**
* 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
* [Folder.fullName] to its localized base label (a friendly role name, or the raw display name).
*
* A base label shared by 2+ folders is disambiguated: the provider's built-in special folder gets
* " - [providerLabel]" (e.g. "Archive - Gmail"); a nested user folder gets its parent location in
* parentheses (e.g. "Reports (Work)"); a top-level user folder keeps its base label. Among colliding
* user folders at most one is top-level (paths are unique), so the result is distinct. A final pass
* appends the full path to any labels that still tie, guaranteeing uniqueness.
* parentheses (e.g. "Reports (Work)"); a top-level user folder keeps the base label only when that
* is its own name, otherwise it shows its real name (see [userFolderLabel]). These rules cover the
* common collisions but not every one (e.g. "Work/2024/Reports" and "Home/2024/Reports" both yield
* "Reports (2024)"), so a final pass appends the full path to any labels that still tie, telling
* the tied entries apart.
*/
fun resolveDrawerLabels(
folders: List<Folder>,
@@ -42,18 +55,34 @@ fun resolveDrawerLabels(
val label = when {
base !in duplicated -> base
folder.specialUse -> "$base - $providerLabel"
else -> parentOf(folder.fullName, folder.displayName)?.let { "$base ($it)" } ?: base
else -> userFolderLabel(folder, base)
}
folder.fullName to label
}
// Safety net for a residual tie (e.g. two special folders mapping to one role): fall back to the
// unambiguous full path so every drawer entry stays distinct.
// 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 }
}
/**
* A colliding user folder's label. A nested folder shows its parent location. Top-level folders
* tie-break on the display name: several can share one role-derived base label (on servers without
* SPECIAL-USE, "Sent" and "Sent Items" both classify as the Sent role), so only the folder actually
* 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 {
val parent = parentOf(folder.fullName, folder.displayName)
return when {
parent != null -> "$base ($parent)"
folder.displayName.equals(base, ignoreCase = true) -> base
else -> folder.displayName
}
}
/**
* The immediate parent segment of [fullName] (its location), or null when the folder is top-level.
* [displayName] is the leaf, so the character just before it in [fullName] is the server's hierarchy
app/src/main/kotlin/org/libremail/ui/mailbox/MailboxScreen.kt
+15 -3
View File
@@ -197,7 +197,10 @@ fun MailboxScreen(
val current = folders.firstOrNull { it.fullName == selectedFolder }
Text(
if (current != null) {
folderDisplayLabel(current)
// The drawer's de-duplicated label, so "Drafts - Gmail"
// doesn't collapse to an ambiguous "Drafts" once opened.
resolvedFolderLabels(folders, accounts)[current.fullName]
?: folderDisplayLabel(current)
} else {
stringResource(R.string.title_mailbox)
},
@@ -315,6 +318,10 @@ fun MailboxScreen(
if (showMovePicker) {
MoveFolderDialog(
folders = moveTargetFolders.filter { it.selectable && it.fullName != selectedFolder },
// Labels resolved against the unfiltered list, so rows keep the drawer's
// disambiguation even when a colliding twin (e.g. the current folder) is
// filtered out of the picker itself.
labels = resolvedFolderLabels(moveTargetFolders, accounts),
onSelect = { folder ->
showMovePicker = false
viewModel.moveSelected(folder.fullName)
@@ -664,7 +671,12 @@ private fun ConfirmActionDialog(pending: PendingAction, onConfirm: () -> Unit, o
}
@Composable
private fun MoveFolderDialog(folders: List<Folder>, onSelect: (Folder) -> Unit, onDismiss: () -> Unit) {
private fun MoveFolderDialog(
folders: List<Folder>,
labels: Map<String, String>,
onSelect: (Folder) -> Unit,
onDismiss: () -> Unit,
) {
AlertDialog(
onDismissRequest = onDismiss,
title = { Text(stringResource(R.string.move_picker_title)) },
@@ -672,7 +684,7 @@ private fun MoveFolderDialog(folders: List<Folder>, onSelect: (Folder) -> Unit,
Column(Modifier.verticalScroll(rememberScrollState())) {
folders.forEach { folder ->
Text(
text = folderDisplayLabel(folder),
text = labels[folder.fullName] ?: folderDisplayLabel(folder),
style = MaterialTheme.typography.bodyLarge,
modifier = Modifier
.fillMaxWidth()
app/src/test/kotlin/org/libremail/ui/mailbox/FolderLabelsTest.kt
+80 -3
View File
@@ -17,10 +17,16 @@ class FolderLabelsTest {
displayName: String = fullName.substringAfterLast('/'),
role: FolderRole = FolderRole.NORMAL,
specialUse: Boolean = false,
) = Folder("acct", fullName, displayName, role, selectable = true, specialUse = specialUse)
accountId: String = "acct",
) = Folder(accountId, fullName, displayName, role, selectable = true, specialUse = specialUse)
private fun account(email: String, imapHost: String, authType: AuthType = AuthType.PASSWORD_IMAP) = Account(
id = "acct",
private fun account(
email: String,
imapHost: String,
authType: AuthType = AuthType.PASSWORD_IMAP,
id: String = "acct",
) = Account(
id = id,
email = email,
displayName = email,
authType = authType,
@@ -117,6 +123,77 @@ class FolderLabelsTest {
assertEquals(2, labels.values.toSet().size, "every drawer entry must be unique")
}
// Issue #60: on a server without SPECIAL-USE (e.g. GreenMail) top-level "Sent" and "Sent Items"
// both classify as SENT via the name fallback and share the friendly "Sent" base label. The tie
// must break on the display name, not net out as a self-referential "Sent [Sent]".
@Test
fun `top-level folders sharing a role tie-break on their names, not a self-referential path`() {
val folders = listOf(
folder("Sent", "Sent", FolderRole.SENT),
folder("Sent Items", "Sent Items", FolderRole.SENT),
)
val labels = resolve(folders, "example.org")
assertEquals("Sent", labels["Sent"])
assertEquals("Sent Items", labels["Sent Items"])
}
@Test
fun `same-role top-level folders where none matches the friendly name keep their own names`() {
val folders = listOf(
folder("Sent Mail", "Sent Mail", FolderRole.SENT),
folder("Sent Items", "Sent Items", FolderRole.SENT),
)
val labels = resolve(folders, "example.org")
assertEquals("Sent Mail", labels["Sent Mail"])
assertEquals("Sent Items", labels["Sent Items"])
}
@Test
fun `provider special, canonical, and synonym folders for one role all stay apart`() {
val folders = listOf(
folder("[Gmail]/Sent Mail", "Sent Mail", FolderRole.SENT, specialUse = true),
folder("Sent", "Sent", FolderRole.SENT),
folder("Sent Items", "Sent Items", FolderRole.SENT),
)
val labels = resolve(folders, "Gmail")
assertEquals("Sent - Gmail", labels["[Gmail]/Sent Mail"])
assertEquals("Sent", labels["Sent"])
assertEquals("Sent Items", labels["Sent Items"])
}
// Issue #61: while switching accounts the drawer's account updates before its folder list, so
// the provider suffix must derive from the rendered folders' own account — never from the newer
// drawer selection.
@Test
fun `providerLabelFor derives the suffix from the folder list's owner account`() {
val gmail = account("user@gmail.com", "imap.gmail.com", id = "acct-gmail")
val outlook =
account("user@outlook.com", "outlook.office365.com", AuthType.OAUTH_OUTLOOK, id = "acct-outlook")
// The drawer account has already switched to Outlook, but these stale folders are Gmail's.
val staleGmailFolders = listOf(
folder("[Gmail]/Drafts", "Drafts", FolderRole.DRAFTS, specialUse = true, accountId = "acct-gmail"),
folder("Drafts", "Drafts", FolderRole.DRAFTS, accountId = "acct-gmail"),
)
val provider = providerLabelFor(staleGmailFolders, listOf(gmail, outlook))
assertEquals("Gmail", provider)
val labels =
resolveDrawerLabels(staleGmailFolders, baseLabelsOf(staleGmailFolders, friendlyNames), provider)
assertEquals("Drafts - Gmail", labels["[Gmail]/Drafts"])
assertEquals("Drafts", labels["Drafts"])
}
@Test
fun `providerLabelFor is empty for an empty or orphaned folder list`() {
val outlook =
account("user@outlook.com", "outlook.office365.com", AuthType.OAUTH_OUTLOOK, id = "acct-outlook")
assertEquals("", providerLabelFor(emptyList(), listOf(outlook)))
val orphaned = listOf(folder("INBOX", "INBOX", FolderRole.INBOX, accountId = "acct-gone"))
assertEquals("", providerLabelFor(orphaned, listOf(outlook)))
}
@Test
fun `providerLabel resolves brands, Outlook, and falls back to the email domain`() {
assertEquals("Gmail", providerLabel(account("a@gmail.com", "imap.gmail.com")))
app/src/test/kotlin/org/libremail/ui/mailbox/MailboxViewModelTest.kt
+39
View File
@@ -2,12 +2,14 @@
package org.libremail.ui.mailbox
import androidx.lifecycle.SavedStateHandle
import app.cash.turbine.test
import io.mockk.coEvery
import io.mockk.coVerify
import io.mockk.every
import io.mockk.mockk
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.flow.MutableSharedFlow
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.flow.flowOf
@@ -157,6 +159,43 @@ class MailboxViewModelTest {
assertTrue(vm.folders.value.none { it.fullName == "Archive" }, "alice's folders should no longer show")
}
// Issue #61: switching the drawer account emits the new account immediately, while the folder
// list keeps the old account's folders until the new account's query emits. The UI must derive
// provider labels from the rendered folders' own accountId (providerLabelFor) so those gap
// frames never show the old folders with the new account's brand.
@Test
fun `folder list lags a drawer-account switch until the new folders arrive`() = runTest(testDispatcher) {
val repo = mockk<MailRepository>(relaxed = true)
every { repo.observeMessages() } returns MutableStateFlow(emptyList<Message>())
every { repo.observeDrafts() } returns flowOf(emptyList())
every { repo.observeOutbox() } returns flowOf(emptyList())
every { repo.observeFolders("imap:a") } returns
MutableStateFlow(listOf(folder("imap:a", "INBOX", FolderRole.INBOX)))
// Bob's folder query stays in flight (no emission yet) to reproduce the switch gap.
val bobFolders = MutableSharedFlow<List<Folder>>()
every { repo.observeFolders("imap:b") } returns bobFolders
val accountRepository = mockk<AccountRepository>(relaxed = true)
every { accountRepository.observeAccounts() } returns MutableStateFlow(listOf(alice, bob))
val vm = MailboxViewModel(repo, accountRepository, mockk(relaxed = true), SavedStateHandle())
vm.folders.test {
var current = awaitItem() // the StateFlow's initial empty value, or alice's list
while (current.isEmpty()) current = awaitItem()
assertEquals(listOf("imap:a"), current.map { it.accountId }.distinct())
vm.setDrawerAccount("imap:b")
// The drawer account has already switched, but the rendered folder list has not —
// exactly the transient frames issue #61 is about.
assertEquals("imap:b", vm.drawerAccount.value?.id)
assertEquals(listOf("imap:a"), vm.folders.value.map { it.accountId }.distinct())
expectNoEvents()
bobFolders.emit(listOf(folder("imap:b", "Work", FolderRole.NORMAL)))
assertEquals(listOf("imap:b"), awaitItem().map { it.accountId }.distinct())
}
}
@Test
fun `toggle adds then removes a message from the selection`() = runTest(testDispatcher) {
val vm = createViewModel(accounts = listOf(alice), messages = emptyList())