msglist: Add and manage outbox message objects in message list view

This adds some overhead on message-event handling, linear in the
number of outbox messages in a view. We rely on that number being
small.

We add outboxMessages as a list independent from messages on
_MessageSequence.  Because outbox messages are not rendered
(the raw content is shown as plain text), we leave the 1-1
relationship between `messages` and `contents` unchanged.

When computing `items`, we now start to look at `outboxMessages` as
well, with the guarantee that the items related to outbox messages
always come after those for other messages.  Look for places that call
`_processOutboxMessage(int index)` for references, and the changes to
`checkInvariants` on how this affects the message list invariants.

This implements minimal support to display outbox message message item
widgets in the message list, without indicators for theirs states.
Retrieving content from failed sent requests and the full UI are
implemented in a later commit.

Co-authored-by: Chris Bobbe <[email protected]>

Author: 2 people

lib/model/message.dart

@@ -394,6 +394,8 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore, _OutboxMes
       }
     }
 
+    // TODO predict outbox message moves using propagateMode
+
     for (final view in _messageListViews) {
       view.messagesMoved(messageMove: messageMove, messageIds: event.messageIds);
     }

lib/model/message_list.dart

@@ -66,6 +66,22 @@ class MessageListMessageItem extends MessageListMessageBaseItem {
   });
 }
 
+/// An [OutboxMessage] to show in the message list.
+class MessageListOutboxMessageItem extends MessageListMessageBaseItem {
+  @override
+  final OutboxMessage message;
+  @override
+  final ZulipContent content;
+
+  MessageListOutboxMessageItem(
+    this.message, {
+    required super.showSender,
+    required super.isLastInBlock,
+  }) : content = ZulipContent(nodes: [
+    ParagraphNode(links: null, nodes: [TextNode(message.contentMarkdown)]),
+  ]);
+}
+
 /// The status of outstanding or recent fetch requests from a [MessageListView].
 enum FetchingStatus {
   /// The model has not made any fetch requests (since its last reset, if any).
@@ -158,14 +174,24 @@ mixin _MessageSequence {
   /// It exists as an optimization, to memoize the work of parsing.
   final List<ZulipMessageContent> contents = [];
 
+  /// The [OutboxMessage]s sent by the self-user, retrieved from
+  /// [MessageStore.outboxMessages].
+  ///
+  /// See also [items].
+  ///
+  /// O(N) iterations through this list are acceptable
+  /// because it won't normally have more than a few items.
+  final List<OutboxMessage> outboxMessages = [];
+
   /// The messages and their siblings in the UI, in order.
   ///
   /// This has a [MessageListMessageItem] corresponding to each element
   /// of [messages], in order.  It may have additional items interspersed
-  /// before, between, or after the messages.
+  /// before, between, or after the messages. Then, similarly,
+  /// [MessageListOutboxMessageItem]s corresponding to [outboxMessages].
   ///
-  /// This information is completely derived from [messages] and
-  /// the flags [haveOldest], [haveNewest], and [busyFetchingMore].
+  /// This information is completely derived from [messages], [outboxMessages],
+  /// and the flags [haveOldest], [haveNewest], and [busyFetchingMore].
   /// It exists as an optimization, to memoize that computation.
   ///
   /// See also [middleItem], an index which divides this list
@@ -177,11 +203,14 @@ mixin _MessageSequence {
   /// The indices 0 to before [middleItem] are the top slice of [items],
   /// and the indices from [middleItem] to the end are the bottom slice.
   ///
-  /// The top and bottom slices of [items] correspond to
-  /// the top and bottom slices of [messages] respectively.
-  /// Either the bottom slices of both [items] and [messages] are empty,
-  /// or the first item in the bottom slice of [items] is a [MessageListMessageItem]
-  /// for the first message in the bottom slice of [messages].
+  /// The top slice of [items] corresponds to the top slice of [messages].
+  /// The bottom slice of [items] corresponds to the bottom slice of [messages]
+  /// plus any [outboxMessages].
+  ///
+  /// The bottom slice will either be empty
+  /// or start with a [MessageListMessageBaseItem].
+  /// It will not start with a [MessageListDateSeparatorItem]
+  /// or a [MessageListRecipientHeaderItem].
   int middleItem = 0;
 
   int _findMessageWithId(int messageId) {
@@ -197,9 +226,10 @@ mixin _MessageSequence {
     switch (item) {
       case MessageListRecipientHeaderItem(:var message):
       case MessageListDateSeparatorItem(:var message):
-        if (message.id == null)                  return 1;  // TODO(#1441): test
+        if (message.id == null)                  return 1;
         return message.id! <= messageId ? -1 : 1;
       case MessageListMessageItem(:var message): return message.id.compareTo(messageId);
+      case MessageListOutboxMessageItem():       return 1;
     }
   }
 
@@ -316,11 +346,48 @@ mixin _MessageSequence {
     _reprocessAll();
   }
 
+  /// Append [outboxMessage] to [outboxMessages] and update derived data
+  /// accordingly.
+  ///
+  /// The caller is responsible for ensuring this is an appropriate thing to do
+  /// given [narrow] and other concerns.
+  void _addOutboxMessage(OutboxMessage outboxMessage) {
+    assert(haveNewest);
+    assert(!outboxMessages.contains(outboxMessage));
+    outboxMessages.add(outboxMessage);
+    _processOutboxMessage(outboxMessages.length - 1);
+  }
+
+  /// Remove the [outboxMessage] from the view.
+  ///
+  /// Returns true if the outbox message was removed, false otherwise.
+  bool _removeOutboxMessage(OutboxMessage outboxMessage) {
+    if (!outboxMessages.remove(outboxMessage)) {
+      return false;
+    }
+    _reprocessOutboxMessages();
+    return true;
+  }
+
+  /// Remove all outbox messages that satisfy [test] from [outboxMessages].
+  ///
+  /// Returns true if any outbox messages were removed, false otherwise.
+  bool _removeOutboxMessagesWhere(bool Function(OutboxMessage) test) {
+    final count = outboxMessages.length;
+    outboxMessages.removeWhere(test);
+    if (outboxMessages.length == count) {
+      return false;
+    }
+    _reprocessOutboxMessages();
+    return true;
+  }
+
   /// Reset all [_MessageSequence] data, and cancel any active fetches.
   void _reset() {
     generation += 1;
     messages.clear();
     middleMessage = 0;
+    outboxMessages.clear();
     _haveOldest = false;
     _haveNewest = false;
     _status = FetchingStatus.unstarted;
@@ -379,7 +446,6 @@ mixin _MessageSequence {
     assert(item.showSender == !canShareSender);
     assert(item.isLastInBlock);
     if (shouldSetMiddleItem) {
-      assert(item is MessageListMessageItem);
       middleItem = items.length;
     }
     items.add(item);
@@ -390,6 +456,7 @@ mixin _MessageSequence {
   /// The previous messages in the list must already have been processed.
   /// This message must already have been parsed and reflected in [contents].
   void _processMessage(int index) {
+    assert(items.lastOrNull is! MessageListOutboxMessageItem);
     final prevMessage = index == 0 ? null : messages[index - 1];
     final message = messages[index];
     final content = contents[index];
@@ -401,13 +468,67 @@ mixin _MessageSequence {
         message, content, showSender: !canShareSender, isLastInBlock: true));
   }
 
-  /// Recompute [items] from scratch, based on [messages], [contents], and flags.
+  /// Append to [items] based on the index-th message in [outboxMessages].
+  ///
+  /// All [messages] and previous messages in [outboxMessages] must already have
+  /// been processed.
+  void _processOutboxMessage(int index) {
+    final prevMessage = index == 0 ? messages.lastOrNull
+                                   : outboxMessages[index - 1];
+    final message = outboxMessages[index];
+
+    _addItemsForMessage(message,
+      // The first outbox message item becomes the middle item
+      // when the bottom slice of [messages] is empty.
+      shouldSetMiddleItem: index == 0 && middleMessage == messages.length,
+      prevMessage: prevMessage,
+      buildItem: (bool canShareSender) => MessageListOutboxMessageItem(
+        message, showSender: !canShareSender, isLastInBlock: true));
+  }
+
+  /// Remove items associated with [outboxMessages] from [items].
+  ///
+  /// This is designed to be idempotent; repeated calls will not change the
+  /// content of [items].
+  ///
+  /// This is efficient due to the expected small size of [outboxMessages].
+  void _removeOutboxMessageItems() {
+    // This loop relies on the assumption that all items that follow
+    // the last [MessageListMessageItem] are derived from outbox messages.
+    while (items.isNotEmpty && items.last is! MessageListMessageItem) {
+      items.removeLast();
+    }
+
+    if (items.isNotEmpty) {
+      final lastItem = items.last as MessageListMessageItem;
+      lastItem.isLastInBlock = true;
+    }
+    if (middleMessage == messages.length) middleItem = items.length;
+  }
+
+  /// Recompute the portion of [items] derived from outbox messages,
+  /// based on [outboxMessages] and [messages].
+  ///
+  /// All [messages] should have been processed when this is called.
+  void _reprocessOutboxMessages() {
+    assert(haveNewest);
+    _removeOutboxMessageItems();
+    for (var i = 0; i < outboxMessages.length; i++) {
+      _processOutboxMessage(i);
+    }
+  }
+
+  /// Recompute [items] from scratch, based on [messages], [contents],
+  /// [outboxMessages] and flags.
   void _reprocessAll() {
     items.clear();
     for (var i = 0; i < messages.length; i++) {
       _processMessage(i);
     }
     if (middleMessage == messages.length) middleItem = items.length;
+    for (var i = 0; i < outboxMessages.length; i++) {
+      _processOutboxMessage(i);
+    }
   }
 }
 
@@ -602,6 +723,11 @@ class MessageListView with ChangeNotifier, _MessageSequence {
     }
     _haveOldest = result.foundOldest;
     _haveNewest = result.foundNewest;
+
+    if (haveNewest) {
+      _syncOutboxMessagesFromStore();
+    }
+
     _setStatus(FetchingStatus.idle, was: FetchingStatus.fetchInitial);
   }
 
@@ -706,6 +832,10 @@ class MessageListView with ChangeNotifier, _MessageSequence {
           }
         }
         _haveNewest = result.foundNewest;
+
+        if (haveNewest) {
+          _syncOutboxMessagesFromStore();
+        }
       });
   }
 
@@ -770,9 +900,42 @@ class MessageListView with ChangeNotifier, _MessageSequence {
     fetchInitial();
   }
 
+  bool _shouldAddOutboxMessage(OutboxMessage outboxMessage) {
+    assert(haveNewest);
+    return !outboxMessage.hidden
+      && narrow.containsMessage(outboxMessage)
+      && _messageVisible(outboxMessage);
+  }
+
+  /// Reads [MessageStore.outboxMessages] and copies to [outboxMessages]
+  /// the ones belonging to this view.
+  ///
+  /// This should only be called when [haveNewest] is true
+  /// because outbox messages are considered newer than regular messages.
+  ///
+  /// This does not call [notifyListeners].
+  void _syncOutboxMessagesFromStore() {
+    assert(haveNewest);
+    assert(outboxMessages.isEmpty);
+    for (final outboxMessage in store.outboxMessages.values) {
+      if (_shouldAddOutboxMessage(outboxMessage)) {
+        _addOutboxMessage(outboxMessage);
+      }
+    }
+  }
+
   /// Add [outboxMessage] if it belongs to the view.
   void addOutboxMessage(OutboxMessage outboxMessage) {
-    // TODO(#1441) implement this
+    // We don't have the newest messages;
+    // we shouldn't show any outbox messages until we do.
+    if (!haveNewest) return;
+
+    assert(outboxMessages.none(
+      (message) => message.localMessageId == outboxMessage.localMessageId));
+    if (_shouldAddOutboxMessage(outboxMessage)) {
+      _addOutboxMessage(outboxMessage);
+      notifyListeners();
+    }
   }
 
   /// Remove the [outboxMessage] from the view.
@@ -781,7 +944,9 @@ class MessageListView with ChangeNotifier, _MessageSequence {
   ///
   /// This should only be called from [MessageStore.takeOutboxMessage].
   void removeOutboxMessage(OutboxMessage outboxMessage) {
-    // TODO(#1441) implement this
+    if (_removeOutboxMessage(outboxMessage)) {
+      notifyListeners();
+    }
   }
 
   void handleUserTopicEvent(UserTopicEvent event) {
@@ -790,10 +955,17 @@ class MessageListView with ChangeNotifier, _MessageSequence {
         return;
 
       case VisibilityEffect.muted:
-        if (_removeMessagesWhere((message) =>
-            (message is StreamMessage
-             && message.streamId == event.streamId
-             && message.topic == event.topicName))) {
+        bool removed = _removeMessagesWhere((message) =>
+          message is StreamMessage
+            && message.streamId == event.streamId
+            && message.topic == event.topicName);
+
+        removed |= _removeOutboxMessagesWhere((message) =>
+          message is StreamOutboxMessage
+            && message.conversation.streamId == event.streamId
+            && message.conversation.topic == event.topicName);
+
+        if (removed) {
           notifyListeners();
         }
 
@@ -819,6 +991,8 @@ class MessageListView with ChangeNotifier, _MessageSequence {
   void handleMessageEvent(MessageEvent event) {
     final message = event.message;
     if (!narrow.containsMessage(message) || !_messageVisible(message)) {
+      assert(event.localMessageId == null || outboxMessages.none((message) =>
+        message.localMessageId == int.parse(event.localMessageId!, radix: 10)));
       return;
     }
     if (!haveNewest) {
@@ -833,8 +1007,20 @@ class MessageListView with ChangeNotifier, _MessageSequence {
       //   didn't include this message.
       return;
     }
-    // TODO insert in middle instead, when appropriate
+
+    // Remove the outbox messages temporarily.
+    // We'll add them back after the new message.
+    _removeOutboxMessageItems();
+    // TODO insert in middle of [messages] instead, when appropriate
     _addMessage(message);
+    if (event.localMessageId != null) {
+      final localMessageId = int.parse(event.localMessageId!, radix: 10);
+      // [outboxMessages] is expected to be short, so removing the corresponding
+      // outbox message and reprocessing them all in linear time is efficient.
+      outboxMessages.removeWhere(
+        (message) => message.localMessageId == localMessageId);
+    }
+    _reprocessOutboxMessages();
     notifyListeners();
   }
 
@@ -955,7 +1141,11 @@ class MessageListView with ChangeNotifier, _MessageSequence {
 
   /// Notify listeners if the given outbox message is present in this view.
   void notifyListenersIfOutboxMessagePresent(int localMessageId) {
-    // TODO(#1441) implement this
+    final isAnyPresent =
+      outboxMessages.any((message) => message.localMessageId == localMessageId);
+    if (isAnyPresent) {
+      notifyListeners();
+    }
   }
 
   /// Called when the app is reassembled during debugging, e.g. for hot reload.