Skip to content

Commit b76304e

Browse files
committed
msglist: Add and manage outbox message objects in message list view
This adds some overhead in magnitude of O(1) (where the constant is the number of outbox messages in a view, expected to be small) on message event handling. 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. `addOutboxMessage` is similar to `handleMessage`. However, outbox messages do not rely on the fetched state, i.e. they can be synchronously updated when the message list view was first initialized. 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.
1 parent 8f6e253 commit b76304e

File tree

7 files changed

+867
-31
lines changed

7 files changed

+867
-31
lines changed

lib/model/message.dart

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -635,6 +635,8 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore, _OutboxMes
635635
}
636636
}
637637

638+
// TODO predict outbox message moves using propagateMode
639+
638640
for (final view in _messageListViews) {
639641
view.messagesMoved(messageMove: messageMove, messageIds: event.messageIds);
640642
}

lib/model/message_list.dart

Lines changed: 221 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,22 @@ class MessageListMessageItem extends MessageListMessageBaseItem {
6464
});
6565
}
6666

67+
/// An [OutboxMessage] to show in the message list.
68+
class MessageListOutboxMessageItem extends MessageListMessageBaseItem {
69+
@override
70+
final OutboxMessage message;
71+
@override
72+
final ZulipContent content;
73+
74+
MessageListOutboxMessageItem(
75+
this.message, {
76+
required super.showSender,
77+
required super.isLastInBlock,
78+
}) : content = ZulipContent(nodes: [
79+
ParagraphNode(links: [], nodes: [TextNode(message.content)]),
80+
]);
81+
}
82+
6783
/// Indicates the app is loading more messages at the top.
6884
// TODO(#80): or loading at the bottom, by adding a [MessageListDirection.newer]
6985
class MessageListLoadingItem extends MessageListItem {
@@ -140,14 +156,25 @@ mixin _MessageSequence {
140156
/// It exists as an optimization, to memoize the work of parsing.
141157
final List<ZulipMessageContent> contents = [];
142158

159+
/// The messages sent by the self-user, retrieved from
160+
/// [MessageStore.outboxMessages].
161+
///
162+
/// See also [items].
163+
///
164+
/// Usually this should not have that many items, so we do not anticipate
165+
/// performance issues with unoptimized O(N) iterations through this list.
166+
final List<OutboxMessage> outboxMessages = [];
167+
143168
/// The messages and their siblings in the UI, in order.
144169
///
145170
/// This has a [MessageListMessageItem] corresponding to each element
146-
/// of [messages], in order. It may have additional items interspersed
147-
/// before, between, or after the messages.
171+
/// of [messages], then a [MessageListOutboxMessageItem] corresponding to each
172+
/// element of [outboxMessages], in order.
173+
/// It may have additional items interspersed before, between, or after the
174+
/// messages.
148175
///
149-
/// This information is completely derived from [messages] and
150-
/// the flags [haveOldest], [fetchingOlder] and [fetchOlderCoolingDown].
176+
/// This information is completely derived from [messages], [outboxMessages]
177+
/// and the flags [haveOldest], [fetchingOlder] and [fetchOlderCoolingDown].
151178
/// It exists as an optimization, to memoize that computation.
152179
final QueueList<MessageListItem> items = QueueList();
153180

@@ -169,9 +196,10 @@ mixin _MessageSequence {
169196
}
170197
case MessageListRecipientHeaderItem(:var message):
171198
case MessageListDateSeparatorItem(:var message):
172-
if (message.id == null) return 1; // TODO(#1441): test
199+
if (message.id == null) return 1;
173200
return message.id! <= messageId ? -1 : 1;
174201
case MessageListMessageItem(:var message): return message.id.compareTo(messageId);
202+
case MessageListOutboxMessageItem(): return 1;
175203
}
176204
}
177205

@@ -275,10 +303,46 @@ mixin _MessageSequence {
275303
_reprocessAll();
276304
}
277305

306+
/// Append [outboxMessage] to [outboxMessages], and update derived data
307+
/// accordingly.
308+
///
309+
/// The caller is responsible for ensuring this is an appropriate thing to do
310+
/// given [narrow] and other concerns.
311+
void _addOutboxMessage(OutboxMessage outboxMessage) {
312+
assert(!outboxMessages.contains(outboxMessage));
313+
outboxMessages.add(outboxMessage);
314+
_processOutboxMessage(outboxMessages.length - 1);
315+
}
316+
317+
/// Remove the [outboxMessage] from the view.
318+
///
319+
/// Returns true if the outbox message was removed, false otherwise.
320+
bool _removeOutboxMessage(OutboxMessage outboxMessage) {
321+
if (!outboxMessages.remove(outboxMessage)) {
322+
return false;
323+
}
324+
_reprocessOutboxMessages();
325+
return true;
326+
}
327+
328+
/// Remove all outbox messages that satisfy [test] from [outboxMessages].
329+
///
330+
/// Returns true if any outbox messages were removed, false otherwise.
331+
bool _removeOutboxMessagesWhere(bool Function(OutboxMessage) test) {
332+
final count = outboxMessages.length;
333+
outboxMessages.removeWhere(test);
334+
if (outboxMessages.length == count) {
335+
return false;
336+
}
337+
_reprocessOutboxMessages();
338+
return true;
339+
}
340+
278341
/// Reset all [_MessageSequence] data, and cancel any active fetches.
279342
void _reset() {
280343
generation += 1;
281344
messages.clear();
345+
outboxMessages.clear();
282346
_fetched = false;
283347
_haveOldest = false;
284348
_fetchingOlder = false;
@@ -341,6 +405,7 @@ mixin _MessageSequence {
341405
/// The previous messages in the list must already have been processed.
342406
/// This message must already have been parsed and reflected in [contents].
343407
void _processMessage(int index) {
408+
assert(items.lastOrNull is! MessageListOutboxMessageItem);
344409
final prevMessage = index == 0 ? null : messages[index - 1];
345410
final message = messages[index];
346411
final content = contents[index];
@@ -351,6 +416,21 @@ mixin _MessageSequence {
351416
message, content, showSender: !canShareSender, isLastInBlock: true));
352417
}
353418

419+
/// Append to [items] based on the index-th outbox message.
420+
///
421+
/// All [messages] and previous messages in [outboxMessages] must already have
422+
/// been processed.
423+
void _processOutboxMessage(int index) {
424+
final prevMessage = index == 0 ? messages.lastOrNull
425+
: outboxMessages[index - 1];
426+
final message = outboxMessages[index];
427+
428+
_addItemsForMessage(message,
429+
prevMessage: prevMessage,
430+
buildItem: (bool canShareSender) => MessageListOutboxMessageItem(
431+
message, showSender: !canShareSender, isLastInBlock: true));
432+
}
433+
354434
/// Update [items] to include markers at start and end as appropriate.
355435
void _updateEndMarkers() {
356436
assert(fetched);
@@ -375,12 +455,55 @@ mixin _MessageSequence {
375455
}
376456
}
377457

378-
/// Recompute [items] from scratch, based on [messages], [contents], and flags.
458+
/// Remove items associated with [outboxMessages] from [items].
459+
///
460+
/// This is designed to be idempotent; repeated calls will not change the
461+
/// content of [items].
462+
///
463+
/// This is efficient due to the expected small size of [outboxMessages].
464+
void _removeOutboxMessageItems() {
465+
// This loop relies on the assumption that all items that follow
466+
// the last [MessageListMessageItem] are derived from outbox messages.
467+
// If there is no [MessageListMessageItem] at all,
468+
// this will end up removing end markers.
469+
while (items.isNotEmpty && items.last is! MessageListMessageItem) {
470+
items.removeLast();
471+
}
472+
assert(items.none((e) => e is MessageListOutboxMessageItem));
473+
474+
if (items.isNotEmpty) {
475+
final lastItem = items.last as MessageListMessageItem;
476+
lastItem.isLastInBlock = true;
477+
}
478+
479+
if (fetched) {
480+
// Restore the end markers in case they were removed; only do so when
481+
// [fetched] is true, since the markers are not there otherwise.
482+
_updateEndMarkers();
483+
}
484+
}
485+
486+
/// Recompute the portion of [items] derived from outbox messages,
487+
/// based on [outboxMessages] and [messages].
488+
///
489+
/// All [messages] should have been processed when this is called.
490+
void _reprocessOutboxMessages() {
491+
_removeOutboxMessageItems();
492+
for (var i = 0; i < outboxMessages.length; i++) {
493+
_processOutboxMessage(i);
494+
}
495+
}
496+
497+
/// Recompute [items] from scratch, based on [messages], [contents],
498+
/// [outboxMessages] and flags.
379499
void _reprocessAll() {
380500
items.clear();
381501
for (var i = 0; i < messages.length; i++) {
382502
_processMessage(i);
383503
}
504+
for (var i = 0; i < outboxMessages.length; i++) {
505+
_processOutboxMessage(i);
506+
}
384507
_updateEndMarkers();
385508
}
386509
}
@@ -425,7 +548,9 @@ class MessageListView with ChangeNotifier, _MessageSequence {
425548

426549
factory MessageListView.init(
427550
{required PerAccountStore store, required Narrow narrow}) {
428-
final view = MessageListView._(store: store, narrow: narrow);
551+
final view = MessageListView._(store: store, narrow: narrow)
552+
.._syncOutboxMessages()
553+
.._reprocessOutboxMessages();
429554
store.registerMessageList(view);
430555
return view;
431556
}
@@ -524,11 +649,13 @@ class MessageListView with ChangeNotifier, _MessageSequence {
524649
_adjustNarrowForTopicPermalink(result.messages.firstOrNull);
525650
store.reconcileMessages(result.messages);
526651
store.recentSenders.handleMessages(result.messages); // TODO(#824)
652+
_removeOutboxMessageItems();
527653
for (final message in result.messages) {
528654
if (_messageVisible(message)) {
529655
_addMessage(message);
530656
}
531657
}
658+
_reprocessOutboxMessages();
532659
_fetched = true;
533660
_haveOldest = result.foundOldest;
534661
_updateEndMarkers();
@@ -636,16 +763,51 @@ class MessageListView with ChangeNotifier, _MessageSequence {
636763
}
637764
}
638765

766+
bool _shouldAddOutboxMessage(OutboxMessage outboxMessage, {
767+
bool wasUnmuted = false,
768+
}) {
769+
return !outboxMessage.hidden
770+
&& narrow.containsMessage(outboxMessage)
771+
&& (_messageVisible(outboxMessage) || wasUnmuted);
772+
}
773+
774+
/// Copy outbox messages from the store, keeping the ones belong to the view.
775+
///
776+
/// This does not recompute [items]. The caller is expected to call
777+
/// [_reprocessOutboxMessages] later to keep [items] up-to-date.
778+
///
779+
/// This assumes that [outboxMessages] is empty.
780+
void _syncOutboxMessages() {
781+
assert(outboxMessages.isEmpty);
782+
for (final outboxMessage in store.outboxMessages.values) {
783+
if (_shouldAddOutboxMessage(outboxMessage)) {
784+
outboxMessages.add(outboxMessage);
785+
}
786+
}
787+
}
788+
639789
/// Add [outboxMessage] if it belongs to the view.
640790
void addOutboxMessage(OutboxMessage outboxMessage) {
641-
// TODO(#1441) implement this
791+
assert(outboxMessages.none(
792+
(message) => message.localMessageId == outboxMessage.localMessageId));
793+
if (_shouldAddOutboxMessage(outboxMessage)) {
794+
_addOutboxMessage(outboxMessage);
795+
if (fetched) {
796+
// Only need to notify listeners when [fetched] is true, because
797+
// otherwise the message list just shows a loading indicator with
798+
// no other items.
799+
notifyListeners();
800+
}
801+
}
642802
}
643803

644804
/// Remove the [outboxMessage] from the view.
645805
///
646806
/// This is a no-op if the message is not found.
647807
void removeOutboxMessage(OutboxMessage outboxMessage) {
648-
// TODO(#1441) implement this
808+
if (_removeOutboxMessage(outboxMessage)) {
809+
notifyListeners();
810+
}
649811
}
650812

651813
void handleUserTopicEvent(UserTopicEvent event) {
@@ -654,10 +816,17 @@ class MessageListView with ChangeNotifier, _MessageSequence {
654816
return;
655817

656818
case VisibilityEffect.muted:
657-
if (_removeMessagesWhere((message) =>
658-
(message is StreamMessage
659-
&& message.streamId == event.streamId
660-
&& message.topic == event.topicName))) {
819+
bool removed = _removeOutboxMessagesWhere((message) =>
820+
message is StreamOutboxMessage
821+
&& message.conversation.streamId == event.streamId
822+
&& message.conversation.topic == event.topicName);
823+
824+
removed |= _removeMessagesWhere((message) =>
825+
message is StreamMessage
826+
&& message.streamId == event.streamId
827+
&& message.topic == event.topicName);
828+
829+
if (removed) {
661830
notifyListeners();
662831
}
663832

@@ -670,6 +839,18 @@ class MessageListView with ChangeNotifier, _MessageSequence {
670839
notifyListeners();
671840
fetchInitial();
672841
}
842+
843+
outboxMessages.clear();
844+
for (final outboxMessage in store.outboxMessages.values) {
845+
if (_shouldAddOutboxMessage(
846+
outboxMessage,
847+
wasUnmuted: outboxMessage is StreamOutboxMessage
848+
&& outboxMessage.conversation.streamId == event.streamId
849+
&& outboxMessage.conversation.topic == event.topicName,
850+
)) {
851+
outboxMessages.add(outboxMessage);
852+
}
853+
}
673854
}
674855
}
675856

@@ -683,14 +864,34 @@ class MessageListView with ChangeNotifier, _MessageSequence {
683864
void handleMessageEvent(MessageEvent event) {
684865
final message = event.message;
685866
if (!narrow.containsMessage(message) || !_messageVisible(message)) {
867+
assert(event.localMessageId == null || outboxMessages.none((message) =>
868+
message.localMessageId == int.parse(event.localMessageId!, radix: 10)));
686869
return;
687870
}
688871
if (!_fetched) {
689872
// TODO mitigate this fetch/event race: save message to add to list later
690873
return;
691874
}
875+
if (outboxMessages.isEmpty) {
876+
assert(items.none((item) => item is MessageListOutboxMessageItem));
877+
_addMessage(message);
878+
notifyListeners();
879+
return;
880+
}
881+
882+
// We always remove all outbox message items
883+
// to ensure that message items come before them.
884+
_removeOutboxMessageItems();
692885
// TODO insert in middle instead, when appropriate
693886
_addMessage(message);
887+
if (event.localMessageId != null) {
888+
final localMessageId = int.parse(event.localMessageId!);
889+
// [outboxMessages] is epxected to be short, so removing the corresponding
890+
// outbox message and reprocessing them all in linear time is efficient.
891+
outboxMessages.removeWhere(
892+
(message) => message.localMessageId == localMessageId);
893+
}
894+
_reprocessOutboxMessages();
694895
notifyListeners();
695896
}
696897

@@ -722,6 +923,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
722923
// TODO in cases where we do have data to do better, do better.
723924
_reset();
724925
notifyListeners();
926+
_syncOutboxMessages();
725927
fetchInitial();
726928
}
727929

@@ -737,6 +939,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
737939
case PropagateMode.changeLater:
738940
narrow = newNarrow;
739941
_reset();
942+
_syncOutboxMessages();
740943
fetchInitial();
741944
case PropagateMode.changeOne:
742945
}
@@ -811,7 +1014,11 @@ class MessageListView with ChangeNotifier, _MessageSequence {
8111014

8121015
/// Notify listeners if the given outbox message is present in this view.
8131016
void notifyListenersIfOutboxMessagePresent(int localMessageId) {
814-
// TODO(#1441) implement this
1017+
final isAnyPresent =
1018+
outboxMessages.any((message) => message.localMessageId == localMessageId);
1019+
if (isAnyPresent) {
1020+
notifyListeners();
1021+
}
8151022
}
8161023

8171024
/// Called when the app is reassembled during debugging, e.g. for hot reload.

0 commit comments

Comments
 (0)