msglist [nfc]: Introduce MessageListRecipientHeaderItem

Instead of showing recipient headers as part of each
MessageListMessageItem, they're now their own message-list items.
At this stage, we still show one for each message.

Author: gnprice

lib/model/message_list.dart

@@ -19,12 +19,19 @@ sealed class MessageListItem {
   const MessageListItem();
 }
 
+class MessageListRecipientHeaderItem extends MessageListItem {
+  final Message message;
+
+  MessageListRecipientHeaderItem(this.message);
+}
+
 /// A message to show in the message list.
 class MessageListMessageItem extends MessageListItem {
   final Message message;
   final ZulipContent content;
+  final bool isLastInBlock;
 
-  MessageListMessageItem(this.message, this.content);
+  MessageListMessageItem(this.message, this.content, {required this.isLastInBlock});
 }
 
 /// Indicates the app is loading more messages at the top.
@@ -104,6 +111,8 @@ mixin _MessageSequence {
         switch (item.direction) {
           case MessageListDirection.older:       return -1;
         }
+      case MessageListRecipientHeaderItem(:var message):
+        return (message.id <= messageId) ? -1 : 1;
       case MessageListMessageItem(:var message): return message.id.compareTo(messageId);
     }
   }
@@ -118,7 +127,7 @@ mixin _MessageSequence {
     assert(itemIndex > -1
       && items[itemIndex] is MessageListMessageItem
       && identical((items[itemIndex] as MessageListMessageItem).message, message));
-    items[itemIndex] = MessageListMessageItem(message, content);
+    items[itemIndex] = MessageListMessageItem(isLastInBlock: true, message, content);
   }
 
   /// Append [message] to [messages], and update derived data accordingly.
@@ -163,7 +172,11 @@ mixin _MessageSequence {
     // This will get more complicated to handle the ways that messages interact
     // with the display of neighboring messages: sender headings #175,
     // recipient headings #174, and date separators #173.
-    items.add(MessageListMessageItem(messages[index], contents[index]));
+    items.add(MessageListRecipientHeaderItem(messages[index]));
+    items.add(MessageListMessageItem(
+      isLastInBlock: true,
+      messages[index], contents[index],
+    ));
   }
 
   /// Update [items] to include markers at start and end as appropriate.

lib/widgets/message_list.dart

@@ -281,6 +281,10 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
               child: Padding(
                 padding: EdgeInsets.symmetric(vertical: 16.0),
                 child: CircularProgressIndicator())); // TODO perhaps a different indicator
+          case MessageListRecipientHeaderItem():
+            final header = RecipientHeader(message: data.message);
+            return StickyHeaderItem(allowOverflow: true,
+              header: header, child: header);
           case MessageListMessageItem():
             return MessageItem(
               trailing: i == 0 ? const SizedBox(height: 8) : const SizedBox(height: 11),
@@ -376,17 +380,19 @@ class MessageItem extends StatelessWidget {
       // right than at bottom and in the recipient header: black 10% alpha,
       // vs. 88% lightness.  Assume that's an accident.
       shape: Border(
-        left: recipientBorder, bottom: restBorder, right: restBorder));
+        left: recipientBorder,
+        right: restBorder,
+        bottom: item.isLastInBlock ? restBorder : BorderSide.none,
+      ));
 
-    final recipientHeader = RecipientHeader(message: message);
     return StickyHeaderItem(
-      header: recipientHeader,
+      allowOverflow: !item.isLastInBlock,
+      header: RecipientHeader(message: message),
       child: Column(children: [
-        recipientHeader,
         DecoratedBox(
           decoration: borderDecoration,
           child: MessageWithSender(message: message, content: item.content)),
-        if (trailing != null) trailing!,
+        if (trailing != null && item.isLastInBlock) trailing!,
       ]));
 
     // Web handles the left-side recipient marker in a funky way:

test/model/message_list_test.dart

@@ -486,7 +486,7 @@ void checkInvariants(MessageListView model) {
 
   check(model).items.length.equals(
     ((model.haveOldest || model.fetchingOlder) ? 1 : 0)
-    + model.messages.length);
+    + 2 * model.messages.length);
   int i = 0;
   if (model.haveOldest) {
     check(model.items[i++]).isA<MessageListHistoryStartItem>();
@@ -495,15 +495,23 @@ void checkInvariants(MessageListView model) {
     check(model.items[i++]).isA<MessageListLoadingItem>();
   }
   for (int j = 0; j < model.messages.length; j++) {
+    check(model.items[i++]).isA<MessageListRecipientHeaderItem>()
+      .message.identicalTo(model.messages[j]);
     check(model.items[i++]).isA<MessageListMessageItem>()
       ..message.identicalTo(model.messages[j])
-      ..content.identicalTo(model.contents[j]);
+      ..content.identicalTo(model.contents[j])
+      ..isLastInBlock.isTrue();
   }
 }
 
+extension MessageListRecipientHeaderItemChecks on Subject<MessageListRecipientHeaderItem> {
+  Subject<Message> get message => has((x) => x.message, 'message');
+}
+
 extension MessageListMessageItemChecks on Subject<MessageListMessageItem> {
   Subject<Message> get message => has((x) => x.message, 'message');
   Subject<ZulipContent> get content => has((x) => x.content, 'content');
+  Subject<bool> get isLastInBlock => has((x) => x.isLastInBlock, 'isLastInBlock');
 }
 
 extension MessageListViewChecks on Subject<MessageListView> {

test/widgets/message_list_test.dart

@@ -70,7 +70,7 @@ 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(100);
+      check(itemCount(tester)).equals(200);
 
       // Fling-scroll upward...
       await tester.fling(find.byType(MessageListPage), const Offset(0, 300), 8000);
@@ -83,13 +83,13 @@ void main() {
       await tester.pump(Duration.zero); // Allow a frame for the response to arrive.
 
       // Now we have more messages.
-      check(itemCount(tester)).equals(200);
+      check(itemCount(tester)).equals(400);
     });
 
     testWidgets('observe double-fetch glitch', (tester) async {
       await setupMessageListPage(tester, foundOldest: false,
         messages: List.generate(100, (i) => eg.streamMessage(id: 950 + i, sender: eg.selfUser)));
-      check(itemCount(tester)).equals(100);
+      check(itemCount(tester)).equals(200);
 
       // Fling-scroll upward...
       await tester.fling(find.byType(MessageListPage), const Offset(0, 300), 8000);
@@ -101,18 +101,18 @@ void main() {
       for (int i = 0; i < 30; i++) {
         // Find the point in the fling where the fetch starts.
         await tester.pump(const Duration(milliseconds: 100));
-        if (itemCount(tester)! > 100) break; // The loading indicator appeared.
+        if (itemCount(tester)! > 200) break; // The loading indicator appeared.
       }
       await tester.pump(Duration.zero); // Allow a frame for the response to arrive.
-      check(itemCount(tester)).equals(200);
+      check(itemCount(tester)).equals(400);
 
       // On the next frame, we promptly fetch *another* batch.
       // This is a glitch and it'd be nicer if we didn't.
       connection.prepare(json: olderResult(anchor: 850, foundOldest: false,
         messages: List.generate(100, (i) => eg.streamMessage(id: 750 + i, sender: eg.selfUser))).toJson());
       await tester.pump(const Duration(milliseconds: 1));
       await tester.pump(Duration.zero);
-      check(itemCount(tester)).equals(300);
+      check(itemCount(tester)).equals(600);
     }, skip: true); // TODO this still reproduces manually, still needs debugging,
                     // but has become harder to reproduce in a test.
   });