Skip to content

Commit 3fa0366

Browse files
committed
msglist: Support outbox messages in message list
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. `handleOutboxMessage`, similar to `handleMessage`, also look at `fetched` before adding the outbox message. However, there is no data race to prevent — we can totally not clear `outboxMessages` in `_reset`, and do the initial fetch from `MessageListView.init`, so that outbox messages do not rely on the fetched state at all. I decided against it since it is easy to check `fetched`, and making `fetchInitial` reprocess `outboxMessages` from a clean state (which is inexpensive) helps ensuring message list invariants.
1 parent efcaee0 commit 3fa0366

File tree

5 files changed

+511
-29
lines changed

5 files changed

+511
-29
lines changed

lib/model/message_list.dart

+126-11
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,21 @@ class MessageListMessageItem extends MessageListMessageBaseItem {
6363
});
6464
}
6565

66+
class MessageListOutboxMessageItem extends MessageListMessageBaseItem {
67+
@override
68+
final OutboxMessage message;
69+
@override
70+
final ZulipContent content;
71+
72+
MessageListOutboxMessageItem(
73+
this.message, {
74+
required super.showSender,
75+
required super.isLastInBlock,
76+
}) : content = ZulipContent(nodes: [
77+
ParagraphNode(links: [], nodes: [TextNode(message.content)]),
78+
]);
79+
}
80+
6681
/// Indicates the app is loading more messages at the top.
6782
// TODO(#80): or loading at the bottom, by adding a [MessageListDirection.newer]
6883
class MessageListLoadingItem extends MessageListItem {
@@ -90,7 +105,15 @@ mixin _MessageSequence {
90105
/// See also [contents] and [items].
91106
final List<Message> messages = [];
92107

93-
/// Whether [messages] and [items] represent the results of a fetch.
108+
/// The messages sent by the self-user.
109+
///
110+
/// See also [items].
111+
// Usually this should not have that many items, so we do not anticipate
112+
// performance issues with unoptimized O(N) iterations through this list.
113+
final List<OutboxMessage> outboxMessages = [];
114+
115+
/// Whether [messages], [outboxMessages], and [items] represent the results
116+
/// of a fetch.
94117
///
95118
/// This allows the UI to distinguish "still working on fetching messages"
96119
/// from "there are in fact no messages here".
@@ -142,11 +165,12 @@ mixin _MessageSequence {
142165
/// The messages and their siblings in the UI, in order.
143166
///
144167
/// This has a [MessageListMessageItem] corresponding to each element
145-
/// of [messages], in order. It may have additional items interspersed
146-
/// before, between, or after the messages.
168+
/// of [messages], followed by each element in [outboxMessages] in order.
169+
/// It may have additional items interspersed before, between, or after the
170+
/// messages.
147171
///
148-
/// This information is completely derived from [messages] and
149-
/// the flags [haveOldest], [fetchingOlder] and [fetchOlderCoolingDown].
172+
/// This information is completely derived from [messages], [outboxMessages]
173+
/// and the flags [haveOldest], [fetchingOlder] and [fetchOlderCoolingDown].
150174
/// It exists as an optimization, to memoize that computation.
151175
final QueueList<MessageListItem> items = QueueList();
152176

@@ -171,6 +195,7 @@ mixin _MessageSequence {
171195
if (message.id == null) return 1; // TODO(#1441): test
172196
return message.id! <= messageId ? -1 : 1;
173197
case MessageListMessageItem(:var message): return message.id.compareTo(messageId);
198+
case MessageListOutboxMessageItem(): return 1;
174199
}
175200
}
176201

@@ -278,6 +303,7 @@ mixin _MessageSequence {
278303
void _reset() {
279304
generation += 1;
280305
messages.clear();
306+
outboxMessages.clear();
281307
_fetched = false;
282308
_haveOldest = false;
283309
_fetchingOlder = false;
@@ -301,7 +327,8 @@ mixin _MessageSequence {
301327
///
302328
/// Returns whether an item has been appended or not.
303329
///
304-
/// The caller must append a [MessageListMessageBaseItem] after this.
330+
/// The caller must append a [MessageListMessageBaseItem] for [message]
331+
/// after this.
305332
bool _maybeAppendAuxillaryItem(MessageBase message, {
306333
required MessageBase? prevMessage,
307334
}) {
@@ -338,6 +365,40 @@ mixin _MessageSequence {
338365
isLastInBlock: true));
339366
}
340367

368+
/// Append to [items] based on the index-th outbox message.
369+
///
370+
/// All [messages] and previous messages in [outboxMessages] must already have
371+
/// been processed.
372+
void _processOutboxMessage(int index) {
373+
final prevMessage = index == 0 ? messages.lastOrNull : outboxMessages[index - 1];
374+
final message = outboxMessages[index];
375+
376+
final appended = _maybeAppendAuxillaryItem(message, prevMessage: prevMessage);
377+
items.add(MessageListOutboxMessageItem(message,
378+
showSender: appended || prevMessage?.senderId != message.senderId,
379+
isLastInBlock: true));
380+
}
381+
382+
/// Remove items associated with [outboxMessages] from [items].
383+
///
384+
/// This is efficient due to the expected small size of [outboxMessages].
385+
void _removeOutboxMessageItems() {
386+
// This loop relies on the assumption that all [MessageListMessageItem]
387+
// items comes before those associated with outbox messages. If there
388+
// is no [MessageListMessageItem] at all, this will end up removing
389+
// end markers as well.
390+
while (items.isNotEmpty && items.last is! MessageListMessageItem) {
391+
items.removeLast();
392+
}
393+
assert(items.none((e) => e is MessageListOutboxMessageItem));
394+
395+
if (items.isNotEmpty) {
396+
final lastItem = items.last as MessageListMessageItem;
397+
lastItem.isLastInBlock = true;
398+
}
399+
_updateEndMarkers();
400+
}
401+
341402
/// Update [items] to include markers at start and end as appropriate.
342403
void _updateEndMarkers() {
343404
assert(fetched);
@@ -362,12 +423,16 @@ mixin _MessageSequence {
362423
}
363424
}
364425

365-
/// Recompute [items] from scratch, based on [messages], [contents], and flags.
426+
/// Recompute [items] from scratch, based on [messages], [contents],
427+
/// [outboxMessages] and flags.
366428
void _reprocessAll() {
367429
items.clear();
368430
for (var i = 0; i < messages.length; i++) {
369431
_processMessage(i);
370432
}
433+
for (var i = 0; i < outboxMessages.length; i++) {
434+
_processOutboxMessage(i);
435+
}
371436
_updateEndMarkers();
372437
}
373438
}
@@ -528,7 +593,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
528593
// TODO(#80): fetch from anchor firstUnread, instead of newest
529594
// TODO(#82): fetch from a given message ID as anchor
530595
assert(!fetched && !haveOldest && !fetchingOlder && !fetchOlderCoolingDown);
531-
assert(messages.isEmpty && contents.isEmpty);
596+
assert(messages.isEmpty && contents.isEmpty && outboxMessages.isEmpty);
532597
// TODO schedule all this in another isolate
533598
final generation = this.generation;
534599
final result = await getMessages(store.connection,
@@ -546,6 +611,9 @@ class MessageListView with ChangeNotifier, _MessageSequence {
546611
_addMessage(message);
547612
}
548613
}
614+
for (final outboxMessage in store.outboxMessages.values) {
615+
_maybeAddOutboxMessage(outboxMessage);
616+
}
549617
_fetched = true;
550618
_haveOldest = result.foundOldest;
551619
_updateEndMarkers();
@@ -652,15 +720,43 @@ class MessageListView with ChangeNotifier, _MessageSequence {
652720
}
653721
}
654722

723+
/// Add [outboxMessage] if it belongs to the view.
724+
///
725+
/// Returns true if the message was added, false otherwise.
726+
bool _maybeAddOutboxMessage(OutboxMessage outboxMessage) {
727+
assert(outboxMessages.none(
728+
(message) => message.localMessageId == outboxMessage.localMessageId));
729+
if (!outboxMessage.hidden
730+
&& narrow.containsMessage(outboxMessage)
731+
&& _messageVisible(outboxMessage)) {
732+
outboxMessages.add(outboxMessage);
733+
_processOutboxMessage(outboxMessages.length - 1);
734+
return true;
735+
}
736+
return false;
737+
}
738+
655739
void handleOutboxMessage(OutboxMessage outboxMessage) {
656-
// TODO(#1441) implement this
740+
if (!fetched) return;
741+
if (_maybeAddOutboxMessage(outboxMessage)) {
742+
notifyListeners();
743+
}
657744
}
658745

659746
/// Remove the [outboxMessage] from the view.
660747
///
661748
/// This is a no-op if the message is not found.
662749
void removeOutboxMessageIfExists(OutboxMessage outboxMessage) {
663-
// TODO(#1441) implement this
750+
final removed = outboxMessages.remove(outboxMessage);
751+
if (!removed) {
752+
return;
753+
}
754+
755+
_removeOutboxMessageItems();
756+
for (int i = 0; i < outboxMessages.length; i++) {
757+
_processOutboxMessage(i);
758+
}
759+
notifyListeners();
664760
}
665761

666762
void handleUserTopicEvent(UserTopicEvent event) {
@@ -698,14 +794,29 @@ class MessageListView with ChangeNotifier, _MessageSequence {
698794
void handleMessageEvent(MessageEvent event) {
699795
final message = event.message;
700796
if (!narrow.containsMessage(message) || !_messageVisible(message)) {
797+
assert(event.localMessageId == null || outboxMessages.none((message) =>
798+
message.localMessageId == int.parse(event.localMessageId!, radix: 10)));
701799
return;
702800
}
703801
if (!_fetched) {
704802
// TODO mitigate this fetch/event race: save message to add to list later
705803
return;
706804
}
805+
// We always remove all outbox message items
806+
// to ensure that message items come before them.
807+
_removeOutboxMessageItems();
707808
// TODO insert in middle instead, when appropriate
708809
_addMessage(message);
810+
if (event.localMessageId != null) {
811+
final localMessageId = int.parse(event.localMessageId!);
812+
// [outboxMessages] is epxected to be short, so removing the corresponding
813+
// outbox message and reprocessing them all in linear time is efficient.
814+
outboxMessages.removeWhere(
815+
(message) => message.localMessageId == localMessageId);
816+
}
817+
for (int i = 0; i < outboxMessages.length; i++) {
818+
_processOutboxMessage(i);
819+
}
709820
notifyListeners();
710821
}
711822

@@ -826,7 +937,11 @@ class MessageListView with ChangeNotifier, _MessageSequence {
826937

827938
/// Notify listeners if the given outbox message is present in this view.
828939
void notifyListenersIfOutboxMessagePresent(int localMessageId) {
829-
// TODO(#1441) implement this
940+
final isAnyPresent =
941+
outboxMessages.any((message) => message.localMessageId == localMessageId);
942+
if (isAnyPresent) {
943+
notifyListeners();
944+
}
830945
}
831946

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

lib/widgets/message_list.dart

+38-2
Original file line numberDiff line numberDiff line change
@@ -687,6 +687,12 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
687687
header: header,
688688
trailingWhitespace: i == 1 ? 8 : 11,
689689
item: data);
690+
case MessageListOutboxMessageItem():
691+
final header = RecipientHeader(message: data.message, narrow: widget.narrow);
692+
return MessageItem(
693+
header: header,
694+
trailingWhitespace: 11,
695+
item: data);
690696
}
691697
}
692698
}
@@ -982,6 +988,7 @@ class MessageItem extends StatelessWidget {
982988
child: Column(children: [
983989
switch (item) {
984990
MessageListMessageItem() => MessageWithPossibleSender(item: item),
991+
MessageListOutboxMessageItem() => OutboxMessageWithPossibleSender(item: item),
985992
},
986993
if (trailingWhitespace != null && item.isLastInBlock) SizedBox(height: trailingWhitespace!),
987994
]));
@@ -1336,7 +1343,7 @@ final _kMessageTimestampFormat = DateFormat('h:mm aa', 'en_US');
13361343
class _SenderRow extends StatelessWidget {
13371344
const _SenderRow({required this.message, required this.showTimestamp});
13381345

1339-
final Message message;
1346+
final MessageBase message;
13401347
final bool showTimestamp;
13411348

13421349
@override
@@ -1366,7 +1373,9 @@ class _SenderRow extends StatelessWidget {
13661373
userId: message.senderId),
13671374
const SizedBox(width: 8),
13681375
Flexible(
1369-
child: Text(message.senderFullName, // TODO(#716): use `store.senderDisplayName`
1376+
child: Text(message is Message
1377+
? store.senderDisplayName(message as Message)
1378+
: store.userDisplayName(message.senderId),
13701379
style: TextStyle(
13711380
fontSize: 18,
13721381
height: (22 / 18),
@@ -1465,3 +1474,30 @@ class MessageWithPossibleSender extends StatelessWidget {
14651474
])));
14661475
}
14671476
}
1477+
1478+
/// A placeholder for Zulip message sent by the self-user.
1479+
///
1480+
/// See also [OutboxMessage].
1481+
class OutboxMessageWithPossibleSender extends StatelessWidget {
1482+
const OutboxMessageWithPossibleSender({super.key, required this.item});
1483+
1484+
final MessageListOutboxMessageItem item;
1485+
1486+
@override
1487+
Widget build(BuildContext context) {
1488+
final message = item.message;
1489+
return Padding(
1490+
padding: const EdgeInsets.symmetric(vertical: 4),
1491+
child: Column(children: [
1492+
if (item.showSender) _SenderRow(message: message, showTimestamp: false),
1493+
Padding(
1494+
padding: const EdgeInsets.symmetric(horizontal: 16),
1495+
// This is adapated from [MessageContent].
1496+
// TODO(#576): Offer InheritedMessage ancestor once we are ready
1497+
// to support local echoing images and lightbox.
1498+
child: DefaultTextStyle(
1499+
style: ContentTheme.of(context).textStylePlainParagraph,
1500+
child: BlockContentList(nodes: item.content.nodes))),
1501+
]));
1502+
}
1503+
}

0 commit comments

Comments
 (0)