From df313970b1f7e296c809487efe1861dff43c480e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 15 Jul 2026 14:53:20 +0000 Subject: [PATCH] refactor: extract bottom-bar settings state, share the entry resolver, read-only picker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Quality follow-up to the customizable bottom nav, addressing the audit's three highest-impact items — behavior unchanged. - Extract BottomBarSettingsState + the pure BottomBarEditing transforms out of BottomBarSettingsContent, so pin/unpin/reorder/restore-default are unit-tested (BottomBarSettingsStateTest) instead of only exercisable through the drag UI. The composable now just renders and forwards events; moveTransient reorders mid-drag and commit() persists once on drag end. - Unify the phone bottom bar and the navigation rail on one rememberBottomBarSlot resolver (route + icon per entry), removing the duplicated built-in/favorite/ group branches and the divergent selection logic that had drifted between them. - Give the group resolvers a `subscribe` flag: the live bar/rail keeps a REQ open per pinned group (bounded by the few slots), but the settings picker reads cached metadata only — so expanding a chat category with many joined groups no longer fans out into one relay subscription per row. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_017mxdSFQcarsKaL179tSub1 --- .../ui/navigation/bottombars/AppBottomBar.kt | 126 ++++++---------- .../bottombars/AppNavigationRail.kt | 115 +++++---------- .../bottombars/GroupBottomBarEntries.kt | 71 +++++++-- .../settings/BottomBarSettingsScreen.kt | 59 +++----- .../settings/BottomBarSettingsState.kt | 108 ++++++++++++++ .../navigation/BottomBarSettingsStateTest.kt | 139 ++++++++++++++++++ 6 files changed, 409 insertions(+), 209 deletions(-) create mode 100644 amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/BottomBarSettingsState.kt create mode 100644 amethyst/src/test/java/com/vitorpamplona/amethyst/navigation/BottomBarSettingsStateTest.kt diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/bottombars/AppBottomBar.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/bottombars/AppBottomBar.kt index d97ed95b59..dedc9b5521 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/bottombars/AppBottomBar.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/bottombars/AppBottomBar.kt @@ -24,7 +24,6 @@ import androidx.compose.foundation.Canvas import androidx.compose.foundation.background import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.Column -import androidx.compose.foundation.layout.RowScope import androidx.compose.foundation.layout.Spacer import androidx.compose.foundation.layout.consumeWindowInsets import androidx.compose.foundation.layout.fillMaxWidth @@ -185,91 +184,64 @@ private fun RenderBottomMenu( containerColor = MaterialTheme.colorScheme.background, tonalElevation = Size0dp, ) { - // Render in the user's saved order, built-ins and favorites interleaved. + // Render in the user's saved order — built-ins, favorites and pinned groups interleaved. + // Each entry resolves to a shared BottomBarSlot (route + icon), the same one the rail uses. items.forEach { entry -> - when (entry) { - is BottomBarEntry.BuiltIn -> { - val def = NavBarCatalog[entry.item] ?: return@forEach - val destination = remember(def, accountViewModel) { def.resolveRoute(accountViewModel) } - HasNewItemsIcon(destination == selectedRoute, def, destination, accountViewModel, nav) - } - is BottomBarEntry.Favorite -> { - val fav = favoritesById[entry.favoriteId] ?: return@forEach - val destination = - when (fav) { - is FavoriteApp.WebApp -> Route.WebApp(fav.url) - is FavoriteApp.NostrApp -> Route.NostrApp(fav.coordinate) - } - FavoriteNavItem(destination == selectedRoute, fav, rememberFavoriteIconModel(fav), destination, nav) - } - is BottomBarEntry.PublicChat, - is BottomBarEntry.RelayGroup, - is BottomBarEntry.Concord, - -> { - val display = rememberGroupEntryDisplay(entry, accountViewModel) ?: return@forEach - GroupNavItem(display.route == selectedRoute, display, accountViewModel, display.route, nav) - } - } + val slot = rememberBottomBarSlot(entry, favoritesById, accountViewModel) ?: return@forEach + val selected = slot.route == selectedRoute + NavigationBarItem( + alwaysShowLabel = false, + icon = { slot.icon(selected) }, + selected = selected, + onClick = { nav(slot.route) }, + ) } } } } -@Composable -private fun RowScope.FavoriteNavItem( - selected: Boolean, - fav: FavoriteApp, - iconModel: Any?, - destination: Route, - nav: (Route) -> Unit, -) { - NavigationBarItem( - alwaysShowLabel = false, - icon = { FavoriteEntryIcon(fav, selected, iconModel) }, - // No label — favorite tabs match the built-in items, which show icon only. - selected = selected, - onClick = { nav(destination) }, - ) -} +/** + * A resolved bottom-bar/rail slot: the [route] to navigate to and the [icon] to draw for a given + * selected state. Built from a [BottomBarEntry] by [rememberBottomBarSlot], so the phone bar and the + * large-screen rail render every entry kind (built-in, favorite, pinned group) through one code path. + */ +class BottomBarSlot( + val route: Route, + val icon: @Composable (selected: Boolean) -> Unit, +) +/** Resolves an entry to its live [BottomBarSlot], or null if it no longer resolves (deleted favorite, etc.). */ @Composable -private fun RowScope.GroupNavItem( - selected: Boolean, - display: GroupEntryDisplay, +internal fun rememberBottomBarSlot( + entry: BottomBarEntry, + favoritesById: Map, accountViewModel: AccountViewModel, - destination: Route, - nav: (Route) -> Unit, -) { - NavigationBarItem( - alwaysShowLabel = false, - // A pinned chat/group shows its avatar, like the favorite-app tabs — icon only. - icon = { Box(Size27Modifier, contentAlignment = Alignment.Center) { GroupEntryAvatar(display, 25.dp, accountViewModel) } }, - selected = selected, - onClick = { nav(destination) }, - ) -} - -@Composable -private fun RowScope.HasNewItemsIcon( - selected: Boolean, - def: NavBarItemDef, - destination: Route, - accountViewModel: AccountViewModel, - nav: (Route) -> Unit, -) { - NavigationBarItem( - alwaysShowLabel = false, - icon = { - NotifiableIcon( - selected, - def, - destination, - accountViewModel, - ) - }, - selected = selected, - onClick = { nav(destination) }, - ) +): BottomBarSlot? { + return when (entry) { + is BottomBarEntry.BuiltIn -> { + val def = NavBarCatalog[entry.item] ?: return null + val route = remember(def, accountViewModel) { def.resolveRoute(accountViewModel) } + BottomBarSlot(route) { selected -> NotifiableIcon(selected, def, route, accountViewModel) } + } + is BottomBarEntry.Favorite -> { + val fav = favoritesById[entry.favoriteId] ?: return null + val route = + when (fav) { + is FavoriteApp.WebApp -> Route.WebApp(fav.url) + is FavoriteApp.NostrApp -> Route.NostrApp(fav.coordinate) + } + val iconModel = rememberFavoriteIconModel(fav) + BottomBarSlot(route) { selected -> FavoriteEntryIcon(fav, selected, iconModel) } + } + is BottomBarEntry.PublicChat, + is BottomBarEntry.RelayGroup, + is BottomBarEntry.Concord, + -> { + val display = rememberGroupEntryDisplay(entry, accountViewModel) ?: return null + // A pinned chat/group shows its avatar, like the favorite-app tabs — icon only. + BottomBarSlot(display.route) { Box(Size27Modifier, contentAlignment = Alignment.Center) { GroupEntryAvatar(display, 25.dp, accountViewModel) } } + } + } } /** The icon block for a built-in entry (catalog icon + new-items dot), shared by the bottom bar and the rail. */ diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/bottombars/AppNavigationRail.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/bottombars/AppNavigationRail.kt index db3c4a9692..90ad79490a 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/bottombars/AppNavigationRail.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/bottombars/AppNavigationRail.kt @@ -31,11 +31,11 @@ import androidx.compose.runtime.getValue import androidx.compose.runtime.remember import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier -import androidx.compose.ui.unit.dp import androidx.lifecycle.compose.collectAsStateWithLifecycle +import androidx.navigation.NavDestination import androidx.navigation.NavDestination.Companion.hasRoute +import androidx.navigation.NavHostController import androidx.navigation.compose.currentBackStackEntryAsState -import com.vitorpamplona.amethyst.commons.favorites.FavoriteApp import com.vitorpamplona.amethyst.favorites.FavoriteAppsRegistry import com.vitorpamplona.amethyst.ui.navigation.navs.Nav import com.vitorpamplona.amethyst.ui.navigation.routes.Route @@ -76,85 +76,42 @@ fun AppNavigationRail( modifier = Modifier.weight(1f).verticalScroll(rememberScrollState()), horizontalAlignment = Alignment.CenterHorizontally, ) { + // Same entry list + resolution as the phone bottom bar; only the item chrome and the + // selection source (live back stack vs the bar's passed-in route) differ per surface. items.forEach { entry -> - when (entry) { - is BottomBarEntry.BuiltIn -> { - val def = NavBarCatalog[entry.item] ?: return@forEach - val destination = remember(def, accountViewModel) { def.resolveRoute(accountViewModel) } - val selected = currentDestination?.hasRoute(destination::class) == true - NavigationRailItem( - selected = selected, - onClick = { - if (selected) { - reselectCoordinator.reselect(destination) - } else { - nav.navBottomBar(destination) - } - }, - icon = { NotifiableIcon(selected, def, destination, accountViewModel) }, - ) - } - - is BottomBarEntry.Favorite -> { - val fav = favoritesById[entry.favoriteId] ?: return@forEach - val destination = - when (fav) { - is FavoriteApp.WebApp -> Route.WebApp(fav.url) - is FavoriteApp.NostrApp -> Route.NostrApp(fav.coordinate) - } - // Favorites carry arguments (url / coordinate), so class matching alone - // would light up every pinned app of the same kind; compare the full route. - val selected = - remember(navBackStackEntry, destination) { - when (destination) { - is Route.WebApp -> getRouteWithArguments(Route.WebApp::class, nav.controller) == destination - is Route.NostrApp -> getRouteWithArguments(Route.NostrApp::class, nav.controller) == destination - else -> false - } - } - NavigationRailItem( - selected = selected, - onClick = { - if (selected) { - reselectCoordinator.reselect(destination) - } else { - nav.navBottomBar(destination) - } - }, - icon = { FavoriteEntryIcon(fav, selected, rememberFavoriteIconModel(fav)) }, - ) - } - - is BottomBarEntry.PublicChat, - is BottomBarEntry.RelayGroup, - is BottomBarEntry.Concord, - -> { - val display = rememberGroupEntryDisplay(entry, accountViewModel) ?: return@forEach - val destination = display.route - // Group routes carry ids, so match the full route (not just its class). - val selected = - remember(navBackStackEntry, destination) { - when (destination) { - is Route.PublicChatChannel -> getRouteWithArguments(Route.PublicChatChannel::class, nav.controller) == destination - is Route.RelayGroup -> getRouteWithArguments(Route.RelayGroup::class, nav.controller) == destination - is Route.ConcordServer -> getRouteWithArguments(Route.ConcordServer::class, nav.controller) == destination - else -> false - } - } - NavigationRailItem( - selected = selected, - onClick = { - if (selected) { - reselectCoordinator.reselect(destination) - } else { - nav.navBottomBar(destination) - } - }, - icon = { GroupEntryAvatar(display, 25.dp, accountViewModel) }, - ) - } - } + val slot = rememberBottomBarSlot(entry, favoritesById, accountViewModel) ?: return@forEach + val selected = remember(navBackStackEntry, slot.route) { railSelected(slot.route, nav.controller, currentDestination) } + NavigationRailItem( + selected = selected, + onClick = { + if (selected) { + reselectCoordinator.reselect(slot.route) + } else { + nav.navBottomBar(slot.route) + } + }, + icon = { slot.icon(selected) }, + ) } } } } + +/** + * Whether [route] is the rail's currently-selected destination. Parameterized routes (favorites and + * pinned groups carry a url / coordinate / id) must match the full route — class matching alone would + * light up every pinned app or group of the same kind; parameterless object routes match on class. + */ +private fun railSelected( + route: Route, + controller: NavHostController, + currentDestination: NavDestination?, +): Boolean = + when (route) { + is Route.WebApp -> getRouteWithArguments(Route.WebApp::class, controller) == route + is Route.NostrApp -> getRouteWithArguments(Route.NostrApp::class, controller) == route + is Route.PublicChatChannel -> getRouteWithArguments(Route.PublicChatChannel::class, controller) == route + is Route.RelayGroup -> getRouteWithArguments(Route.RelayGroup::class, controller) == route + is Route.ConcordServer -> getRouteWithArguments(Route.ConcordServer::class, controller) == route + else -> currentDestination?.hasRoute(route::class) == true + } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/bottombars/GroupBottomBarEntries.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/bottombars/GroupBottomBarEntries.kt index 9b2910e684..7b0a2a60ef 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/bottombars/GroupBottomBarEntries.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/bottombars/GroupBottomBarEntries.kt @@ -24,17 +24,21 @@ import androidx.compose.foundation.layout.size import androidx.compose.foundation.shape.CircleShape import androidx.compose.runtime.Composable import androidx.compose.runtime.Immutable +import androidx.compose.runtime.State import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.ui.Modifier import androidx.compose.ui.draw.clip import androidx.compose.ui.unit.Dp import androidx.lifecycle.compose.collectAsStateWithLifecycle import com.vitorpamplona.amethyst.R +import com.vitorpamplona.amethyst.commons.model.Channel +import com.vitorpamplona.amethyst.commons.model.ChannelState import com.vitorpamplona.amethyst.commons.model.nip28PublicChats.PublicChatChannel import com.vitorpamplona.amethyst.commons.model.nip29RelayGroups.RelayGroupChannel import com.vitorpamplona.amethyst.model.LocalCache -import com.vitorpamplona.amethyst.service.relayClient.reqCommand.channel.observeChannel +import com.vitorpamplona.amethyst.service.relayClient.reqCommand.channel.ChannelFinderFilterAssemblerSubscription import com.vitorpamplona.amethyst.ui.components.RobohashFallbackAsyncImage import com.vitorpamplona.amethyst.ui.navigation.routes.Route import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel @@ -56,13 +60,35 @@ data class GroupEntryDisplay( val route: Route, ) +/** + * Observes a channel's metadata for a display row. [subscribe] gates the relay REQ: + * - the live bottom bar / rail passes true — a pinned group keeps a REQ open so its name/avatar + * refresh even if the group is never opened (bounded by the handful of pinned slots); + * - the settings picker passes false — it reads whatever metadata is already cached (filled by the + * chats/group screens) so expanding a category with many joined groups doesn't fan out into one + * subscription per row. + */ +@Composable +private fun observeChannelMetadata( + channel: Channel, + accountViewModel: AccountViewModel, + subscribe: Boolean, +): State { + if (subscribe) ChannelFinderFilterAssemblerSubscription(channel, accountViewModel) + return channel + .flow() + .metadata.stateFlow + .collectAsStateWithLifecycle() +} + @Composable fun rememberPublicChatEntryDisplay( entry: BottomBarEntry.PublicChat, accountViewModel: AccountViewModel, + subscribe: Boolean = true, ): GroupEntryDisplay { val channel = remember(entry.channelId) { LocalCache.getOrCreatePublicChatChannel(entry.channelId) } - val state by observeChannel(channel, accountViewModel) + val state by observeChannelMetadata(channel, accountViewModel, subscribe) val current = (state?.channel as? PublicChatChannel) ?: channel return GroupEntryDisplay( label = current.toBestDisplayName(), @@ -76,25 +102,33 @@ fun rememberPublicChatEntryDisplay( fun rememberRelayGroupEntryDisplay( entry: BottomBarEntry.RelayGroup, accountViewModel: AccountViewModel, + subscribe: Boolean = true, ): GroupEntryDisplay { val relay = remember(entry.relayUrl) { RelayUrlNormalizer.normalizeOrNull(entry.relayUrl) } - val route = Route.RelayGroup(entry.groupId, entry.relayUrl) - - if (relay == null) { - return GroupEntryDisplay(entry.groupId, entry.groupId, null, route) - } - - val channel = remember(entry.groupId, relay) { LocalCache.getOrCreateRelayGroupChannel(GroupId(entry.groupId, relay)) } - val state by observeChannel(channel, accountViewModel) + val channel = remember(entry.groupId, relay) { relay?.let { LocalCache.getOrCreateRelayGroupChannel(GroupId(entry.groupId, it)) } } + // Always call the observer (with a null channel when the relay won't normalize) so the composable + // call structure is unconditional; a null channel just yields a null state and the id fallback. + val state by observeChannelMetadataOrNull(channel, accountViewModel, subscribe) val current = (state?.channel as? RelayGroupChannel) ?: channel return GroupEntryDisplay( - label = current.toBestDisplayName(), + label = current?.toBestDisplayName() ?: entry.groupId, robotSeed = entry.groupId, - model = current.profilePicture(), - route = route, + model = current?.profilePicture(), + route = Route.RelayGroup(entry.groupId, entry.relayUrl), ) } +/** [observeChannelMetadata] tolerant of a null channel (unresolvable relay), so callers avoid an early return. */ +@Composable +private fun observeChannelMetadataOrNull( + channel: Channel?, + accountViewModel: AccountViewModel, + subscribe: Boolean, +): State { + if (channel == null) return remember { mutableStateOf(null) } + return observeChannelMetadata(channel, accountViewModel, subscribe) +} + @Composable fun rememberConcordEntryDisplay( entry: BottomBarEntry.Concord, @@ -125,15 +159,20 @@ fun rememberConcordEntryDisplay( ) } -/** Resolves any chat/group entry to its live display, or null for a non-group entry. */ +/** + * Resolves any chat/group entry to its live display, or null for a non-group entry. [subscribe] is + * forwarded to the channel observers: true (the default) keeps a relay REQ open — used by the live + * bar/rail; false reads only cached metadata — used by the settings picker (see [observeChannelMetadata]). + */ @Composable fun rememberGroupEntryDisplay( entry: BottomBarEntry, accountViewModel: AccountViewModel, + subscribe: Boolean = true, ): GroupEntryDisplay? = when (entry) { - is BottomBarEntry.PublicChat -> rememberPublicChatEntryDisplay(entry, accountViewModel) - is BottomBarEntry.RelayGroup -> rememberRelayGroupEntryDisplay(entry, accountViewModel) + is BottomBarEntry.PublicChat -> rememberPublicChatEntryDisplay(entry, accountViewModel, subscribe) + is BottomBarEntry.RelayGroup -> rememberRelayGroupEntryDisplay(entry, accountViewModel, subscribe) is BottomBarEntry.Concord -> rememberConcordEntryDisplay(entry, accountViewModel) is BottomBarEntry.BuiltIn -> null is BottomBarEntry.Favorite -> null 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 6e69fdaf9d..f4cc7bbe80 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 @@ -43,11 +43,11 @@ import androidx.compose.material3.Switch import androidx.compose.material3.Text import androidx.compose.material3.TextButton import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableFloatStateOf import androidx.compose.runtime.mutableIntStateOf import androidx.compose.runtime.mutableStateMapOf -import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment @@ -68,7 +68,6 @@ import com.vitorpamplona.amethyst.commons.icons.symbols.MaterialSymbols import com.vitorpamplona.amethyst.favorites.FavoriteAppsRegistry import com.vitorpamplona.amethyst.ui.navigation.bottombars.BottomBarCategories import com.vitorpamplona.amethyst.ui.navigation.bottombars.BottomBarEntry -import com.vitorpamplona.amethyst.ui.navigation.bottombars.DefaultBottomBarEntries import com.vitorpamplona.amethyst.ui.navigation.bottombars.GroupEntryAvatar import com.vitorpamplona.amethyst.ui.navigation.bottombars.GroupEntryDisplay import com.vitorpamplona.amethyst.ui.navigation.bottombars.NavBarCatalog @@ -125,24 +124,14 @@ fun BottomBarSettingsContent(accountViewModel: AccountViewModel) { val bottomBarItemsFlow = accountViewModel.settings.uiSettingsFlow.bottomBarItems val savedItems by bottomBarItemsFlow.collectAsStateWithLifecycle() - // A local, drag-mutable copy of the pinned list. Re-seeded whenever the saved list changes from - // elsewhere (e.g. a favorite/group got pinned from its picker row, or Restore Default ran). - var pinned by remember(savedItems) { mutableStateOf(savedItems) } + // All pin/unpin/reorder logic lives in the holder (unit-tested); the composable only renders and + // forwards events. syncFrom re-seeds when the saved list changes elsewhere (a picker toggle, an + // external device edit, Restore Default) without clobbering an in-progress drag. + val state = remember { BottomBarSettingsState(savedItems) { bottomBarItemsFlow.tryEmit(it) } } + LaunchedEffect(savedItems) { state.syncFrom(savedItems) } - fun save(newItems: List) { - pinned = newItems - bottomBarItemsFlow.tryEmit(newItems) - } - - val pinnedKeys = remember(pinned) { pinned.map { it.stableKey }.toSet() } - - fun togglePin(entry: BottomBarEntry) { - if (entry.stableKey in pinnedKeys) { - save(pinned.filter { it.stableKey != entry.stableKey }) - } else { - save(pinned + entry) - } - } + val pinned = state.pinned + val pinnedKeys = remember(pinned) { state.pinnedKeys() } var draggedItemIndex by remember { mutableIntStateOf(-1) } var dragOffset by remember { mutableFloatStateOf(0f) } @@ -179,7 +168,7 @@ fun BottomBarSettingsContent(accountViewModel: AccountViewModel) { onClick = { draggedItemIndex = -1 dragOffset = 0f - save(DefaultBottomBarEntries) + state.restoreDefault() }, ) { Text(stringRes(R.string.bottom_bar_settings_restore_default)) @@ -203,7 +192,7 @@ fun BottomBarSettingsContent(accountViewModel: AccountViewModel) { isDragging = rowIsDragging, dragOffsetY = if (rowIsDragging) dragOffset else 0f, elevation = animatedElevation, - onUnpin = { togglePin(entry) }, + onUnpin = { state.togglePin(entry) }, onMeasured = { height -> itemHeights[index] = height }, onDragStart = { draggedItemIndex = index @@ -218,11 +207,8 @@ fun BottomBarSettingsContent(accountViewModel: AccountViewModel) { if (dragOffset < 0 && currentIndex > 0) { val aboveHeight = itemHeights[currentIndex - 1] ?: 0f if (-dragOffset > aboveHeight / 2f) { - val newItems = pinned.toMutableList() - val temp = newItems[currentIndex - 1] - newItems[currentIndex - 1] = newItems[currentIndex] - newItems[currentIndex] = temp - pinned = newItems + // Swap with the row above (transient — persisted on drag end). + state.moveTransient(currentIndex, currentIndex - 1) val h1 = itemHeights[currentIndex] val h2 = itemHeights[currentIndex - 1] @@ -237,11 +223,8 @@ fun BottomBarSettingsContent(accountViewModel: AccountViewModel) { if (dragOffset > 0 && currentIndex < pinned.lastIndex) { val belowHeight = itemHeights[currentIndex + 1] ?: 0f if (dragOffset > belowHeight / 2f) { - val newItems = pinned.toMutableList() - val temp = newItems[currentIndex + 1] - newItems[currentIndex + 1] = newItems[currentIndex] - newItems[currentIndex] = temp - pinned = newItems + // Swap with the row below (transient — persisted on drag end). + state.moveTransient(currentIndex, currentIndex + 1) val h1 = itemHeights[currentIndex] val h2 = itemHeights[currentIndex + 1] @@ -256,7 +239,7 @@ fun BottomBarSettingsContent(accountViewModel: AccountViewModel) { onDragEnd = { draggedItemIndex = -1 dragOffset = 0f - save(pinned) + state.commit() }, onDragCancel = { draggedItemIndex = -1 @@ -299,17 +282,17 @@ fun BottomBarSettingsContent(accountViewModel: AccountViewModel) { label = stringRes(def.labelRes), pinned = entry.stableKey in pinnedKeys, expanded = expandedItems[item] ?: false, - onTogglePin = { togglePin(entry) }, + onTogglePin = { state.togglePin(entry) }, onToggleExpand = { expandedItems[item] = !(expandedItems[item] ?: false) }, ) { - PickerChildren(item, pinnedKeys, accountViewModel, ::togglePin) + PickerChildren(item, pinnedKeys, accountViewModel, state::togglePin) } } else { SimpleAvailableRow( icon = def.icon, label = stringRes(def.labelRes), pinned = entry.stableKey in pinnedKeys, - onToggle = { togglePin(entry) }, + onToggle = { state.togglePin(entry) }, ) } } @@ -396,7 +379,8 @@ private fun GroupChildRow( accountViewModel: AccountViewModel, onTogglePin: (BottomBarEntry) -> Unit, ) { - val display = rememberGroupEntryDisplay(entry, accountViewModel) ?: return + // Read-only: the picker resolves names/avatars from cache, it must not open a REQ per row. + val display = rememberGroupEntryDisplay(entry, accountViewModel, subscribe = false) ?: return ChildRow( leading = { GroupEntryAvatar(display, 28.dp, accountViewModel) }, label = display.label, @@ -675,7 +659,8 @@ private fun rememberPinnedVisual( is BottomBarEntry.RelayGroup, is BottomBarEntry.Concord, -> { - val display = rememberGroupEntryDisplay(entry, accountViewModel) + // Read-only: the settings list resolves from cache; the live bar owns the subscription. + val display = rememberGroupEntryDisplay(entry, accountViewModel, subscribe = false) if (display != null) PinnedVisual.Avatar(display) else PinnedVisual.Glyph(MaterialSymbols.Group, "") } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/BottomBarSettingsState.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/BottomBarSettingsState.kt new file mode 100644 index 0000000000..075e38f086 --- /dev/null +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/BottomBarSettingsState.kt @@ -0,0 +1,108 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.ui.screen.loggedIn.settings + +import androidx.compose.runtime.Stable +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.setValue +import com.vitorpamplona.amethyst.ui.navigation.bottombars.BottomBarEntry +import com.vitorpamplona.amethyst.ui.navigation.bottombars.DefaultBottomBarEntries +import com.vitorpamplona.amethyst.ui.navigation.bottombars.stableKey + +/** + * Pure list transforms for the bottom-bar pinned list. Kept free of Compose/Android so the pin, + * unpin and reorder rules are exercised directly by unit tests (BottomBarSettingsStateTest) rather + * than only through the drag UI. [BottomBarSettingsState] is the thin stateful wrapper the screen uses. + */ +object BottomBarEditing { + fun isPinned( + items: List, + entry: BottomBarEntry, + ): Boolean = items.any { it.stableKey == entry.stableKey } + + /** Adds [entry] to the end if absent, or removes it (by stable identity) if present. */ + fun togglePin( + items: List, + entry: BottomBarEntry, + ): List = + if (isPinned(items, entry)) { + items.filter { it.stableKey != entry.stableKey } + } else { + items + entry + } + + /** Moves the item at [from] to index [to]; a no-op if either index is out of bounds. */ + fun move( + items: List, + from: Int, + to: Int, + ): List { + if (from == to || from !in items.indices || to !in items.indices) return items + return items.toMutableList().apply { add(to, removeAt(from)) } + } +} + +/** + * State holder for the Bottom Bar settings screen: owns the ordered pinned list and the pin / unpin / + * reorder / restore-default operations, so the composable only renders and forwards events. During a + * drag, [moveTransient] reorders without persisting; [commit] writes the final order once the drag ends. + */ +@Stable +class BottomBarSettingsState( + initial: List, + private val persist: (List) -> Unit, +) { + var pinned by mutableStateOf(initial) + private set + + fun isPinned(entry: BottomBarEntry): Boolean = BottomBarEditing.isPinned(pinned, entry) + + fun pinnedKeys(): Set = pinned.mapTo(HashSet(pinned.size)) { it.stableKey } + + fun togglePin(entry: BottomBarEntry) = update(BottomBarEditing.togglePin(pinned, entry)) + + fun restoreDefault() = update(DefaultBottomBarEntries) + + /** Reorder mid-drag WITHOUT persisting. Pair with [commit] when the gesture ends. */ + fun moveTransient( + from: Int, + to: Int, + ) { + pinned = BottomBarEditing.move(pinned, from, to) + } + + /** Persist the current (post-drag) order. */ + fun commit() = persist(pinned) + + /** + * Re-seed from an external change (the saved settings flow emitted) without re-persisting. A no-op + * when equal, so the echo of our own [persist] doesn't clobber an in-progress edit. + */ + fun syncFrom(items: List) { + if (items != pinned) pinned = items + } + + private fun update(newItems: List) { + pinned = newItems + persist(newItems) + } +} diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/navigation/BottomBarSettingsStateTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/navigation/BottomBarSettingsStateTest.kt new file mode 100644 index 0000000000..ddc52a8cb0 --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/navigation/BottomBarSettingsStateTest.kt @@ -0,0 +1,139 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.navigation + +import com.vitorpamplona.amethyst.ui.navigation.bottombars.BottomBarEntry +import com.vitorpamplona.amethyst.ui.navigation.bottombars.DefaultBottomBarEntries +import com.vitorpamplona.amethyst.ui.navigation.bottombars.NavBarItem +import com.vitorpamplona.amethyst.ui.navigation.bottombars.stableKey +import com.vitorpamplona.amethyst.ui.screen.loggedIn.settings.BottomBarEditing +import com.vitorpamplona.amethyst.ui.screen.loggedIn.settings.BottomBarSettingsState +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +class BottomBarSettingsStateTest { + private val home = BottomBarEntry.BuiltIn(NavBarItem.HOME) + private val messages = BottomBarEntry.BuiltIn(NavBarItem.MESSAGES) + private val group = BottomBarEntry.RelayGroup("abcd", "wss://relay.example") + private val chat = BottomBarEntry.PublicChat("f".repeat(64)) + + // --- Pure edit operations --- + + @Test + fun togglePinAddsWhenAbsentAndRemovesWhenPresent() { + val once = BottomBarEditing.togglePin(listOf(home), messages) + assertEquals(listOf(home, messages), once) + + val twice = BottomBarEditing.togglePin(once, messages) + assertEquals(listOf(home), twice) + } + + @Test + fun togglePinIsIdempotentOverTwoCalls() { + val start = listOf(home, group) + val roundTrip = BottomBarEditing.togglePin(BottomBarEditing.togglePin(start, chat), chat) + assertEquals(start, roundTrip) + } + + @Test + fun togglePinRemovesByStableIdentityNotReferenceEquality() { + val start = listOf(BottomBarEntry.RelayGroup("abcd", "wss://relay.example")) + // A distinct instance with the same (id, relay) resolves to the same stableKey. + val result = BottomBarEditing.togglePin(start, BottomBarEntry.RelayGroup("abcd", "wss://relay.example")) + assertTrue(result.isEmpty()) + } + + @Test + fun moveReordersAndClampsOutOfBounds() { + val start = listOf(home, messages, group) + assertEquals(listOf(messages, group, home), BottomBarEditing.move(start, 0, 2)) + assertEquals(start, BottomBarEditing.move(start, 1, 1)) // no-op: same index + assertEquals(start, BottomBarEditing.move(start, -1, 2)) // no-op: out of bounds + assertEquals(start, BottomBarEditing.move(start, 0, 9)) // no-op: out of bounds + } + + @Test + fun stableKeyIsUniquePerTypeEvenWhenUnderlyingStringMatches() { + val a = BottomBarEntry.PublicChat("x").stableKey + val b = BottomBarEntry.Concord("x").stableKey + val c = BottomBarEntry.Favorite("x").stableKey + assertEquals(3, setOf(a, b, c).size) + } + + // --- Holder: persistence semantics --- + + @Test + fun togglePinPersistsImmediately() { + var saved: List? = null + val state = BottomBarSettingsState(listOf(home)) { saved = it } + + state.togglePin(messages) + + assertEquals(listOf(home, messages), state.pinned) + assertEquals(listOf(home, messages), saved) + } + + @Test + fun moveTransientDoesNotPersistUntilCommit() { + var saveCount = 0 + val state = BottomBarSettingsState(listOf(home, messages)) { saveCount++ } + + state.moveTransient(0, 1) + assertEquals(listOf(messages, home), state.pinned) + assertEquals(0, saveCount) // dragging does not write + + state.commit() + assertEquals(1, saveCount) // drag end writes once + } + + @Test + fun restoreDefaultPersistsTheDefaults() { + var saved: List? = null + val state = BottomBarSettingsState(listOf(home)) { saved = it } + + state.restoreDefault() + + assertEquals(DefaultBottomBarEntries, state.pinned) + assertEquals(state.pinned, saved) + } + + @Test + fun syncFromOnlyUpdatesWhenDifferent() { + var saveCount = 0 + val state = BottomBarSettingsState(listOf(home)) { saveCount++ } + + state.syncFrom(listOf(home)) // equal → no change, and never persists + assertEquals(listOf(home), state.pinned) + + state.syncFrom(listOf(home, messages)) // external change adopted + assertEquals(listOf(home, messages), state.pinned) + assertEquals(0, saveCount) // syncFrom must never persist (it would echo-loop) + } + + @Test + fun isPinnedReflectsMembership() { + val state = BottomBarSettingsState(listOf(home, group)) {} + assertTrue(state.isPinned(group)) + assertFalse(state.isPinned(messages)) + } +}