From 2a5c741ee9a6719cf99ca62c87e1e6f34383ace5 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 11 Jun 2025 20:57:29 -0700 Subject: [PATCH 1/2] message test: Add some is-message-list-notified checks With comments on additional notifyListeners calls we expect when we start having the message list actually show the local echo in its various states. --- test/model/message_test.dart | 42 ++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/test/model/message_test.dart b/test/model/message_test.dart index 0d28ed7dc0..e2e10133a3 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -169,32 +169,40 @@ void main() { await prepareOutboxMessage(destination: DmDestination( userIds: [eg.selfUser.userId, eg.otherUser.userId])); checkState().equals(OutboxMessageState.hidden); + checkNotNotified(); async.elapse(kLocalEchoDebounceDuration); checkState().equals(OutboxMessageState.waiting); + checkNotNotified(); // TODO once (it appears) await receiveMessage(eg.dmMessage(from: eg.selfUser, to: [eg.otherUser])); check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); })); test('smoke stream message: hidden -> waiting -> (delete)', () => awaitFakeAsync((async) async { await prepareOutboxMessage(destination: StreamDestination( stream.streamId, eg.t('foo'))); checkState().equals(OutboxMessageState.hidden); + checkNotNotified(); async.elapse(kLocalEchoDebounceDuration); checkState().equals(OutboxMessageState.waiting); + checkNotNotified(); // TODO once (it appears) await receiveMessage(eg.streamMessage(stream: stream, topic: 'foo')); check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); })); test('hidden -> waiting and never transition to waitPeriodExpired', () => awaitFakeAsync((async) async { await prepareOutboxMessage(); checkState().equals(OutboxMessageState.hidden); + checkNotNotified(); async.elapse(kLocalEchoDebounceDuration); checkState().equals(OutboxMessageState.waiting); + checkNotNotified(); // TODO once (it appears) // Wait till we reach at least [kSendMessageOfferRestoreWaitPeriod] after // the send request was initiated. @@ -204,6 +212,7 @@ void main() { // The outbox message should stay in the waiting state; // it should not transition to waitPeriodExpired. checkState().equals(OutboxMessageState.waiting); + checkNotNotified(); })); test('waiting -> waitPeriodExpired', () => awaitFakeAsync((async) async { @@ -211,9 +220,11 @@ void main() { kSendMessageOfferRestoreWaitPeriod + Duration(seconds: 1)); async.elapse(kLocalEchoDebounceDuration); checkState().equals(OutboxMessageState.waiting); + checkNotNotified(); // TODO once (it appears) async.elapse(kSendMessageOfferRestoreWaitPeriod - kLocalEchoDebounceDuration); checkState().equals(OutboxMessageState.waitPeriodExpired); + checkNotNotified(); // TODO once (it offers restore) await check(outboxMessageFailFuture).throws(); })); @@ -231,10 +242,12 @@ void main() { destination: streamDestination, content: 'content'); async.elapse(kSendMessageOfferRestoreWaitPeriod); checkState().equals(OutboxMessageState.waitPeriodExpired); + checkNotNotified(); // TODO twice (it appears; it offers restore) // Wait till the [sendMessage] request succeeds. await future; checkState().equals(OutboxMessageState.waiting); + checkNotNotified(); // TODO once (it un-offers restore) // Wait till we reach at least [kSendMessageOfferRestoreWaitPeriod] after // returning to the waiting state. @@ -243,15 +256,18 @@ void main() { // The outbox message should stay in the waiting state; // it should not transition to waitPeriodExpired. checkState().equals(OutboxMessageState.waiting); + checkNotNotified(); })); group('… -> failed', () { test('hidden -> failed', () => awaitFakeAsync((async) async { await prepareOutboxMessageToFailAfterDelay(Duration.zero); checkState().equals(OutboxMessageState.hidden); + checkNotNotified(); await check(outboxMessageFailFuture).throws(); checkState().equals(OutboxMessageState.failed); + checkNotNotified(); // TODO once (it appears, offering restore) // Wait till we reach at least [kSendMessageOfferRestoreWaitPeriod] after // the send request was initiated. @@ -260,6 +276,7 @@ void main() { // The outbox message should stay in the failed state; // it should not transition to waitPeriodExpired. checkState().equals(OutboxMessageState.failed); + checkNotNotified(); })); test('waiting -> failed', () => awaitFakeAsync((async) async { @@ -267,9 +284,11 @@ void main() { kLocalEchoDebounceDuration + Duration(seconds: 1)); async.elapse(kLocalEchoDebounceDuration); checkState().equals(OutboxMessageState.waiting); + checkNotNotified(); // TODO once (it appears) await check(outboxMessageFailFuture).throws(); checkState().equals(OutboxMessageState.failed); + checkNotNotified(); // TODO once (it offers restore) })); test('waitPeriodExpired -> failed', () => awaitFakeAsync((async) async { @@ -277,9 +296,11 @@ void main() { kSendMessageOfferRestoreWaitPeriod + Duration(seconds: 1)); async.elapse(kSendMessageOfferRestoreWaitPeriod); checkState().equals(OutboxMessageState.waitPeriodExpired); + checkNotNotified(); // TODO twice (it appears; it offers restore) await check(outboxMessageFailFuture).throws(); checkState().equals(OutboxMessageState.failed); + checkNotNotified(); // TODO once (it shows failure text) })); }); @@ -287,9 +308,11 @@ void main() { test('hidden -> (delete) because event received', () => awaitFakeAsync((async) async { await prepareOutboxMessage(); checkState().equals(OutboxMessageState.hidden); + checkNotNotified(); await receiveMessage(); check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); })); test('hidden -> (delete) when event arrives before send request fails', () => awaitFakeAsync((async) async { @@ -297,25 +320,30 @@ void main() { // the message event to arrive. await prepareOutboxMessageToFailAfterDelay(const Duration(seconds: 1)); checkState().equals(OutboxMessageState.hidden); + checkNotNotified(); // Received the message event while the message is being sent. await receiveMessage(); check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); // Complete the send request. There should be no error despite // the send request failure, because the outbox message is not // in the store any more. await check(outboxMessageFailFuture).completes(); async.elapse(const Duration(seconds: 1)); + checkNotNotified(); })); test('waiting -> (delete) because event received', () => awaitFakeAsync((async) async { await prepareOutboxMessage(); async.elapse(kLocalEchoDebounceDuration); checkState().equals(OutboxMessageState.waiting); + checkNotNotified(); // TODO once (it appears) await receiveMessage(); check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); })); test('waiting -> (delete) when event arrives before send request fails', () => awaitFakeAsync((async) async { @@ -325,15 +353,18 @@ void main() { kLocalEchoDebounceDuration + Duration(seconds: 1)); async.elapse(kLocalEchoDebounceDuration); checkState().equals(OutboxMessageState.waiting); + checkNotNotified(); // TODO once (it appears) // Received the message event while the message is being sent. await receiveMessage(); check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); // Complete the send request. There should be no error despite // the send request failure, because the outbox message is not // in the store any more. await check(outboxMessageFailFuture).completes(); + checkNotNotified(); })); test('waitPeriodExpired -> (delete) when event arrives before send request fails', () => awaitFakeAsync((async) async { @@ -343,15 +374,18 @@ void main() { kSendMessageOfferRestoreWaitPeriod + Duration(seconds: 1)); async.elapse(kSendMessageOfferRestoreWaitPeriod); checkState().equals(OutboxMessageState.waitPeriodExpired); + checkNotNotified(); // TODO twice (it appears; it offers restore) // Received the message event while the message is being sent. await receiveMessage(); check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); // Complete the send request. There should be no error despite // the send request failure, because the outbox message is not // in the store any more. await check(outboxMessageFailFuture).completes(); + checkNotNotified(); })); test('waitPeriodExpired -> (delete) because outbox message was taken', () => awaitFakeAsync((async) async { @@ -361,27 +395,33 @@ void main() { kSendMessageOfferRestoreWaitPeriod + Duration(seconds: 1)); async.elapse(kSendMessageOfferRestoreWaitPeriod); checkState().equals(OutboxMessageState.waitPeriodExpired); + checkNotNotified(); // TODO twice (it appears; it offers restore) store.takeOutboxMessage(store.outboxMessages.keys.single); check(store.outboxMessages).isEmpty(); + checkNotNotified(); // TODO once (it disappears) })); test('failed -> (delete) because event received', () => awaitFakeAsync((async) async { await prepareOutboxMessageToFailAfterDelay(Duration.zero); await check(outboxMessageFailFuture).throws(); checkState().equals(OutboxMessageState.failed); + checkNotNotified(); // TODO once (it appears, offering restore) await receiveMessage(); check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); })); test('failed -> (delete) because outbox message was taken', () => awaitFakeAsync((async) async { await prepareOutboxMessageToFailAfterDelay(Duration.zero); await check(outboxMessageFailFuture).throws(); checkState().equals(OutboxMessageState.failed); + checkNotNotified(); // TODO once (it appears, offering restore) store.takeOutboxMessage(store.outboxMessages.keys.single); check(store.outboxMessages).isEmpty(); + checkNotNotified(); // TODO once (it disappears) })); }); @@ -423,11 +463,13 @@ void main() { await check(store.sendMessage( destination: StreamDestination(stream.streamId, eg.t('topic')), content: 'content')).throws(); + checkNotNotified(); // TODO once (it appears, offering restore) } final localMessageIds = store.outboxMessages.keys.toList(); store.takeOutboxMessage(localMessageIds.removeAt(5)); check(store.outboxMessages).keys.deepEquals(localMessageIds); + checkNotNotified(); // TODO once (it disappears) }); group('reconcileMessages', () { From 11f05dc3747f6383473fbafec20ebe575b9fc94e Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 27 Mar 2025 18:47:16 -0400 Subject: [PATCH 2/2] 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 --- lib/model/message.dart | 2 + lib/model/message_list.dart | 228 +++++++++- lib/widgets/message_list.dart | 32 ++ test/api/model/model_checks.dart | 4 + test/model/message_list_test.dart | 622 +++++++++++++++++++++++++++- test/model/message_test.dart | 44 +- test/widgets/message_list_test.dart | 37 ++ 7 files changed, 912 insertions(+), 57 deletions(-) diff --git a/lib/model/message.dart b/lib/model/message.dart index 719d0704f6..e8cfa6e6e1 100644 --- a/lib/model/message.dart +++ b/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); } diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 458478f248..a7aff0dcbc 100644 --- a/lib/model/message_list.dart +++ b/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 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 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. diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 513043fea6..b49e64a474 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -814,6 +814,9 @@ class _MessageListState extends State with PerAccountStoreAwareStat key: ValueKey(data.message.id), header: header, item: data); + case MessageListOutboxMessageItem(): + final header = RecipientHeader(message: data.message, narrow: widget.narrow); + return MessageItem(header: header, item: data); } } } @@ -1152,6 +1155,7 @@ class MessageItem extends StatelessWidget { child: Column(children: [ switch (item) { MessageListMessageItem() => MessageWithPossibleSender(item: item), + MessageListOutboxMessageItem() => OutboxMessageWithPossibleSender(item: item), }, // TODO refine this padding; discussion: // https://github.com/zulip/zulip-flutter/pull/1453#discussion_r2106526985 @@ -1732,3 +1736,31 @@ class _RestoreEditMessageGestureDetector extends StatelessWidget { child: child); } } + +/// A "local echo" placeholder for a Zulip message to be sent by the self-user. +/// +/// See also [OutboxMessage]. +class OutboxMessageWithPossibleSender extends StatelessWidget { + const OutboxMessageWithPossibleSender({super.key, required this.item}); + + final MessageListOutboxMessageItem item; + + @override + Widget build(BuildContext context) { + final message = item.message; + return Padding( + padding: const EdgeInsets.symmetric(vertical: 4), + child: Column(children: [ + if (item.showSender) + _SenderRow(message: message, showTimestamp: false), + Padding( + padding: const EdgeInsets.symmetric(horizontal: 16), + // This is adapted from [MessageContent]. + // TODO(#576): Offer InheritedMessage ancestor once we are ready + // to support local echoing images and lightbox. + child: DefaultTextStyle( + style: ContentTheme.of(context).textStylePlainParagraph, + child: BlockContentList(nodes: item.content.nodes))), + ])); + } +} diff --git a/test/api/model/model_checks.dart b/test/api/model/model_checks.dart index 17bd86ee9e..262395d955 100644 --- a/test/api/model/model_checks.dart +++ b/test/api/model/model_checks.dart @@ -42,6 +42,10 @@ extension StreamConversationChecks on Subject { Subject get displayRecipient => has((x) => x.displayRecipient, 'displayRecipient'); } +extension DmConversationChecks on Subject { + Subject> get allRecipientIds => has((x) => x.allRecipientIds, 'allRecipientIds'); +} + extension MessageBaseChecks on Subject> { Subject get id => has((e) => e.id, 'id'); Subject get senderId => has((e) => e.senderId, 'senderId'); diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 0eb30c1cbb..a19229e4a2 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -1,8 +1,11 @@ import 'dart:convert'; +import 'dart:io'; import 'package:checks/checks.dart'; import 'package:collection/collection.dart'; +import 'package:fake_async/fake_async.dart'; import 'package:flutter/foundation.dart'; +import 'package:clock/clock.dart'; import 'package:http/http.dart' as http; import 'package:test/scaffolding.dart'; import 'package:zulip/api/backoff.dart'; @@ -10,8 +13,10 @@ import 'package:zulip/api/exception.dart'; import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/model.dart'; import 'package:zulip/api/model/narrow.dart'; +import 'package:zulip/api/route/messages.dart'; import 'package:zulip/model/algorithms.dart'; import 'package:zulip/model/content.dart'; +import 'package:zulip/model/message.dart'; import 'package:zulip/model/message_list.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; @@ -21,7 +26,9 @@ import '../api/model/model_checks.dart'; import '../example_data.dart' as eg; import '../fake_async.dart'; import '../stdlib_checks.dart'; +import 'binding.dart'; import 'content_checks.dart'; +import 'message_checks.dart'; import 'recent_senders_test.dart' as recent_senders_test; import 'test_store.dart'; @@ -49,6 +56,8 @@ void main() { FlutterError.dumpErrorToConsole(details, forceReport: true); }; + TestZulipBinding.ensureInitialized(); + // These variables are the common state operated on by each test. // Each test case calls [prepare] to initialize them. late Subscription subscription; @@ -71,8 +80,9 @@ void main() { Future prepare({ Narrow narrow = const CombinedFeedNarrow(), Anchor anchor = AnchorCode.newest, + ZulipStream? stream, }) async { - final stream = eg.stream(streamId: eg.defaultStreamMessageStreamId); + stream ??= eg.stream(streamId: eg.defaultStreamMessageStreamId); subscription = eg.subscription(stream); store = eg.store(); await store.addStream(stream); @@ -108,6 +118,26 @@ void main() { checkNotifiedOnce(); } + Future prepareOutboxMessages({ + required int count, + required ZulipStream stream, + String topic = 'some topic', + }) async { + for (int i = 0; i < count; i++) { + connection.prepare(json: SendMessageResult(id: 123).toJson()); + await store.sendMessage( + destination: StreamDestination(stream.streamId, eg.t(topic)), + content: 'content'); + } + } + + Future prepareOutboxMessagesTo(List destinations) async { + for (final destination in destinations) { + connection.prepare(json: SendMessageResult(id: 123).toJson()); + await store.sendMessage(destination: destination, content: 'content'); + } + } + void checkLastRequest({ required ApiNarrow narrow, required String anchor, @@ -246,6 +276,105 @@ void main() { test('numeric', () => checkFetchWithAnchor(NumericAnchor(12345))); }); + test('no messages found in fetch; outbox messages present', () => awaitFakeAsync((async) async { + final stream = eg.stream(); + await prepare( + narrow: eg.topicNarrow(stream.streamId, 'topic'), stream: stream); + + await prepareOutboxMessages(count: 1, stream: stream, topic: 'topic'); + async.elapse(kLocalEchoDebounceDuration); + checkNotNotified(); + check(model) + ..fetched.isFalse() + ..outboxMessages.isEmpty(); + + connection.prepare( + json: newestResult(foundOldest: true, messages: []).toJson()); + await model.fetchInitial(); + checkNotifiedOnce(); + check(model) + ..fetched.isTrue() + ..outboxMessages.length.equals(1); + })); + + test('some messages found in fetch; outbox messages present', () => awaitFakeAsync((async) async { + final stream = eg.stream(); + await prepare( + narrow: eg.topicNarrow(stream.streamId, 'topic'), stream: stream); + + await prepareOutboxMessages(count: 1, stream: stream, topic: 'topic'); + async.elapse(kLocalEchoDebounceDuration); + checkNotNotified(); + check(model) + ..fetched.isFalse() + ..outboxMessages.isEmpty(); + + connection.prepare(json: newestResult(foundOldest: true, + messages: [eg.streamMessage(stream: stream, topic: 'topic')]).toJson()); + await model.fetchInitial(); + checkNotifiedOnce(); + check(model) + ..fetched.isTrue() + ..outboxMessages.length.equals(1); + })); + + test('outbox messages not added until haveNewest', () => awaitFakeAsync((async) async { + final stream = eg.stream(); + await prepare( + narrow: eg.topicNarrow(stream.streamId, 'topic'), + anchor: AnchorCode.firstUnread, + stream: stream); + + await prepareOutboxMessages(count: 1, stream: stream, topic: 'topic'); + async.elapse(kLocalEchoDebounceDuration); + checkNotNotified(); + check(model)..fetched.isFalse()..outboxMessages.isEmpty(); + + final message = eg.streamMessage(stream: stream, topic: 'topic'); + connection.prepare(json: nearResult( + anchor: message.id, + foundOldest: true, + foundNewest: false, + messages: [message]).toJson()); + await model.fetchInitial(); + checkNotifiedOnce(); + check(model)..fetched.isTrue()..haveNewest.isFalse()..outboxMessages.isEmpty(); + + connection.prepare(json: newerResult(anchor: message.id, foundNewest: true, + messages: [eg.streamMessage(stream: stream, topic: 'topic')]).toJson()); + final fetchFuture = model.fetchNewer(); + checkNotifiedOnce(); + await fetchFuture; + checkNotifiedOnce(); + check(model)..haveNewest.isTrue()..outboxMessages.length.equals(1); + })); + + test('ignore [OutboxMessage]s outside narrow or with `hidden: true`', () => awaitFakeAsync((async) async { + final stream = eg.stream(); + final otherStream = eg.stream(); + await prepare(narrow: ChannelNarrow(stream.streamId)); + await store.addUserTopic(stream, 'muted', UserTopicVisibilityPolicy.muted); + await prepareOutboxMessagesTo([ + StreamDestination(stream.streamId, eg.t('topic')), + StreamDestination(stream.streamId, eg.t('muted')), + StreamDestination(otherStream.streamId, eg.t('topic')), + ]); + async.elapse(kLocalEchoDebounceDuration); + checkNotNotified(); + + await prepareOutboxMessagesTo( + [StreamDestination(stream.streamId, eg.t('topic'))]); + assert(store.outboxMessages.values.last.hidden); + + connection.prepare(json: + newestResult(foundOldest: true, messages: []).toJson()); + await model.fetchInitial(); + checkNotifiedOnce(); + check(model).outboxMessages.single.isA().conversation + ..streamId.equals(stream.streamId) + ..topic.equals(eg.t('topic')); + })); + // TODO(#824): move this test test('recent senders track all the messages', () async { const narrow = CombinedFeedNarrow(); @@ -614,6 +743,199 @@ void main() { checkNotNotified(); check(model).fetched.isFalse(); }); + + test('when there are outbox messages', () => awaitFakeAsync((async) async { + final stream = eg.stream(); + await prepare(narrow: ChannelNarrow(stream.streamId)); + await prepareMessages(foundOldest: true, messages: + List.generate(30, (i) => eg.streamMessage(stream: stream))); + + await prepareOutboxMessages(count: 5, stream: stream); + async.elapse(kLocalEchoDebounceDuration); + checkNotified(count: 5); + check(model) + ..messages.length.equals(30) + ..outboxMessages.length.equals(5); + + await store.handleEvent(eg.messageEvent(eg.streamMessage(stream: stream))); + checkNotifiedOnce(); + check(model) + ..messages.length.equals(31) + ..outboxMessages.length.equals(5); + })); + + test('from another client (localMessageId present but unrecognized)', () => awaitFakeAsync((async) async { + final stream = eg.stream(); + await prepare(narrow: eg.topicNarrow(stream.streamId, 'topic')); + await prepareMessages(foundOldest: true, messages: + List.generate(30, (i) => eg.streamMessage(stream: stream, topic: 'topic'))); + + check(model) + ..messages.length.equals(30) + ..outboxMessages.isEmpty(); + + await store.handleEvent(eg.messageEvent( + eg.streamMessage(stream: stream, topic: 'topic'), + localMessageId: 1234)); + check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); + check(model) + ..messages.length.equals(31) + ..outboxMessages.isEmpty(); + + async.elapse(kLocalEchoDebounceDuration); + checkNotNotified(); + })); + + test('for an OutboxMessage in the narrow', () => awaitFakeAsync((async) async { + final stream = eg.stream(); + await prepare(narrow: ChannelNarrow(stream.streamId)); + await prepareMessages(foundOldest: true, messages: + List.generate(30, (i) => eg.streamMessage(stream: stream))); + + await prepareOutboxMessages(count: 5, stream: stream); + async.elapse(kLocalEchoDebounceDuration); + checkNotified(count: 5); + final localMessageId = store.outboxMessages.keys.first; + check(model) + ..messages.length.equals(30) + ..outboxMessages.length.equals(5) + ..outboxMessages.any((message) => + message.localMessageId.equals(localMessageId)); + + await store.handleEvent(eg.messageEvent(eg.streamMessage(stream: stream), + localMessageId: localMessageId)); + checkNotifiedOnce(); + check(model) + ..messages.length.equals(31) + ..outboxMessages.length.equals(4) + ..outboxMessages.every((message) => + message.localMessageId.not((m) => m.equals(localMessageId))); + })); + + test('for an OutboxMessage outside the narrow', () => awaitFakeAsync((async) async { + final stream = eg.stream(); + await prepare(narrow: eg.topicNarrow(stream.streamId, 'topic')); + await prepareMessages(foundOldest: true, messages: + List.generate(30, (i) => eg.streamMessage(stream: stream, topic: 'topic'))); + + await prepareOutboxMessages(count: 5, stream: stream, topic: 'other'); + final localMessageId = store.outboxMessages.keys.first; + check(model) + ..messages.length.equals(30) + ..outboxMessages.isEmpty(); + + await store.handleEvent(eg.messageEvent( + eg.streamMessage(stream: stream, topic: 'other'), + localMessageId: localMessageId)); + checkNotNotified(); + check(model) + ..messages.length.equals(30) + ..outboxMessages.isEmpty(); + + async.elapse(kLocalEchoDebounceDuration); + checkNotNotified(); + })); + }); + + group('addOutboxMessage', () { + final stream = eg.stream(); + + test('in narrow', () => awaitFakeAsync((async) async { + await prepare(narrow: ChannelNarrow(stream.streamId), stream: stream); + await prepareMessages(foundOldest: true, messages: + List.generate(30, (i) => eg.streamMessage(stream: stream))); + await prepareOutboxMessages(count: 5, stream: stream); + check(model).outboxMessages.isEmpty(); + + async.elapse(kLocalEchoDebounceDuration); + checkNotified(count: 5); + check(model).outboxMessages.length.equals(5); + })); + + test('not in narrow', () => awaitFakeAsync((async) async { + await prepare(narrow: eg.topicNarrow(stream.streamId, 'topic'), stream: stream); + await prepareMessages(foundOldest: true, messages: + List.generate(30, (i) => eg.streamMessage(stream: stream, topic: 'topic'))); + await prepareOutboxMessages(count: 5, stream: stream, topic: 'other topic'); + check(model).outboxMessages.isEmpty(); + + async.elapse(kLocalEchoDebounceDuration); + checkNotNotified(); + check(model).outboxMessages.isEmpty(); + })); + + test('before fetch', () => awaitFakeAsync((async) async { + await prepare(narrow: ChannelNarrow(stream.streamId)); + await prepareOutboxMessages(count: 5, stream: stream); + check(model) + ..fetched.isFalse() + ..outboxMessages.isEmpty(); + + async.elapse(kLocalEchoDebounceDuration); + checkNotNotified(); + check(model) + ..fetched.isFalse() + ..outboxMessages.isEmpty(); + })); + }); + + group('removeOutboxMessage', () { + final stream = eg.stream(); + + Future prepareFailedOutboxMessages(FakeAsync async, { + required int count, + required ZulipStream stream, + String topic = 'some topic', + }) async { + for (int i = 0; i < count; i++) { + connection.prepare(httpException: SocketException('failed')); + await check(store.sendMessage( + destination: StreamDestination(stream.streamId, eg.t(topic)), + content: 'content')).throws(); + } + } + + test('in narrow', () => awaitFakeAsync((async) async { + await prepare(narrow: ChannelNarrow(stream.streamId), stream: stream); + await prepareMessages(foundOldest: true, messages: + List.generate(30, (i) => eg.streamMessage(stream: stream, topic: 'topic'))); + await prepareFailedOutboxMessages(async, + count: 5, stream: stream); + check(model).outboxMessages.length.equals(5); + checkNotified(count: 5); + + store.takeOutboxMessage(store.outboxMessages.keys.first); + checkNotifiedOnce(); + check(model).outboxMessages.length.equals(4); + })); + + test('not in narrow', () => awaitFakeAsync((async) async { + await prepare(narrow: eg.topicNarrow(stream.streamId, 'topic'), stream: stream); + await prepareMessages(foundOldest: true, messages: + List.generate(30, (i) => eg.streamMessage(stream: stream, topic: 'topic'))); + await prepareFailedOutboxMessages(async, + count: 5, stream: stream, topic: 'other topic'); + check(model).outboxMessages.isEmpty(); + checkNotNotified(); + + store.takeOutboxMessage(store.outboxMessages.keys.first); + check(model).outboxMessages.isEmpty(); + checkNotNotified(); + })); + + test('removed outbox message is the only message in narrow', () => awaitFakeAsync((async) async { + await prepare(narrow: ChannelNarrow(stream.streamId), stream: stream); + await prepareMessages(foundOldest: true, messages: []); + await prepareFailedOutboxMessages(async, + count: 1, stream: stream); + check(model).outboxMessages.single; + checkNotified(count: 1); + + store.takeOutboxMessage(store.outboxMessages.keys.first); + check(model).outboxMessages.isEmpty(); + checkNotifiedOnce(); + })); }); group('UserTopicEvent', () { @@ -637,7 +959,7 @@ void main() { await setVisibility(policy); } - test('mute a visible topic', () async { + test('mute a visible topic', () => awaitFakeAsync((async) async { await prepare(narrow: const CombinedFeedNarrow()); await prepareMutes(); final otherStream = eg.stream(); @@ -651,10 +973,49 @@ void main() { ]); checkHasMessageIds([1, 2, 3, 4]); + await prepareOutboxMessagesTo([ + StreamDestination(stream.streamId, eg.t(topic)), + StreamDestination(stream.streamId, eg.t('elsewhere')), + DmDestination(userIds: [eg.selfUser.userId]), + ]); + async.elapse(kLocalEchoDebounceDuration); + checkNotified(count: 3); + check(model).outboxMessages.deepEquals(>[ + (it) => it.isA() + .conversation.topic.equals(eg.t(topic)), + (it) => it.isA() + .conversation.topic.equals(eg.t('elsewhere')), + (it) => it.isA() + .conversation.allRecipientIds.deepEquals([eg.selfUser.userId]), + ]); + await setVisibility(UserTopicVisibilityPolicy.muted); checkNotifiedOnce(); checkHasMessageIds([1, 3, 4]); - }); + check(model).outboxMessages.deepEquals(>[ + (it) => it.isA() + .conversation.topic.equals(eg.t('elsewhere')), + (it) => it.isA() + .conversation.allRecipientIds.deepEquals([eg.selfUser.userId]), + ]); + })); + + test('mute a visible topic containing only outbox messages', () => awaitFakeAsync((async) async { + await prepare(narrow: const CombinedFeedNarrow()); + await prepareMutes(); + await prepareMessages(foundOldest: true, messages: []); + await prepareOutboxMessagesTo([ + StreamDestination(stream.streamId, eg.t(topic)), + StreamDestination(stream.streamId, eg.t(topic)), + ]); + async.elapse(kLocalEchoDebounceDuration); + check(model).outboxMessages.length.equals(2); + checkNotified(count: 2); + + await setVisibility(UserTopicVisibilityPolicy.muted); + check(model).outboxMessages.isEmpty(); + checkNotifiedOnce(); + })); test('in CombinedFeedNarrow, use combined-feed visibility', () async { // Compare the parallel ChannelNarrow test below. @@ -729,7 +1090,7 @@ void main() { checkHasMessageIds([1]); }); - test('no affected messages -> no notification', () async { + test('no affected messages -> no notification', () => awaitFakeAsync((async) async { await prepare(narrow: const CombinedFeedNarrow()); await prepareMutes(); await prepareMessages(foundOldest: true, messages: [ @@ -737,10 +1098,17 @@ void main() { ]); checkHasMessageIds([1]); + await prepareOutboxMessagesTo( + [StreamDestination(stream.streamId, eg.t('bar'))]); + async.elapse(kLocalEchoDebounceDuration); + final outboxMessage = model.outboxMessages.single; + checkNotifiedOnce(); + await setVisibility(UserTopicVisibilityPolicy.muted); checkNotNotified(); checkHasMessageIds([1]); - }); + check(model).outboxMessages.single.equals(outboxMessage); + })); test('unmute a topic -> refetch from scratch', () => awaitFakeAsync((async) async { await prepare(narrow: const CombinedFeedNarrow()); @@ -750,7 +1118,14 @@ void main() { eg.streamMessage(id: 2, stream: stream, topic: topic), ]; await prepareMessages(foundOldest: true, messages: messages); + await store.addUserTopic(stream, 'muted', UserTopicVisibilityPolicy.muted); + await prepareOutboxMessagesTo([ + StreamDestination(stream.streamId, eg.t(topic)), + StreamDestination(stream.streamId, eg.t('muted')), + ]); + async.elapse(kLocalEchoDebounceDuration); checkHasMessageIds([1]); + check(model).outboxMessages.isEmpty(); connection.prepare( json: newestResult(foundOldest: true, messages: messages).toJson()); @@ -758,10 +1133,14 @@ void main() { checkNotifiedOnce(); check(model).fetched.isFalse(); checkHasMessageIds([]); + check(model).outboxMessages.isEmpty(); async.elapse(Duration.zero); checkNotifiedOnce(); checkHasMessageIds([1, 2]); + check(model).outboxMessages.single.isA().conversation + ..streamId.equals(stream.streamId) + ..topic.equals(eg.t(topic)); })); test('unmute a topic before initial fetch completes -> do nothing', () => awaitFakeAsync((async) async { @@ -907,6 +1286,38 @@ void main() { }); }); + group('notifyListenersIfOutboxMessagePresent', () { + final stream = eg.stream(); + + test('message present', () => awaitFakeAsync((async) async { + await prepare(narrow: const CombinedFeedNarrow(), stream: stream); + await prepareMessages(foundOldest: true, messages: []); + await prepareOutboxMessages(count: 5, stream: stream); + + async.elapse(kLocalEchoDebounceDuration); + checkNotified(count: 5); + + model.notifyListenersIfOutboxMessagePresent( + store.outboxMessages.keys.first); + checkNotifiedOnce(); + })); + + test('message not present', () => awaitFakeAsync((async) async { + await prepare( + narrow: eg.topicNarrow(stream.streamId, 'some topic'), stream: stream); + await prepareMessages(foundOldest: true, messages: []); + await prepareOutboxMessages(count: 5, + stream: stream, topic: 'other topic'); + + async.elapse(kLocalEchoDebounceDuration); + checkNotNotified(); + + model.notifyListenersIfOutboxMessagePresent( + store.outboxMessages.keys.first); + checkNotNotified(); + })); + }); + group('messageContentChanged', () { test('message present', () async { await prepare(narrow: const CombinedFeedNarrow()); @@ -1036,6 +1447,26 @@ void main() { checkNotifiedOnce(); }); + test('channel -> new channel (with outbox messages): remove moved messages; outbox messages unaffected', () => awaitFakeAsync((async) async { + final narrow = ChannelNarrow(stream.streamId); + await prepareNarrow(narrow, initialMessages + movedMessages); + connection.prepare(json: SendMessageResult(id: 1).toJson()); + await prepareOutboxMessages(count: 5, stream: stream); + + async.elapse(kLocalEchoDebounceDuration); + checkNotified(count: 5); + final outboxMessagesCopy = model.outboxMessages.toList(); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: movedMessages, + newTopicStr: 'new', + newStreamId: otherStream.streamId, + )); + checkHasMessages(initialMessages); + check(model).outboxMessages.deepEquals(outboxMessagesCopy); + checkNotifiedOnce(); + })); + test('unrelated channel -> new channel: unaffected', () async { final thirdStream = eg.stream(); await prepareNarrow(narrow, initialMessages); @@ -1737,6 +2168,39 @@ void main() { checkHasMessageIds(expected); }); + test('handle outbox messages', () => awaitFakeAsync((async) async { + final stream = eg.stream(); + await prepare(narrow: ChannelNarrow(stream.streamId)); + await store.addStream(stream); + await store.addSubscription(eg.subscription(stream)); + await store.addUserTopic(stream, 'muted', UserTopicVisibilityPolicy.muted); + await prepareMessages(foundOldest: true, messages: []); + + // Check filtering on sent messages… + await prepareOutboxMessagesTo([ + StreamDestination(stream.streamId, eg.t('not muted')), + StreamDestination(stream.streamId, eg.t('muted')), + ]); + async.elapse(kLocalEchoDebounceDuration); + checkNotifiedOnce(); + check(model.outboxMessages).single.isA() + .conversation.topic.equals(eg.t('not muted')); + + final messages = [eg.streamMessage(stream: stream)]; + connection.prepare(json: newestResult( + foundOldest: true, messages: messages).toJson()); + // Check filtering on fetchInitial… + await store.handleEvent(eg.updateMessageEventMoveTo( + newMessages: messages, + origStreamId: eg.stream().streamId)); + checkNotifiedOnce(); + check(model).fetched.isFalse(); + async.elapse(Duration.zero); + check(model).fetched.isTrue(); + check(model.outboxMessages).single.isA() + .conversation.topic.equals(eg.t('not muted')); + })); + test('in TopicNarrow', () async { final stream = eg.stream(); await prepare(narrow: eg.topicNarrow(stream.streamId, 'A')); @@ -2115,7 +2579,55 @@ void main() { }); }); - test('recipient headers are maintained consistently', () async { + group('findItemWithMessageId', () { + test('has MessageListDateSeparatorItem with null message ID', () => awaitFakeAsync((async) async { + final stream = eg.stream(); + final message = eg.streamMessage(stream: stream, topic: 'topic', + timestamp: eg.utcTimestamp(clock.daysAgo(1))); + await prepare(narrow: ChannelNarrow(stream.streamId)); + await prepareMessages(foundOldest: true, messages: [message]); + + // `findItemWithMessageId` uses binary search. Set up just enough + // outbox message items, so that a [MessageListDateSeparatorItem] for + // the outbox messages is right in the middle. + await prepareOutboxMessages(count: 2, stream: stream, topic: 'topic'); + async.elapse(kLocalEchoDebounceDuration); + checkNotified(count: 2); + check(model.items).deepEquals(>[ + (it) => it.isA(), + (it) => it.isA(), + (it) => it.isA().message.id.isNull(), + (it) => it.isA(), + (it) => it.isA(), + ]); + check(model.findItemWithMessageId(message.id)).equals(1); + })); + + test('has MessageListOutboxMessageItem', () => awaitFakeAsync((async) async { + final stream = eg.stream(); + final message = eg.streamMessage(stream: stream, topic: 'topic', + timestamp: eg.utcTimestamp(clock.now())); + await prepare(narrow: ChannelNarrow(stream.streamId)); + await prepareMessages(foundOldest: true, messages: [message]); + + // `findItemWithMessageId` uses binary search. Set up just enough + // outbox message items, so that a [MessageListOutboxMessageItem] + // is right in the middle. + await prepareOutboxMessages(count: 3, stream: stream, topic: 'topic'); + async.elapse(kLocalEchoDebounceDuration); + checkNotified(count: 3); + check(model.items).deepEquals(>[ + (it) => it.isA(), + (it) => it.isA(), + (it) => it.isA(), + (it) => it.isA(), + (it) => it.isA(), + ]); + check(model.findItemWithMessageId(message.id)).equals(1); + })); + }); + + test('recipient headers are maintained consistently', () => awaitFakeAsync((async) async { // TODO test date separators are maintained consistently too // This tests the code that maintains the invariant that recipient headers // are present just where they're required. @@ -2128,7 +2640,7 @@ void main() { // just needs messages that have the same recipient, and that don't, and // doesn't need to exercise the different reasons that messages don't. - const timestamp = 1693602618; + final timestamp = eg.utcTimestamp(clock.now()); final stream = eg.stream(streamId: eg.defaultStreamMessageStreamId); Message streamMessage(int id) => eg.streamMessage(id: id, stream: stream, topic: 'foo', timestamp: timestamp); @@ -2187,6 +2699,20 @@ void main() { model.reassemble(); checkNotifiedOnce(); + // Then test outbox message, where a new header is needed… + connection.prepare(json: SendMessageResult(id: 1).toJson()); + await store.sendMessage( + destination: DmDestination(userIds: [eg.selfUser.userId]), content: 'hi'); + async.elapse(kLocalEchoDebounceDuration); + checkNotifiedOnce(); + + // … and where it's not. + connection.prepare(json: SendMessageResult(id: 1).toJson()); + await store.sendMessage( + destination: DmDestination(userIds: [eg.selfUser.userId]), content: 'hi'); + async.elapse(kLocalEchoDebounceDuration); + checkNotifiedOnce(); + // Have a new fetchOlder reach the oldest, so that a history-start marker appears… connection.prepare(json: olderResult( anchor: model.messages[0].id, @@ -2199,17 +2725,33 @@ void main() { // … and then test reassemble again. model.reassemble(); checkNotifiedOnce(); - }); - test('showSender is maintained correctly', () async { + final outboxMessageIds = store.outboxMessages.keys.toList(); + // Then test removing the first outbox message… + await store.handleEvent(eg.messageEvent( + dmMessage(15), localMessageId: outboxMessageIds.first)); + checkNotifiedOnce(); + + // … and handling a new non-outbox message… + await store.handleEvent(eg.messageEvent(streamMessage(16))); + checkNotifiedOnce(); + + // … and removing the second outbox message. + await store.handleEvent(eg.messageEvent( + dmMessage(17), localMessageId: outboxMessageIds.last)); + checkNotifiedOnce(); + })); + + test('showSender is maintained correctly', () => awaitFakeAsync((async) async { // TODO(#150): This will get more complicated with message moves. // Until then, we always compute this sequentially from oldest to newest. // So we just need to exercise the different cases of the logic for // whether the sender should be shown, but the difference between // fetchInitial and handleMessageEvent etc. doesn't matter. - const t1 = 1693602618; - const t2 = t1 + 86400; + final now = clock.now(); + final t1 = eg.utcTimestamp(now.subtract(Duration(days: 1))); + final t2 = eg.utcTimestamp(now); final stream = eg.stream(streamId: eg.defaultStreamMessageStreamId); Message streamMessage(int id, int timestamp, User sender) => eg.streamMessage(id: id, sender: sender, @@ -2217,6 +2759,8 @@ void main() { Message dmMessage(int id, int timestamp, User sender) => eg.dmMessage(id: id, from: sender, timestamp: timestamp, to: [sender.userId == eg.selfUser.userId ? eg.otherUser : eg.selfUser]); + DmDestination dmDestination(List users) => + DmDestination(userIds: users.map((user) => user.userId).toList()); await prepare(); await prepareMessages(foundOldest: true, messages: [ @@ -2226,6 +2770,13 @@ void main() { dmMessage(4, t1, eg.otherUser), // same sender, but new recipient dmMessage(5, t2, eg.otherUser), // same sender/recipient, but new day ]); + await prepareOutboxMessagesTo([ + dmDestination([eg.selfUser, eg.otherUser]), // same day, but new sender + dmDestination([eg.selfUser, eg.otherUser]), // hide sender + ]); + assert( + store.outboxMessages.values.every((message) => message.timestamp == t2)); + async.elapse(kLocalEchoDebounceDuration); // We check showSender has the right values in [checkInvariants], // but to make this test explicit: @@ -2238,8 +2789,10 @@ void main() { (it) => it.isA().showSender.isTrue(), (it) => it.isA(), (it) => it.isA().showSender.isTrue(), + (it) => it.isA().showSender.isTrue(), + (it) => it.isA().showSender.isFalse(), ]); - }); + })); group('haveSameRecipient', () { test('stream messages vs DMs, no match', () { @@ -2310,6 +2863,16 @@ void main() { doTest('same letters, different diacritics', 'ma', 'mǎ', false); doTest('having different CJK characters', '嗎', '馬', false); }); + + test('outbox messages', () { + final stream = eg.stream(); + final streamMessage1 = eg.streamOutboxMessage(stream: stream, topic: 'foo'); + final streamMessage2 = eg.streamOutboxMessage(stream: stream, topic: 'bar'); + final dmMessage = eg.dmOutboxMessage(from: eg.selfUser, to: [eg.otherUser]); + check(haveSameRecipient(streamMessage1, streamMessage1)).isTrue(); + check(haveSameRecipient(streamMessage1, streamMessage2)).isFalse(); + check(haveSameRecipient(streamMessage1, dmMessage)).isFalse(); + }); }); test('messagesSameDay', () { @@ -2345,6 +2908,14 @@ void main() { eg.dmMessage(from: eg.selfUser, to: [], timestamp: timestampFromLocalTime(time0)), eg.dmMessage(from: eg.selfUser, to: [], timestamp: timestampFromLocalTime(time1)), )).equals(i0 == i1); + check(because: 'times $time0, $time1', messagesSameDay( + eg.streamOutboxMessage(timestamp: timestampFromLocalTime(time0)), + eg.streamOutboxMessage(timestamp: timestampFromLocalTime(time1)), + )).equals(i0 == i1); + check(because: 'times $time0, $time1', messagesSameDay( + eg.dmOutboxMessage(from: eg.selfUser, to: [], timestamp: timestampFromLocalTime(time0)), + eg.dmOutboxMessage(from: eg.selfUser, to: [], timestamp: timestampFromLocalTime(time1)), + )).equals(i0 == i1); } } } @@ -2360,6 +2931,7 @@ void checkInvariants(MessageListView model) { if (!model.fetched) { check(model) ..messages.isEmpty() + ..outboxMessages.isEmpty() ..haveOldest.isFalse() ..haveNewest.isFalse() ..busyFetchingMore.isFalse(); @@ -2371,8 +2943,15 @@ void checkInvariants(MessageListView model) { for (final message in model.messages) { check(model.store.messages)[message.id].isNotNull().identicalTo(message); } + if (model.outboxMessages.isNotEmpty) { + check(model.haveNewest).isTrue(); + } + for (final message in model.outboxMessages) { + check(message).hidden.isFalse(); + check(model.store.outboxMessages)[message.localMessageId].isNotNull().identicalTo(message); + } - final allMessages = >[...model.messages]; + final allMessages = [...model.messages, ...model.outboxMessages]; for (final message in allMessages) { check(model.narrow.containsMessage(message)).isTrue(); @@ -2395,6 +2974,8 @@ void checkInvariants(MessageListView model) { check(isSortedWithoutDuplicates(model.messages.map((m) => m.id).toList())) .isTrue(); + check(isSortedWithoutDuplicates(model.outboxMessages.map((m) => m.localMessageId).toList())) + .isTrue(); check(model).middleMessage ..isGreaterOrEqual(0) @@ -2444,7 +3025,8 @@ void checkInvariants(MessageListView model) { ..message.identicalTo(model.messages[j]) ..content.identicalTo(model.contents[j]); } else { - assert(false); + check(model.items[i]).isA() + .message.identicalTo(model.outboxMessages[j-model.messages.length]); } check(model.items[i++]).isA() ..showSender.equals( @@ -2452,6 +3034,7 @@ void checkInvariants(MessageListView model) { ..isLastInBlock.equals( i == model.items.length || switch (model.items[i]) { MessageListMessageItem() + || MessageListOutboxMessageItem() || MessageListDateSeparatorItem() => false, MessageListRecipientHeaderItem() => true, }); @@ -2461,8 +3044,14 @@ void checkInvariants(MessageListView model) { check(model).middleItem ..isGreaterOrEqual(0) ..isLessOrEqual(model.items.length); - if (model.middleItem == model.items.length) { - check(model.middleMessage).equals(model.messages.length); + if (model.middleMessage == model.messages.length) { + if (model.outboxMessages.isEmpty) { + // the bottom slice of `model.messages` is empty + check(model).middleItem.equals(model.items.length); + } else { + check(model.items[model.middleItem]).isA() + .message.identicalTo(model.outboxMessages.first); + } } else { check(model.items[model.middleItem]).isA() .message.identicalTo(model.messages[model.middleMessage]); @@ -2503,6 +3092,7 @@ extension MessageListViewChecks on Subject { Subject get store => has((x) => x.store, 'store'); Subject get narrow => has((x) => x.narrow, 'narrow'); Subject> get messages => has((x) => x.messages, 'messages'); + Subject> get outboxMessages => has((x) => x.outboxMessages, 'outboxMessages'); Subject get middleMessage => has((x) => x.middleMessage, 'middleMessage'); Subject> get contents => has((x) => x.contents, 'contents'); Subject> get items => has((x) => x.items, 'items'); diff --git a/test/model/message_test.dart b/test/model/message_test.dart index e2e10133a3..762cc41452 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -173,7 +173,7 @@ void main() { async.elapse(kLocalEchoDebounceDuration); checkState().equals(OutboxMessageState.waiting); - checkNotNotified(); // TODO once (it appears) + checkNotifiedOnce(); await receiveMessage(eg.dmMessage(from: eg.selfUser, to: [eg.otherUser])); check(store.outboxMessages).isEmpty(); @@ -188,7 +188,7 @@ void main() { async.elapse(kLocalEchoDebounceDuration); checkState().equals(OutboxMessageState.waiting); - checkNotNotified(); // TODO once (it appears) + checkNotifiedOnce(); await receiveMessage(eg.streamMessage(stream: stream, topic: 'foo')); check(store.outboxMessages).isEmpty(); @@ -202,7 +202,7 @@ void main() { async.elapse(kLocalEchoDebounceDuration); checkState().equals(OutboxMessageState.waiting); - checkNotNotified(); // TODO once (it appears) + checkNotifiedOnce(); // Wait till we reach at least [kSendMessageOfferRestoreWaitPeriod] after // the send request was initiated. @@ -220,11 +220,11 @@ void main() { kSendMessageOfferRestoreWaitPeriod + Duration(seconds: 1)); async.elapse(kLocalEchoDebounceDuration); checkState().equals(OutboxMessageState.waiting); - checkNotNotified(); // TODO once (it appears) + checkNotifiedOnce(); async.elapse(kSendMessageOfferRestoreWaitPeriod - kLocalEchoDebounceDuration); checkState().equals(OutboxMessageState.waitPeriodExpired); - checkNotNotified(); // TODO once (it offers restore) + checkNotifiedOnce(); await check(outboxMessageFailFuture).throws(); })); @@ -242,12 +242,12 @@ void main() { destination: streamDestination, content: 'content'); async.elapse(kSendMessageOfferRestoreWaitPeriod); checkState().equals(OutboxMessageState.waitPeriodExpired); - checkNotNotified(); // TODO twice (it appears; it offers restore) + checkNotified(count: 2); // Wait till the [sendMessage] request succeeds. await future; checkState().equals(OutboxMessageState.waiting); - checkNotNotified(); // TODO once (it un-offers restore) + checkNotifiedOnce(); // Wait till we reach at least [kSendMessageOfferRestoreWaitPeriod] after // returning to the waiting state. @@ -267,7 +267,7 @@ void main() { await check(outboxMessageFailFuture).throws(); checkState().equals(OutboxMessageState.failed); - checkNotNotified(); // TODO once (it appears, offering restore) + checkNotifiedOnce(); // Wait till we reach at least [kSendMessageOfferRestoreWaitPeriod] after // the send request was initiated. @@ -284,11 +284,11 @@ void main() { kLocalEchoDebounceDuration + Duration(seconds: 1)); async.elapse(kLocalEchoDebounceDuration); checkState().equals(OutboxMessageState.waiting); - checkNotNotified(); // TODO once (it appears) + checkNotifiedOnce(); await check(outboxMessageFailFuture).throws(); checkState().equals(OutboxMessageState.failed); - checkNotNotified(); // TODO once (it offers restore) + checkNotifiedOnce(); })); test('waitPeriodExpired -> failed', () => awaitFakeAsync((async) async { @@ -296,11 +296,11 @@ void main() { kSendMessageOfferRestoreWaitPeriod + Duration(seconds: 1)); async.elapse(kSendMessageOfferRestoreWaitPeriod); checkState().equals(OutboxMessageState.waitPeriodExpired); - checkNotNotified(); // TODO twice (it appears; it offers restore) + checkNotified(count: 2); await check(outboxMessageFailFuture).throws(); checkState().equals(OutboxMessageState.failed); - checkNotNotified(); // TODO once (it shows failure text) + checkNotifiedOnce(); })); }); @@ -339,7 +339,7 @@ void main() { await prepareOutboxMessage(); async.elapse(kLocalEchoDebounceDuration); checkState().equals(OutboxMessageState.waiting); - checkNotNotified(); // TODO once (it appears) + checkNotifiedOnce(); await receiveMessage(); check(store.outboxMessages).isEmpty(); @@ -353,7 +353,7 @@ void main() { kLocalEchoDebounceDuration + Duration(seconds: 1)); async.elapse(kLocalEchoDebounceDuration); checkState().equals(OutboxMessageState.waiting); - checkNotNotified(); // TODO once (it appears) + checkNotifiedOnce(); // Received the message event while the message is being sent. await receiveMessage(); @@ -374,7 +374,7 @@ void main() { kSendMessageOfferRestoreWaitPeriod + Duration(seconds: 1)); async.elapse(kSendMessageOfferRestoreWaitPeriod); checkState().equals(OutboxMessageState.waitPeriodExpired); - checkNotNotified(); // TODO twice (it appears; it offers restore) + checkNotified(count: 2); // Received the message event while the message is being sent. await receiveMessage(); @@ -395,18 +395,18 @@ void main() { kSendMessageOfferRestoreWaitPeriod + Duration(seconds: 1)); async.elapse(kSendMessageOfferRestoreWaitPeriod); checkState().equals(OutboxMessageState.waitPeriodExpired); - checkNotNotified(); // TODO twice (it appears; it offers restore) + checkNotified(count: 2); store.takeOutboxMessage(store.outboxMessages.keys.single); check(store.outboxMessages).isEmpty(); - checkNotNotified(); // TODO once (it disappears) + checkNotifiedOnce(); })); test('failed -> (delete) because event received', () => awaitFakeAsync((async) async { await prepareOutboxMessageToFailAfterDelay(Duration.zero); await check(outboxMessageFailFuture).throws(); checkState().equals(OutboxMessageState.failed); - checkNotNotified(); // TODO once (it appears, offering restore) + checkNotifiedOnce(); await receiveMessage(); check(store.outboxMessages).isEmpty(); @@ -417,11 +417,11 @@ void main() { await prepareOutboxMessageToFailAfterDelay(Duration.zero); await check(outboxMessageFailFuture).throws(); checkState().equals(OutboxMessageState.failed); - checkNotNotified(); // TODO once (it appears, offering restore) + checkNotifiedOnce(); store.takeOutboxMessage(store.outboxMessages.keys.single); check(store.outboxMessages).isEmpty(); - checkNotNotified(); // TODO once (it disappears) + checkNotifiedOnce(); })); }); @@ -463,13 +463,13 @@ void main() { await check(store.sendMessage( destination: StreamDestination(stream.streamId, eg.t('topic')), content: 'content')).throws(); - checkNotNotified(); // TODO once (it appears, offering restore) + checkNotifiedOnce(); } final localMessageIds = store.outboxMessages.keys.toList(); store.takeOutboxMessage(localMessageIds.removeAt(5)); check(store.outboxMessages).keys.deepEquals(localMessageIds); - checkNotNotified(); // TODO once (it disappears) + checkNotifiedOnce(); }); group('reconcileMessages', () { diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 5ba2712e76..01e40cf7cf 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -15,6 +15,7 @@ import 'package:zulip/api/route/channels.dart'; import 'package:zulip/api/route/messages.dart'; import 'package:zulip/model/actions.dart'; import 'package:zulip/model/localizations.dart'; +import 'package:zulip/model/message.dart'; import 'package:zulip/model/message_list.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; @@ -1625,6 +1626,42 @@ void main() { }); }); + group('OutboxMessageWithPossibleSender', () { + final stream = eg.stream(); + final topic = 'topic'; + final topicNarrow = eg.topicNarrow(stream.streamId, topic); + const content = 'outbox message content'; + + final contentInputFinder = find.byWidgetPredicate( + (widget) => widget is TextField && widget.controller is ComposeContentController); + + Finder outboxMessageFinder = find.widgetWithText( + OutboxMessageWithPossibleSender, content, skipOffstage: true); + + Future sendMessageAndSucceed(WidgetTester tester, { + Duration delay = Duration.zero, + }) async { + connection.prepare(json: SendMessageResult(id: 1).toJson(), delay: delay); + await tester.enterText(contentInputFinder, content); + await tester.tap(find.byIcon(ZulipIcons.send)); + await tester.pump(Duration.zero); + } + + // State transitions are tested more thoroughly in + // test/model/message_test.dart . + + testWidgets('hidden -> waiting, outbox message appear', (tester) async { + await setupMessageListPage(tester, + narrow: topicNarrow, streams: [stream], + messages: []); + await sendMessageAndSucceed(tester); + check(outboxMessageFinder).findsNothing(); + + await tester.pump(kLocalEchoDebounceDuration); + check(outboxMessageFinder).findsOne(); + }); + }); + group('Starred messages', () { testWidgets('unstarred message', (tester) async { final message = eg.streamMessage(flags: []);