diff --git a/app/src/main/java/com/bitchat/android/ui/AboutSections.kt b/app/src/main/java/com/bitchat/android/ui/AboutSections.kt index 8dfb63fb..8685705e 100644 --- a/app/src/main/java/com/bitchat/android/ui/AboutSections.kt +++ b/app/src/main/java/com/bitchat/android/ui/AboutSections.kt @@ -73,6 +73,15 @@ internal val SheetRowLeadingSlot = 22.dp internal val SheetRowLeadingGutter = 16.dp internal val SheetRowHorizontal = 16.dp internal val SheetRowVertical = 13.dp + +/** + * Exact height of a people-list row. + * + * Fixed rather than derived from content: these lists reorder themselves constantly, and a row + * whose height depends on its content makes the whole card change height every time the order + * changes. Equals the leading glyph plus [SheetRowVertical] above and below. + */ +internal val SheetRowHeight = SheetRowLeadingSlot + SheetRowVertical * 2 internal val SheetRowDividerInset = SheetRowHorizontal + SheetRowLeadingSlot + SheetRowLeadingGutter /** Selection indicator sized for [SheetRowLeadingSlot]. */ internal val SheetRowSelectedDot = 12.dp diff --git a/app/src/main/java/com/bitchat/android/ui/GeohashPeopleList.kt b/app/src/main/java/com/bitchat/android/ui/GeohashPeopleList.kt index 75fe577c..bca7fe62 100644 --- a/app/src/main/java/com/bitchat/android/ui/GeohashPeopleList.kt +++ b/app/src/main/java/com/bitchat/android/ui/GeohashPeopleList.kt @@ -3,6 +3,7 @@ package com.bitchat.android.ui import androidx.compose.material.icons.Icons import androidx.compose.material.icons.filled.Email import androidx.compose.material.icons.outlined.Explore +import androidx.compose.material.icons.outlined.Person import androidx.compose.material.icons.outlined.LocationOn import android.util.Log import androidx.compose.animation.AnimatedVisibility @@ -125,7 +126,15 @@ fun GeohashPeopleList( viewModel.isPersonTeleported(person.id) } - val (teleportedPeople, localPeople) = orderedPeople.partition { personTeleported(it) } + // Anonymous participants form their own trailing section rather than a tail on each + // of the others. A busy geohash is mostly anons, and splitting them across "on + // location" and "teleported in" pushed the few recognisable names out of view twice + // over. Self is never grouped as an anon even when unnamed — you always want to find + // yourself where you actually are. + val isSelf: (GeoPerson) -> Boolean = { myHex != null && it.id == myHex } + val namedPeople = orderedPeople.filter { isSelf(it) || !it.isAnonymous() } + val anonPeople = orderedPeople.filter { !isSelf(it) && it.isAnonymous() } + val (teleportedPeople, localPeople) = namedPeople.partition { personTeleported(it) } @Composable fun personRow(person: GeoPerson) { @@ -164,6 +173,17 @@ fun GeohashPeopleList( ) PeopleCard(people = teleportedPeople, row = { personRow(it) }) } + + if (anonPeople.isNotEmpty()) { + SheetIconSectionHeader( + icon = Icons.Outlined.Person, + title = stringResource(R.string.section_anonymous), + modifier = Modifier.padding( + top = if (localPeople.isNotEmpty() || teleportedPeople.isNotEmpty()) 20.dp else 0.dp + ) + ) + PeopleCard(people = anonPeople, capped = true, row = { personRow(it) }) + } } } } @@ -183,26 +203,29 @@ internal fun GeoPerson.isAnonymous(): Boolean { } /** - * One grouped card of people, with a cap on how many anonymous participants are shown. + * One grouped card of people. * - * A popular geohash can hold dozens of anons, which pushed everyone worth recognising off screen and - * turned the sheet into a wall of near-identical rows. Named participants are always listed in full; - * anons are trimmed to [MaxVisibleAnons], and the overflow is collapsed behind a count. The last - * visible anon fades out under a gradient so the truncation is legible as truncation rather than - * looking like the list simply ended. + * When [capped] the list is trimmed to [MaxVisibleAnons] rows and the remainder is collapsed behind + * a count. That matters for the anonymous section: a popular geohash can hold dozens of anons, which + * pushed everyone worth recognising off screen and turned the sheet into a wall of near-identical + * rows. + * + * The capped card is a **fixed height** — [MaxVisibleAnons] rows plus the overflow line, always, + * regardless of how many anons are currently present beyond the cap. Anons join and leave a busy + * geohash constantly, and sizing to the live count made the card grow and shrink under the reader + * every few seconds. */ @Composable private fun PeopleCard( people: List, - row: @Composable (GeoPerson) -> Unit + row: @Composable (GeoPerson) -> Unit, + capped: Boolean = false ) { val palette = LocalBitchatPalette.current - val named = people.filterNot { it.isAnonymous() } - val anons = people.filter { it.isAnonymous() } - val visibleAnons = anons.take(MaxVisibleAnons) - val hiddenAnonCount = anons.size - visibleAnons.size - val visible = named + visibleAnons + val visible = if (capped) people.take(MaxVisibleAnons) else people + val hiddenCount = people.size - visible.size + val isTrimmed = capped && people.size > MaxVisibleAnons Surface( modifier = Modifier @@ -213,50 +236,57 @@ private fun PeopleCard( shape = AboutCardShape ) { Column { - AnimatedRowColumn(items = visible, key = { it.id }) { index, person -> - Column { - if (index > 0) SheetCardDivider() - if (hiddenAnonCount > 0 && index == visible.lastIndex) { - // Fade only the final row, so the gradient reads as "the list continues" - // rather than dimming content that is still meant to be read. - Box { - row(person) - Box( - modifier = Modifier - .matchParentSize() - .background( - Brush.verticalGradient( - listOf( - palette.surface.copy(alpha = 0f), - palette.surface.copy(alpha = 0.85f) + Box( + // Reserve the full capped height up front so the card cannot resize as anons + // churn. Rows are a fixed height, so this is exact rather than an estimate. + modifier = if (isTrimmed) { + Modifier.height(SheetRowHeight * MaxVisibleAnons) + } else { + Modifier + } + ) { + AnimatedRowColumn(items = visible, key = { it.id }) { index, person -> + Column { + if (index > 0) SheetCardDivider() + if (isTrimmed && index == visible.lastIndex) { + // Fade only the final row, so the gradient reads as "the list + // continues" rather than dimming content still meant to be read. + Box { + row(person) + Box( + modifier = Modifier + .matchParentSize() + .background( + Brush.verticalGradient( + listOf( + palette.surface.copy(alpha = 0f), + palette.surface.copy(alpha = 0.85f) + ) ) ) - ) - ) + ) + } + } else { + row(person) } - } else { - row(person) } } } - AnimatedVisibility( - visible = hiddenAnonCount > 0, - enter = fadeIn(tween(BitchatMotion.STANDARD_MS)), - exit = fadeOut(tween(BitchatMotion.QUICK_MS)) - ) { - Text( - text = stringResource(R.string.people_n_more, hiddenAnonCount), + if (isTrimmed) { + // Always laid out when trimmed, so the count changing never moves anything. + // Only the number itself animates. + AnimatedCountLabel( + count = hiddenCount, + text = stringResource(R.string.people_n_more, hiddenCount), fontFamily = FontFamily.Monospace, fontSize = 11.sp, color = palette.textTertiary, - modifier = Modifier - .fillMaxWidth() - .padding( - start = SheetRowHorizontal, - end = SheetRowHorizontal, - bottom = SheetRowVertical - ) + modifier = Modifier.padding( + start = SheetRowHorizontal, + end = SheetRowHorizontal, + bottom = SheetRowVertical + ) ) } } @@ -298,8 +328,11 @@ private fun GeohashPersonItem( Row( modifier = Modifier .fillMaxWidth() + // Exact height, not padding: a row that sizes to its content makes the card change + // height whenever the list reorders. + .height(SheetRowHeight) .clickable(onClick = onTap) - .padding(horizontal = SheetRowHorizontal, vertical = SheetRowVertical), + .padding(horizontal = SheetRowHorizontal), verticalAlignment = Alignment.CenterVertically ) { Box( 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 aac25e91..31db2ef1 100644 --- a/app/src/main/java/com/bitchat/android/ui/MessageComponents.kt +++ b/app/src/main/java/com/bitchat/android/ui/MessageComponents.kt @@ -72,6 +72,7 @@ import com.bitchat.android.ui.theme.BitchatMotion import com.bitchat.android.ui.theme.LocalBitchatPalette import com.bitchat.android.ui.theme.MessageBodyTextStyle import com.bitchat.android.ui.theme.MessageSenderTextStyle +import kotlinx.coroutines.delay import java.text.SimpleDateFormat import java.util.Locale @@ -106,6 +107,14 @@ private val MessagePlacementSpec: FiniteAnimationSpec = spring( /** Removals are not worth dwelling on. */ private val MessageFadeOutSpec: FiniteAnimationSpec = tween(BitchatMotion.QUICK_MS) +/** + * How long placement animation stays armed after the list gains or loses a message. + * + * Comfortably longer than [MessagePlacementSpec] takes to settle, so an arrival's push is never cut + * short. + */ +private const val PlacementArmWindowMs = 600L + /** * Above this many simultaneous arrivals, entry animations are skipped. * @@ -226,6 +235,24 @@ fun MessagesList( arrivalTracker.arrivals(messages) } + // Placement animation exists to soften insertions and removals. But *any* relayout moves every + // item — the keyboard opening behind a bottom sheet, that sheet closing again, the composer + // 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) { + val previous = previousMessageCount + previousMessageCount = messages.size + // Skip the first composition: the list settling into its initial padding is not a change + // worth animating. + if (previous == null || previous == messages.size) return@LaunchedEffect + placementArmed = true + delay(PlacementArmWindowMs) + placementArmed = false + } + val layoutDirection = LocalLayoutDirection.current LazyColumn( state = listState, @@ -294,7 +321,7 @@ fun MessagesList( // Entry fade is handled by entryModifier, together with the slide, so the // two cannot drift out of step. fadeInSpec = null, - placementSpec = MessagePlacementSpec, + placementSpec = if (placementArmed) MessagePlacementSpec else null, fadeOutSpec = MessageFadeOutSpec ) .then(entryModifier) diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 275c5f79..357eedbd 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -316,6 +316,7 @@ People (%1$d) On location Teleported in + Anonymous %1$d more\u2026 Grant location permission diff --git a/app/src/test/java/com/bitchat/android/ui/GeohashAnonOrderingTest.kt b/app/src/test/java/com/bitchat/android/ui/GeohashAnonOrderingTest.kt index 8a00d1c8..342e03b0 100644 --- a/app/src/test/java/com/bitchat/android/ui/GeohashAnonOrderingTest.kt +++ b/app/src/test/java/com/bitchat/android/ui/GeohashAnonOrderingTest.kt @@ -102,6 +102,78 @@ class GeohashAnonOrderingTest { assertEquals(12 - MaxVisibleAnons, anons.size - visible.size) } + // MARK: - Sectioning + + /** Mirrors the grouping in GeohashPeopleList: self is never treated as an anon. */ + private fun sections(people: List, myId: String?): Triple, List, List> { + val isSelf: (GeoPerson) -> Boolean = { myId != null && it.id == myId } + val named = people.filter { isSelf(it) || !it.isAnonymous() } + val anons = people.filter { !isSelf(it) && it.isAnonymous() } + return Triple( + named.map { it.displayName }, + anons.map { it.displayName }, + people.map { it.displayName } + ) + } + + @Test + fun `anons are grouped out of the named sections entirely`() { + val people = listOf(person("alice"), person("anon1"), person("bob"), person("anon2")) + val (named, anons, _) = sections(people, myId = null) + + assertEquals(listOf("alice", "bob"), named) + assertEquals(listOf("anon1", "anon2"), anons) + } + + @Test + fun `self stays in the named sections even when unnamed`() { + // You always want to find yourself where you actually are, not buried in the anon section. + val me = person("anon") + val people = listOf(me, person("alice"), person("anon2")) + val (named, anons, _) = sections(people, myId = me.id) + + assertTrue("self must not be grouped as an anon", named.contains("anon")) + assertFalse(anons.contains("anon")) + assertEquals(listOf("anon2"), anons) + } + + @Test + fun `a list of only anons yields no named section`() { + val people = (1..4).map { person("anon$it") } + val (named, anons, _) = sections(people, myId = null) + + assertTrue(named.isEmpty()) + assertEquals(4, anons.size) + } + + // MARK: - Stable length + + @Test + fun `a trimmed anon list always renders exactly the cap`() { + // The reserved height is MaxVisibleAnons rows whenever trimmed, so the card cannot resize + // as anons churn above the cap. + for (total in listOf(MaxVisibleAnons + 1, MaxVisibleAnons + 7, MaxVisibleAnons + 40)) { + val anons = (1..total).map { person("anon$it") } + val visible = anons.take(MaxVisibleAnons) + assertEquals( + "row count must not depend on how many anons are present beyond the cap", + MaxVisibleAnons, + visible.size + ) + assertEquals(total - MaxVisibleAnons, anons.size - visible.size) + } + } + + @Test + fun `reordering never changes how many rows are rendered`() { + val anons = (1..9).map { person("anon$it", secondsAgo = it.toLong()) } + val byRecency = anons.sortedByDescending { it.lastSeen }.take(MaxVisibleAnons) + val reversed = anons.sortedBy { it.lastSeen }.take(MaxVisibleAnons) + + assertEquals(byRecency.size, reversed.size) + assertEquals(MaxVisibleAnons, byRecency.size) + } + @Test fun `a short anon list is shown in full with nothing hidden`() { val people = listOf(person("alice"), person("anon1"), person("anon2"))