Skip to content

Commit d2f378a

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 4da7d21 commit d2f378a

File tree

7 files changed

+858
-31
lines changed

7 files changed

+858
-31
lines changed

lib/model/message.dart

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -696,6 +696,8 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore, _OutboxMes
696696
}
697697
}
698698

699+
// TODO predict outbox message moves using propagateMode
700+
699701
for (final view in _messageListViews) {
700702
view.messagesMoved(messageMove: messageMove, messageIds: event.messageIds);
701703
}

lib/model/message_list.dart

Lines changed: 215 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
/// The sequence of messages in a message list, and how to display them.
6884
///
6985
/// This comprises much of the guts of [MessageListView].
@@ -125,14 +141,25 @@ mixin _MessageSequence {
125141
/// It exists as an optimization, to memoize the work of parsing.
126142
final List<ZulipMessageContent> contents = [];
127143

144+
/// The messages sent by the self-user, retrieved from
145+
/// [MessageStore.outboxMessages].
146+
///
147+
/// See also [items].
148+
///
149+
/// Usually this should not have that many items, so we do not anticipate
150+
/// performance issues with unoptimized O(N) iterations through this list.
151+
final List<OutboxMessage> outboxMessages = [];
152+
128153
/// The messages and their siblings in the UI, in order.
129154
///
130155
/// This has a [MessageListMessageItem] corresponding to each element
131-
/// of [messages], in order. It may have additional items interspersed
132-
/// before, between, or after the messages.
156+
/// of [messages], then a [MessageListOutboxMessageItem] corresponding to each
157+
/// element of [outboxMessages], in order.
158+
/// It may have additional items interspersed before, between, or after the
159+
/// messages.
133160
///
134-
/// This information is completely derived from [messages] and
135-
/// the flags [haveOldest], [fetchingOlder] and [fetchOlderCoolingDown].
161+
/// This information is completely derived from [messages], [outboxMessages]
162+
/// and the flags [haveOldest], [fetchingOlder] and [fetchOlderCoolingDown].
136163
/// It exists as an optimization, to memoize that computation.
137164
final QueueList<MessageListItem> items = QueueList();
138165

@@ -149,9 +176,10 @@ mixin _MessageSequence {
149176
switch (item) {
150177
case MessageListRecipientHeaderItem(:var message):
151178
case MessageListDateSeparatorItem(:var message):
152-
if (message.id == null) return 1; // TODO(#1441): test
179+
if (message.id == null) return 1;
153180
return message.id! <= messageId ? -1 : 1;
154181
case MessageListMessageItem(:var message): return message.id.compareTo(messageId);
182+
case MessageListOutboxMessageItem(): return 1;
155183
}
156184
}
157185

@@ -255,10 +283,46 @@ mixin _MessageSequence {
255283
_reprocessAll();
256284
}
257285

286+
/// Append [outboxMessage] to [outboxMessages], and update derived data
287+
/// accordingly.
288+
///
289+
/// The caller is responsible for ensuring this is an appropriate thing to do
290+
/// given [narrow] and other concerns.
291+
void _addOutboxMessage(OutboxMessage outboxMessage) {
292+
assert(!outboxMessages.contains(outboxMessage));
293+
outboxMessages.add(outboxMessage);
294+
_processOutboxMessage(outboxMessages.length - 1);
295+
}
296+
297+
/// Remove the [outboxMessage] from the view.
298+
///
299+
/// Returns true if the outbox message was removed, false otherwise.
300+
bool _removeOutboxMessage(OutboxMessage outboxMessage) {
301+
if (!outboxMessages.remove(outboxMessage)) {
302+
return false;
303+
}
304+
_reprocessOutboxMessages();
305+
return true;
306+
}
307+
308+
/// Remove all outbox messages that satisfy [test] from [outboxMessages].
309+
///
310+
/// Returns true if any outbox messages were removed, false otherwise.
311+
bool _removeOutboxMessagesWhere(bool Function(OutboxMessage) test) {
312+
final count = outboxMessages.length;
313+
outboxMessages.removeWhere(test);
314+
if (outboxMessages.length == count) {
315+
return false;
316+
}
317+
_reprocessOutboxMessages();
318+
return true;
319+
}
320+
258321
/// Reset all [_MessageSequence] data, and cancel any active fetches.
259322
void _reset() {
260323
generation += 1;
261324
messages.clear();
325+
outboxMessages.clear();
262326
_fetched = false;
263327
_haveOldest = false;
264328
_fetchingOlder = false;
@@ -321,6 +385,7 @@ mixin _MessageSequence {
321385
/// The previous messages in the list must already have been processed.
322386
/// This message must already have been parsed and reflected in [contents].
323387
void _processMessage(int index) {
388+
assert(items.lastOrNull is! MessageListOutboxMessageItem);
324389
final prevMessage = index == 0 ? null : messages[index - 1];
325390
final message = messages[index];
326391
final content = contents[index];
@@ -331,12 +396,64 @@ mixin _MessageSequence {
331396
message, content, showSender: !canShareSender, isLastInBlock: true));
332397
}
333398

334-
/// Recompute [items] from scratch, based on [messages], [contents], and flags.
399+
/// Append to [items] based on the index-th outbox message.
400+
///
401+
/// All [messages] and previous messages in [outboxMessages] must already have
402+
/// been processed.
403+
void _processOutboxMessage(int index) {
404+
final prevMessage = index == 0 ? messages.lastOrNull
405+
: outboxMessages[index - 1];
406+
final message = outboxMessages[index];
407+
408+
_addItemsForMessage(message,
409+
prevMessage: prevMessage,
410+
buildItem: (bool canShareSender) => MessageListOutboxMessageItem(
411+
message, showSender: !canShareSender, isLastInBlock: true));
412+
}
413+
414+
/// Remove items associated with [outboxMessages] from [items].
415+
///
416+
/// This is designed to be idempotent; repeated calls will not change the
417+
/// content of [items].
418+
///
419+
/// This is efficient due to the expected small size of [outboxMessages].
420+
void _removeOutboxMessageItems() {
421+
// This loop relies on the assumption that all items that follow
422+
// the last [MessageListMessageItem] are derived from outbox messages.
423+
// If there is no [MessageListMessageItem] at all,
424+
// this will end up removing end markers.
425+
while (items.isNotEmpty && items.last is! MessageListMessageItem) {
426+
items.removeLast();
427+
}
428+
assert(items.none((e) => e is MessageListOutboxMessageItem));
429+
430+
if (items.isNotEmpty) {
431+
final lastItem = items.last as MessageListMessageItem;
432+
lastItem.isLastInBlock = true;
433+
}
434+
}
435+
436+
/// Recompute the portion of [items] derived from outbox messages,
437+
/// based on [outboxMessages] and [messages].
438+
///
439+
/// All [messages] should have been processed when this is called.
440+
void _reprocessOutboxMessages() {
441+
_removeOutboxMessageItems();
442+
for (var i = 0; i < outboxMessages.length; i++) {
443+
_processOutboxMessage(i);
444+
}
445+
}
446+
447+
/// Recompute [items] from scratch, based on [messages], [contents],
448+
/// [outboxMessages] and flags.
335449
void _reprocessAll() {
336450
items.clear();
337451
for (var i = 0; i < messages.length; i++) {
338452
_processMessage(i);
339453
}
454+
for (var i = 0; i < outboxMessages.length; i++) {
455+
_processOutboxMessage(i);
456+
}
340457
}
341458
}
342459

@@ -380,7 +497,9 @@ class MessageListView with ChangeNotifier, _MessageSequence {
380497

381498
factory MessageListView.init(
382499
{required PerAccountStore store, required Narrow narrow}) {
383-
final view = MessageListView._(store: store, narrow: narrow);
500+
final view = MessageListView._(store: store, narrow: narrow)
501+
.._syncOutboxMessages()
502+
.._reprocessOutboxMessages();
384503
store.registerMessageList(view);
385504
return view;
386505
}
@@ -479,11 +598,13 @@ class MessageListView with ChangeNotifier, _MessageSequence {
479598
_adjustNarrowForTopicPermalink(result.messages.firstOrNull);
480599
store.reconcileMessages(result.messages);
481600
store.recentSenders.handleMessages(result.messages); // TODO(#824)
601+
_removeOutboxMessageItems();
482602
for (final message in result.messages) {
483603
if (_messageVisible(message)) {
484604
_addMessage(message);
485605
}
486606
}
607+
_reprocessOutboxMessages();
487608
_fetched = true;
488609
_haveOldest = result.foundOldest;
489610
notifyListeners();
@@ -587,9 +708,42 @@ class MessageListView with ChangeNotifier, _MessageSequence {
587708
}
588709
}
589710

711+
bool _shouldAddOutboxMessage(OutboxMessage outboxMessage, {
712+
bool wasUnmuted = false,
713+
}) {
714+
return !outboxMessage.hidden
715+
&& narrow.containsMessage(outboxMessage)
716+
&& (_messageVisible(outboxMessage) || wasUnmuted);
717+
}
718+
719+
/// Copy outbox messages from the store, keeping the ones belong to the view.
720+
///
721+
/// This does not recompute [items]. The caller is expected to call
722+
/// [_reprocessOutboxMessages] later to keep [items] up-to-date.
723+
///
724+
/// This assumes that [outboxMessages] is empty.
725+
void _syncOutboxMessages() {
726+
assert(outboxMessages.isEmpty);
727+
for (final outboxMessage in store.outboxMessages.values) {
728+
if (_shouldAddOutboxMessage(outboxMessage)) {
729+
outboxMessages.add(outboxMessage);
730+
}
731+
}
732+
}
733+
590734
/// Add [outboxMessage] if it belongs to the view.
591735
void addOutboxMessage(OutboxMessage outboxMessage) {
592-
// TODO(#1441) implement this
736+
assert(outboxMessages.none(
737+
(message) => message.localMessageId == outboxMessage.localMessageId));
738+
if (_shouldAddOutboxMessage(outboxMessage)) {
739+
_addOutboxMessage(outboxMessage);
740+
if (fetched) {
741+
// Only need to notify listeners when [fetched] is true, because
742+
// otherwise the message list just shows a loading indicator with
743+
// no other items.
744+
notifyListeners();
745+
}
746+
}
593747
}
594748

595749
/// Remove the [outboxMessage] from the view.
@@ -598,7 +752,9 @@ class MessageListView with ChangeNotifier, _MessageSequence {
598752
///
599753
/// This should only be called from [MessageStore.takeOutboxMessage].
600754
void removeOutboxMessage(OutboxMessage outboxMessage) {
601-
// TODO(#1441) implement this
755+
if (_removeOutboxMessage(outboxMessage)) {
756+
notifyListeners();
757+
}
602758
}
603759

604760
void handleUserTopicEvent(UserTopicEvent event) {
@@ -607,10 +763,17 @@ class MessageListView with ChangeNotifier, _MessageSequence {
607763
return;
608764

609765
case VisibilityEffect.muted:
610-
if (_removeMessagesWhere((message) =>
611-
(message is StreamMessage
612-
&& message.streamId == event.streamId
613-
&& message.topic == event.topicName))) {
766+
bool removed = _removeOutboxMessagesWhere((message) =>
767+
message is StreamOutboxMessage
768+
&& message.conversation.streamId == event.streamId
769+
&& message.conversation.topic == event.topicName);
770+
771+
removed |= _removeMessagesWhere((message) =>
772+
message is StreamMessage
773+
&& message.streamId == event.streamId
774+
&& message.topic == event.topicName);
775+
776+
if (removed) {
614777
notifyListeners();
615778
}
616779

@@ -623,6 +786,18 @@ class MessageListView with ChangeNotifier, _MessageSequence {
623786
notifyListeners();
624787
fetchInitial();
625788
}
789+
790+
outboxMessages.clear();
791+
for (final outboxMessage in store.outboxMessages.values) {
792+
if (_shouldAddOutboxMessage(
793+
outboxMessage,
794+
wasUnmuted: outboxMessage is StreamOutboxMessage
795+
&& outboxMessage.conversation.streamId == event.streamId
796+
&& outboxMessage.conversation.topic == event.topicName,
797+
)) {
798+
outboxMessages.add(outboxMessage);
799+
}
800+
}
626801
}
627802
}
628803

@@ -636,14 +811,34 @@ class MessageListView with ChangeNotifier, _MessageSequence {
636811
void handleMessageEvent(MessageEvent event) {
637812
final message = event.message;
638813
if (!narrow.containsMessage(message) || !_messageVisible(message)) {
814+
assert(event.localMessageId == null || outboxMessages.none((message) =>
815+
message.localMessageId == int.parse(event.localMessageId!, radix: 10)));
639816
return;
640817
}
641818
if (!_fetched) {
642819
// TODO mitigate this fetch/event race: save message to add to list later
643820
return;
644821
}
822+
if (outboxMessages.isEmpty) {
823+
assert(items.none((item) => item is MessageListOutboxMessageItem));
824+
_addMessage(message);
825+
notifyListeners();
826+
return;
827+
}
828+
829+
// We always remove all outbox message items
830+
// to ensure that message items come before them.
831+
_removeOutboxMessageItems();
645832
// TODO insert in middle instead, when appropriate
646833
_addMessage(message);
834+
if (event.localMessageId != null) {
835+
final localMessageId = int.parse(event.localMessageId!);
836+
// [outboxMessages] is epxected to be short, so removing the corresponding
837+
// outbox message and reprocessing them all in linear time is efficient.
838+
outboxMessages.removeWhere(
839+
(message) => message.localMessageId == localMessageId);
840+
}
841+
_reprocessOutboxMessages();
647842
notifyListeners();
648843
}
649844

@@ -675,6 +870,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
675870
// TODO in cases where we do have data to do better, do better.
676871
_reset();
677872
notifyListeners();
873+
_syncOutboxMessages();
678874
fetchInitial();
679875
}
680876

@@ -690,6 +886,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
690886
case PropagateMode.changeLater:
691887
narrow = newNarrow;
692888
_reset();
889+
_syncOutboxMessages();
693890
fetchInitial();
694891
case PropagateMode.changeOne:
695892
}
@@ -764,7 +961,11 @@ class MessageListView with ChangeNotifier, _MessageSequence {
764961

765962
/// Notify listeners if the given outbox message is present in this view.
766963
void notifyListenersIfOutboxMessagePresent(int localMessageId) {
767-
// TODO(#1441) implement this
964+
final isAnyPresent =
965+
outboxMessages.any((message) => message.localMessageId == localMessageId);
966+
if (isAnyPresent) {
967+
notifyListeners();
968+
}
768969
}
769970

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

0 commit comments

Comments
 (0)