diff --git a/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/ChatViewModel.kt b/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/ChatViewModel.kt index 11ae602285..b5213963c4 100644 --- a/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/ChatViewModel.kt +++ b/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/ChatViewModel.kt @@ -507,6 +507,10 @@ internal class ChatViewModel @Inject constructor( capabilities = resolveCapabilities(message, policy), quote = quote, sender = sender, + // Independent of the profile lookup above: the runs have to break by + // author from the first frame, and the map that names the authors lands + // after the first page does. + senderId = message.senderId?.takeIf { !message.isFromSelf }, ) } }.insertSeparators { before: ChatListItem.ContentBubble?, after: ChatListItem.ContentBubble? -> diff --git a/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/screens/components/MessageRow.kt b/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/screens/components/MessageRow.kt index d4e2062233..aab1ad74eb 100644 --- a/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/screens/components/MessageRow.kt +++ b/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/screens/components/MessageRow.kt @@ -222,6 +222,11 @@ internal fun MessageRow( // message has no item above it — a one-message transcript crashed here. older = if (index + 1 < messages.itemCount) messages.peek(index + 1) else null, ) + val runEnd = endsSenderRun( + current = item, + // index - 1 is the row drawn below — the newer message, under `reverseLayout`. + newer = if (index > 0) messages.peek(index - 1) else null, + ) Row( modifier = Modifier.fillMaxWidth(), verticalAlignment = Alignment.Bottom, @@ -235,7 +240,7 @@ internal fun MessageRow( // when the name arrived. if (showsSenderGutter && !item.isFromSelf) { Box(modifier = Modifier.requiredSize(CodeTheme.dimens.staticGrid.x6)) { - if (sender != null && runStart) { + if (sender != null && runEnd) { ContactAvatar( image = sender.picture, displayName = sender.displayName, @@ -419,17 +424,36 @@ internal fun rowGapBelow(item: ChatListItem, below: ChatListItem?, config: Separ } /** - * Whether [current] is the bubble that wears its sender's name and picture. + * Whether [current] is the bubble that wears its sender's name — the top of a run. * * `older` is the item drawn *above* [current] — under `reverseLayout` that is `peek(index + 1)`, - * the mirror of [bottomSpacingFor]'s `index - 1`. A run is labelled at its top, so the bubble that + * the mirror of [bottomSpacingFor]'s `index - 1`. A run is named at its top, so the bubble that * starts one is the one whose upper neighbour came from someone else. * + * Keyed off `authorId` rather than off the resolved `sender`, so the runs hold their shape from the + * first frame: a group's profile map arrives after the first page, and until it does every member's + * bubble has a null `sender`. + * * Pure so it can be tested without a `PagingData`: the messenger module has no `paging-testing` * dependency, and the peek belongs to the caller anyway. */ internal fun startsSenderRun(current: ChatListItem.ContentBubble, older: ChatListItem?): Boolean { - val sender = current.sender ?: return false + val author = current.authorId ?: return false val olderBubble = older as? ChatListItem.ContentBubble ?: return true - return olderBubble.sender?.userId != sender.userId + return olderBubble.authorId != author +} + +/** + * Whether [current] is the bubble that wears its sender's picture — the bottom of a run. + * + * The picture sits on the run's last message, next to the newest thing that person said, while the + * name stays on its first: the two mark the run's ends rather than stacking on one bubble. + * + * `newer` is the item drawn *below* [current] — `peek(index - 1)` under `reverseLayout`, the mirror + * of [startsSenderRun]'s `older`. + */ +internal fun endsSenderRun(current: ChatListItem.ContentBubble, newer: ChatListItem?): Boolean { + val author = current.authorId ?: return false + val newerBubble = newer as? ChatListItem.ContentBubble ?: return true + return newerBubble.authorId != author } diff --git a/apps/flipcash/features/messenger/src/test/kotlin/com/flipcash/app/messenger/internal/SenderRunTest.kt b/apps/flipcash/features/messenger/src/test/kotlin/com/flipcash/app/messenger/internal/SenderRunTest.kt index 4f1f5bc3d6..9beac0a69c 100644 --- a/apps/flipcash/features/messenger/src/test/kotlin/com/flipcash/app/messenger/internal/SenderRunTest.kt +++ b/apps/flipcash/features/messenger/src/test/kotlin/com/flipcash/app/messenger/internal/SenderRunTest.kt @@ -1,5 +1,6 @@ package com.flipcash.app.messenger.internal +import com.flipcash.app.messenger.internal.screens.components.endsSenderRun import com.flipcash.app.messenger.internal.screens.components.startsSenderRun import com.flipcash.shared.chat.models.ChatListItem import com.flipcash.shared.chat.models.SenderIdentity @@ -10,26 +11,31 @@ import kotlin.test.assertTrue import kotlin.time.Instant /** - * Which bubble in a run wears the name and the picture. + * Which bubble in a run wears the name, and which wears the picture. * - * The list is `reverseLayout`, so index 0 is the newest message at the bottom and index + 1 is the - * message drawn *above*. A run therefore starts at its oldest message — the topmost one — which is - * the one whose upper neighbour is someone else, a date separator, or nothing. + * The list is `reverseLayout`, so index 0 is the newest message at the bottom, index + 1 is the + * message drawn *above* and index - 1 the one drawn *below*. A run therefore starts at its oldest + * message — the topmost one, which wears the name — and ends at its newest, the bottom one, which + * wears the picture. */ class SenderRunTest { private val alice = SenderIdentity(userId = listOf(1), displayName = "Alice", picture = null) private val bob = SenderIdentity(userId = listOf(2), displayName = "Bob", picture = null) - private fun bubble(sender: SenderIdentity?, isFromSelf: Boolean = false) = - ChatListItem.ContentBubble( - messageId = 1L, - contentIndex = 0, - content = MessageContent.Text("hi"), - isFromSelf = isFromSelf, - timestamp = Instant.fromEpochMilliseconds(1_000), - sender = sender, - ) + private fun bubble( + sender: SenderIdentity?, + isFromSelf: Boolean = false, + senderId: List? = null, + ) = ChatListItem.ContentBubble( + messageId = 1L, + contentIndex = 0, + content = MessageContent.Text("hi"), + isFromSelf = isFromSelf, + timestamp = Instant.fromEpochMilliseconds(1_000), + sender = sender, + senderId = senderId, + ) @Test fun `a bubble with no sender never starts a run`() { @@ -62,4 +68,48 @@ class SenderRunTest { val separator = ChatListItem.DateSeparator(Instant.fromEpochMilliseconds(1_000)) assertTrue(startsSenderRun(bubble(alice), older = separator)) } + + @Test + fun `an unresolved profile still breaks the run at its sender`() { + // A group's profile map lands after the first page, so `sender` is null on every member's + // bubble to begin with. The id is what has to tell them apart until then. + val unnamedAlice = bubble(sender = null, senderId = alice.userId) + val unnamedBob = bubble(sender = null, senderId = bob.userId) + assertTrue(startsSenderRun(unnamedAlice, older = unnamedBob)) + assertFalse(startsSenderRun(unnamedAlice, older = unnamedAlice)) + assertTrue(endsSenderRun(unnamedAlice, newer = unnamedBob)) + assertFalse(endsSenderRun(unnamedAlice, newer = unnamedAlice)) + } + + @Test + fun `a bubble with no sender never ends a run`() { + assertFalse(endsSenderRun(bubble(sender = null), newer = null)) + assertFalse(endsSenderRun(bubble(sender = null, isFromSelf = true), newer = bubble(alice))) + } + + @Test + fun `the newest loaded bubble ends a run`() { + assertTrue(endsSenderRun(bubble(alice), newer = null)) + } + + @Test + fun `a bubble over a different sender ends a run`() { + assertTrue(endsSenderRun(bubble(alice), newer = bubble(bob))) + } + + @Test + fun `a bubble over the same sender continues the run`() { + assertFalse(endsSenderRun(bubble(alice), newer = bubble(alice))) + } + + @Test + fun `a bubble over one of the viewer's own ends a run`() { + assertTrue(endsSenderRun(bubble(alice), newer = bubble(sender = null, isFromSelf = true))) + } + + @Test + fun `a bubble over a date separator ends a run`() { + val separator = ChatListItem.DateSeparator(Instant.fromEpochMilliseconds(1_000)) + assertTrue(endsSenderRun(bubble(alice), newer = separator)) + } } diff --git a/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/models/ChatListItem.kt b/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/models/ChatListItem.kt index b29a07660e..538149f428 100644 --- a/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/models/ChatListItem.kt +++ b/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/models/ChatListItem.kt @@ -2,6 +2,7 @@ package com.flipcash.shared.chat.models import com.flipcash.services.models.chat.MessageContent import com.flipcash.shared.chat.MessageCapability +import com.getcode.opencode.model.core.ID import kotlin.time.Instant enum class ReceiptStatus { SENDING, SENT, READ, FAILED } @@ -48,20 +49,34 @@ sealed interface ChatListItem { * message in a DM; set only for another member's message in a group. */ val sender: SenderIdentity? = null, + /** + * The id of whoever sent this, for every incoming message — set whether or not their + * profile has resolved, and whether or not the chat attributes its bubbles. + * + * [sender] cannot stand in for it: a group's profile map arrives after the first page does, + * so early on every member's bubble has a null [sender] and two members read as one author. + */ + val senderId: ID? = null, ) : ChatListItem { + /** + * Who this bubble is attributed to, for grouping. [senderId] is the answer whenever the + * pipeline supplies it; [sender] covers a caller that builds a bubble from a resolved + * profile alone. + */ + val authorId: ID? get() = senderId ?: sender?.userId + /** * Whether [other] was sent by the same person as this bubble. * * `isFromSelf` cannot answer this in a group, where every other member's message is - * incoming alike: two members' bubbles would read as one run. [sender] is what tells them - * apart, and it is null only for the viewer's own messages and for DMs — cases - * `isFromSelf` already decides. Two bubbles whose sender profiles have not resolved yet do - * read as one author until they do. + * incoming alike: two members' bubbles would read as one run. [authorId] is what tells them + * apart, and it is null only for the viewer's own messages — a case `isFromSelf` already + * decides — and for an incoming message whose sender the backend did not name. */ fun isSameAuthorAs(other: ContentBubble): Boolean = when { isFromSelf != other.isFromSelf -> false isFromSelf -> true - else -> sender?.userId == other.sender?.userId + else -> authorId == other.authorId } /** The body a Copy or an Edit acts on, or `null` for a bubble that carries no text. */ diff --git a/apps/flipcash/shared/chat-ui/src/test/kotlin/com/flipcash/shared/chat/ui/BubblePositionTest.kt b/apps/flipcash/shared/chat-ui/src/test/kotlin/com/flipcash/shared/chat/ui/BubblePositionTest.kt new file mode 100644 index 0000000000..3634282beb --- /dev/null +++ b/apps/flipcash/shared/chat-ui/src/test/kotlin/com/flipcash/shared/chat/ui/BubblePositionTest.kt @@ -0,0 +1,88 @@ +package com.flipcash.shared.chat.ui + +import com.flipcash.services.models.chat.MessageContent +import com.flipcash.shared.chat.models.ChatListItem +import com.flipcash.shared.chat.models.SenderIdentity +import com.flipcash.shared.chat.models.SeparatorConfig +import org.junit.Test +import kotlin.test.assertEquals +import kotlin.time.Duration.Companion.seconds +import kotlin.time.Instant + +/** + * How a group's bubbles are cornered: each member's run is shaped on its own, First at the top + * through Last at the bottom, and a change of member restarts it. + * + * The list is held newest-first, as the paging snapshot holds it — index + 1 is the bubble drawn + * above, index - 1 the one drawn below. + */ +class BubblePositionTest { + + private val alice = + SenderIdentity(userId = listOf(1), displayName = "Alice", picture = null) + private val bob = SenderIdentity(userId = listOf(2), displayName = "Bob", picture = null) + private val start = Instant.fromEpochMilliseconds(1_700_000_000_000) + + private fun bubble( + id: Long, + secondsIn: Long, + sender: SenderIdentity? = null, + senderId: List? = null, + ) = ChatListItem.ContentBubble( + messageId = id, + contentIndex = 0, + content = MessageContent.Text("m$id"), + isFromSelf = false, + timestamp = start + secondsIn.seconds, + sender = sender, + senderId = senderId, + ) + + private fun positionsOf(items: List) = + items.mapIndexed { index, item -> + bubblePositionOf(index, item, items, SeparatorConfig.DayOnly) + } + + @Test + fun `a run is cornered on its own and restarts at a change of sender`() { + val items = listOf( + bubble(4, secondsIn = 30, sender = bob), + bubble(3, secondsIn = 20, sender = bob), + bubble(2, secondsIn = 10, sender = alice), + bubble(1, secondsIn = 0, sender = alice), + ) + + assertEquals( + listOf( + BubblePosition.Last, // bob, newest — bottom of his run + BubblePosition.First, // bob, top of his run + BubblePosition.Last, // alice, bottom of hers + BubblePosition.First, // alice, oldest + ), + positionsOf(items), + ) + } + + @Test + fun `two members read as two runs before their profiles resolve`() { + // What the first frame of a group transcript looks like: the profile map has not arrived, + // so no bubble carries a `sender` yet. Grouping them by the resolved profile merged every + // member into one run and cornered it as one. + val items = listOf( + bubble(4, secondsIn = 30, senderId = bob.userId), + bubble(3, secondsIn = 20, senderId = bob.userId), + bubble(2, secondsIn = 10, senderId = alice.userId), + bubble(1, secondsIn = 0, senderId = alice.userId), + ) + + assertEquals( + listOf( + BubblePosition.Last, + BubblePosition.First, + BubblePosition.Last, + BubblePosition.First, + ), + positionsOf(items), + ) + } +}