From b91e84b416835b4259805111606662c252b7db00 Mon Sep 17 00:00:00 2001 From: davotoula Date: Tue, 12 May 2026 08:44:51 +0200 Subject: [PATCH] Code review: - Gate the new notificationsFollowing / notificationsEveryone fan-out in updateFeedsWith / deleteNotes on splitNotificationsEnabled.value so the default-off case stops doing two extra filter scans per event bundle. - Lazy-init NotificationFeedFilter.overrideFollowLists so the SharingStarted.Eagerly topNavFilter pipeline only starts when the split UI actually opens the filter. - Switch SplitNotificationsScaffold to rememberForeverPagerState so the active tab survives recomposition, matching HomeScreen/DiscoverScreen. - Promote the duplicated "Notification" last-read key string to a NOTIFICATION_LAST_READ_KEY const. - Move the opaque-background fix into SummaryBar itself so callers don't need a band-aid Modifier.background on their topBar Column. - Trim narrating / WHAT comments; keep only the deep-link scroll WHY. --- .../ui/feeds/RememberForeverStates.kt | 1 + .../loggedIn/AccountFeedContentStates.kt | 17 +++++----- .../ui/screen/loggedIn/AccountViewModel.kt | 10 +++--- .../notifications/NotificationScreen.kt | 28 ++++++---------- .../notifications/NotificationSummaryView.kt | 32 +++++++++++-------- .../notifications/NotificationTopBar.kt | 2 -- .../dal/NotificationFeedFilter.kt | 11 +++---- 7 files changed, 46 insertions(+), 55 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/feeds/RememberForeverStates.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/feeds/RememberForeverStates.kt index d92ffc0de..ede1702f2 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/feeds/RememberForeverStates.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/feeds/RememberForeverStates.kt @@ -83,6 +83,7 @@ object PagerStateKeys { const val HOME_SCREEN = "PagerHome" const val DISCOVER_SCREEN = "PagerDiscover" const val POLLS_SCREEN = "PagerPolls" + const val NOTIFICATION_SCREEN = "PagerNotification" } @Composable diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountFeedContentStates.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountFeedContentStates.kt index 92c6f965c..857b6c6fb 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountFeedContentStates.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountFeedContentStates.kt @@ -109,11 +109,6 @@ class AccountFeedContentStates( val articlesFeed = FeedContentState(ArticlesFeedFilter(account), scope, LocalCache) val notifications = CardFeedContentState(NotificationFeedFilter(account), scope) - - // Split-notifications mode (Issue #197): when [AccountSettings.splitNotificationsEnabled] - // is on, the screen renders these two pinned-mode feeds in tabs instead of [notifications]. - // Following = kind:3 follow list members only; Everyone = global. Both are always - // constructed so updateFeedsWith can fan out incoming events without rebuilding state. val notificationsFollowing = CardFeedContentState(NotificationFeedFilter(account, TopFilter.AllFollows), scope) val notificationsEveryone = CardFeedContentState(NotificationFeedFilter(account, TopFilter.Global), scope) @@ -192,8 +187,10 @@ class AccountFeedContentStates( articlesFeed.updateFeedWith(newNotes) notifications.updateFeedWith(newNotes) - notificationsFollowing.updateFeedWith(newNotes) - notificationsEveryone.updateFeedWith(newNotes) + if (account.settings.splitNotificationsEnabled.value) { + notificationsFollowing.updateFeedWith(newNotes) + notificationsEveryone.updateFeedWith(newNotes) + } notificationSummary.invalidateInsertData(newNotes) drafts.updateFeedWith(newNotes) @@ -242,8 +239,10 @@ class AccountFeedContentStates( articlesFeed.deleteFromFeed(newNotes) notifications.deleteFromFeed(newNotes) - notificationsFollowing.deleteFromFeed(newNotes) - notificationsEveryone.deleteFromFeed(newNotes) + if (account.settings.splitNotificationsEnabled.value) { + notificationsFollowing.deleteFromFeed(newNotes) + notificationsEveryone.deleteFromFeed(newNotes) + } notificationSummary.invalidateInsertData(newNotes) drafts.deleteFromFeed(newNotes) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountViewModel.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountViewModel.kt index 3b459aac8..bc3a4a331 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountViewModel.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountViewModel.kt @@ -81,6 +81,7 @@ import com.vitorpamplona.amethyst.ui.note.showAmount import com.vitorpamplona.amethyst.ui.note.showAmountInteger import com.vitorpamplona.amethyst.ui.screen.UiSettingsState import com.vitorpamplona.amethyst.ui.screen.loggedIn.notifications.CombinedZap +import com.vitorpamplona.amethyst.ui.screen.loggedIn.notifications.NOTIFICATION_LAST_READ_KEY import com.vitorpamplona.amethyst.ui.screen.loggedIn.relays.eventsync.EventSync import com.vitorpamplona.amethyst.ui.stringRes import com.vitorpamplona.amethyst.ui.tor.TorSettingsFlow @@ -319,16 +320,13 @@ class AccountViewModel( @OptIn(ExperimentalCoroutinesApi::class) val notificationHasNewItems = - // Issue #197: when split-notifications is ON, the bottom-bar dot must glow only - // for items in the Following tab. When OFF, fall back to the user-selected single - // feed. Switch sources on the toggle flow so changing the setting takes effect - // without a restart. + // When split-notifications is on, the badge tracks only the Following feed. account.settings.splitNotificationsEnabled .flatMapLatest { isSplit -> val source = if (isSplit) feedStates.notificationsFollowing else feedStates.notifications combineTransform( - account.loadLastReadFlow("Notification"), + account.loadLastReadFlow(NOTIFICATION_LAST_READ_KEY), source.feedContent .flatMapLatest { if (it is CardFeedState.Loaded) { @@ -347,7 +345,7 @@ class AccountViewModel( } else { feedStates.notifications } - val lastRead = account.loadLastReadFlow("Notification").value + val lastRead = account.loadLastReadFlow(NOTIFICATION_LAST_READ_KEY).value val cards = source.feedContent.value if (cards is CardFeedState.Loaded) { val newestItemCreatedAt = diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/NotificationScreen.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/NotificationScreen.kt index 34dc8e4a9..1f179678f 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/NotificationScreen.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/NotificationScreen.kt @@ -20,11 +20,9 @@ */ package com.vitorpamplona.amethyst.ui.screen.loggedIn.notifications -import androidx.compose.foundation.background import androidx.compose.foundation.layout.Column import androidx.compose.foundation.pager.HorizontalPager import androidx.compose.foundation.pager.PagerState -import androidx.compose.foundation.pager.rememberPagerState import androidx.compose.material3.MaterialTheme import androidx.compose.material3.SecondaryTabRow import androidx.compose.material3.Tab @@ -33,15 +31,16 @@ import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue import androidx.compose.runtime.rememberCoroutineScope -import androidx.compose.ui.Modifier import androidx.lifecycle.compose.collectAsStateWithLifecycle import com.vitorpamplona.amethyst.R import com.vitorpamplona.amethyst.model.UiSettingsFlow import com.vitorpamplona.amethyst.ui.components.SelectNotificationProvider +import com.vitorpamplona.amethyst.ui.feeds.PagerStateKeys import com.vitorpamplona.amethyst.ui.feeds.RefresheableBox import com.vitorpamplona.amethyst.ui.feeds.ScrollStateKeys import com.vitorpamplona.amethyst.ui.feeds.WatchScrollToTop import com.vitorpamplona.amethyst.ui.feeds.rememberForeverLazyListState +import com.vitorpamplona.amethyst.ui.feeds.rememberForeverPagerState import com.vitorpamplona.amethyst.ui.layouts.DisappearingScaffold import com.vitorpamplona.amethyst.ui.navigation.bottombars.AppBottomBar import com.vitorpamplona.amethyst.ui.navigation.navs.INav @@ -51,6 +50,8 @@ import com.vitorpamplona.amethyst.ui.stringRes import com.vitorpamplona.amethyst.ui.theme.TabRowHeight import kotlinx.coroutines.launch +const val NOTIFICATION_LAST_READ_KEY = "Notification" + @Composable fun NotificationScreen( scrollToEventId: String? = null, @@ -124,10 +125,7 @@ private fun SingleNotificationsScaffold( DisappearingScaffold( isInvertedLayout = false, topBar = { - // DisappearingScaffold layers content under a translating topBar; without an - // opaque background, feed items scroll visibly through the SummaryBar gap - // between NotificationTopBar and the divider. - Column(modifier = Modifier.background(MaterialTheme.colorScheme.background)) { + Column { NotificationTopBar(accountViewModel, nav, showSpinner = true) SummaryBar(state = notifSummaryState) } @@ -163,18 +161,13 @@ private fun SplitNotificationsScaffold( accountViewModel: AccountViewModel, nav: INav, ) { - val pagerState = rememberPagerState(pageCount = { 2 }) + val pagerState = rememberForeverPagerState(key = PagerStateKeys.NOTIFICATION_SCREEN) { 2 } val coroutineScope = rememberCoroutineScope() DisappearingScaffold( isInvertedLayout = false, topBar = { - // Mirror HomeScreen: the TabRow lives in the topBar Column so it inherits - // the scaffold's status-bar inset handling and scrolls together with the - // title + summary instead of floating into the status bar area. - // Opaque background hides feed items that scroll under the topBar through - // the SummaryBar gap (which has no container of its own). - Column(modifier = Modifier.background(MaterialTheme.colorScheme.background)) { + Column { NotificationTopBar(accountViewModel, nav, showSpinner = false) SummaryBar(state = notifSummaryState) SecondaryTabRow( @@ -240,7 +233,7 @@ private fun SingleNotificationsBody( accountViewModel = accountViewModel, listState = listState, nav = nav, - routeForLastRead = "Notification", + routeForLastRead = NOTIFICATION_LAST_READ_KEY, scrollToEventId = scrollToEventId, headerContent = { ObserveInboxRelayListAndDisplayIfNotFound(accountViewModel, nav) }, ) @@ -306,10 +299,7 @@ private fun NotificationPagerPage( accountViewModel = accountViewModel, listState = listState, nav = nav, - // Both tabs share the "Notification" last-read marker (Section H #7): the - // unread indicator is gated to the Following tab elsewhere, so a single - // marker keeps migration trivial and avoids a stale marker per tab. - routeForLastRead = "Notification", + routeForLastRead = NOTIFICATION_LAST_READ_KEY, scrollToEventId = scrollToEventId, headerContent = { ObserveInboxRelayListAndDisplayIfNotFound(accountViewModel, nav) }, ) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/NotificationSummaryView.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/NotificationSummaryView.kt index d5db5bb8f..04ed75de7 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/NotificationSummaryView.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/NotificationSummaryView.kt @@ -25,7 +25,9 @@ import androidx.compose.animation.expandVertically import androidx.compose.animation.shrinkVertically import androidx.compose.animation.slideInVertically import androidx.compose.animation.slideOutVertically +import androidx.compose.foundation.background import androidx.compose.foundation.clickable +import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.Row import androidx.compose.foundation.layout.padding import androidx.compose.material3.MaterialTheme @@ -46,21 +48,25 @@ import com.vitorpamplona.amethyst.ui.theme.chartStyle fun SummaryBar(state: NotificationSummaryState) { var showChart by remember { mutableStateOf(false) } - UserReactionsRow(state) { showChart = !showChart } + // Opaque background: DisappearingScaffold layers feed content under the topBar + // on scroll, and SummaryBar otherwise has no container of its own. + Column(modifier = Modifier.background(MaterialTheme.colorScheme.background)) { + UserReactionsRow(state) { showChart = !showChart } - AnimatedVisibility( - visible = showChart, - enter = slideInVertically() + expandVertically(), - exit = slideOutVertically() + shrinkVertically(), - ) { - Row( - modifier = - Modifier - .padding(vertical = 0.dp, horizontal = 20.dp) - .clickable(onClick = { showChart = !showChart }), + AnimatedVisibility( + visible = showChart, + enter = slideInVertically() + expandVertically(), + exit = slideOutVertically() + shrinkVertically(), ) { - ProvideVicoTheme(MaterialTheme.colorScheme.chartStyle) { - ObserveAndShowChart(state) + Row( + modifier = + Modifier + .padding(vertical = 0.dp, horizontal = 20.dp) + .clickable(onClick = { showChart = !showChart }), + ) { + ProvideVicoTheme(MaterialTheme.colorScheme.chartStyle) { + ObserveAndShowChart(state) + } } } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/NotificationTopBar.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/NotificationTopBar.kt index 14af718e7..781076c58 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/NotificationTopBar.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/NotificationTopBar.kt @@ -52,8 +52,6 @@ fun NotificationTopBar( onChange = accountViewModel.account.settings::changeDefaultNotificationFollowList, ) } else { - // Split-notifications (Issue #197): tabs replace the list-filter spinner, so - // render a plain title in the top bar's content slot. Text(text = stringRes(R.string.route_notifications)) } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/dal/NotificationFeedFilter.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/dal/NotificationFeedFilter.kt index 81afe71ea..3e96624a0 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/dal/NotificationFeedFilter.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/dal/NotificationFeedFilter.kt @@ -81,13 +81,12 @@ class NotificationFeedFilter( val account: Account, val modeOverride: TopFilter? = null, ) : AdditiveFeedFilter() { - // When [modeOverride] is set (split-notifications tabs), build a dedicated - // IFeedTopNavFilter flow so this filter is independent of the user-controlled - // [Account.liveNotificationFollowLists]. The Following tab pins AllFollows; the - // Everyone tab pins Global. When the override is null we fall back to the - // shared, spinner-driven flow used by the single-feed layout. - private val overrideFollowLists: StateFlow? = + // Pin to modeOverride for split-tab mode; otherwise follow the spinner. + // Lazy so the eagerly-collected topNavFilter pipeline is only built when + // the split UI actually opens this filter. + private val overrideFollowLists: StateFlow? by lazy { modeOverride?.let { account.topNavFilterFlow(MutableStateFlow(it)) } + } companion object { val ADDRESSABLE_KINDS =