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 2617f18b68..a0e2bb2c0b 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(); + } }); } @@ -756,9 +886,42 @@ class MessageListView with ChangeNotifier, _MessageSequence { } } + 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. @@ -767,7 +930,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) { @@ -776,10 +941,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(); } @@ -805,6 +977,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) { @@ -819,8 +993,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(); } @@ -941,7 +1127,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 542d0a52b2..03733547a8 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -768,6 +768,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); } } } @@ -1079,6 +1082,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 @@ -1659,3 +1663,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 d58de664d8..d2905ef7bb 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: newestResult(foundOldest: false, + 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(); @@ -612,6 +741,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', () { @@ -635,7 +957,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(); @@ -649,10 +971,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. @@ -727,7 +1088,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: [ @@ -735,10 +1096,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()); @@ -748,18 +1116,30 @@ 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()); await setVisibility(UserTopicVisibilityPolicy.unmuted); checkNotifiedOnce(); - check(model).fetched.isFalse(); + check(model) + ..fetched.isFalse() + ..outboxMessages.isEmpty(); checkHasMessageIds([]); 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 { @@ -905,6 +1285,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()); @@ -1034,6 +1446,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); @@ -1735,6 +2167,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')); @@ -2113,7 +2578,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. @@ -2126,7 +2639,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); @@ -2185,6 +2698,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, @@ -2197,17 +2724,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, @@ -2215,6 +2758,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: [ @@ -2224,6 +2769,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: @@ -2236,8 +2788,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', () { @@ -2308,6 +2862,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', () { @@ -2343,6 +2907,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); } } } @@ -2369,8 +2941,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(); @@ -2393,6 +2972,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) @@ -2442,7 +3023,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( @@ -2450,6 +3032,7 @@ void checkInvariants(MessageListView model) { ..isLastInBlock.equals( i == model.items.length || switch (model.items[i]) { MessageListMessageItem() + || MessageListOutboxMessageItem() || MessageListDateSeparatorItem() => false, MessageListRecipientHeaderItem() => true, }); @@ -2459,8 +3042,15 @@ 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]); @@ -2501,6 +3091,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 0d28ed7dc0..762cc41452 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); + checkNotifiedOnce(); 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); + checkNotifiedOnce(); 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); + checkNotifiedOnce(); // 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); + checkNotifiedOnce(); async.elapse(kSendMessageOfferRestoreWaitPeriod - kLocalEchoDebounceDuration); checkState().equals(OutboxMessageState.waitPeriodExpired); + checkNotifiedOnce(); await check(outboxMessageFailFuture).throws(); })); @@ -231,10 +242,12 @@ void main() { destination: streamDestination, content: 'content'); async.elapse(kSendMessageOfferRestoreWaitPeriod); checkState().equals(OutboxMessageState.waitPeriodExpired); + checkNotified(count: 2); // Wait till the [sendMessage] request succeeds. await future; checkState().equals(OutboxMessageState.waiting); + checkNotifiedOnce(); // 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); + checkNotifiedOnce(); // 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); + checkNotifiedOnce(); await check(outboxMessageFailFuture).throws(); checkState().equals(OutboxMessageState.failed); + checkNotifiedOnce(); })); test('waitPeriodExpired -> failed', () => awaitFakeAsync((async) async { @@ -277,9 +296,11 @@ void main() { kSendMessageOfferRestoreWaitPeriod + Duration(seconds: 1)); async.elapse(kSendMessageOfferRestoreWaitPeriod); checkState().equals(OutboxMessageState.waitPeriodExpired); + checkNotified(count: 2); await check(outboxMessageFailFuture).throws(); checkState().equals(OutboxMessageState.failed); + checkNotifiedOnce(); })); }); @@ -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); + checkNotifiedOnce(); 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); + checkNotifiedOnce(); // 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); + checkNotified(count: 2); // 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); + checkNotified(count: 2); store.takeOutboxMessage(store.outboxMessages.keys.single); check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); })); test('failed -> (delete) because event received', () => awaitFakeAsync((async) async { await prepareOutboxMessageToFailAfterDelay(Duration.zero); await check(outboxMessageFailFuture).throws(); checkState().equals(OutboxMessageState.failed); + checkNotifiedOnce(); 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); + checkNotifiedOnce(); store.takeOutboxMessage(store.outboxMessages.keys.single); check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); })); }); @@ -423,11 +463,13 @@ void main() { await check(store.sendMessage( destination: StreamDestination(stream.streamId, eg.t('topic')), content: 'content')).throws(); + checkNotifiedOnce(); } final localMessageIds = store.outboxMessages.keys.toList(); store.takeOutboxMessage(localMessageIds.removeAt(5)); check(store.outboxMessages).keys.deepEquals(localMessageIds); + checkNotifiedOnce(); }); group('reconcileMessages', () { diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 3b3b01b323..13788deda9 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'; @@ -1551,6 +1552,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: []);