msglist: Show sender only when needed

Fixes: zulip#175

Author: gnprice

lib/model/message_list.dart

@@ -180,16 +180,20 @@ mixin _MessageSequence {
     // and date separators #173.
     final message = messages[index];
     final content = contents[index];
+    bool canShareSender;
     if (index > 0 && canShareRecipientHeader(messages[index - 1], message)) {
       assert(items.last is MessageListMessageItem);
       final prevMessageItem = items.last as MessageListMessageItem;
       assert(identical(prevMessageItem.message, messages[index - 1]));
       assert(prevMessageItem.isLastInBlock);
       prevMessageItem.isLastInBlock = false;
+      canShareSender = (prevMessageItem.message.senderId == message.senderId);
     } else {
       items.add(MessageListRecipientHeaderItem(message));
+      canShareSender = false;
     }
-    items.add(MessageListMessageItem(message, content, showSender: true, isLastInBlock: true));
+    items.add(MessageListMessageItem(message, content,
+      showSender: !canShareSender, isLastInBlock: true));
   }
 
   /// Update [items] to include markers at start and end as appropriate.
@@ -403,7 +407,8 @@ class MessageListView with ChangeNotifier, _MessageSequence {
   /// were changed, and ignores any changes to its stream or topic.
   ///
   /// TODO(#150): Handle message moves.
-  // NB that when handling message moves (#150), recipient headers may need updating.
+  // NB that when handling message moves (#150), recipient headers
+  // may need updating, and consequently showSender too.
   void maybeUpdateMessage(UpdateMessageEvent event) {
     final idx = _findMessageWithId(event.messageId);
     if (idx == -1)  {

test/model/message_list_test.dart

@@ -549,6 +549,43 @@ void main() async {
     checkNotifiedOnce();
   });
 
+  test('showSender is maintained correctly', () async {
+    // TODO(#150): This will get more complicated with message moves.
+    // Until then, we always compute this sequentially from oldest to newest.
+    // So we just need to exercise the different cases of the logic for
+    // whether the sender should be shown, but the difference between
+    // fetchInitial and maybeAddMessage etc. doesn't matter.
+
+    const timestamp = 1693602618;
+    final stream = eg.stream();
+    Message streamMessage(int id, User sender) =>
+      eg.streamMessage(id: id, sender: sender,
+        stream: stream, topic: 'foo', timestamp: timestamp);
+    Message dmMessage(int id, User sender) =>
+      eg.dmMessage(id: id, from: sender, timestamp: timestamp,
+        to: [sender.userId == eg.selfUser.userId ? eg.otherUser : eg.selfUser]);
+
+    prepare();
+    await prepareMessages(foundOldest: true, messages: [
+      streamMessage(1, eg.selfUser),  // first message, so show sender
+      streamMessage(2, eg.selfUser),  // hide sender
+      streamMessage(3, eg.otherUser), // no recipient header, but new sender
+      dmMessage(4,     eg.otherUser), // same sender, but recipient header
+    ]);
+
+    // We check showSender has the right values in [checkInvariants],
+    // but to make this test explicit:
+    check(model.items).deepEquals([
+      it()..isA<MessageListHistoryStartItem>(),
+      it()..isA<MessageListRecipientHeaderItem>(),
+      it()..isA<MessageListMessageItem>().showSender.isTrue(),
+      it()..isA<MessageListMessageItem>().showSender.isFalse(),
+      it()..isA<MessageListMessageItem>().showSender.isTrue(),
+      it()..isA<MessageListRecipientHeaderItem>(),
+      it()..isA<MessageListMessageItem>().showSender.isTrue(),
+    ]);
+  });
+
   group('canShareRecipientHeader', () {
     test('stream messages vs DMs, no share', () {
       final dmMessage = eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]);
@@ -669,15 +706,18 @@ void checkInvariants(MessageListView model) {
     check(model.items[i++]).isA<MessageListLoadingItem>();
   }
   for (int j = 0; j < model.messages.length; j++) {
+    bool isFirstInBlock = false;
     if (j == 0
         || !canShareRecipientHeader(model.messages[j-1], model.messages[j])) {
       check(model.items[i++]).isA<MessageListRecipientHeaderItem>()
         .message.identicalTo(model.messages[j]);
+      isFirstInBlock = true;
     }
     check(model.items[i++]).isA<MessageListMessageItem>()
       ..message.identicalTo(model.messages[j])
       ..content.identicalTo(model.contents[j])
-      ..showSender.isTrue()
+      ..showSender.equals(
+        isFirstInBlock || model.messages[j].senderId != model.messages[j-1].senderId)
       ..isLastInBlock.equals(
         i == model.items.length || model.items[i] is! MessageListMessageItem);
   }

test/widgets/message_list_test.dart

@@ -69,8 +69,8 @@ void main() {
 
     testWidgets('basic', (tester) async {
       await setupMessageListPage(tester, foundOldest: false,
-        messages: List.generate(100, (i) => eg.streamMessage(id: 950 + i, sender: eg.selfUser)));
-      check(itemCount(tester)).equals(101);
+        messages: List.generate(200, (i) => eg.streamMessage(id: 950 + i, sender: eg.selfUser)));
+      check(itemCount(tester)).equals(201);
 
       // Fling-scroll upward...
       await tester.fling(find.byType(MessageListPage), const Offset(0, 300), 8000);
@@ -83,7 +83,7 @@ void main() {
       await tester.pump(Duration.zero); // Allow a frame for the response to arrive.
 
       // Now we have more messages.
-      check(itemCount(tester)).equals(201);
+      check(itemCount(tester)).equals(301);
     });
 
     testWidgets('observe double-fetch glitch', (tester) async {