From 93fe3ac727b2cf41ead25e85a954c5d784fb6170 Mon Sep 17 00:00:00 2001 From: davotoula Date: Sun, 2 Aug 2026 09:16:35 +0200 Subject: [PATCH] Code review: - fold the pickers onto shared row/expand-state UI --- .../ui/navigation/bottombars/NavBarItem.kt | 7 +++ .../ui/navigation/drawer/DrawerContent.kt | 3 +- .../ui/navigation/drawer/DrawerSections.kt | 12 +++-- .../settings/BottomBarSettingsScreen.kt | 50 ++++++------------- .../loggedIn/settings/DrawerSettingsScreen.kt | 35 ++++++------- .../screen/loggedIn/settings/NavPickerUi.kt | 24 ++++++++- .../navigation/DrawerItemVisibilityTest.kt | 6 +-- .../amethyst/navigation/DrawerSectionsTest.kt | 16 ++++++ 8 files changed, 86 insertions(+), 67 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/bottombars/NavBarItem.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/bottombars/NavBarItem.kt index 2209c1bec6..5959cb5c25 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/bottombars/NavBarItem.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/bottombars/NavBarItem.kt @@ -463,6 +463,7 @@ val DefaultBottomBarEntries: List = DefaultBottomBarItems.map { */ data class NavBarCategory( val titleRes: Int, + val icon: MaterialSymbol, val items: List, ) @@ -475,6 +476,7 @@ val BottomBarCategories: List = listOf( NavBarCategory( R.string.bottom_bar_category_main, + MaterialSymbols.Home, listOf( NavBarItem.HOME, NavBarItem.MESSAGES, @@ -485,6 +487,7 @@ val BottomBarCategories: List = ), NavBarCategory( R.string.bottom_bar_category_chats, + MaterialSymbols.Group, listOf( NavBarItem.PUBLIC_CHATS, NavBarItem.RELAY_GROUPS, @@ -494,6 +497,7 @@ val BottomBarCategories: List = ), NavBarCategory( R.string.bottom_bar_category_you, + MaterialSymbols.AccountCircle, listOf( NavBarItem.PROFILE, NavBarItem.MY_LISTS, @@ -511,6 +515,7 @@ val BottomBarCategories: List = ), NavBarCategory( R.string.bottom_bar_category_feeds, + MaterialSymbols.Subscriptions, listOf( NavBarItem.ARTICLES, NavBarItem.LONGS, @@ -537,6 +542,7 @@ val BottomBarCategories: List = ), NavBarCategory( R.string.bottom_bar_category_apps, + MaterialSymbols.Apps, listOf( NavBarItem.BROWSER, NavBarItem.FAVORITE_APPS, @@ -547,6 +553,7 @@ val BottomBarCategories: List = ), NavBarCategory( R.string.bottom_bar_category_other, + MaterialSymbols.Settings, listOf( NavBarItem.SETTINGS, ), diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/drawer/DrawerContent.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/drawer/DrawerContent.kt index a179e627c8..6009583b38 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/drawer/DrawerContent.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/drawer/DrawerContent.kt @@ -648,8 +648,7 @@ fun CatalogSection( val onBackground = MaterialTheme.colorScheme.onBackground val visible = remember(section, hidden) { DrawerItemVisibility.visibleItems(section, hidden) } - val hasFixedRows = section.id == DrawerSectionId.CREATE || section.id == DrawerSectionId.SYSTEM - if (visible.isEmpty() && !hasFixedRows) return + if (visible.isEmpty() && !section.hasFixedRows) return CollapsibleSection(title = section.titleRes) { when (section.id) { diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/drawer/DrawerSections.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/drawer/DrawerSections.kt index 4e591f71f9..2cf323c39b 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/drawer/DrawerSections.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/drawer/DrawerSections.kt @@ -47,6 +47,12 @@ data class DrawerSection( val titleRes: Int, val icon: MaterialSymbol, val items: List, + /** + * True for a section that renders rows of its own on top of its catalog items (see [CatalogSection]). + * Such a section stays in the drawer even with every catalog row switched off, and — since a fixed + * row is not a catalog destination — it never appears in the Side Menu settings screen's counts. + */ + val hasFixedRows: Boolean = false, ) /** @@ -133,12 +139,10 @@ val DrawerSections: List = DrawerSection(DrawerSectionId.YOU, R.string.drawer_section_you, MaterialSymbols.AccountCircle, DrawerYouItems), DrawerSection(DrawerSectionId.NAVIGATE, R.string.drawer_section_navigate, MaterialSymbols.Home, DrawerNavigateItems), DrawerSection(DrawerSectionId.FEEDS, R.string.drawer_section_feeds, MaterialSymbols.Subscriptions, DrawerFeedsItems), - DrawerSection(DrawerSectionId.CREATE, R.string.drawer_section_create, MaterialSymbols.Edit, emptyList()), - DrawerSection(DrawerSectionId.SYSTEM, R.string.drawer_section_system, MaterialSymbols.Settings, listOf(NavBarItem.SETTINGS)), + DrawerSection(DrawerSectionId.CREATE, R.string.drawer_section_create, MaterialSymbols.Edit, emptyList(), hasFixedRows = true), + DrawerSection(DrawerSectionId.SYSTEM, R.string.drawer_section_system, MaterialSymbols.Settings, listOf(NavBarItem.SETTINGS), hasFixedRows = true), ) -fun drawerSection(id: DrawerSectionId): DrawerSection = DrawerSections.first { it.id == id } - /** * Catalog ids deliberately absent from every [DrawerSections] list, with the reason. Only Favorite * Apps qualifies: [DrawerFeedsItems] gates it on API 30+ (its inline tabs need SurfaceControlViewHost), diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/BottomBarSettingsScreen.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/BottomBarSettingsScreen.kt index f23ace6d4b..b3d24014fa 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/BottomBarSettingsScreen.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/BottomBarSettingsScreen.kt @@ -54,7 +54,6 @@ import androidx.compose.runtime.mutableIntStateOf import androidx.compose.runtime.mutableStateMapOf import androidx.compose.runtime.remember import androidx.compose.runtime.setValue -import androidx.compose.runtime.snapshots.SnapshotStateMap import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.draw.clip @@ -91,6 +90,7 @@ import com.vitorpamplona.amethyst.ui.navigation.topbars.TopBarWithBackButton import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel import com.vitorpamplona.amethyst.ui.screen.loggedIn.mockAccountViewModel import com.vitorpamplona.amethyst.ui.stringRes +import com.vitorpamplona.amethyst.ui.theme.Size22Modifier import com.vitorpamplona.amethyst.ui.theme.ThemeComparisonRow import com.vitorpamplona.quartz.concord.cord02Community.ConcordCommunityListEntry import com.vitorpamplona.quartz.nip51Lists.simpleGroupList.GroupTag @@ -158,8 +158,8 @@ fun BottomBarSettingsContent(accountViewModel: AccountViewModel) { val pinned = state.pinned val pinnedKeys = remember(pinned) { state.pinnedKeys() } - val expandedCategories = remember { mutableStateMapOf() } - val expandedItems = remember { mutableStateMapOf() } + val expandedCategories = rememberExpandedKeys() + val expandedItems = rememberExpandedKeys() Column( modifier = @@ -183,8 +183,8 @@ fun BottomBarSettingsContent(accountViewModel: AccountViewModel) { CategoryCard( category = category, pinnedKeys = pinnedKeys, - expanded = expandedCategories[category.titleRes] ?: false, - onToggleExpand = { expandedCategories[category.titleRes] = !(expandedCategories[category.titleRes] ?: false) }, + expanded = expandedCategories.isExpanded(category.titleRes), + onToggleExpand = { expandedCategories.toggle(category.titleRes) }, expandedItems = expandedItems, accountViewModel = accountViewModel, onTogglePin = state::togglePin, @@ -437,12 +437,12 @@ private fun CategoryCard( pinnedKeys: Set, expanded: Boolean, onToggleExpand: () -> Unit, - expandedItems: SnapshotStateMap, + expandedItems: ExpandedKeys, accountViewModel: AccountViewModel, onTogglePin: (BottomBarEntry) -> Unit, ) { CatalogCard( - icon = categoryIcon(category.titleRes), + icon = category.icon, title = stringRes(category.titleRes), expanded = expanded, onToggleExpand = onToggleExpand, @@ -455,9 +455,9 @@ private fun CategoryCard( icon = def.icon, label = stringRes(def.labelRes), pinned = entry.stableKey in pinnedKeys, - expanded = expandedItems[item] ?: false, + expanded = expandedItems.isExpanded(item), onTogglePin = { onTogglePin(entry) }, - onToggleExpand = { expandedItems[item] = !(expandedItems[item] ?: false) }, + onToggleExpand = { expandedItems.toggle(item) }, ) { PickerChildren(item, pinnedKeys, accountViewModel, onTogglePin) } @@ -717,27 +717,15 @@ private fun ExpandableAvailableRow( onToggleExpand: () -> Unit, children: @Composable () -> Unit, ) { - Row( - modifier = - Modifier - .fillMaxWidth() - .clickable(onClick = onToggleExpand) - .padding(start = 13.dp, end = 13.dp, top = 7.dp, bottom = 7.dp), - verticalAlignment = Alignment.CenterVertically, - horizontalArrangement = Arrangement.spacedBy(12.dp), + CatalogRow( + leading = { LeadingGlyph(icon) }, + label = label, + onToggle = onToggleExpand, ) { - LeadingGlyph(icon) - Text( - text = label, - style = MaterialTheme.typography.bodyLarge, - maxLines = 1, - overflow = TextOverflow.Ellipsis, - modifier = Modifier.weight(1f), - ) Icon( symbol = if (expanded) MaterialSymbols.ExpandLess else MaterialSymbols.ExpandMore, contentDescription = stringRes(R.string.bottom_bar_settings_expand), - modifier = Modifier.size(22.dp), + modifier = Size22Modifier, tint = MaterialTheme.colorScheme.onSurfaceVariant, ) AddPill(added = pinned, onClick = onTogglePin) @@ -777,16 +765,6 @@ private fun FavoriteLeading(app: FavoriteApp) { } } -private fun categoryIcon(titleRes: Int): MaterialSymbol = - when (titleRes) { - R.string.bottom_bar_category_main -> MaterialSymbols.Home - R.string.bottom_bar_category_chats -> MaterialSymbols.Group - R.string.bottom_bar_category_you -> MaterialSymbols.AccountCircle - R.string.bottom_bar_category_feeds -> MaterialSymbols.Subscriptions - R.string.bottom_bar_category_apps -> MaterialSymbols.Apps - else -> MaterialSymbols.Settings - } - // ------------------------------------------------------------------------------------------------ // Leading/label resolution for a pinned entry (built-in glyph, favorite icon, or group avatar). // Computed once so a group's channel is subscribed at most once per row. diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/DrawerSettingsScreen.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/DrawerSettingsScreen.kt index 9b1d7cdb48..9a32cc8a11 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/DrawerSettingsScreen.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/DrawerSettingsScreen.kt @@ -38,7 +38,6 @@ import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.derivedStateOf import androidx.compose.runtime.getValue -import androidx.compose.runtime.mutableStateMapOf import androidx.compose.runtime.remember import androidx.compose.ui.Modifier import androidx.compose.ui.text.font.FontWeight @@ -113,11 +112,9 @@ fun DrawerSettingsContent(accountViewModel: AccountViewModel) { // Sections start collapsed: expanded, they are ~50 rows of scrolling. The header's hidden // counter is what tells the user which one to open. - val expandedSections = remember { mutableStateMapOf() } + val expandedSections = rememberExpandedKeys() - // derivedStateOf so a toggle that leaves this total unchanged doesn't re-run the whole screen - // body — every SectionCard below reads the same coarse `hidden` state. - val totalHidden by remember { derivedStateOf { state.totalHidden() } } + val totalHidden = state.totalHidden() Column( modifier = @@ -142,8 +139,8 @@ fun DrawerSettingsContent(accountViewModel: AccountViewModel) { SectionCard( section = section, state = state, - expanded = expandedSections[section.id] ?: false, - onToggleExpand = { expandedSections[section.id] = !(expandedSections[section.id] ?: false) }, + expanded = expandedSections.isExpanded(section.id), + onToggleExpand = { expandedSections.toggle(section.id) }, ) } @@ -241,22 +238,18 @@ private fun VisibilityPill( mandatory: Boolean, onClick: () -> Unit, ) { + // One branch decides both halves of the pill, so a label can't drift away from its glyph. + val (labelRes, icon) = + when { + mandatory -> R.string.drawer_settings_always_on to MaterialSymbols.Lock + visible -> R.string.drawer_settings_visible to MaterialSymbols.Visibility + else -> R.string.drawer_settings_hidden to MaterialSymbols.VisibilityOff + } + TogglePill( on = visible, - label = - stringRes( - when { - mandatory -> R.string.drawer_settings_always_on - visible -> R.string.drawer_settings_visible - else -> R.string.drawer_settings_hidden - }, - ), - icon = - when { - mandatory -> MaterialSymbols.Lock - visible -> MaterialSymbols.Visibility - else -> MaterialSymbols.VisibilityOff - }, + label = stringRes(labelRes), + icon = icon, enabled = !mandatory, onClick = onClick, ) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/NavPickerUi.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/NavPickerUi.kt index 0793edcf81..78be2e8f20 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/NavPickerUi.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/NavPickerUi.kt @@ -42,6 +42,9 @@ import androidx.compose.material3.Surface import androidx.compose.material3.Text import androidx.compose.material3.TextButton import androidx.compose.runtime.Composable +import androidx.compose.runtime.Stable +import androidx.compose.runtime.mutableStateMapOf +import androidx.compose.runtime.remember import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.draw.clip @@ -90,13 +93,32 @@ val SectionCollapse = shrinkVertically(shrinkTowards = Alignment.Top) + fadeOut( * category (a favorite, or a relay/community "server" row), 2 = a room nested under its server (a * NIP-29 group under its relay, or a Concord channel under its community). */ -fun indentPadding(level: Int) = +private fun indentPadding(level: Int) = when (level) { 0 -> Size13dp 1 -> Size24dp else -> Size40dp } +/** + * Which collapsible rows of a picker are currently open, keyed by whatever identifies a row (a + * section id, a string-resource id, a catalog item). Absent means collapsed, so the initial state + * costs nothing and no list has to be seeded. + */ +@Stable +class ExpandedKeys { + private val open = mutableStateMapOf() + + fun isExpanded(key: K): Boolean = open[key] == true + + fun toggle(key: K) { + open[key] = !isExpanded(key) + } +} + +@Composable +fun rememberExpandedKeys(): ExpandedKeys = remember { ExpandedKeys() } + @Composable fun PickerSectionHeader(title: String) { Text( diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/navigation/DrawerItemVisibilityTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/navigation/DrawerItemVisibilityTest.kt index d5fcf5f47a..3d193b7f08 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/navigation/DrawerItemVisibilityTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/navigation/DrawerItemVisibilityTest.kt @@ -23,7 +23,7 @@ package com.vitorpamplona.amethyst.navigation import com.vitorpamplona.amethyst.ui.navigation.bottombars.NavBarItem import com.vitorpamplona.amethyst.ui.navigation.drawer.DrawerItemVisibility import com.vitorpamplona.amethyst.ui.navigation.drawer.DrawerSectionId -import com.vitorpamplona.amethyst.ui.navigation.drawer.drawerSection +import com.vitorpamplona.amethyst.ui.navigation.drawer.DrawerSections import com.vitorpamplona.amethyst.ui.screen.loggedIn.settings.DrawerSettingsState import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse @@ -38,8 +38,8 @@ import org.junit.Test * rows, which no code path may switch off. */ class DrawerItemVisibilityTest { - private val you = drawerSection(DrawerSectionId.YOU) - private val system = drawerSection(DrawerSectionId.SYSTEM) + private val you = DrawerSections.first { it.id == DrawerSectionId.YOU } + private val system = DrawerSections.first { it.id == DrawerSectionId.SYSTEM } @Test fun nothingHiddenMeansEverythingVisible() { diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/navigation/DrawerSectionsTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/navigation/DrawerSectionsTest.kt index 3cf1687a83..f77c3d7523 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/navigation/DrawerSectionsTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/navigation/DrawerSectionsTest.kt @@ -90,6 +90,22 @@ class DrawerSectionsTest { assertEquals("two sections share a DrawerSectionId", ids.size, ids.toSet().size) } + @Test + fun aSectionWithNoCatalogItemsRendersFixedRowsOrNothingAtAll() { + // hasFixedRows is declared on the section but consumed by CatalogSection's `when (section.id)`, + // in another file — so the flag and the branch that honours it can drift apart with no compile + // error. A section that carries neither is unreachable in both directions at once: the settings + // screen skips it on items.isEmpty(), and CatalogSection returns before rendering a heading. + val unreachable = DrawerSections.filter { it.items.isEmpty() && !it.hasFixedRows } + + assertEquals( + "a drawer section has no catalog items and no fixed rows, so it renders nowhere — " + + "give it items, set hasFixedRows and a branch in CatalogSection, or delete it", + emptyList(), + unreachable.map { it.id }, + ) + } + @Test fun mandatoryItemsAreActuallyRenderedByASection() { // A mandatory item that no section renders would be unhideable *and* invisible — the worst