Skip to content

Commit 5579e00

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 b0dc888 commit 5579e00

File tree

7 files changed

+885
-31
lines changed

7 files changed

+885
-31
lines changed

lib/model/message.dart

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -394,6 +394,8 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore, _OutboxMes
394394
}
395395
}
396396

397+
// TODO predict outbox message moves using propagateMode
398+
397399
for (final view in _messageListViews) {
398400
view.messagesMoved(messageMove: messageMove, messageIds: event.messageIds);
399401
}

lib/model/message_list.dart

Lines changed: 218 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.contentMarkdown)]),
80+
]);
81+
}
82+
6783
/// The sequence of messages in a message list, and how to display them.
6884
///
6985
/// This comprises much of the guts of [MessageListView].
@@ -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
///
153180
/// See also [middleItem], an index which divides this list
@@ -164,6 +191,7 @@ mixin _MessageSequence {
164191
/// Either the bottom slices of both [items] and [messages] are empty,
165192
/// or the first item in the bottom slice of [items] is a [MessageListMessageItem]
166193
/// for the first message in the bottom slice of [messages].
194+
// TODO(#1453) update this in the context of outbox messages.
167195
int middleItem = 0;
168196

169197
int _findMessageWithId(int messageId) {
@@ -179,9 +207,10 @@ mixin _MessageSequence {
179207
switch (item) {
180208
case MessageListRecipientHeaderItem(:var message):
181209
case MessageListDateSeparatorItem(:var message):
182-
if (message.id == null) return 1; // TODO(#1441): test
210+
if (message.id == null) return 1;
183211
return message.id! <= messageId ? -1 : 1;
184212
case MessageListMessageItem(:var message): return message.id.compareTo(messageId);
213+
case MessageListOutboxMessageItem(): return 1;
185214
}
186215
}
187216

@@ -298,10 +327,46 @@ mixin _MessageSequence {
298327
_reprocessAll();
299328
}
300329

330+
/// Append [outboxMessage] to [outboxMessages], and update derived data
331+
/// accordingly.
332+
///
333+
/// The caller is responsible for ensuring this is an appropriate thing to do
334+
/// given [narrow] and other concerns.
335+
void _addOutboxMessage(OutboxMessage outboxMessage) {
336+
assert(!outboxMessages.contains(outboxMessage));
337+
outboxMessages.add(outboxMessage);
338+
_processOutboxMessage(outboxMessages.length - 1);
339+
}
340+
341+
/// Remove the [outboxMessage] from the view.
342+
///
343+
/// Returns true if the outbox message was removed, false otherwise.
344+
bool _removeOutboxMessage(OutboxMessage outboxMessage) {
345+
if (!outboxMessages.remove(outboxMessage)) {
346+
return false;
347+
}
348+
_reprocessOutboxMessages();
349+
return true;
350+
}
351+
352+
/// Remove all outbox messages that satisfy [test] from [outboxMessages].
353+
///
354+
/// Returns true if any outbox messages were removed, false otherwise.
355+
bool _removeOutboxMessagesWhere(bool Function(OutboxMessage) test) {
356+
final count = outboxMessages.length;
357+
outboxMessages.removeWhere(test);
358+
if (outboxMessages.length == count) {
359+
return false;
360+
}
361+
_reprocessOutboxMessages();
362+
return true;
363+
}
364+
301365
/// Reset all [_MessageSequence] data, and cancel any active fetches.
302366
void _reset() {
303367
generation += 1;
304368
messages.clear();
369+
outboxMessages.clear();
305370
middleMessage = 0;
306371
_fetched = false;
307372
_haveOldest = false;
@@ -374,6 +439,7 @@ mixin _MessageSequence {
374439
/// The previous messages in the list must already have been processed.
375440
/// This message must already have been parsed and reflected in [contents].
376441
void _processMessage(int index) {
442+
assert(items.lastOrNull is! MessageListOutboxMessageItem);
377443
final prevMessage = index == 0 ? null : messages[index - 1];
378444
final message = messages[index];
379445
final content = contents[index];
@@ -385,13 +451,67 @@ mixin _MessageSequence {
385451
message, content, showSender: !canShareSender, isLastInBlock: true));
386452
}
387453

388-
/// Recompute [items] from scratch, based on [messages], [contents], and flags.
454+
/// Append to [items] based on the index-th outbox message.
455+
///
456+
/// All [messages] and previous messages in [outboxMessages] must already have
457+
/// been processed.
458+
void _processOutboxMessage(int index) {
459+
final prevMessage = index == 0 ? messages.lastOrNull
460+
: outboxMessages[index - 1];
461+
final message = outboxMessages[index];
462+
463+
_addItemsForMessage(message,
464+
isMiddleMessage: false,
465+
prevMessage: prevMessage,
466+
buildItem: (bool canShareSender) => MessageListOutboxMessageItem(
467+
message, showSender: !canShareSender, isLastInBlock: true));
468+
}
469+
470+
/// Remove items associated with [outboxMessages] from [items].
471+
///
472+
/// This is designed to be idempotent; repeated calls will not change the
473+
/// content of [items].
474+
///
475+
/// This is efficient due to the expected small size of [outboxMessages].
476+
void _removeOutboxMessageItems() {
477+
// This loop relies on the assumption that all items that follow
478+
// the last [MessageListMessageItem] are derived from outbox messages.
479+
// If there is no [MessageListMessageItem] at all,
480+
// this will end up removing end markers.
481+
while (items.isNotEmpty && items.last is! MessageListMessageItem) {
482+
items.removeLast();
483+
assert(items.length >= middleItem);
484+
}
485+
assert(items.none((e) => e is MessageListOutboxMessageItem));
486+
487+
if (items.isNotEmpty) {
488+
final lastItem = items.last as MessageListMessageItem;
489+
lastItem.isLastInBlock = true;
490+
}
491+
}
492+
493+
/// Recompute the portion of [items] derived from outbox messages,
494+
/// based on [outboxMessages] and [messages].
495+
///
496+
/// All [messages] should have been processed when this is called.
497+
void _reprocessOutboxMessages() {
498+
_removeOutboxMessageItems();
499+
for (var i = 0; i < outboxMessages.length; i++) {
500+
_processOutboxMessage(i);
501+
}
502+
}
503+
504+
/// Recompute [items] from scratch, based on [messages], [contents],
505+
/// [outboxMessages] and flags.
389506
void _reprocessAll() {
390507
items.clear();
391508
for (var i = 0; i < messages.length; i++) {
392509
_processMessage(i);
393510
}
394511
if (middleMessage == messages.length) middleItem = items.length;
512+
for (var i = 0; i < outboxMessages.length; i++) {
513+
_processOutboxMessage(i);
514+
}
395515
}
396516
}
397517

@@ -435,7 +555,9 @@ class MessageListView with ChangeNotifier, _MessageSequence {
435555

436556
factory MessageListView.init(
437557
{required PerAccountStore store, required Narrow narrow}) {
438-
final view = MessageListView._(store: store, narrow: narrow);
558+
final view = MessageListView._(store: store, narrow: narrow)
559+
.._syncOutboxMessages()
560+
.._reprocessOutboxMessages();
439561
store.registerMessageList(view);
440562
return view;
441563
}
@@ -538,12 +660,14 @@ class MessageListView with ChangeNotifier, _MessageSequence {
538660
store.recentSenders.handleMessages(result.messages); // TODO(#824)
539661

540662
// We'll make the bottom slice start at the last visible message, if any.
663+
_removeOutboxMessageItems();
541664
for (final message in result.messages) {
542665
if (!_messageVisible(message)) continue;
543666
middleMessage = messages.length;
544667
_addMessage(message);
545668
// Now [middleMessage] is the last message (the one just added).
546669
}
670+
_reprocessOutboxMessages();
547671
_fetched = true;
548672
_haveOldest = result.foundOldest;
549673
notifyListeners();
@@ -647,9 +771,42 @@ class MessageListView with ChangeNotifier, _MessageSequence {
647771
}
648772
}
649773

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

655812
/// Remove the [outboxMessage] from the view.
@@ -658,7 +815,9 @@ class MessageListView with ChangeNotifier, _MessageSequence {
658815
///
659816
/// This should only be called from [MessageStore.takeOutboxMessage].
660817
void removeOutboxMessage(OutboxMessage outboxMessage) {
661-
// TODO(#1441) implement this
818+
if (_removeOutboxMessage(outboxMessage)) {
819+
notifyListeners();
820+
}
662821
}
663822

664823
void handleUserTopicEvent(UserTopicEvent event) {
@@ -667,10 +826,17 @@ class MessageListView with ChangeNotifier, _MessageSequence {
667826
return;
668827

669828
case VisibilityEffect.muted:
670-
if (_removeMessagesWhere((message) =>
671-
(message is StreamMessage
672-
&& message.streamId == event.streamId
673-
&& message.topic == event.topicName))) {
829+
bool removed = _removeOutboxMessagesWhere((message) =>
830+
message is StreamOutboxMessage
831+
&& message.conversation.streamId == event.streamId
832+
&& message.conversation.topic == event.topicName);
833+
834+
removed |= _removeMessagesWhere((message) =>
835+
message is StreamMessage
836+
&& message.streamId == event.streamId
837+
&& message.topic == event.topicName);
838+
839+
if (removed) {
674840
notifyListeners();
675841
}
676842

@@ -683,6 +849,18 @@ class MessageListView with ChangeNotifier, _MessageSequence {
683849
notifyListeners();
684850
fetchInitial();
685851
}
852+
853+
outboxMessages.clear();
854+
for (final outboxMessage in store.outboxMessages.values) {
855+
if (_shouldAddOutboxMessage(
856+
outboxMessage,
857+
wasUnmuted: outboxMessage is StreamOutboxMessage
858+
&& outboxMessage.conversation.streamId == event.streamId
859+
&& outboxMessage.conversation.topic == event.topicName,
860+
)) {
861+
outboxMessages.add(outboxMessage);
862+
}
863+
}
686864
}
687865
}
688866

@@ -696,14 +874,34 @@ class MessageListView with ChangeNotifier, _MessageSequence {
696874
void handleMessageEvent(MessageEvent event) {
697875
final message = event.message;
698876
if (!narrow.containsMessage(message) || !_messageVisible(message)) {
877+
assert(event.localMessageId == null || outboxMessages.none((message) =>
878+
message.localMessageId == int.parse(event.localMessageId!, radix: 10)));
699879
return;
700880
}
701881
if (!_fetched) {
702882
// TODO mitigate this fetch/event race: save message to add to list later
703883
return;
704884
}
885+
if (outboxMessages.isEmpty) {
886+
assert(items.none((item) => item is MessageListOutboxMessageItem));
887+
_addMessage(message);
888+
notifyListeners();
889+
return;
890+
}
891+
892+
// We always remove all outbox message items
893+
// to ensure that message items come before them.
894+
_removeOutboxMessageItems();
705895
// TODO insert in middle instead, when appropriate
706896
_addMessage(message);
897+
if (event.localMessageId != null) {
898+
final localMessageId = int.parse(event.localMessageId!);
899+
// [outboxMessages] is epxected to be short, so removing the corresponding
900+
// outbox message and reprocessing them all in linear time is efficient.
901+
outboxMessages.removeWhere(
902+
(message) => message.localMessageId == localMessageId);
903+
}
904+
_reprocessOutboxMessages();
707905
notifyListeners();
708906
}
709907

@@ -735,6 +933,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
735933
// TODO in cases where we do have data to do better, do better.
736934
_reset();
737935
notifyListeners();
936+
_syncOutboxMessages();
738937
fetchInitial();
739938
}
740939

@@ -750,6 +949,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
750949
case PropagateMode.changeLater:
751950
narrow = newNarrow;
752951
_reset();
952+
_syncOutboxMessages();
753953
fetchInitial();
754954
case PropagateMode.changeOne:
755955
}
@@ -824,7 +1024,11 @@ class MessageListView with ChangeNotifier, _MessageSequence {
8241024

8251025
/// Notify listeners if the given outbox message is present in this view.
8261026
void notifyListenersIfOutboxMessagePresent(int localMessageId) {
827-
// TODO(#1441) implement this
1027+
final isAnyPresent =
1028+
outboxMessages.any((message) => message.localMessageId == localMessageId);
1029+
if (isAnyPresent) {
1030+
notifyListeners();
1031+
}
8281032
}
8291033

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

0 commit comments

Comments
 (0)