From fa43821cf4e7acf71d1f8eb7765c06823c8e95fc Mon Sep 17 00:00:00 2001 From: callebtc <93376500+callebtc@users.noreply.github.com> Date: Mon, 27 Jul 2026 12:36:09 +0200 Subject: [PATCH] Key message list state per conversation Switching channels reused every piece of state in MessagesList, because none of it was keyed on which conversation was being shown: - The LazyListState carried the previous channel's scroll offset, so the new channel opened at a stale position and then corrected itself. - hasScrolledToInitialPosition and followIncomingMessages carried over, so a channel entered after scrolling up in another one did not land on its newest message at all. - The arrival tracker had never seen the incoming channel's ids, so a backlog of six or fewer messages was treated as six simultaneous arrivals and each one slid in. - previousMessageCount carried over, arming placement animation for the relayout that the switch itself caused. All of it is now keyed on a conversationKey derived the same way displayMessages is. The tracker also detects a list sharing no ids with the previous one and adopts it silently, which covers /clear and any caller that does not supply a distinct key. Adds 4 tests for wholesale replacement, including the case that the burst cap cannot catch on its own. --- .../java/com/bitchat/android/ui/ChatScreen.kt | 15 ++++++ .../android/ui/LocationChannelsSheet.kt | 50 +++++++++++++++++++ .../bitchat/android/ui/MeshPeerListSheet.kt | 1 + .../bitchat/android/ui/MessageComponents.kt | 48 ++++++++++++++---- .../android/ui/LocationChannelsSheetTest.kt | 49 ++++++++++++++++++ .../android/ui/MessageArrivalTrackerTest.kt | 45 +++++++++++++++++ 6 files changed, 197 insertions(+), 11 deletions(-) create mode 100644 app/src/test/java/com/bitchat/android/ui/LocationChannelsSheetTest.kt diff --git a/app/src/main/java/com/bitchat/android/ui/ChatScreen.kt b/app/src/main/java/com/bitchat/android/ui/ChatScreen.kt index ba3c627c..746135a7 100644 --- a/app/src/main/java/com/bitchat/android/ui/ChatScreen.kt +++ b/app/src/main/java/com/bitchat/android/ui/ChatScreen.kt @@ -187,6 +187,20 @@ fun ChatScreen(viewModel: ChatViewModel) { } } + // Identity of the timeline on screen, derived exactly like displayMessages above. Drives the + // per-conversation scroll position and animation state in MessagesList. + val conversationKey = when { + currentChannel != null -> "channel:$currentChannel" + else -> { + val locationChannel = selectedLocationChannel + if (locationChannel is com.bitchat.android.geohash.ChannelID.Location) { + "geo:${locationChannel.channel.geohash}" + } else { + "mesh" + } + } + } + // Determine whether to show media buttons (only hide in geohash location chats) val showMediaButtons = when { currentChannel != null -> true @@ -229,6 +243,7 @@ fun ChatScreen(viewModel: ChatViewModel) { currentUserNickname = nickname, meshService = viewModel.meshServiceFacade, modifier = Modifier.fillMaxSize(), + conversationKey = conversationKey, contentPadding = PaddingValues( top = statusBarHeight + headerHeight + (if (showNotesStrip) notesStripHeight else 0.dp), diff --git a/app/src/main/java/com/bitchat/android/ui/LocationChannelsSheet.kt b/app/src/main/java/com/bitchat/android/ui/LocationChannelsSheet.kt index 740673be..cb43cab0 100644 --- a/app/src/main/java/com/bitchat/android/ui/LocationChannelsSheet.kt +++ b/app/src/main/java/com/bitchat/android/ui/LocationChannelsSheet.kt @@ -154,6 +154,9 @@ fun LocationChannelsSheet( val nearbyChannels = remember(availableChannels) { availableChannels.filter { it.level != GeohashChannelLevel.BUILDING } } + val selectedChannelOutsideNearby = remember(selectedChannel, nearbyChannels) { + selectedLocationChannelOutsideNearby(selectedChannel, nearbyChannels) + } val showNearbyLoading = nearbyChannels.isEmpty() && permissionState == LocationChannelManager.PermissionState.AUTHORIZED && locationServicesEnabled @@ -272,6 +275,35 @@ fun LocationChannelsSheet( shape = AboutCardShape ) { Column { + selectedChannelOutsideNearby?.let { channel -> + val coverage = coverageString(channel.geohash.length) + val name = bookmarkNames[channel.geohash] + val subtitle = "#${channel.geohash} • $coverage" + + (name?.let { " • ${formattedNamePrefix(channel.level)}$it" } ?: "") + val participantCount = geohashParticipantCounts[channel.geohash] ?: 0 + val isBookmarked = bookmarksStore.isBookmarked(channel.geohash) + + ChannelOptionRow( + title = geohashTitleWithCount(channel, participantCount), + subtitle = subtitle, + isSelected = true, + participantCount = participantCount, + titleColor = standardGreen, + titleBold = participantCount > 0, + trailingContent = { + ChannelBookmarkButton( + bookmarked = isBookmarked, + onClick = { bookmarksStore.toggle(channel.geohash) } + ) + }, + onClick = { + locationManager.select(ChannelID.Location(channel)) + onDismiss() + } + ) + SheetCardDivider() + } + if (locationServicesEnabled) { if (nearbyChannels.isNotEmpty()) { nearbyChannels.forEachIndexed { index, channel -> @@ -952,6 +984,24 @@ private fun isChannelSelected(channel: GeohashChannel, selectedChannel: ChannelI } } +/** + * Returns the active location channel when it has no row in the nearby-channel list. + * + * This commonly happens after teleporting to a remote geohash. Keeping the selected channel in the + * main channel card makes its selection and bookmark action available even before it is bookmarked. + */ +internal fun selectedLocationChannelOutsideNearby( + selectedChannel: ChannelID?, + nearbyChannels: List +): GeohashChannel? { + val selected = (selectedChannel as? ChannelID.Location)?.channel ?: return null + return selected.takeUnless { active -> + nearbyChannels.any { nearby -> + nearby.geohash.equals(active.geohash, ignoreCase = true) + } + } +} + private fun validateGeohash(geohash: String): Boolean { if (geohash.isEmpty() || geohash.length > 12) return false val allowed = "0123456789bcdefghjkmnpqrstuvwxyz".toSet() diff --git a/app/src/main/java/com/bitchat/android/ui/MeshPeerListSheet.kt b/app/src/main/java/com/bitchat/android/ui/MeshPeerListSheet.kt index cfda3952..baaed09e 100644 --- a/app/src/main/java/com/bitchat/android/ui/MeshPeerListSheet.kt +++ b/app/src/main/java/com/bitchat/android/ui/MeshPeerListSheet.kt @@ -887,6 +887,7 @@ fun PrivateChatSheet( currentUserNickname = nickname, meshService = viewModel.meshServiceFacade, modifier = Modifier.weight(1f), + conversationKey = "dm:$peerID", forceScrollToBottom = forceScrollToBottom, onScrolledUpChanged = { isUp -> isScrolledUp = isUp }, onNicknameClick = { /* handle mention */ }, diff --git a/app/src/main/java/com/bitchat/android/ui/MessageComponents.kt b/app/src/main/java/com/bitchat/android/ui/MessageComponents.kt index 31db2ef1..c53aa7c0 100644 --- a/app/src/main/java/com/bitchat/android/ui/MessageComponents.kt +++ b/app/src/main/java/com/bitchat/android/ui/MessageComponents.kt @@ -29,8 +29,8 @@ import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.size import androidx.compose.foundation.lazy.LazyColumn +import androidx.compose.foundation.lazy.LazyListState import androidx.compose.foundation.lazy.itemsIndexed -import androidx.compose.foundation.lazy.rememberLazyListState import androidx.compose.foundation.shape.CircleShape import androidx.compose.material3.ColorScheme import androidx.compose.material3.Icon @@ -42,6 +42,7 @@ import androidx.compose.runtime.derivedStateOf import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember +import androidx.compose.runtime.saveable.rememberSaveable import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier @@ -153,6 +154,12 @@ internal fun MessageArrivalTracker.arrivals(messages: List): Set return emptySet() } + // A list with nothing in common with the last one is a different conversation, not a burst of + // arrivals — /clear, or a switch the caller did not give us a distinct key for. Adopt it + // silently rather than sliding in every message at once. + val isWholesaleReplacement = + messages.isNotEmpty() && known.isNotEmpty() && messages.none { it.id in known } + // `HashSet.add` reports whether the id was new, so this both diffs and updates in one pass. val added = messages.filter { known.add(it.id) } @@ -163,6 +170,7 @@ internal fun MessageArrivalTracker.arrivals(messages: List): Set } return when { + isWholesaleReplacement -> emptySet() added.isEmpty() || added.size > MaxAnimatedArrivals -> emptySet() else -> added.mapTo(HashSet(added.size)) { it.id } } @@ -181,6 +189,14 @@ fun MessagesList( * has to reserve room for their heights here rather than by shrinking the viewport. */ contentPadding: PaddingValues = PaddingValues(0.dp), + /** + * Identity of the conversation being shown — a channel, a geohash, a peer. + * + * Everything below that is per-conversation state is keyed on this. Without it, switching + * channels reused the previous conversation's scroll offset, follow flag and seen-message set, + * so the new channel opened at a stale position and then animated itself into place. + */ + conversationKey: Any? = null, forceScrollToBottom: Boolean = false, onScrolledUpChanged: ((Boolean) -> Unit)? = null, onNicknameClick: ((String) -> Unit)? = null, @@ -188,11 +204,19 @@ fun MessagesList( onCancelTransfer: ((BitchatMessage) -> Unit)? = null, onImageClick: ((String, List, Int) -> Unit)? = null ) { - val listState = rememberLazyListState() - + // A fresh scroll position per conversation. Sharing one state meant a switch inherited the + // previous channel's offset and then had to correct itself, which is what the jump was. + // + // Passing the key as an *input* rather than as `key =` is deliberate: it discards the saved + // offset on every switch, so a conversation always opens on its newest message instead of + // wherever the reader happened to be some time ago, with unseen messages below them. + val listState = rememberSaveable(conversationKey, saver = LazyListState.Saver) { + LazyListState() + } + // Track if this is the first time messages are being loaded - var hasScrolledToInitialPosition by remember { mutableStateOf(false) } - var followIncomingMessages by remember { mutableStateOf(true) } + var hasScrolledToInitialPosition by remember(conversationKey) { mutableStateOf(false) } + var followIncomingMessages by remember(conversationKey) { mutableStateOf(true) } // Smart scroll: auto-scroll to bottom for initial load, then follow unless user scrolls away LaunchedEffect(messages.size) { @@ -208,7 +232,7 @@ fun MessagesList( } // Track whether user has scrolled away from the latest messages - val isAtLatest by remember { + val isAtLatest by remember(listState) { derivedStateOf { val firstVisibleIndex = listState.layoutInfo.visibleItemsInfo.firstOrNull()?.index ?: -1 firstVisibleIndex <= 2 @@ -230,8 +254,10 @@ fun MessagesList( // Recomputed only when the list actually gains or loses a message, and synchronously, so the // arriving item can read its cue during the same composition pass in which it first appears. - val arrivalTracker = remember { MessageArrivalTracker() } - val enteringIds = remember(messages.size, messages.lastOrNull()?.id) { + // Reset per conversation, so a switch adopts the incoming messages silently instead of + // treating a whole channel's backlog as brand-new arrivals and sliding each one in. + val arrivalTracker = remember(conversationKey) { MessageArrivalTracker() } + val enteringIds = remember(conversationKey, messages.size, messages.lastOrNull()?.id) { arrivalTracker.arrivals(messages) } @@ -240,9 +266,9 @@ fun MessagesList( // growing a line — and animating those made the whole conversation lurch. So it is armed only // briefly around a genuine change to the list, and is otherwise off, letting items track the // viewport exactly. - var placementArmed by remember { mutableStateOf(false) } - var previousMessageCount by remember { mutableStateOf(null) } - LaunchedEffect(messages.size) { + var placementArmed by remember(conversationKey) { mutableStateOf(false) } + var previousMessageCount by remember(conversationKey) { mutableStateOf(null) } + LaunchedEffect(conversationKey, messages.size) { val previous = previousMessageCount previousMessageCount = messages.size // Skip the first composition: the list settling into its initial padding is not a change diff --git a/app/src/test/java/com/bitchat/android/ui/LocationChannelsSheetTest.kt b/app/src/test/java/com/bitchat/android/ui/LocationChannelsSheetTest.kt new file mode 100644 index 00000000..e53665ca --- /dev/null +++ b/app/src/test/java/com/bitchat/android/ui/LocationChannelsSheetTest.kt @@ -0,0 +1,49 @@ +package com.bitchat.android.ui + +import com.bitchat.android.geohash.ChannelID +import com.bitchat.android.geohash.GeohashChannel +import com.bitchat.android.geohash.GeohashChannelLevel +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test + +class LocationChannelsSheetTest { + + @Test + fun `teleported channel gets a standalone row`() { + val nearby = listOf(channel("u33dc")) + val teleported = channel("dr5ru") + + assertEquals( + teleported, + selectedLocationChannelOutsideNearby(ChannelID.Location(teleported), nearby) + ) + } + + @Test + fun `nearby selected channel is not duplicated`() { + val nearby = channel("u33dc") + + assertNull( + selectedLocationChannelOutsideNearby( + ChannelID.Location(channel("U33DC")), + listOf(nearby) + ) + ) + } + + @Test + fun `mesh selection has no standalone location row`() { + assertNull( + selectedLocationChannelOutsideNearby( + ChannelID.Mesh, + listOf(channel("u33dc")) + ) + ) + } + + private fun channel(geohash: String) = GeohashChannel( + level = GeohashChannelLevel.CITY, + geohash = geohash + ) +} diff --git a/app/src/test/java/com/bitchat/android/ui/MessageArrivalTrackerTest.kt b/app/src/test/java/com/bitchat/android/ui/MessageArrivalTrackerTest.kt index 0c848281..fb534e69 100644 --- a/app/src/test/java/com/bitchat/android/ui/MessageArrivalTrackerTest.kt +++ b/app/src/test/java/com/bitchat/android/ui/MessageArrivalTrackerTest.kt @@ -126,6 +126,51 @@ class MessageArrivalTrackerTest { assertEquals(setOf("a"), tracker.arrivals(listOf(msg("a")))) } + @Test + fun `a wholesale replacement animates nothing`() { + // Switching channels, or /clear followed by fresh content: the incoming list shares no ids + // with the outgoing one, so it is a different conversation rather than a burst of arrivals. + val tracker = MessageArrivalTracker() + tracker.arrivals(listOf(msg("a"), msg("b"))) + + val other = listOf(msg("x"), msg("y")) + assertTrue( + "a different conversation must not slide every message in", + tracker.arrivals(other).isEmpty() + ) + } + + @Test + fun `a small replacement below the burst cap still animates nothing`() { + // The burst cap alone would not catch this: two messages is well under it. + val tracker = MessageArrivalTracker() + tracker.arrivals(listOf(msg("a"), msg("b"), msg("c"))) + + assertTrue(tracker.arrivals(listOf(msg("x"), msg("y"))).isEmpty()) + } + + @Test + fun `a replacement that overlaps is treated as normal arrivals`() { + // Still the same conversation if anything carries over, so genuine new messages animate. + val tracker = MessageArrivalTracker() + val kept = msg("a") + tracker.arrivals(listOf(kept, msg("b"))) + + assertEquals(setOf("c"), tracker.arrivals(listOf(kept, msg("c")))) + } + + @Test + fun `after a wholesale replacement, later arrivals animate normally`() { + val tracker = MessageArrivalTracker() + tracker.arrivals(listOf(msg("a"))) + + val switched = mutableListOf(msg("x")) + tracker.arrivals(switched) // adopted silently + + switched += msg("y") + assertEquals(setOf("y"), tracker.arrivals(switched)) + } + @Test fun `the known set never outgrows the conversation`() { val tracker = MessageArrivalTracker()