From 3b93fe77cbc4af514d8ec58b03bdad53f243ddf2 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Mon, 14 Apr 2025 21:31:02 -0400 Subject: [PATCH 01/19] api [nfc]: Add TopicName.interpretAsServer The point of this helper is to replicate what a topic sent from the client will become, after being processed by the server. This important when trying to create a local copy of a stream message, whose topic can get translated when it's delivered by the server. --- lib/api/model/model.dart | 33 +++++++++++++++++++++++++++++++++ lib/api/route/messages.dart | 9 --------- test/api/model/model_test.dart | 21 +++++++++++++++++++++ 3 files changed, 54 insertions(+), 9 deletions(-) diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index 157307c3d4..27a922e5c2 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -532,6 +532,15 @@ String? tryParseEmojiCodeToUnicode(String emojiCode) { } } +/// The topic servers understand to mean "there is no topic". +/// +/// This should match +/// https://github.com/zulip/zulip/blob/6.0/zerver/actions/message_edit.py#L940 +/// or similar logic at the latest `main`. +// This is hardcoded in the server, and therefore untranslated; that's +// zulip/zulip#3639. +const String kNoTopicTopic = '(no topic)'; + /// The name of a Zulip topic. // TODO(dart): Can we forbid calling Object members on this extension type? // (The lack of "implements Object" ought to do that, but doesn't.) @@ -586,6 +595,30 @@ extension type const TopicName(String _value) { /// using [canonicalize]. bool isSameAs(TopicName other) => canonicalize() == other.canonicalize(); + /// Convert this topic to match how it would appear on a message object from + /// the server, assuming the topic is originally for a send-message request. + /// + /// For a client that does not support empty topics, + /// a modern server (FL>=334) would convert "(no topic)" and empty topics to + /// `store.realmEmptyTopicDisplayName`. + /// + /// See also: https://zulip.com/api/send-message#parameter-topic + TopicName interpretAsServer({ + required int zulipFeatureLevel, + required String? realmEmptyTopicDisplayName, + }) { + if (zulipFeatureLevel < 334) { + assert(_value.isNotEmpty); + return this; + } + if (_value == kNoTopicTopic || _value.isEmpty) { + // TODO(#1250): this assumes that the 'support_empty_topics' + // client_capability is false; update this when we set it to true + return TopicName(realmEmptyTopicDisplayName!); + } + return TopicName(_value); + } + TopicName.fromJson(this._value); String toJson() => apiName; diff --git a/lib/api/route/messages.dart b/lib/api/route/messages.dart index 6a42158b75..23f92485d7 100644 --- a/lib/api/route/messages.dart +++ b/lib/api/route/messages.dart @@ -169,15 +169,6 @@ const int kMaxTopicLengthCodePoints = 60; // https://zulip.com/api/send-message#parameter-content const int kMaxMessageLengthCodePoints = 10000; -/// The topic servers understand to mean "there is no topic". -/// -/// This should match -/// https://github.com/zulip/zulip/blob/6.0/zerver/actions/message_edit.py#L940 -/// or similar logic at the latest `main`. -// This is hardcoded in the server, and therefore untranslated; that's -// zulip/zulip#3639. -const String kNoTopicTopic = '(no topic)'; - /// https://zulip.com/api/send-message Future sendMessage( ApiConnection connection, { diff --git a/test/api/model/model_test.dart b/test/api/model/model_test.dart index b1552deb5b..fac180ed9d 100644 --- a/test/api/model/model_test.dart +++ b/test/api/model/model_test.dart @@ -161,6 +161,27 @@ void main() { doCheck(eg.t('✔ a'), eg.t('✔ b'), false); }); + + test('interpretAsServer', () { + final emptyTopicDisplayName = eg.defaultRealmEmptyTopicDisplayName; + void doCheck(TopicName topicA, TopicName expected, int zulipFeatureLevel) { + check(topicA.interpretAsServer( + zulipFeatureLevel: zulipFeatureLevel, + realmEmptyTopicDisplayName: emptyTopicDisplayName), + ).equals(expected); + } + + check(() => doCheck(eg.t(''), eg.t(''), 333)) + .throws(); + doCheck(eg.t('(no topic)'), eg.t('(no topic)'), 333); + doCheck(eg.t(emptyTopicDisplayName), eg.t(emptyTopicDisplayName), 333); + doCheck(eg.t('other topic'), eg.t('other topic'), 333); + + doCheck(eg.t(''), eg.t(emptyTopicDisplayName), 334); + doCheck(eg.t('(no topic)'), eg.t(emptyTopicDisplayName), 334); + doCheck(eg.t(emptyTopicDisplayName), eg.t(emptyTopicDisplayName), 334); + doCheck(eg.t('other topic'), eg.t('other topic'), 334); + }); }); group('DmMessage', () { From 3d1b7bfe56d0137479e019902b94ea4b566c36d8 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Mon, 14 Apr 2025 21:33:41 -0400 Subject: [PATCH 02/19] store [nfc]: Move zulip{FeatureLevel,Version} to PerAccountStoreBase --- lib/model/store.dart | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index 939120113e..5fd3dd6518 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -385,6 +385,12 @@ abstract class PerAccountStoreBase { /// This returns null if [reference] fails to parse as a URL. Uri? tryResolveUrl(String reference) => _tryResolveUrl(realmUrl, reference); + /// Always equal to `connection.zulipFeatureLevel` + /// and `account.zulipFeatureLevel`. + int get zulipFeatureLevel => connection.zulipFeatureLevel!; + + String get zulipVersion => account.zulipVersion; + //////////////////////////////// // Data attached to the self-account on the realm. @@ -558,11 +564,6 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor //////////////////////////////// // Data attached to the realm or the server. - /// Always equal to `connection.zulipFeatureLevel` - /// and `account.zulipFeatureLevel`. - int get zulipFeatureLevel => connection.zulipFeatureLevel!; - - String get zulipVersion => account.zulipVersion; final RealmWildcardMentionPolicy realmWildcardMentionPolicy; // TODO(#668): update this realm setting final bool realmMandatoryTopics; // TODO(#668): update this realm setting /// For docs, please see [InitialSnapshot.realmWaitingPeriodThreshold]. From a4115cedd6b370e9207acf95d63f7f09b0bd8615 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 22 Apr 2025 00:04:54 -0400 Subject: [PATCH 03/19] test [nfc]: Generate timestamps --- test/example_data.dart | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/test/example_data.dart b/test/example_data.dart index f31337d303..8e64692d05 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -68,6 +68,20 @@ ZulipApiException apiExceptionUnauthorized({String routeName = 'someRoute'}) { data: {}, message: 'Invalid API key'); } +//////////////////////////////////////////////////////////////// +// Time values. +// + +final timeInPast = DateTime.utc(2025, 4, 1, 8, 30, 0); + +/// The UNIX timestamp, in UTC seconds. +/// +/// This is the commonly used format in the Zulip API for timestamps. +int utcTimestamp([DateTime? dateTime]) { + dateTime ??= timeInPast; + return dateTime.toUtc().millisecondsSinceEpoch ~/ 1000; +} + //////////////////////////////////////////////////////////////// // Realm-wide (or server-wide) metadata. // @@ -469,7 +483,7 @@ StreamMessage streamMessage({ 'last_edit_timestamp': lastEditTimestamp, 'subject': topic ?? _defaultTopic, 'submessages': submessages ?? [], - 'timestamp': timestamp ?? 1678139636, + 'timestamp': timestamp ?? utcTimestamp(), 'type': 'stream', }) as Map); } @@ -510,7 +524,7 @@ DmMessage dmMessage({ 'last_edit_timestamp': lastEditTimestamp, 'subject': '', 'submessages': submessages ?? [], - 'timestamp': timestamp ?? 1678139636, + 'timestamp': timestamp ?? utcTimestamp(), 'type': 'private', }) as Map); } @@ -659,7 +673,7 @@ UpdateMessageEvent updateMessageEditEvent( messageId: messageId, messageIds: [messageId], flags: flags ?? origMessage.flags, - editTimestamp: editTimestamp ?? 1234567890, // TODO generate timestamp + editTimestamp: editTimestamp ?? utcTimestamp(), moveData: null, origContent: 'some probably-mismatched old Markdown', origRenderedContent: origMessage.content, @@ -690,7 +704,7 @@ UpdateMessageEvent _updateMessageMoveEvent( messageId: messageIds.first, messageIds: messageIds, flags: flags, - editTimestamp: 1234567890, // TODO generate timestamp + editTimestamp: utcTimestamp(), moveData: UpdateMessageMoveData( origStreamId: origStreamId, newStreamId: newStreamId ?? origStreamId, From 01e2c9405db614000f5b7c4bb7c93eee3fbb3eec Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 22 Apr 2025 13:03:37 -0400 Subject: [PATCH 04/19] binding [nfc]: Add utcNow This will be the same as `DateTime.timestamp()` in live code (therefore the NFC). For testing, utcNow uses a clock instance that can be controlled by FakeAsync. We could have made call sites of `DateTime.now()` use it too, but those for now don't need it for testing. --- lib/model/binding.dart | 8 ++++++++ test/model/binding.dart | 3 +++ 2 files changed, 11 insertions(+) diff --git a/lib/model/binding.dart b/lib/model/binding.dart index 9c66346bec..18af4c5d94 100644 --- a/lib/model/binding.dart +++ b/lib/model/binding.dart @@ -113,6 +113,11 @@ abstract class ZulipBinding { /// This wraps [url_launcher.closeInAppWebView]. Future closeInAppWebView(); + /// Provides access to the current UTC date and time. + /// + /// Outside tests, this just calls [DateTime.timestamp]. + DateTime utcNow(); + /// Provides access to a new stopwatch. /// /// Outside tests, this just calls the [Stopwatch] constructor. @@ -365,6 +370,9 @@ class LiveZulipBinding extends ZulipBinding { return url_launcher.closeInAppWebView(); } + @override + DateTime utcNow() => DateTime.timestamp(); + @override Stopwatch stopwatch() => Stopwatch(); diff --git a/test/model/binding.dart b/test/model/binding.dart index 73b4ed5513..29f5e4e1f7 100644 --- a/test/model/binding.dart +++ b/test/model/binding.dart @@ -238,6 +238,9 @@ class TestZulipBinding extends ZulipBinding { _closeInAppWebViewCallCount++; } + @override + DateTime utcNow() => clock.now().toUtc(); + @override Stopwatch stopwatch() => clock.stopwatch(); From e6c9192f05fdab13dee0d5b7c0138528399ad386 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 25 Mar 2025 16:32:30 -0400 Subject: [PATCH 05/19] message: Create an outbox message on send; manage its states While we do create outbox messages, there are in no way user-visible changes since the outbox messages don't end up in message list views. We create skeletons for helpers needed from message list view, but don't implement them yet, to make the diff smaller. For testing, similar to TypingNotifier.debugEnable, we add MessageStoreImpl.debugOutboxEnable for tests that do not intend to cover outbox messages. Some of the delays to fake responses added in tests are not necessary because the future of sendMessage is not completed immediately, but we still add them to keep the tests realistic. --- lib/model/message.dart | 348 ++++++++++++++++++++++++- lib/model/message_list.dart | 18 ++ lib/model/store.dart | 8 +- test/api/model/model_checks.dart | 1 + test/example_data.dart | 4 +- test/fake_async_checks.dart | 6 + test/model/message_checks.dart | 9 + test/model/message_test.dart | 379 +++++++++++++++++++++++++++- test/model/narrow_test.dart | 81 +++--- test/model/store_test.dart | 5 +- test/widgets/compose_box_test.dart | 11 + test/widgets/message_list_test.dart | 11 +- 12 files changed, 821 insertions(+), 60 deletions(-) create mode 100644 test/fake_async_checks.dart create mode 100644 test/model/message_checks.dart diff --git a/lib/model/message.dart b/lib/model/message.dart index fd5de1adbd..e2f6b02063 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -1,19 +1,310 @@ +import 'dart:async'; +import 'dart:collection'; import 'dart:convert'; +import 'package:flutter/foundation.dart'; + import '../api/model/events.dart'; import '../api/model/model.dart'; import '../api/route/messages.dart'; import '../log.dart'; +import 'binding.dart'; import 'message_list.dart'; import 'store.dart'; const _apiSendMessage = sendMessage; // Bit ugly; for alternatives, see: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20PerAccountStore.20methods/near/1545809 +const kLocalEchoDebounceDuration = Duration(milliseconds: 300); // TODO(#1441) find the right values for this +const kSendMessageRetryWaitPeriod = Duration(seconds: 10); // TODO(#1441) find the right values for this + +/// States of an [OutboxMessage] since its creation from a +/// [MessageStore.sendMessage] call and before its eventual deletion. +/// +/// ``` +/// 4xx or other User restores +/// error. the draft. +/// ┌──────┬─────────────────┬──► failed ──────────┐ +/// │ ▲ ▲ ▼ +/// (create) ─► hidden └─── waiting └─ waitPeriodExpired ─┴► (delete) +/// │ ▲ │ ▲ +/// └────────────┘ └──────────┘ +/// Debounce Wait period +/// timed out. timed out. +/// +/// Event received. +/// Or we abandoned the queue. +/// (any state) ────────────────────────────► (delete) +/// ``` +/// +/// During its lifecycle, it is guaranteed that the outbox message is deleted +/// as soon a message event with a matching [MessageEvent.localMessageId] +/// arrives. +enum OutboxMessageState { + /// The [sendMessage] request has started but hasn't finished, and the + /// outbox message is hidden to the user. + /// + /// This is the initial state when an [OutboxMessage] is created. + hidden, + + /// The [sendMessage] request has started but hasn't finished, and the + /// outbox message is shown to the user. + /// + /// This state can be reached after staying in [hidden] for + /// [kLocalEchoDebounceDuration]. + waiting, + + /// The message was assumed not delivered after some time it was sent. + /// + /// This state can be reached when the message event hasn't arrived in + /// [kSendMessageRetryWaitPeriod] since the outbox message's creation. + waitPeriodExpired, + + /// The message could not be delivered. + /// + /// This state can be reached when we got a 4xx or other error in the HTTP + /// response. + failed, +} + +/// A message sent by the self-user. +sealed class OutboxMessage extends MessageBase { + OutboxMessage({ + required this.localMessageId, + required int selfUserId, + required super.timestamp, + required this.content, + }) : _state = OutboxMessageState.hidden, + super(senderId: selfUserId); + + /// As in [MessageEvent.localMessageId]. + /// + /// This uniquely identifies this outbox message's corresponding message object + /// in events from the same event queue. + /// + /// See also: + /// * [MessageStoreImpl.sendMessage], where this ID is assigned. + final int localMessageId; + @override + int? get id => null; + final String content; + + OutboxMessageState get state => _state; + OutboxMessageState _state; + set state(OutboxMessageState value) { + // See [OutboxMessageState] for valid state transitions. + assert(_state != value); + switch (value) { + case OutboxMessageState.hidden: + assert(false); + case OutboxMessageState.waiting: + assert(_state == OutboxMessageState.hidden); + case OutboxMessageState.waitPeriodExpired: + assert(_state == OutboxMessageState.waiting); + case OutboxMessageState.failed: + assert(_state == OutboxMessageState.hidden + || _state == OutboxMessageState.waiting + || _state == OutboxMessageState.waitPeriodExpired); + } + _state = value; + } + + /// Whether the [OutboxMessage] is hidden to [MessageListView] or not. + bool get hidden => _state == OutboxMessageState.hidden; +} + +class StreamOutboxMessage extends OutboxMessage { + StreamOutboxMessage({ + required super.localMessageId, + required super.selfUserId, + required super.timestamp, + required this.conversation, + required super.content, + }); + + @override + final StreamConversation conversation; +} + +class DmOutboxMessage extends OutboxMessage { + DmOutboxMessage({ + required super.localMessageId, + required super.selfUserId, + required super.timestamp, + required this.conversation, + required super.content, + }) : assert(conversation.allRecipientIds.contains(selfUserId)); + + @override + final DmConversation conversation; +} + +/// Manages the outbox messages portion of [MessageStore]. +mixin _OutboxMessageStore on PerAccountStoreBase { + late final UnmodifiableMapView outboxMessages = + UnmodifiableMapView(_outboxMessages); + final Map _outboxMessages = {}; + + /// A map of timers to show outbox messages after a delay, + /// indexed by [OutboxMessage.localMessageId]. + /// + /// If the send message request failed within the time limit, + /// the outbox message's timer gets removed and cancelled. + final Map _outboxMessageDebounceTimers = {}; + + /// A map of timers to update outbox messages state to + /// [OutboxMessageState.waitPeriodExpired] after a delay, + /// indexed by [OutboxMessage.localMessageId]. + /// + /// If the send message request failed within the time limit, + /// the outbox message's timer gets removed and cancelled. + final Map _outboxMessageWaitPeriodTimers = {}; + + /// A fresh ID to use for [OutboxMessage.localMessageId], + /// unique within this instance. + int _nextLocalMessageId = 0; + + Set get _messageListViews; + + /// Update the state of the [OutboxMessage] with the given [localMessageId], + /// and notify listeners if necessary. + /// + /// This is a no-op if the outbox message does not exist, or that + /// [OutboxMessage.state] already equals [newState]. + void _updateOutboxMessage(int localMessageId, { + required OutboxMessageState newState, + }) { + final outboxMessage = outboxMessages[localMessageId]; + if (outboxMessage == null || outboxMessage.state == newState) { + return; + } + final wasFirstShown = outboxMessage.state == OutboxMessageState.hidden; + outboxMessage.state = newState; + for (final view in _messageListViews) { + if (wasFirstShown) { + view.addOutboxMessage(outboxMessage); + } else { + view.notifyListenersIfOutboxMessagePresent(localMessageId); + } + } + } + + /// Send a message and create an entry of [OutboxMessage]. + Future outboxSendMessage({ + required MessageDestination destination, + required String content, + required String? realmEmptyTopicDisplayName, + }) async { + final localMessageId = _nextLocalMessageId++; + assert(!outboxMessages.containsKey(localMessageId)); + + final now = (ZulipBinding.instance.utcNow().millisecondsSinceEpoch / 1000).toInt(); + _outboxMessages[localMessageId] = switch (destination) { + StreamDestination(:final streamId, :final topic) => StreamOutboxMessage( + localMessageId: localMessageId, + selfUserId: selfUserId, + timestamp: now, + conversation: StreamConversation( + streamId, + topic.interpretAsServer( + // Because either of the values can get updated, the actual topic + // can change, for example, between "(no topic)" and "general chat", + // or between different names of "general chat". This should be + // uncommon during the lifespan of an outbox message. + // + // There's also an unavoidable race that has the same effect: + // an admin could change the name of "general chat" + // (i.e. the value of realmEmptyTopicDisplayName) concurrently with + // the user making the send request, so that the setting in effect + // by the time the request arrives is different from the setting the + // client last heard about. The realm update events do not have + // information about this race for us to update the prediction + // correctly. + zulipFeatureLevel: zulipFeatureLevel, + realmEmptyTopicDisplayName: realmEmptyTopicDisplayName), + displayRecipient: null), + content: content), + DmDestination(:final userIds) => DmOutboxMessage( + localMessageId: localMessageId, + selfUserId: selfUserId, + timestamp: now, + conversation: DmConversation(allRecipientIds: userIds), + content: content), + }; + + _outboxMessageDebounceTimers[localMessageId] = Timer(kLocalEchoDebounceDuration, () { + assert(outboxMessages.containsKey(localMessageId)); + _outboxMessageDebounceTimers.remove(localMessageId); + _updateOutboxMessage(localMessageId, newState: OutboxMessageState.waiting); + }); + + _outboxMessageWaitPeriodTimers[localMessageId] = Timer(kSendMessageRetryWaitPeriod, () { + assert(outboxMessages.containsKey(localMessageId)); + _outboxMessageWaitPeriodTimers.remove(localMessageId); + _updateOutboxMessage(localMessageId, newState: OutboxMessageState.waitPeriodExpired); + }); + + try { + await _apiSendMessage(connection, + destination: destination, + content: content, + readBySender: true, + queueId: queueId, + localId: localMessageId.toString()); + } catch (e) { + // `localMessageId` is not necessarily in the store. This is because + // message event can still arrive before the send request fails to + // networking issues. + _outboxMessageDebounceTimers.remove(localMessageId)?.cancel(); + _outboxMessageWaitPeriodTimers.remove(localMessageId)?.cancel(); + _updateOutboxMessage(localMessageId, newState: OutboxMessageState.failed); + rethrow; + } + } + + void removeOutboxMessage(int localMessageId) { + final removed = _outboxMessages.remove(localMessageId); + _outboxMessageDebounceTimers.remove(localMessageId)?.cancel(); + _outboxMessageWaitPeriodTimers.remove(localMessageId)?.cancel(); + if (removed == null) { + assert(false, 'Removing unknown outbox message with localMessageId: $localMessageId'); + return; + } + for (final view in _messageListViews) { + view.removeOutboxMessage(removed); + } + } + + void _handleMessageEventOutbox(MessageEvent event) { + if (event.localMessageId != null) { + final localMessageId = int.parse(event.localMessageId!, radix: 10); + // The outbox message can be missing if the user removes it (to be + // implemented in #1441) before the event arrives. + // Nothing to do in that case. + _outboxMessages.remove(localMessageId); + _outboxMessageDebounceTimers.remove(localMessageId)?.cancel(); + _outboxMessageWaitPeriodTimers.remove(localMessageId)?.cancel(); + } + } + + /// Remove all outbox messages, and cancel pending timers. + void _clearOutboxMessages() { + for (final localMessageId in outboxMessages.keys) { + _outboxMessageDebounceTimers.remove(localMessageId)?.cancel(); + _outboxMessageWaitPeriodTimers.remove(localMessageId)?.cancel(); + } + _outboxMessages.clear(); + assert(_outboxMessageDebounceTimers.isEmpty); + assert(_outboxMessageWaitPeriodTimers.isEmpty); + } +} /// The portion of [PerAccountStore] for messages and message lists. mixin MessageStore { /// All known messages, indexed by [Message.id]. Map get messages; + /// Messages sent by the user, indexed by [OutboxMessage.localMessageId]. + Map get outboxMessages; + Set get debugMessageListViews; void registerMessageList(MessageListView view); @@ -24,6 +315,11 @@ mixin MessageStore { required String content, }); + /// Remove from [outboxMessages] given the [localMessageId]. + /// + /// The message to remove must exist. + void removeOutboxMessage(int localMessageId); + /// Reconcile a batch of just-fetched messages with the store, /// mutating the list. /// @@ -37,15 +333,18 @@ mixin MessageStore { void reconcileMessages(List messages); } -class MessageStoreImpl extends PerAccountStoreBase with MessageStore { - MessageStoreImpl({required super.core}) +class MessageStoreImpl extends PerAccountStoreBase with MessageStore, _OutboxMessageStore { + MessageStoreImpl({required super.core, required this.realmEmptyTopicDisplayName}) // There are no messages in InitialSnapshot, so we don't have // a use case for initializing MessageStore with nonempty [messages]. : messages = {}; + final String? realmEmptyTopicDisplayName; + @override final Map messages; + @override final Set _messageListViews = {}; @override @@ -96,17 +395,21 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { // [InheritedNotifier] to rebuild in the next frame) before the owner's // `dispose` or `onNewStore` is called. Discussion: // https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/MessageListView.20lifecycle/near/2086893 + + _clearOutboxMessages(); } @override Future sendMessage({required MessageDestination destination, required String content}) { - // TODO implement outbox; see design at - // https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M3881.20Sending.20outbox.20messages.20is.20fraught.20with.20issues/near/1405739 - return _apiSendMessage(connection, - destination: destination, - content: content, - readBySender: true, - ); + if (!debugOutboxEnable) { + return _apiSendMessage(connection, + destination: destination, + content: content, + readBySender: true); + } + return outboxSendMessage( + destination: destination, content: content, + realmEmptyTopicDisplayName: realmEmptyTopicDisplayName); } @override @@ -144,6 +447,8 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { // See [fetchedMessages] for reasoning. messages[event.message.id] = event.message; + _handleMessageEventOutbox(event); + for (final view in _messageListViews) { view.handleMessageEvent(event); } @@ -330,4 +635,29 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { // [Poll] is responsible for notifying the affected listeners. poll.handleSubmessageEvent(event); } + + /// In debug mode, controls whether outbox messages should be created when + /// [sendMessage] is called. + /// + /// Outside of debug mode, this is always true and the setter has no effect. + static bool get debugOutboxEnable { + bool result = true; + assert(() { + result = _debugOutboxEnable; + return true; + }()); + return result; + } + static bool _debugOutboxEnable = true; + static set debugOutboxEnable(bool value) { + assert(() { + _debugOutboxEnable = value; + return true; + }()); + } + + @visibleForTesting + static void debugReset() { + _debugOutboxEnable = true; + } } diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 58a0e1bb95..e4b3f2150e 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -10,6 +10,7 @@ import '../api/route/messages.dart'; import 'algorithms.dart'; import 'channel.dart'; import 'content.dart'; +import 'message.dart'; import 'narrow.dart'; import 'store.dart'; @@ -626,6 +627,18 @@ class MessageListView with ChangeNotifier, _MessageSequence { } } + /// Add [outboxMessage] if it belongs to the view. + void addOutboxMessage(OutboxMessage outboxMessage) { + // TODO(#1441) implement this + } + + /// Remove the [outboxMessage] from the view. + /// + /// This is a no-op if the message is not found. + void removeOutboxMessage(OutboxMessage outboxMessage) { + // TODO(#1441) implement this + } + void handleUserTopicEvent(UserTopicEvent event) { switch (_canAffectVisibility(event)) { case VisibilityEffect.none: @@ -787,6 +800,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 + } + /// Called when the app is reassembled during debugging, e.g. for hot reload. /// /// This will redo from scratch any computations we can, such as parsing diff --git a/lib/model/store.dart b/lib/model/store.dart index 5fd3dd6518..b56c43e6c8 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -498,7 +498,8 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor typingStartedExpiryPeriod: Duration(milliseconds: initialSnapshot.serverTypingStartedExpiryPeriodMilliseconds), ), channels: channels, - messages: MessageStoreImpl(core: core), + messages: MessageStoreImpl(core: core, + realmEmptyTopicDisplayName: initialSnapshot.realmEmptyTopicDisplayName), unreads: Unreads( initial: initialSnapshot.unreadMsgs, core: core, @@ -732,6 +733,8 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor @override Map get messages => _messages.messages; @override + Map get outboxMessages => _messages.outboxMessages; + @override void registerMessageList(MessageListView view) => _messages.registerMessageList(view); @override @@ -911,6 +914,9 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor return _messages.sendMessage(destination: destination, content: content); } + @override + void removeOutboxMessage(int localMessageId) => _messages.removeOutboxMessage(localMessageId); + static List _sortCustomProfileFields(List initialCustomProfileFields) { // TODO(server): The realm-wide field objects have an `order` property, // but the actual API appears to be that the fields should be shown in diff --git a/test/api/model/model_checks.dart b/test/api/model/model_checks.dart index 8791c1b9d9..2216b2b98f 100644 --- a/test/api/model/model_checks.dart +++ b/test/api/model/model_checks.dart @@ -30,6 +30,7 @@ extension TopicNameChecks on Subject { } extension StreamConversationChecks on Subject { + Subject get topic => has((x) => x.topic, 'topic'); Subject get displayRecipient => has((x) => x.displayRecipient, 'displayRecipient'); } diff --git a/test/example_data.dart b/test/example_data.dart index 8e64692d05..1c1020a805 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -636,8 +636,8 @@ UserTopicEvent userTopicEvent( ); } -MessageEvent messageEvent(Message message) => - MessageEvent(id: 0, message: message, localMessageId: null); +MessageEvent messageEvent(Message message, {int? localMessageId}) => + MessageEvent(id: 0, message: message, localMessageId: localMessageId?.toString()); DeleteMessageEvent deleteMessageEvent(List messages) { assert(messages.isNotEmpty); diff --git a/test/fake_async_checks.dart b/test/fake_async_checks.dart new file mode 100644 index 0000000000..51c653123a --- /dev/null +++ b/test/fake_async_checks.dart @@ -0,0 +1,6 @@ +import 'package:checks/checks.dart'; +import 'package:fake_async/fake_async.dart'; + +extension FakeTimerChecks on Subject { + Subject get duration => has((t) => t.duration, 'duration'); +} diff --git a/test/model/message_checks.dart b/test/model/message_checks.dart new file mode 100644 index 0000000000..b56cd89a79 --- /dev/null +++ b/test/model/message_checks.dart @@ -0,0 +1,9 @@ +import 'package:checks/checks.dart'; +import 'package:zulip/api/model/model.dart'; +import 'package:zulip/model/message.dart'; + +extension OutboxMessageChecks on Subject> { + Subject get localMessageId => has((x) => x.localMessageId, 'localMessageId'); + Subject get state => has((x) => x.state, 'state'); + Subject get hidden => has((x) => x.hidden, 'hidden'); +} diff --git a/test/model/message_test.dart b/test/model/message_test.dart index 1f774e32b9..d090133899 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -1,10 +1,15 @@ +import 'dart:async'; import 'dart:convert'; import 'package:checks/checks.dart'; +import 'package:fake_async/fake_async.dart'; +import 'package:http/http.dart' as http; import 'package:test/scaffolding.dart'; import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/model.dart'; import 'package:zulip/api/model/submessage.dart'; +import 'package:zulip/api/route/messages.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'; @@ -13,12 +18,18 @@ import '../api/fake_api.dart'; import '../api/model/model_checks.dart'; import '../api/model/submessage_checks.dart'; import '../example_data.dart' as eg; +import '../fake_async.dart'; +import '../fake_async_checks.dart'; import '../stdlib_checks.dart'; +import 'binding.dart'; +import 'message_checks.dart'; import 'message_list_test.dart'; import 'store_checks.dart'; import 'test_store.dart'; void main() { + TestZulipBinding.ensureInitialized(); + // These "late" variables are the common state operated on by each test. // Each test case calls [prepare] to initialize them. late Subscription subscription; @@ -37,10 +48,16 @@ void main() { void checkNotifiedOnce() => checkNotified(count: 1); /// Initialize [store] and the rest of the test state. - Future prepare({Narrow narrow = const CombinedFeedNarrow()}) async { - final stream = eg.stream(streamId: eg.defaultStreamMessageStreamId); + Future prepare({ + Narrow narrow = const CombinedFeedNarrow(), + ZulipStream? stream, + int? zulipFeatureLevel, + }) async { + stream ??= eg.stream(streamId: eg.defaultStreamMessageStreamId); subscription = eg.subscription(stream); - store = eg.store(); + final account = eg.selfAccount.copyWith(zulipFeatureLevel: zulipFeatureLevel); + store = eg.store(account: account, + initialSnapshot: eg.initialSnapshot(zulipFeatureLevel: zulipFeatureLevel)); await store.addStream(stream); await store.addSubscription(subscription); connection = store.connection as FakeApiConnection; @@ -49,8 +66,12 @@ void main() { ..addListener(() { notifiedCount++; }); + addTearDown(messageList.dispose); check(messageList).fetched.isFalse(); checkNotNotified(); + + // This cleans up possibly pending timers from [MessageStoreImpl]. + addTearDown(store.dispose); } /// Perform the initial message fetch for [messageList]. @@ -75,6 +96,358 @@ void main() { checkNotified(count: messageList.fetched ? messages.length : 0); } + test('dispose cancels pending timers', () => awaitFakeAsync((async) async { + final stream = eg.stream(); + final store = eg.store(); + await store.addStream(stream); + await store.addSubscription(eg.subscription(stream)); + + (store.connection as FakeApiConnection).prepare( + json: SendMessageResult(id: 1).toJson()); + await store.sendMessage( + destination: StreamDestination(stream.streamId, eg.t('topic')), + content: 'content'); + check(async.pendingTimers).deepEquals(>[ + (it) => it.isA().duration.equals(kLocalEchoDebounceDuration), + (it) => it.isA().duration.equals(kSendMessageRetryWaitPeriod), + ]); + + store.dispose(); + check(async.pendingTimers).isEmpty(); + })); + + group('sendMessage', () { + final stream = eg.stream(); + final message = eg.streamMessage(stream: stream); + final streamDestination = StreamDestination(stream.streamId, eg.t('some topic')); + + test('outbox messages get unique localMessageId', () async { + await prepare(stream: stream); + await prepareMessages([]); + + for (int i = 0; i < 10; i++) { + connection.prepare(json: SendMessageResult(id: 1).toJson()); + await store.sendMessage(destination: streamDestination, content: 'content'); + } + // [store.outboxMessages] has the same number of keys (localMessageId) + // as the number of sent messages, which are guaranteed to be distinct. + check(store.outboxMessages).keys.length.equals(10); + }); + + late Future sendMessageFuture; + late OutboxMessage outboxMessage; + + Future prepareSendMessageToSucceed({ + MessageDestination? destination, + Duration delay = Duration.zero, + int? zulipFeatureLevel, + }) async { + await prepare(stream: stream, zulipFeatureLevel: zulipFeatureLevel); + await prepareMessages([eg.streamMessage(stream: stream)]); + connection.prepare(json: SendMessageResult(id: 1).toJson(), delay: delay); + sendMessageFuture = store.sendMessage( + destination: destination ?? streamDestination, content: 'content'); + outboxMessage = store.outboxMessages.values.single; + } + + Future prepareSendMessageToFail({ + Duration delay = Duration.zero, + }) async { + await prepare(stream: stream); + await prepareMessages([eg.streamMessage(stream: stream)]); + connection.prepare(apiException: eg.apiBadRequest(), delay: delay); + sendMessageFuture = store.sendMessage( + destination: streamDestination, content: 'content'); + + // This allows `async.elapse` to not fail when `sendMessageFuture` throws. + // The caller should still await the future since this does not await it. + unawaited(check(sendMessageFuture).throws()); + + outboxMessage = store.outboxMessages.values.single; + } + + test('while message is being sent, message event arrives, then the send succeeds', () => awaitFakeAsync((async) async { + // Send message with a delay in response, leaving time for the message + // event to arrive. + await prepareSendMessageToSucceed(delay: Duration(seconds: 1)); + check(connection.lastRequest).isA() + ..bodyFields['queue_id'].equals(store.queueId) + ..bodyFields['local_id'].equals('${outboxMessage.localMessageId}'); + check(outboxMessage).state.equals(OutboxMessageState.hidden); + + // Handle the message event before `future` completes, i.e. while the + // message is being sent. + await store.handleEvent(eg.messageEvent(message, + localMessageId: outboxMessage.localMessageId)); + check(store.outboxMessages).isEmpty(); + check(outboxMessage).state.equals(OutboxMessageState.hidden); + + // Complete the send request. The outbox message should no longer get + // updated because it is not in the store any more. + async.elapse(const Duration(seconds: 1)); + await sendMessageFuture; + check(outboxMessage).state.equals(OutboxMessageState.hidden); + })); + + test('while message is being sent, message event arrives, then the send fails', () => awaitFakeAsync((async) async { + // Set up an error to fail `sendMessage` with a delay, leaving time for + // the message event to arrive. + await prepareSendMessageToFail(delay: const Duration(seconds: 1)); + check(outboxMessage).state.equals(OutboxMessageState.hidden); + + // Handle the message event before `future` completes, i.e. while the + // message is being sent. + await store.handleEvent(eg.messageEvent(message, + localMessageId: outboxMessage.localMessageId)); + check(store.outboxMessages).isEmpty(); + check(outboxMessage).state.equals(OutboxMessageState.hidden); + + // Complete the send request with an error. The outbox message should no + // longer be updated because it is not in the store any more. + async.elapse(const Duration(seconds: 1)); + await check(sendMessageFuture).throws(); + check(outboxMessage).state.equals(OutboxMessageState.hidden); + })); + + test('message is sent successfully, message event arrives before debounce timeout', () async { + // Set up to successfully send the message immediately. + await prepareSendMessageToSucceed(); + await sendMessageFuture; + check(outboxMessage).state.equals(OutboxMessageState.hidden); + + // Handle the event after the message is sent but before the debounce + // timeout. + await store.handleEvent(eg.messageEvent(message, + localMessageId: outboxMessage.localMessageId)); + check(store.outboxMessages).isEmpty(); + // The outbox message should remain hidden since the send + // request was successful. + check(outboxMessage).state.equals(OutboxMessageState.hidden); + }); + + test('DM message is sent successfully, message event arrives before debounce timeout', () async { + // Set up to successfully send the message immediately. + await prepareSendMessageToSucceed(destination: DmDestination( + userIds: [eg.selfUser.userId, eg.otherUser.userId])); + await sendMessageFuture; + check(outboxMessage).state.equals(OutboxMessageState.hidden); + + // Handle the event after the message is sent but before the debounce + // timeout. + await store.handleEvent(eg.messageEvent( + eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]), + localMessageId: outboxMessage.localMessageId)); + check(store.outboxMessages).isEmpty(); + // The outbox message should remain hidden since the send + // request was successful. + check(outboxMessage).state.equals(OutboxMessageState.hidden); + }); + + test('message is sent successfully, message event arrives after debounce timeout', () => awaitFakeAsync((async) async { + // Set up to successfully send the message immediately. + await prepareSendMessageToSucceed(); + await sendMessageFuture; + check(outboxMessage).state.equals(OutboxMessageState.hidden); + + // Pass enough time without handling the message event, to expire + // the debounce timer. + async.elapse(kLocalEchoDebounceDuration); + check(store.outboxMessages).values.single.identicalTo(outboxMessage); + check(outboxMessage).state.equals(OutboxMessageState.waiting); + + // Handle the event when the outbox message is in waiting state. + // The outbox message should be removed without errors. + await store.handleEvent(eg.messageEvent(message, + localMessageId: outboxMessage.localMessageId)); + check(store.outboxMessages).isEmpty(); + // The outbox message should no longer be updated because it is not in + // the store any more. + check(outboxMessage).state.equals(OutboxMessageState.waiting); + })); + + test('message failed to send before debounce timeout', () => awaitFakeAsync((async) async { + // Set up to fail the send request, but do not complete it yet, to + // check the initial states. + await prepareSendMessageToFail(); + check(outboxMessage).state.equals(OutboxMessageState.hidden); + check(async.pendingTimers).deepEquals(>[ + (it) => it.isA().duration.equals(kLocalEchoDebounceDuration), + (it) => it.isA().duration.equals(kSendMessageRetryWaitPeriod), + (it) => it.isA().duration.equals(Duration.zero), // timer for send-message response + ]); + + // Complete the send request with an error. + await check(sendMessageFuture).throws(); + check(store.outboxMessages).values.single.identicalTo(outboxMessage); + check(outboxMessage).state.equals(OutboxMessageState.failed); + // Both the debounce timer and wait period timer should have been cancelled. + check(async.pendingTimers).isEmpty(); + })); + + test('message failed to send after debounce timeout', () => awaitFakeAsync((async) async { + // Set up to fail the send request, but only after the debounce timeout. + await prepareSendMessageToFail( + delay: kLocalEchoDebounceDuration + const Duration(milliseconds: 1)); + check(outboxMessage).state.equals(OutboxMessageState.hidden); + + // Wait for just enough time for the debounce timer to expire, but not + // for the send request to complete. + async.elapse(kLocalEchoDebounceDuration); + check(store.outboxMessages).values.single.identicalTo(outboxMessage); + check(outboxMessage).state.equals(OutboxMessageState.waiting); + + // Complete the send request with an error. + async.elapse(const Duration(milliseconds: 1)); + await check(sendMessageFuture).throws(); + check(store.outboxMessages).values.single.identicalTo(outboxMessage); + check(outboxMessage).state.equals(OutboxMessageState.failed); + })); + + test('message failed to send, message event arrives', () async { + // Set up to fail the send request immediately. + await prepareSendMessageToFail(); + await check(sendMessageFuture).throws(); + check(outboxMessage).state.equals(OutboxMessageState.failed); + + // Handle the event when the outbox message is in failed state. + // The outbox message should be removed without errors. + await store.handleEvent(eg.messageEvent(message, + localMessageId: outboxMessage.localMessageId)); + check(store.outboxMessages).isEmpty(); + // The outbox message should no longer be updated because it is not in + // the store any more. + check(outboxMessage).state.equals(OutboxMessageState.failed); + }); + + test('send request pending until after kSendMessageRetryWaitPeriod, completes successfully, then message event arrives', () => awaitFakeAsync((async) async { + // Send a message, but keep it pending until after reaching + // [kSendMessageRetryWaitPeriod]. + await prepareSendMessageToSucceed( + delay: kSendMessageRetryWaitPeriod + Duration(seconds: 1)); + async.elapse(kLocalEchoDebounceDuration); + check(outboxMessage).state.equals(OutboxMessageState.waiting); + + // Wait till we reach [kSendMessageRetryWaitPeriod] after the send request + // was initiated, but before it actually completes. + assert(kSendMessageRetryWaitPeriod > kLocalEchoDebounceDuration); + async.elapse(kSendMessageRetryWaitPeriod - kLocalEchoDebounceDuration); + check(outboxMessage).state.equals(OutboxMessageState.waitPeriodExpired); + + // Wait till the send request completes successfully. + async.elapse(const Duration(seconds: 1)); + await sendMessageFuture; + // The outbox message should remain in the store … + check(store.outboxMessages).values.single.identicalTo(outboxMessage); + // … and stay in the waitPeriodExpired state. + check(outboxMessage).state.equals(OutboxMessageState.waitPeriodExpired); + + // Handle the message event. The outbox message should get removed + // without errors. + await store.handleEvent(eg.messageEvent(message, + localMessageId: outboxMessage.localMessageId)); + check(store.outboxMessages).isEmpty(); + // The outbox message should no longer be updated because it is not in + // the store any more. + check(outboxMessage).state.equals(OutboxMessageState.waitPeriodExpired); + })); + + test('send request pending until after kSendMessageRetryWaitPeriod, then fails', () => awaitFakeAsync((async) async { + // Send a message, but keep it pending until after reaching + // [kSendMessageRetryWaitPeriod]. + await prepareSendMessageToFail( + delay: kSendMessageRetryWaitPeriod + Duration(seconds: 1)); + async.elapse(kLocalEchoDebounceDuration); + check(outboxMessage).state.equals(OutboxMessageState.waiting); + + // Wait till we reach [kSendMessageRetryWaitPeriod] after the send request + // was initiated, but before it fails. + assert(kSendMessageRetryWaitPeriod > kLocalEchoDebounceDuration); + async.elapse(kSendMessageRetryWaitPeriod - kLocalEchoDebounceDuration); + check(outboxMessage).state.equals(OutboxMessageState.waitPeriodExpired); + + // Wait till the send request fails. + async.elapse(Duration(seconds: 1)); + await check(sendMessageFuture).throws(); + // The outbox message should remain in the store … + check(store.outboxMessages).values.single.identicalTo(outboxMessage); + // … and transition to failed state. + check(outboxMessage).state.equals(OutboxMessageState.failed); + })); + + test('send request completes, message event does not arrive after kSendMessageRetryWaitPeriod', () => awaitFakeAsync((async) async { + // Send a message and have it complete successfully without wait. + await prepareSendMessageToSucceed(); + async.elapse(kLocalEchoDebounceDuration); + check(outboxMessage).state.equals(OutboxMessageState.waiting); + + // Wait till we reach [kSendMessageRetryWaitPeriod] after the send request + // was initiated. + assert(kSendMessageRetryWaitPeriod > kLocalEchoDebounceDuration); + async.elapse(kSendMessageRetryWaitPeriod - kLocalEchoDebounceDuration); + // The outbox message should transition to waitPeriodExpired state. + check(outboxMessage).state.equals(OutboxMessageState.waitPeriodExpired); + })); + + test('send request fails, message event does not arrive after kSendMessageRetryWaitPeriod', () => awaitFakeAsync((async) async { + // Send a message and have it fail without wait. + await prepareSendMessageToFail(); + async.elapse(kLocalEchoDebounceDuration); + check(outboxMessage).state.equals(OutboxMessageState.failed); + + // Wait till we reach [kSendMessageRetryWaitPeriod] after the send request + // was initiated. + assert(kSendMessageRetryWaitPeriod > kLocalEchoDebounceDuration); + async.elapse(kSendMessageRetryWaitPeriod - kLocalEchoDebounceDuration); + // The outbox message should stay in failed state, + // and it should not transition to waitPeriodExpired state. + check(outboxMessage).state.equals(OutboxMessageState.failed); + })); + + test('when sending to empty topic, interpret topic like the server does when creating outbox message', () => awaitFakeAsync((async) async { + // Send a message and have it complete successfully without wait. + await prepareSendMessageToSucceed( + destination: StreamDestination(stream.streamId, TopicName('(no topic)')), + zulipFeatureLevel: 334); + async.elapse(kLocalEchoDebounceDuration); + check(outboxMessage).conversation.isA() + .topic.equals(eg.t(eg.defaultRealmEmptyTopicDisplayName)); + })); + + test('legacy: when sending to empty topic, interpret topic like the server does when creating outbox message', () => awaitFakeAsync((async) async { + // Send a message and have it complete successfully without wait. + await prepareSendMessageToSucceed( + destination: StreamDestination(stream.streamId, TopicName('(no topic)')), + zulipFeatureLevel: 333); + async.elapse(kLocalEchoDebounceDuration); + check(outboxMessage).conversation.isA() + .topic.equals(eg.t('(no topic)')); + })); + + test('set timestamp to now when creating outbox messages', () => awaitFakeAsync( + initialTime: eg.timeInPast, + (async) async { + await prepareSendMessageToSucceed(); + check(outboxMessage).timestamp.equals(eg.utcTimestamp(eg.timeInPast)); + })); + }); + + test('removeOutboxMessage', () async { + final stream = eg.stream(); + await prepare(stream: stream); + await prepareMessages([]); + + for (int i = 0; i < 10; i++) { + connection.prepare(json: SendMessageResult(id: 1).toJson()); + await store.sendMessage( + destination: StreamDestination(stream.streamId, eg.t('topic')), + content: 'content'); + } + + final localMessageIds = store.outboxMessages.keys.toList(); + store.removeOutboxMessage(localMessageIds.removeAt(5)); + check(store.outboxMessages.keys).deepEquals(localMessageIds); + }); + group('reconcileMessages', () { test('from empty', () async { await prepare(); diff --git a/test/model/narrow_test.dart b/test/model/narrow_test.dart index 06c82ed117..535e6d7e61 100644 --- a/test/model/narrow_test.dart +++ b/test/model/narrow_test.dart @@ -2,38 +2,37 @@ import 'package:checks/checks.dart'; import 'package:test/scaffolding.dart'; import 'package:zulip/api/model/model.dart'; +import 'package:zulip/model/message.dart'; import 'package:zulip/model/narrow.dart'; import '../example_data.dart' as eg; import 'narrow_checks.dart'; -/// A [MessageBase] subclass for testing. -// TODO(#1441): switch to outbox-messages instead -sealed class _TestMessage extends MessageBase { - @override - final int? id = null; - - _TestMessage() : super(senderId: eg.selfUser.userId, timestamp: 123456789); -} - -class _TestStreamMessage extends _TestMessage { - @override - final StreamConversation conversation; - - _TestStreamMessage({required ZulipStream stream, required String topic}) - : conversation = StreamConversation( - stream.streamId, TopicName(topic), displayRecipient: null); -} - -class _TestDmMessage extends _TestMessage { - @override - final DmConversation conversation; - - _TestDmMessage({required List allRecipientIds}) - : conversation = DmConversation(allRecipientIds: allRecipientIds); -} - void main() { + int nextLocalMessageId = 1; + + StreamOutboxMessage streamOutboxMessage({ + required ZulipStream stream, + required String topic, + }) { + return StreamOutboxMessage( + localMessageId: nextLocalMessageId++, + selfUserId: eg.selfUser.userId, + timestamp: 123456789, + conversation: StreamConversation( + stream.streamId, TopicName(topic), displayRecipient: null), + content: 'content'); + } + + DmOutboxMessage dmOutboxMessage({required List allRecipientIds}) { + return DmOutboxMessage( + localMessageId: nextLocalMessageId++, + selfUserId: allRecipientIds[0], + timestamp: 123456789, + conversation: DmConversation(allRecipientIds: allRecipientIds), + content: 'content'); + } + group('SendableNarrow', () { test('ofMessage: stream message', () { final message = eg.streamMessage(); @@ -61,11 +60,11 @@ void main() { eg.streamMessage(stream: stream, topic: 'topic'))).isTrue(); check(narrow.containsMessage( - _TestDmMessage(allRecipientIds: [1]))).isFalse(); + dmOutboxMessage(allRecipientIds: [1]))).isFalse(); check(narrow.containsMessage( - _TestStreamMessage(stream: otherStream, topic: 'topic'))).isFalse(); + streamOutboxMessage(stream: otherStream, topic: 'topic'))).isFalse(); check(narrow.containsMessage( - _TestStreamMessage(stream: stream, topic: 'topic'))).isTrue(); + streamOutboxMessage(stream: stream, topic: 'topic'))).isTrue(); }); }); @@ -91,13 +90,13 @@ void main() { eg.streamMessage(stream: stream, topic: 'topic'))).isTrue(); check(narrow.containsMessage( - _TestDmMessage(allRecipientIds: [1]))).isFalse(); + dmOutboxMessage(allRecipientIds: [1]))).isFalse(); check(narrow.containsMessage( - _TestStreamMessage(stream: otherStream, topic: 'topic'))).isFalse(); + streamOutboxMessage(stream: otherStream, topic: 'topic'))).isFalse(); check(narrow.containsMessage( - _TestStreamMessage(stream: stream, topic: 'topic2'))).isFalse(); + streamOutboxMessage(stream: stream, topic: 'topic2'))).isFalse(); check(narrow.containsMessage( - _TestStreamMessage(stream: stream, topic: 'topic'))).isTrue(); + streamOutboxMessage(stream: stream, topic: 'topic'))).isTrue(); }); }); @@ -223,13 +222,13 @@ void main() { final narrow = DmNarrow(allRecipientIds: [1, 2], selfUserId: 2); check(narrow.containsMessage( - _TestStreamMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); + streamOutboxMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); check(narrow.containsMessage( - _TestDmMessage(allRecipientIds: [2]))).isFalse(); + dmOutboxMessage(allRecipientIds: [2]))).isFalse(); check(narrow.containsMessage( - _TestDmMessage(allRecipientIds: [2, 3]))).isFalse(); + dmOutboxMessage(allRecipientIds: [2, 3]))).isFalse(); check(narrow.containsMessage( - _TestDmMessage(allRecipientIds: [1, 2]))).isTrue(); + dmOutboxMessage(allRecipientIds: [1, 2]))).isTrue(); }); }); @@ -245,9 +244,9 @@ void main() { eg.streamMessage(flags: [MessageFlag.wildcardMentioned]))).isTrue(); check(narrow.containsMessage( - _TestStreamMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); + streamOutboxMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); check(narrow.containsMessage( - _TestDmMessage(allRecipientIds: [eg.selfUser.userId]))).isFalse(); + dmOutboxMessage(allRecipientIds: [eg.selfUser.userId]))).isFalse(); }); }); @@ -261,9 +260,9 @@ void main() { eg.streamMessage(flags:[MessageFlag.starred]))).isTrue(); check(narrow.containsMessage( - _TestStreamMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); + streamOutboxMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); check(narrow.containsMessage( - _TestDmMessage(allRecipientIds: [eg.selfUser.userId]))).isFalse(); + dmOutboxMessage(allRecipientIds: [eg.selfUser.userId]))).isFalse(); }); }); } diff --git a/test/model/store_test.dart b/test/model/store_test.dart index 1dfbb51273..405a46dc9f 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -569,7 +569,8 @@ void main() { group('PerAccountStore.sendMessage', () { test('smoke', () async { - final store = eg.store(); + final store = eg.store(initialSnapshot: eg.initialSnapshot( + queueId: 'fb67bf8a-c031-47cc-84cf-ed80accacda8')); final connection = store.connection as FakeApiConnection; final stream = eg.stream(); connection.prepare(json: SendMessageResult(id: 12345).toJson()); @@ -585,6 +586,8 @@ void main() { 'topic': 'world', 'content': 'hello', 'read_by_sender': 'true', + 'queue_id': 'fb67bf8a-c031-47cc-84cf-ed80accacda8', + 'local_id': store.outboxMessages.keys.single.toString(), }); }); }); diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart index e02fd97a15..4d281f3b13 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -14,6 +14,7 @@ import 'package:zulip/api/model/model.dart'; import 'package:zulip/api/route/channels.dart'; import 'package:zulip/api/route/messages.dart'; import 'package:zulip/model/localizations.dart'; +import 'package:zulip/model/message.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/model/typing_status.dart'; @@ -252,6 +253,8 @@ void main() { Future prepareWithContent(WidgetTester tester, String content) async { TypingNotifier.debugEnable = false; addTearDown(TypingNotifier.debugReset); + MessageStoreImpl.debugOutboxEnable = false; + addTearDown(MessageStoreImpl.debugReset); final narrow = ChannelNarrow(channel.streamId); await prepareComposeBox(tester, narrow: narrow, streams: [channel]); @@ -289,6 +292,8 @@ void main() { Future prepareWithTopic(WidgetTester tester, String topic) async { TypingNotifier.debugEnable = false; addTearDown(TypingNotifier.debugReset); + MessageStoreImpl.debugOutboxEnable = false; + addTearDown(MessageStoreImpl.debugReset); final narrow = ChannelNarrow(channel.streamId); await prepareComposeBox(tester, narrow: narrow, streams: [channel]); @@ -606,6 +611,8 @@ void main() { }); testWidgets('hitting send button sends a "typing stopped" notice', (tester) async { + MessageStoreImpl.debugOutboxEnable = false; + addTearDown(MessageStoreImpl.debugReset); await prepareComposeBox(tester, narrow: narrow, streams: [channel]); await checkStartTyping(tester, narrow); @@ -712,6 +719,8 @@ void main() { }) async { TypingNotifier.debugEnable = false; addTearDown(TypingNotifier.debugReset); + MessageStoreImpl.debugOutboxEnable = false; + addTearDown(MessageStoreImpl.debugReset); final zulipLocalizations = GlobalLocalizations.zulipLocalizations; await prepareComposeBox(tester, narrow: eg.topicNarrow(123, 'some topic'), @@ -766,6 +775,8 @@ void main() { }) async { TypingNotifier.debugEnable = false; addTearDown(TypingNotifier.debugReset); + MessageStoreImpl.debugOutboxEnable = false; + addTearDown(MessageStoreImpl.debugReset); channel = eg.stream(); final narrow = ChannelNarrow(channel.streamId); diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 18717d2c93..d17d032adc 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -841,7 +841,8 @@ void main() { connection.prepare(json: SendMessageResult(id: 1).toJson()); await tester.tap(find.byIcon(ZulipIcons.send)); - await tester.pump(); + await tester.pump(Duration.zero); + final localMessageId = store.outboxMessages.keys.single; check(connection.lastRequest).isA() ..method.equals('POST') ..url.path.equals('/api/v1/messages') @@ -850,8 +851,12 @@ void main() { 'to': '${otherChannel.streamId}', 'topic': 'new topic', 'content': 'Some text', - 'read_by_sender': 'true'}); - await tester.pumpAndSettle(); + 'read_by_sender': 'true', + 'queue_id': store.queueId, + 'local_id': localMessageId.toString()}); + // Remove the outbox message and its timers created when sending message. + await store.handleEvent( + eg.messageEvent(message, localMessageId: localMessageId)); }); testWidgets('Move to narrow with existing messages', (tester) async { From ccc74cd1896f7dc4b44b9822c4c05031be30f9b6 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 1 Apr 2025 15:55:29 -0400 Subject: [PATCH 06/19] msglist [nfc]: Extract _maybeAppendAuxillaryItem Also removed a stale comment that refers to resolved issues (#173 and #175). We will reuse this helper when handling outbox messages. --- lib/model/message_list.dart | 41 ++++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index e4b3f2150e..7fff16814f 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -284,36 +284,45 @@ mixin _MessageSequence { _reprocessAll(); } - /// Append to [items] based on the index-th message and its content. + /// Append to [items] an auxillary item like a date separator and update + /// properties of the previous message item, if necessary. /// - /// 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) { - // This will get more complicated to handle the ways that messages interact - // with the display of neighboring messages: sender headings #175 - // and date separators #173. - final message = messages[index]; - final content = contents[index]; - bool canShareSender; - if (index == 0 || !haveSameRecipient(messages[index - 1], message)) { + /// Returns whether an item has been appended or not. + /// + /// The caller must append a [MessageListMessageItem] after this. + bool _maybeAppendAuxillaryItem(Message message, {required Message? prevMessage}) { + if (prevMessage == null || !haveSameRecipient(prevMessage, message)) { items.add(MessageListRecipientHeaderItem(message)); - canShareSender = false; + return true; } else { assert(items.last is MessageListMessageItem); final prevMessageItem = items.last as MessageListMessageItem; - assert(identical(prevMessageItem.message, messages[index - 1])); + assert(identical(prevMessageItem.message, prevMessage)); assert(prevMessageItem.isLastInBlock); prevMessageItem.isLastInBlock = false; if (!messagesSameDay(prevMessageItem.message, message)) { items.add(MessageListDateSeparatorItem(message)); - canShareSender = false; + return true; } else { - canShareSender = (prevMessageItem.message.senderId == message.senderId); + return false; } } + } + + /// Append to [items] based on the index-th message and its content. + /// + /// 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) { + final prevMessage = index == 0 ? null : messages[index - 1]; + final message = messages[index]; + final content = contents[index]; + + final appended = _maybeAppendAuxillaryItem(message, prevMessage: prevMessage); items.add(MessageListMessageItem(message, content, - showSender: !canShareSender, isLastInBlock: true)); + showSender: appended || prevMessage?.senderId != message.senderId, + isLastInBlock: true)); } /// Update [items] to include markers at start and end as appropriate. From cb8e2c47857b5601d7adcb11de88e2aa6785e0bb Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 22 Apr 2025 17:16:35 -0400 Subject: [PATCH 07/19] msglist [nfc]: Extract _SenderRow widget --- lib/widgets/message_list.dart | 51 ++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 22 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index f00f873b1a..79f7b15ea1 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -1323,14 +1323,13 @@ String formatHeaderDate( } } -/// A Zulip message, showing the sender's name and avatar if specified. -// Design referenced from: -// - https://github.com/zulip/zulip-mobile/issues/5511 -// - https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=538%3A20849&mode=dev -class MessageWithPossibleSender extends StatelessWidget { - const MessageWithPossibleSender({super.key, required this.item}); +// TODO(i18n): web seems to ignore locale in formatting time, but we could do better +final _kMessageTimestampFormat = DateFormat('h:mm aa', 'en_US'); - final MessageListMessageItem item; +class _SenderRow extends StatelessWidget { + const _SenderRow({required this.message}); + + final Message message; @override Widget build(BuildContext context) { @@ -1338,14 +1337,12 @@ class MessageWithPossibleSender extends StatelessWidget { final messageListTheme = MessageListTheme.of(context); final designVariables = DesignVariables.of(context); - final message = item.message; final sender = store.getUser(message.senderId); - - Widget? senderRow; - if (item.showSender) { - final time = _kMessageTimestampFormat - .format(DateTime.fromMillisecondsSinceEpoch(1000 * message.timestamp)); - senderRow = Row( + final time = _kMessageTimestampFormat + .format(DateTime.fromMillisecondsSinceEpoch(1000 * message.timestamp)); + return Padding( + padding: const EdgeInsets.fromLTRB(16, 2, 16, 0), + child: Row( mainAxisAlignment: MainAxisAlignment.spaceBetween, crossAxisAlignment: CrossAxisAlignment.baseline, textBaseline: localizedTextBaseline(context), @@ -1385,8 +1382,23 @@ class MessageWithPossibleSender extends StatelessWidget { height: (18 / 16), fontFeatures: const [FontFeature.enable('c2sc'), FontFeature.enable('smcp')], ).merge(weightVariableTextStyle(context))), - ]); - } + ])); + } +} + +/// A Zulip message, showing the sender's name and avatar if specified. +// Design referenced from: +// - https://github.com/zulip/zulip-mobile/issues/5511 +// - https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=538%3A20849&mode=dev +class MessageWithPossibleSender extends StatelessWidget { + const MessageWithPossibleSender({super.key, required this.item}); + + final MessageListMessageItem item; + + @override + Widget build(BuildContext context) { + final designVariables = DesignVariables.of(context); + final message = item.message; final localizations = ZulipLocalizations.of(context); String? editStateText; @@ -1415,9 +1427,7 @@ class MessageWithPossibleSender extends StatelessWidget { child: Padding( padding: const EdgeInsets.symmetric(vertical: 4), child: Column(children: [ - if (senderRow != null) - Padding(padding: const EdgeInsets.fromLTRB(16, 2, 16, 0), - child: senderRow), + if (item.showSender) _SenderRow(message: message), Row( crossAxisAlignment: CrossAxisAlignment.baseline, textBaseline: localizedTextBaseline(context), @@ -1445,6 +1455,3 @@ class MessageWithPossibleSender extends StatelessWidget { ]))); } } - -// TODO(i18n): web seems to ignore locale in formatting time, but we could do better -final _kMessageTimestampFormat = DateFormat('h:mm aa', 'en_US'); From d6dcb0b97690dc4ec9f87a62da0644990a62462b Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 27 Mar 2025 17:06:27 -0400 Subject: [PATCH 08/19] msglist [nfc]: Add showTimestamp to _SenderRow widget This is for the upcoming local-echo feature, to hide the timestamp. There isn't a Figma design for messages without a timestamp. --- lib/widgets/message_list.dart | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 79f7b15ea1..c972f9c84d 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -1327,9 +1327,10 @@ String formatHeaderDate( final _kMessageTimestampFormat = DateFormat('h:mm aa', 'en_US'); class _SenderRow extends StatelessWidget { - const _SenderRow({required this.message}); + const _SenderRow({required this.message, required this.showTimestamp}); final Message message; + final bool showTimestamp; @override Widget build(BuildContext context) { @@ -1374,14 +1375,16 @@ class _SenderRow extends StatelessWidget { ), ], ]))), - const SizedBox(width: 4), - Text(time, - style: TextStyle( - color: messageListTheme.labelTime, - fontSize: 16, - height: (18 / 16), - fontFeatures: const [FontFeature.enable('c2sc'), FontFeature.enable('smcp')], - ).merge(weightVariableTextStyle(context))), + if (showTimestamp) ...[ + const SizedBox(width: 4), + Text(time, + style: TextStyle( + color: messageListTheme.labelTime, + fontSize: 16, + height: (18 / 16), + fontFeatures: const [FontFeature.enable('c2sc'), FontFeature.enable('smcp')], + ).merge(weightVariableTextStyle(context))), + ] ])); } } @@ -1427,7 +1430,7 @@ class MessageWithPossibleSender extends StatelessWidget { child: Padding( padding: const EdgeInsets.symmetric(vertical: 4), child: Column(children: [ - if (item.showSender) _SenderRow(message: message), + if (item.showSender) _SenderRow(message: message, showTimestamp: true), Row( crossAxisAlignment: CrossAxisAlignment.baseline, textBaseline: localizedTextBaseline(context), From e3c11172a33a52f1d9c7511893b0c417bcd88877 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 22 Apr 2025 17:29:52 -0400 Subject: [PATCH 09/19] api [nfc]: Extract Conversation.isSameAs This will make it easier to support comparing the conversations between subclasses of MessageBase. The message list tests on displayRecipient are now mostly exercising the logic on Conversation.isSameAs, which makes it reasonable to move the tests. Keep them here for now since this logic is more relevant to message lists than it is to the rest of the app. --- lib/api/model/model.dart | 25 ++++++++++++++++++++++++- lib/model/message_list.dart | 31 +------------------------------ 2 files changed, 25 insertions(+), 31 deletions(-) diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index 27a922e5c2..ce60864921 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -629,7 +629,10 @@ extension type const TopicName(String _value) { /// Different from [MessageDestination], this information comes from /// [getMessages] or [getEvents], identifying the conversation that contains a /// message. -sealed class Conversation {} +sealed class Conversation { + /// Whether [this] and [other] have the same canonical form. + bool isSameAs(Conversation other); +} /// The conversation a stream message is in. @JsonSerializable(fieldRename: FieldRename.snake, createToJson: false) @@ -655,6 +658,13 @@ class StreamConversation extends Conversation { factory StreamConversation.fromJson(Map json) => _$StreamConversationFromJson(json); + + @override + bool isSameAs(Conversation other) { + return other is StreamConversation + && streamId == other.streamId + && topic.isSameAs(other.topic); + } } /// The conversation a DM message is in. @@ -668,6 +678,19 @@ class DmConversation extends Conversation { DmConversation({required this.allRecipientIds}) : assert(isSortedWithoutDuplicates(allRecipientIds.toList())); + + @override + bool isSameAs(Conversation other) { + if (other is! DmConversation) return false; + final xs = allRecipientIds; + final ys = other.allRecipientIds; + if (xs.length != ys.length) return false; + final xs_ = xs.iterator; final ys_ = ys.iterator; + while (xs_.moveNext() && ys_.moveNext()) { + if (xs_.current != ys_.current) return false; + } + return true; + } } /// A message or message-like object, for showing in a message list. diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 7fff16814f..a6558fcf73 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -361,26 +361,7 @@ mixin _MessageSequence { @visibleForTesting bool haveSameRecipient(Message prevMessage, Message message) { - if (prevMessage is StreamMessage && message is StreamMessage) { - if (prevMessage.streamId != message.streamId) return false; - if (prevMessage.topic.canonicalize() != message.topic.canonicalize()) return false; - } else if (prevMessage is DmMessage && message is DmMessage) { - if (!_equalIdSequences(prevMessage.allRecipientIds, message.allRecipientIds)) { - return false; - } - } else { - return false; - } - return true; - - // switch ((prevMessage, message)) { - // case (StreamMessage(), StreamMessage()): - // // TODO(dart-3): this doesn't type-narrow prevMessage and message - // case (DmMessage(), DmMessage()): - // // … - // default: - // return false; - // } + return prevMessage.conversation.isSameAs(message.conversation); } @visibleForTesting @@ -392,16 +373,6 @@ bool messagesSameDay(Message prevMessage, Message message) { return true; } -// Intended for [Message.allRecipientIds]. Assumes efficient `length`. -bool _equalIdSequences(Iterable xs, Iterable ys) { - if (xs.length != ys.length) return false; - final xs_ = xs.iterator; final ys_ = ys.iterator; - while (xs_.moveNext() && ys_.moveNext()) { - if (xs_.current != ys_.current) return false; - } - return true; -} - bool _sameDay(DateTime date1, DateTime date2) { if (date1.year != date2.year) return false; if (date1.month != date2.month) return false; From 387bbbf4c0b3daec4523b0060ff487ddb624c0f5 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Fri, 4 Apr 2025 17:37:46 -0400 Subject: [PATCH 10/19] msglist [nfc]: Extract MessageListMessageBaseItem This is NFC because MessageListMessageItem is still the only subclass of it. --- lib/model/message_list.dart | 52 ++++++++++++++++++++----------- test/model/message_list_test.dart | 8 +++-- 2 files changed, 39 insertions(+), 21 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index a6558fcf73..bd6f82c8dd 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -37,17 +37,29 @@ class MessageListDateSeparatorItem extends MessageListItem { } /// A message to show in the message list. -class MessageListMessageItem extends MessageListItem { - final Message message; - ZulipMessageContent content; +sealed class MessageListMessageBaseItem extends MessageListItem { + MessageBase get message; + ZulipMessageContent get content; bool showSender; bool isLastInBlock; + MessageListMessageBaseItem({ + required this.showSender, + required this.isLastInBlock, + }); +} + +class MessageListMessageItem extends MessageListMessageBaseItem { + @override + final Message message; + @override + ZulipMessageContent content; + MessageListMessageItem( this.message, this.content, { - required this.showSender, - required this.isLastInBlock, + required super.showSender, + required super.isLastInBlock, }); } @@ -289,14 +301,15 @@ mixin _MessageSequence { /// /// Returns whether an item has been appended or not. /// - /// The caller must append a [MessageListMessageItem] after this. - bool _maybeAppendAuxillaryItem(Message message, {required Message? prevMessage}) { + /// The caller must append a [MessageListMessageBaseItem] after this. + bool _maybeAppendAuxillaryItem(MessageBase message, { + required MessageBase? prevMessage, + }) { if (prevMessage == null || !haveSameRecipient(prevMessage, message)) { items.add(MessageListRecipientHeaderItem(message)); return true; } else { - assert(items.last is MessageListMessageItem); - final prevMessageItem = items.last as MessageListMessageItem; + final prevMessageItem = items.last as MessageListMessageBaseItem; assert(identical(prevMessageItem.message, prevMessage)); assert(prevMessageItem.isLastInBlock); prevMessageItem.isLastInBlock = false; @@ -360,12 +373,12 @@ mixin _MessageSequence { } @visibleForTesting -bool haveSameRecipient(Message prevMessage, Message message) { +bool haveSameRecipient(MessageBase prevMessage, MessageBase message) { return prevMessage.conversation.isSameAs(message.conversation); } @visibleForTesting -bool messagesSameDay(Message prevMessage, Message message) { +bool messagesSameDay(MessageBase prevMessage, MessageBase message) { // TODO memoize [DateTime]s... also use memoized for showing date/time in msglist final prevTime = DateTime.fromMillisecondsSinceEpoch(prevMessage.timestamp * 1000); final time = DateTime.fromMillisecondsSinceEpoch(message.timestamp * 1000); @@ -420,19 +433,20 @@ class MessageListView with ChangeNotifier, _MessageSequence { /// one way or another. /// /// See also [_allMessagesVisible]. - bool _messageVisible(Message message) { + bool _messageVisible(MessageBase message) { switch (narrow) { case CombinedFeedNarrow(): - return switch (message) { - StreamMessage() => - store.isTopicVisible(message.streamId, message.topic), - DmMessage() => true, + return switch (message.conversation) { + StreamConversation(:final streamId, :final topic) => + store.isTopicVisible(streamId, topic), + DmConversation() => true, }; case ChannelNarrow(:final streamId): - assert(message is StreamMessage && message.streamId == streamId); - if (message is! StreamMessage) return false; - return store.isTopicVisibleInStream(streamId, message.topic); + assert(message is MessageBase + && message.conversation.streamId == streamId); + if (message is! MessageBase) return false; + return store.isTopicVisibleInStream(streamId, message.conversation.topic); case TopicNarrow(): case DmNarrow(): diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 2c5f0711dc..374fc8c1e6 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -2001,13 +2001,17 @@ extension MessageListDateSeparatorItemChecks on Subject get message => has((x) => x.message, 'message'); } -extension MessageListMessageItemChecks on Subject { - Subject get message => has((x) => x.message, 'message'); +extension MessageListMessageBaseItemChecks on Subject { + Subject get message => has((x) => x.message, 'message'); Subject get content => has((x) => x.content, 'content'); Subject get showSender => has((x) => x.showSender, 'showSender'); Subject get isLastInBlock => has((x) => x.isLastInBlock, 'isLastInBlock'); } +extension MessageListMessageItemChecks on Subject { + Subject get message => has((x) => x.message, 'message'); +} + extension MessageListViewChecks on Subject { Subject get store => has((x) => x.store, 'store'); Subject get narrow => has((x) => x.narrow, 'narrow'); From 596b7d5cc7529bf32a36d13f8b0ec9e1f0c141c8 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Mon, 7 Apr 2025 15:37:23 -0400 Subject: [PATCH 11/19] msglist [nfc]: Handle MessageListMessageBaseItem with MessageItem This will make MessageItem compatible with other future subclasses of MessageBase, in particular OutboxMessage, which do not need unread markers. --- lib/widgets/message_list.dart | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index c972f9c84d..12577f6bf4 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -968,25 +968,32 @@ class MessageItem extends StatelessWidget { this.trailingWhitespace, }); - final MessageListMessageItem item; + final MessageListMessageBaseItem item; final Widget header; final double? trailingWhitespace; @override Widget build(BuildContext context) { - final message = item.message; final messageListTheme = MessageListTheme.of(context); + + final item = this.item; + Widget child = ColoredBox( + color: messageListTheme.bgMessageRegular, + child: Column(children: [ + switch (item) { + MessageListMessageItem() => MessageWithPossibleSender(item: item), + }, + if (trailingWhitespace != null && item.isLastInBlock) SizedBox(height: trailingWhitespace!), + ])); + if (item case MessageListMessageItem(:final message)) { + child = _UnreadMarker( + isRead: message.flags.contains(MessageFlag.read), + child: child); + } return StickyHeaderItem( allowOverflow: !item.isLastInBlock, header: header, - child: _UnreadMarker( - isRead: message.flags.contains(MessageFlag.read), - child: ColoredBox( - color: messageListTheme.bgMessageRegular, - child: Column(children: [ - MessageWithPossibleSender(item: item), - if (trailingWhitespace != null && item.isLastInBlock) SizedBox(height: trailingWhitespace!), - ])))); + child: child); } } From 67f8f7b9bae2b3da44f4a9e3043e1f2b6c95b85a Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 8 Apr 2025 16:15:23 -0400 Subject: [PATCH 12/19] msglist test [nfc]: Add MessageEvent test group --- test/model/message_list_test.dart | 56 ++++++++++++++++--------------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 374fc8c1e6..efc6fef3ef 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -360,37 +360,39 @@ void main() { }); }); - test('MessageEvent', () async { - final stream = eg.stream(); - await prepare(narrow: ChannelNarrow(stream.streamId)); - await prepareMessages(foundOldest: true, messages: - List.generate(30, (i) => eg.streamMessage(stream: stream))); + group('MessageEvent', () { + test('in narrow', () async { + final stream = eg.stream(); + await prepare(narrow: ChannelNarrow(stream.streamId)); + await prepareMessages(foundOldest: true, messages: + List.generate(30, (i) => eg.streamMessage(stream: stream))); - check(model).messages.length.equals(30); - await store.addMessage(eg.streamMessage(stream: stream)); - checkNotifiedOnce(); - check(model).messages.length.equals(31); - }); + check(model).messages.length.equals(30); + await store.addMessage(eg.streamMessage(stream: stream)); + checkNotifiedOnce(); + check(model).messages.length.equals(31); + }); - test('MessageEvent, not in narrow', () async { - final stream = eg.stream(); - await prepare(narrow: ChannelNarrow(stream.streamId)); - await prepareMessages(foundOldest: true, messages: - List.generate(30, (i) => eg.streamMessage(stream: stream))); + test('not in narrow', () async { + final stream = eg.stream(); + await prepare(narrow: ChannelNarrow(stream.streamId)); + await prepareMessages(foundOldest: true, messages: + List.generate(30, (i) => eg.streamMessage(stream: stream))); - check(model).messages.length.equals(30); - final otherStream = eg.stream(); - await store.addMessage(eg.streamMessage(stream: otherStream)); - checkNotNotified(); - check(model).messages.length.equals(30); - }); + check(model).messages.length.equals(30); + final otherStream = eg.stream(); + await store.addMessage(eg.streamMessage(stream: otherStream)); + checkNotNotified(); + check(model).messages.length.equals(30); + }); - test('MessageEvent, before fetch', () async { - final stream = eg.stream(); - await prepare(narrow: ChannelNarrow(stream.streamId)); - await store.addMessage(eg.streamMessage(stream: stream)); - checkNotNotified(); - check(model).fetched.isFalse(); + test('before fetch', () async { + final stream = eg.stream(); + await prepare(narrow: ChannelNarrow(stream.streamId)); + await store.addMessage(eg.streamMessage(stream: stream)); + checkNotNotified(); + check(model).fetched.isFalse(); + }); }); group('UserTopicEvent', () { From 5c47cffc068cda7453be491b177e437298a43720 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Wed, 23 Apr 2025 12:32:59 -0400 Subject: [PATCH 13/19] msglist: Remove trailingWhitespace This 11px whitespace can be traced back to 311d4d56e in 2022, and is not present in the Figma design. It was there to match the web design while prototyping the app. --- lib/widgets/message_list.dart | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 12577f6bf4..b04ec944d2 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -685,7 +685,6 @@ class _MessageListState extends State with PerAccountStoreAwareStat return MessageItem( key: ValueKey(data.message.id), header: header, - trailingWhitespace: i == 1 ? 8 : 11, item: data); } } @@ -965,12 +964,10 @@ class MessageItem extends StatelessWidget { super.key, required this.item, required this.header, - this.trailingWhitespace, }); final MessageListMessageBaseItem item; final Widget header; - final double? trailingWhitespace; @override Widget build(BuildContext context) { @@ -983,7 +980,6 @@ class MessageItem extends StatelessWidget { switch (item) { MessageListMessageItem() => MessageWithPossibleSender(item: item), }, - if (trailingWhitespace != null && item.isLastInBlock) SizedBox(height: trailingWhitespace!), ])); if (item case MessageListMessageItem(:final message)) { child = _UnreadMarker( From 339b3c0524adefb3f1f1d35960c8c8476cf488b6 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 22 Apr 2025 13:16:03 -0400 Subject: [PATCH 14/19] test [nfc]: Add addOutboxMessage(s) --- test/model/message_test.dart | 21 +++++---------------- test/model/test_store.dart | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/test/model/message_test.dart b/test/model/message_test.dart index d090133899..802ecc1f05 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -102,11 +102,8 @@ void main() { await store.addStream(stream); await store.addSubscription(eg.subscription(stream)); - (store.connection as FakeApiConnection).prepare( - json: SendMessageResult(id: 1).toJson()); - await store.sendMessage( - destination: StreamDestination(stream.streamId, eg.t('topic')), - content: 'content'); + await store.addOutboxMessage( + StreamDestination(stream.streamId, eg.t('topic'))); check(async.pendingTimers).deepEquals(>[ (it) => it.isA().duration.equals(kLocalEchoDebounceDuration), (it) => it.isA().duration.equals(kSendMessageRetryWaitPeriod), @@ -125,10 +122,7 @@ void main() { await prepare(stream: stream); await prepareMessages([]); - for (int i = 0; i < 10; i++) { - connection.prepare(json: SendMessageResult(id: 1).toJson()); - await store.sendMessage(destination: streamDestination, content: 'content'); - } + await store.addOutboxMessages(List.generate(10, (_) => streamDestination)); // [store.outboxMessages] has the same number of keys (localMessageId) // as the number of sent messages, which are guaranteed to be distinct. check(store.outboxMessages).keys.length.equals(10); @@ -435,13 +429,8 @@ void main() { final stream = eg.stream(); await prepare(stream: stream); await prepareMessages([]); - - for (int i = 0; i < 10; i++) { - connection.prepare(json: SendMessageResult(id: 1).toJson()); - await store.sendMessage( - destination: StreamDestination(stream.streamId, eg.t('topic')), - content: 'content'); - } + await store.addOutboxMessages( + List.generate(10, (_) => StreamDestination(stream.streamId, eg.t('topic')))); final localMessageIds = store.outboxMessages.keys.toList(); store.removeOutboxMessage(localMessageIds.removeAt(5)); diff --git a/test/model/test_store.dart b/test/model/test_store.dart index 0196611e1d..b926b59b52 100644 --- a/test/model/test_store.dart +++ b/test/model/test_store.dart @@ -2,8 +2,10 @@ import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/initial_snapshot.dart'; import 'package:zulip/api/model/model.dart'; import 'package:zulip/api/route/events.dart'; +import 'package:zulip/api/route/messages.dart'; import 'package:zulip/api/route/realm.dart'; import 'package:zulip/model/database.dart'; +import 'package:zulip/model/message.dart'; import 'package:zulip/model/settings.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/notifications/receive.dart'; @@ -296,4 +298,17 @@ extension PerAccountStoreTestExtension on PerAccountStore { await addMessage(message); } } + + Future addOutboxMessage(MessageDestination destination) async { + assert(MessageStoreImpl.debugOutboxEnable); + (connection as FakeApiConnection).prepare( + json: SendMessageResult(id: 1).toJson()); + await this.sendMessage(destination: destination, content: 'content'); + } + + Future addOutboxMessages(List destinations) async { + for (final destination in destinations) { + await addOutboxMessage(destination); + } + } } From 47676f037cbbfbbdcded9c17a17fe205007dec34 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 22 Apr 2025 13:38:07 -0400 Subject: [PATCH 15/19] test [nfc]: Extract {dm,stream}OutboxMessage helpers --- test/example_data.dart | 39 +++++++++++++++++++++++++ test/model/narrow_test.dart | 58 ++++++++++++------------------------- 2 files changed, 57 insertions(+), 40 deletions(-) diff --git a/test/example_data.dart b/test/example_data.dart index 1c1020a805..9f7df0accd 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -11,6 +11,7 @@ import 'package:zulip/api/route/messages.dart'; import 'package:zulip/api/route/realm.dart'; import 'package:zulip/api/route/channels.dart'; import 'package:zulip/model/database.dart'; +import 'package:zulip/model/message.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/settings.dart'; import 'package:zulip/model/store.dart'; @@ -566,6 +567,44 @@ GetMessagesResult olderGetMessagesResult({ ); } +int _nextLocalMessageId = 1; + +StreamOutboxMessage streamOutboxMessage({ + int? localMessageId, + int? selfUserId, + int? timestamp, + ZulipStream? stream, + String? topic, + String? content, +}) { + final effectiveStream = stream ?? _stream(streamId: defaultStreamMessageStreamId); + return StreamOutboxMessage( + localMessageId: localMessageId ?? _nextLocalMessageId++, + selfUserId: selfUserId ?? selfUser.userId, + timestamp: timestamp ?? utcTimestamp(), + conversation: StreamConversation( + effectiveStream.streamId, TopicName(topic ?? 'topic'), + displayRecipient: null, + ), + content: content ?? 'content'); +} + +DmOutboxMessage dmOutboxMessage({ + int? localMessageId, + required User from, + required List to, + int? timestamp, + String? content, +}) { + final allRecipientIds = [from, ...to].map((user) => user.userId).toList(); + return DmOutboxMessage( + localMessageId: localMessageId ?? _nextLocalMessageId++, + selfUserId: from.userId, + timestamp: timestamp ?? utcTimestamp(), + conversation: DmConversation(allRecipientIds: allRecipientIds), + content: content ?? 'content'); +} + PollWidgetData pollWidgetData({ required String question, required List options, diff --git a/test/model/narrow_test.dart b/test/model/narrow_test.dart index 535e6d7e61..dff0a6e179 100644 --- a/test/model/narrow_test.dart +++ b/test/model/narrow_test.dart @@ -2,37 +2,12 @@ import 'package:checks/checks.dart'; import 'package:test/scaffolding.dart'; import 'package:zulip/api/model/model.dart'; -import 'package:zulip/model/message.dart'; import 'package:zulip/model/narrow.dart'; import '../example_data.dart' as eg; import 'narrow_checks.dart'; void main() { - int nextLocalMessageId = 1; - - StreamOutboxMessage streamOutboxMessage({ - required ZulipStream stream, - required String topic, - }) { - return StreamOutboxMessage( - localMessageId: nextLocalMessageId++, - selfUserId: eg.selfUser.userId, - timestamp: 123456789, - conversation: StreamConversation( - stream.streamId, TopicName(topic), displayRecipient: null), - content: 'content'); - } - - DmOutboxMessage dmOutboxMessage({required List allRecipientIds}) { - return DmOutboxMessage( - localMessageId: nextLocalMessageId++, - selfUserId: allRecipientIds[0], - timestamp: 123456789, - conversation: DmConversation(allRecipientIds: allRecipientIds), - content: 'content'); - } - group('SendableNarrow', () { test('ofMessage: stream message', () { final message = eg.streamMessage(); @@ -60,11 +35,11 @@ void main() { eg.streamMessage(stream: stream, topic: 'topic'))).isTrue(); check(narrow.containsMessage( - dmOutboxMessage(allRecipientIds: [1]))).isFalse(); + eg.dmOutboxMessage(from: eg.selfUser, to: [eg.otherUser]))).isFalse(); check(narrow.containsMessage( - streamOutboxMessage(stream: otherStream, topic: 'topic'))).isFalse(); + eg.streamOutboxMessage(stream: otherStream, topic: 'topic'))).isFalse(); check(narrow.containsMessage( - streamOutboxMessage(stream: stream, topic: 'topic'))).isTrue(); + eg.streamOutboxMessage(stream: stream, topic: 'topic'))).isTrue(); }); }); @@ -90,13 +65,13 @@ void main() { eg.streamMessage(stream: stream, topic: 'topic'))).isTrue(); check(narrow.containsMessage( - dmOutboxMessage(allRecipientIds: [1]))).isFalse(); + eg.dmOutboxMessage(from: eg.selfUser, to: [eg.otherUser]))).isFalse(); check(narrow.containsMessage( - streamOutboxMessage(stream: otherStream, topic: 'topic'))).isFalse(); + eg.streamOutboxMessage(stream: otherStream, topic: 'topic'))).isFalse(); check(narrow.containsMessage( - streamOutboxMessage(stream: stream, topic: 'topic2'))).isFalse(); + eg.streamOutboxMessage(stream: stream, topic: 'topic2'))).isFalse(); check(narrow.containsMessage( - streamOutboxMessage(stream: stream, topic: 'topic'))).isTrue(); + eg.streamOutboxMessage(stream: stream, topic: 'topic'))).isTrue(); }); }); @@ -219,16 +194,19 @@ void main() { }); test('containsMessage with non-Message', () { + final user1 = eg.user(userId: 1); + final user2 = eg.user(userId: 2); + final user3 = eg.user(userId: 3); final narrow = DmNarrow(allRecipientIds: [1, 2], selfUserId: 2); check(narrow.containsMessage( - streamOutboxMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); + eg.streamOutboxMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); check(narrow.containsMessage( - dmOutboxMessage(allRecipientIds: [2]))).isFalse(); + eg.dmOutboxMessage(from: user2, to: []))).isFalse(); check(narrow.containsMessage( - dmOutboxMessage(allRecipientIds: [2, 3]))).isFalse(); + eg.dmOutboxMessage(from: user2, to: [user3]))).isFalse(); check(narrow.containsMessage( - dmOutboxMessage(allRecipientIds: [1, 2]))).isTrue(); + eg.dmOutboxMessage(from: user1, to: [user2]))).isTrue(); }); }); @@ -244,9 +222,9 @@ void main() { eg.streamMessage(flags: [MessageFlag.wildcardMentioned]))).isTrue(); check(narrow.containsMessage( - streamOutboxMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); + eg.streamOutboxMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); check(narrow.containsMessage( - dmOutboxMessage(allRecipientIds: [eg.selfUser.userId]))).isFalse(); + eg.dmOutboxMessage(from: eg.selfUser, to: []))).isFalse(); }); }); @@ -260,9 +238,9 @@ void main() { eg.streamMessage(flags:[MessageFlag.starred]))).isTrue(); check(narrow.containsMessage( - streamOutboxMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); + eg.streamOutboxMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); check(narrow.containsMessage( - dmOutboxMessage(allRecipientIds: [eg.selfUser.userId]))).isFalse(); + eg.dmOutboxMessage(from: eg.selfUser, to: []))).isFalse(); }); }); } From e2383009c48a2ce5919046fcbf1c736e8f0fdc09 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 24 Apr 2025 14:12:23 -0400 Subject: [PATCH 16/19] msglist test [nfc]: Make checkInvariant compatible with MessageBase --- test/api/model/model_checks.dart | 1 + test/model/message_list_test.dart | 40 ++++++++++++++++++++----------- 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/test/api/model/model_checks.dart b/test/api/model/model_checks.dart index 2216b2b98f..1387c61961 100644 --- a/test/api/model/model_checks.dart +++ b/test/api/model/model_checks.dart @@ -30,6 +30,7 @@ extension TopicNameChecks on Subject { } extension StreamConversationChecks on Subject { + Subject get streamId => has((x) => x.streamId, 'streamId'); Subject get topic => has((x) => x.topic, 'topic'); Subject get displayRecipient => has((x) => x.displayRecipient, 'displayRecipient'); } diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index efc6fef3ef..fdef795010 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -1926,17 +1926,24 @@ void checkInvariants(MessageListView model) { check(model).fetchOlderCoolingDown.isFalse(); } - for (final message in model.messages) { - check(model.store.messages)[message.id].isNotNull().identicalTo(message); + final allMessages = >[...model.messages]; + + for (final message in allMessages) { + if (message is Message) { + check(model.store.messages)[message.id].isNotNull().identicalTo(message); + } else { + assert(false); + } check(model.narrow.containsMessage(message)).isTrue(); - if (message is! StreamMessage) continue; + if (message is! MessageBase) continue; + final conversation = message.conversation; switch (model.narrow) { case CombinedFeedNarrow(): - check(model.store.isTopicVisible(message.streamId, message.topic)) + check(model.store.isTopicVisible(conversation.streamId, conversation.topic)) .isTrue(); case ChannelNarrow(): - check(model.store.isTopicVisibleInStream(message.streamId, message.topic)) + check(model.store.isTopicVisibleInStream(conversation.streamId, conversation.topic)) .isTrue(); case TopicNarrow(): case DmNarrow(): @@ -1966,23 +1973,28 @@ void checkInvariants(MessageListView model) { if (model.fetchingOlder || model.fetchOlderCoolingDown) { check(model.items[i++]).isA(); } - for (int j = 0; j < model.messages.length; j++) { + for (int j = 0; j < allMessages.length; j++) { bool forcedShowSender = false; if (j == 0 - || !haveSameRecipient(model.messages[j-1], model.messages[j])) { + || !haveSameRecipient(allMessages[j-1], allMessages[j])) { check(model.items[i++]).isA() - .message.identicalTo(model.messages[j]); + .message.identicalTo(allMessages[j]); forcedShowSender = true; - } else if (!messagesSameDay(model.messages[j-1], model.messages[j])) { + } else if (!messagesSameDay(allMessages[j-1], allMessages[j])) { check(model.items[i++]).isA() - .message.identicalTo(model.messages[j]); + .message.identicalTo(allMessages[j]); forcedShowSender = true; } - check(model.items[i++]).isA() - ..message.identicalTo(model.messages[j]) - ..content.identicalTo(model.contents[j]) + if (j < model.messages.length) { + check(model.items[i]).isA() + ..message.identicalTo(model.messages[j]) + ..content.identicalTo(model.contents[j]); + } else { + assert(false); + } + check(model.items[i++]).isA() ..showSender.equals( - forcedShowSender || model.messages[j].senderId != model.messages[j-1].senderId) + forcedShowSender || allMessages[j].senderId != allMessages[j-1].senderId) ..isLastInBlock.equals( i == model.items.length || switch (model.items[i]) { MessageListMessageItem() From d7d8b2bbb430962149759dac3ebb1831cef4c9f1 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 27 Mar 2025 18:47:16 -0400 Subject: [PATCH 17/19] msglist: Support outbox messages in message list This adds some overhead in magnitude of O(1) (where the constant is the number of outbox messages in a view, expected to be small) on message event handling. We add outboxMessages as a list independent from messages on _MessageSequence. Because outbox messages are not rendered (the raw content is shown as plain text), we leave the 1-1 relationship between `messages` and `contents` unchanged. When computing `items`, we now start to look at `outboxMessages` as well, with the guarantee that the items related to outbox messages always come after those for other messages. Look for places that call `_processOutboxMessage(int index)` for references, and the changes to `checkInvariants` on how this affects the message list invariants. `addOutboxMessage` is similar to `handleMessage`. However, outbox messages do not rely on the fetched state, i.e. they can be synchronously updated when the message list view was first initialized. --- lib/model/message.dart | 2 + lib/model/message_list.dart | 230 +++++++++++- lib/widgets/message_list.dart | 39 +- test/api/model/model_checks.dart | 4 + test/model/message_list_test.dart | 550 +++++++++++++++++++++++++++- test/model/message_test.dart | 34 ++ test/widgets/message_list_test.dart | 33 ++ 7 files changed, 859 insertions(+), 33 deletions(-) diff --git a/lib/model/message.dart b/lib/model/message.dart index e2f6b02063..835d85ab40 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -542,6 +542,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 bd6f82c8dd..c352e2415e 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -63,6 +63,21 @@ class MessageListMessageItem extends MessageListMessageBaseItem { }); } +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: [], nodes: [TextNode(message.content)]), + ]); +} + /// Indicates the app is loading more messages at the top. // TODO(#80): or loading at the bottom, by adding a [MessageListDirection.newer] class MessageListLoadingItem extends MessageListItem { @@ -90,7 +105,16 @@ mixin _MessageSequence { /// See also [contents] and [items]. final List messages = []; - /// Whether [messages] and [items] represent the results of a fetch. + /// The messages sent by the self-user. + /// + /// See also [items]. + /// + /// Usually this should not have that many items, so we do not anticipate + /// performance issues with unoptimized O(N) iterations through this list. + final List outboxMessages = []; + + /// Whether [messages], [outboxMessages], and [items] represent the results + /// of a fetch. /// /// This allows the UI to distinguish "still working on fetching messages" /// from "there are in fact no messages here". @@ -142,11 +166,12 @@ mixin _MessageSequence { /// 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. + /// of [messages], followed by each element in [outboxMessages] in order. + /// It may have additional items interspersed before, between, or after the + /// messages. /// - /// This information is completely derived from [messages] and - /// the flags [haveOldest], [fetchingOlder] and [fetchOlderCoolingDown]. + /// This information is completely derived from [messages], [outboxMessages] + /// and the flags [haveOldest], [fetchingOlder] and [fetchOlderCoolingDown]. /// It exists as an optimization, to memoize that computation. final QueueList items = QueueList(); @@ -168,9 +193,10 @@ mixin _MessageSequence { } 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; } } @@ -274,10 +300,46 @@ 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], our state of being caught up, and other concerns. + void _addOutboxMessage(OutboxMessage outboxMessage) { + 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(); + outboxMessages.clear(); _fetched = false; _haveOldest = false; _fetchingOlder = false; @@ -301,7 +363,8 @@ mixin _MessageSequence { /// /// Returns whether an item has been appended or not. /// - /// The caller must append a [MessageListMessageBaseItem] after this. + /// The caller must append a [MessageListMessageBaseItem] for [message] + /// after this. bool _maybeAppendAuxillaryItem(MessageBase message, { required MessageBase? prevMessage, }) { @@ -328,6 +391,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]; @@ -338,6 +402,20 @@ mixin _MessageSequence { isLastInBlock: true)); } + /// Append to [items] based on the index-th outbox message. + /// + /// 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]; + + final appended = _maybeAppendAuxillaryItem(message, prevMessage: prevMessage); + items.add(MessageListOutboxMessageItem(message, + showSender: appended || prevMessage?.senderId != message.senderId, + isLastInBlock: true)); + } + /// Update [items] to include markers at start and end as appropriate. void _updateEndMarkers() { assert(fetched); @@ -362,12 +440,55 @@ mixin _MessageSequence { } } - /// Recompute [items] from scratch, based on [messages], [contents], and flags. + /// 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. + // If there is no [MessageListMessageItem] at all, + // this will end up removing end markers. + while (items.isNotEmpty && items.last is! MessageListMessageItem) { + items.removeLast(); + } + assert(items.none((e) => e is MessageListOutboxMessageItem)); + + if (items.isNotEmpty) { + final lastItem = items.last as MessageListMessageItem; + lastItem.isLastInBlock = true; + } + + if (fetched) { + // Restore the end markers in case they were removed; only do so when + // [fetched] is true, since the markers are not there otherwise. + _updateEndMarkers(); + } + } + + /// 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() { + _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); } + for (var i = 0; i < outboxMessages.length; i++) { + _processOutboxMessage(i); + } _updateEndMarkers(); } } @@ -412,7 +533,9 @@ class MessageListView with ChangeNotifier, _MessageSequence { factory MessageListView.init( {required PerAccountStore store, required Narrow narrow}) { - final view = MessageListView._(store: store, narrow: narrow); + final view = MessageListView._(store: store, narrow: narrow) + .._syncOutboxMessages() + .._reprocessOutboxMessages(); store.registerMessageList(view); return view; } @@ -510,11 +633,13 @@ class MessageListView with ChangeNotifier, _MessageSequence { _adjustNarrowForTopicPermalink(result.messages.firstOrNull); store.reconcileMessages(result.messages); store.recentSenders.handleMessages(result.messages); // TODO(#824) + _removeOutboxMessageItems(); for (final message in result.messages) { if (_messageVisible(message)) { _addMessage(message); } } + _reprocessOutboxMessages(); _fetched = true; _haveOldest = result.foundOldest; _updateEndMarkers(); @@ -621,16 +746,51 @@ class MessageListView with ChangeNotifier, _MessageSequence { } } + bool _shouldAddOutboxMessage(OutboxMessage outboxMessage, { + bool wasUnmuted = false, + }) { + return !outboxMessage.hidden + && narrow.containsMessage(outboxMessage) + && (_messageVisible(outboxMessage) || wasUnmuted); + } + + /// Copy outbox messages from the store, keeping the ones belong to the view. + /// + /// This does not recompute [items]. The caller is expected to call + /// [_reprocessOutboxMessages] later to keep [items] up-to-date. + /// + /// This assumes that [outboxMessages] is empty. + void _syncOutboxMessages() { + assert(outboxMessages.isEmpty); + for (final outboxMessage in store.outboxMessages.values) { + if (_shouldAddOutboxMessage(outboxMessage)) { + outboxMessages.add(outboxMessage); + } + } + } + /// Add [outboxMessage] if it belongs to the view. void addOutboxMessage(OutboxMessage outboxMessage) { - // TODO(#1441) implement this + assert(outboxMessages.none( + (message) => message.localMessageId == outboxMessage.localMessageId)); + if (_shouldAddOutboxMessage(outboxMessage)) { + _addOutboxMessage(outboxMessage); + if (fetched) { + // Only need to notify listeners when [fetched] is true, because + // otherwise the message list just shows a loading indicator with + // no other items. + notifyListeners(); + } + } } /// Remove the [outboxMessage] from the view. /// /// This is a no-op if the message is not found. void removeOutboxMessage(OutboxMessage outboxMessage) { - // TODO(#1441) implement this + if (_removeOutboxMessage(outboxMessage)) { + notifyListeners(); + } } void handleUserTopicEvent(UserTopicEvent event) { @@ -639,10 +799,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 = _removeOutboxMessagesWhere((message) => + message is StreamOutboxMessage + && message.conversation.streamId == event.streamId + && message.conversation.topic == event.topicName); + + removed |= _removeMessagesWhere((message) => + message is StreamMessage + && message.streamId == event.streamId + && message.topic == event.topicName); + + if (removed) { notifyListeners(); } @@ -655,6 +822,18 @@ class MessageListView with ChangeNotifier, _MessageSequence { notifyListeners(); fetchInitial(); } + + outboxMessages.clear(); + for (final outboxMessage in store.outboxMessages.values) { + if (_shouldAddOutboxMessage( + outboxMessage, + wasUnmuted: outboxMessage is StreamOutboxMessage + && outboxMessage.conversation.streamId == event.streamId + && outboxMessage.conversation.topic == event.topicName, + )) { + outboxMessages.add(outboxMessage); + } + } } } @@ -668,14 +847,27 @@ 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 (!_fetched) { // TODO mitigate this fetch/event race: save message to add to list later return; } + // We always remove all outbox message items + // to ensure that message items come before them. + _removeOutboxMessageItems(); // TODO insert in middle instead, when appropriate _addMessage(message); + if (event.localMessageId != null) { + final localMessageId = int.parse(event.localMessageId!); + // [outboxMessages] is epxected 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(); } @@ -707,6 +899,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { // TODO in cases where we do have data to do better, do better. _reset(); notifyListeners(); + _syncOutboxMessages(); fetchInitial(); } @@ -722,6 +915,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { case PropagateMode.changeLater: narrow = newNarrow; _reset(); + _syncOutboxMessages(); fetchInitial(); case PropagateMode.changeOne: } @@ -796,7 +990,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 b04ec944d2..8693a61ca7 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -686,6 +686,11 @@ 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); } } } @@ -979,6 +984,7 @@ class MessageItem extends StatelessWidget { child: Column(children: [ switch (item) { MessageListMessageItem() => MessageWithPossibleSender(item: item), + MessageListOutboxMessageItem() => OutboxMessageWithPossibleSender(item: item), }, ])); if (item case MessageListMessageItem(:final message)) { @@ -1332,7 +1338,7 @@ final _kMessageTimestampFormat = DateFormat('h:mm aa', 'en_US'); class _SenderRow extends StatelessWidget { const _SenderRow({required this.message, required this.showTimestamp}); - final Message message; + final MessageBase message; final bool showTimestamp; @override @@ -1362,7 +1368,9 @@ class _SenderRow extends StatelessWidget { userId: message.senderId), const SizedBox(width: 8), Flexible( - child: Text(message.senderFullName, // TODO(#716): use `store.senderDisplayName` + child: Text(message is Message + ? store.senderDisplayName(message as Message) + : store.userDisplayName(message.senderId), style: TextStyle( fontSize: 18, height: (22 / 18), @@ -1461,3 +1469,30 @@ class MessageWithPossibleSender extends StatelessWidget { ]))); } } + +/// A placeholder for Zulip message 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 adapated 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 1387c61961..803d5ccd12 100644 --- a/test/api/model/model_checks.dart +++ b/test/api/model/model_checks.dart @@ -35,6 +35,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 fdef795010..40655ea306 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -1,6 +1,7 @@ import 'dart:convert'; import 'package:checks/checks.dart'; +import 'package:clock/clock.dart'; import 'package:http/http.dart' as http; import 'package:test/scaffolding.dart'; import 'package:zulip/api/backoff.dart'; @@ -8,8 +9,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'; @@ -19,7 +22,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'; @@ -27,6 +32,8 @@ const newestResult = eg.newestGetMessagesResult; const olderResult = eg.olderGetMessagesResult; void main() { + TestZulipBinding.ensureInitialized(); + // These variables are the common state operated on by each test. // Each test case calls [prepare] to initialize them. late Subscription subscription; @@ -46,8 +53,11 @@ void main() { void checkNotifiedOnce() => checkNotified(count: 1); /// Initialize [model] and the rest of the test state. - Future prepare({Narrow narrow = const CombinedFeedNarrow()}) async { - final stream = eg.stream(streamId: eg.defaultStreamMessageStreamId); + Future prepare({ + Narrow narrow = const CombinedFeedNarrow(), + ZulipStream? stream, + }) async { + stream ??= eg.stream(streamId: eg.defaultStreamMessageStreamId); subscription = eg.subscription(stream); store = eg.store(); await store.addStream(stream); @@ -76,6 +86,15 @@ void main() { checkNotifiedOnce(); } + Future prepareOutboxMessages({ + required int count, + required ZulipStream stream, + String topic = 'some topic', + }) async { + await store.addOutboxMessages(List.generate(count, (_) => + StreamDestination(stream.streamId, eg.t(topic)))); + } + void checkLastRequest({ required ApiNarrow narrow, required String anchor, @@ -95,6 +114,25 @@ void main() { }); } + test('MessageListView.init with existing outbox messages', () => awaitFakeAsync((async) async { + final store = eg.store(); + final stream = eg.stream(); + await store.addStream(stream); + await store.addSubscription(eg.subscription(stream)); + await store.addOutboxMessages([ + StreamDestination(stream.streamId, eg.t('topic')), + StreamDestination(stream.streamId, eg.t('other')), + ]); + async.elapse(kLocalEchoDebounceDuration); + + final model = MessageListView.init(store: store, + narrow: eg.topicNarrow(stream.streamId, 'topic')); + checkInvariants(model); + check(model).outboxMessages.single.isA().conversation + ..streamId.equals(stream.streamId) + ..topic.equals(eg.t('topic')); + })); + group('fetchInitial', () { final someChannel = eg.stream(); const someTopic = 'some topic'; @@ -166,6 +204,69 @@ void main() { ..haveOldest.isTrue(); }); + test('only outbox messages found', () => 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.length.equals(1); + + connection.prepare( + json: newestResult(foundOldest: true, messages: []).toJson()); + await model.fetchInitial(); + checkNotifiedOnce(); + check(model) + ..fetched.isTrue() + ..outboxMessages.length.equals(1); + })); + + test('found messages on top of existing outbox messages', () => 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(); + + connection.prepare(json: newestResult(foundOldest: true, + messages: [eg.streamMessage(stream: stream, topic: 'topic')]).toJson()); + // should process messages without errors + await model.fetchInitial(); + checkNotifiedOnce(); + })); + + test('ignore outbox messages not in narrow or not visible', () => 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 store.addOutboxMessages([ + StreamDestination(stream.streamId, eg.t('topic')), + StreamDestination(stream.streamId, eg.t('muted')), + StreamDestination(otherStream.streamId, eg.t('topic')), + ]); + async.elapse(kLocalEchoDebounceDuration); + checkNotNotified(); + + await store.addOutboxMessage( + 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(); @@ -393,6 +494,165 @@ 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('when no matching localMessageId is found', () => 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'))); + + // Initially, the outbox message should be hidden to + // the view until its debounce timeout expires. + 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: 'topic'), + localMessageId: localMessageId)); + checkNotifiedOnce(); + check(model) + ..messages.length.equals(31) + ..outboxMessages.isEmpty(); + + async.elapse(kLocalEchoDebounceDuration); + checkNotNotified(); + })); + + test('when a matching localMessageId is found', () => 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))); + })); + }); + + 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); + 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.length.equals(5); + })); + }); + + group('removeOutboxMessage', () { + 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, topic: 'topic'))); + await prepareOutboxMessages(count: 5, stream: stream); + async.elapse(kLocalEchoDebounceDuration); + checkNotified(count: 5); + check(model).outboxMessages.length.equals(5); + + store.removeOutboxMessage(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 prepareOutboxMessages(count: 5, stream: stream, topic: 'other'); + async.elapse(kLocalEchoDebounceDuration); + checkNotNotified(); + check(model).outboxMessages.isEmpty(); + + store.removeOutboxMessage(store.outboxMessages.keys.first); + checkNotNotified(); + check(model).outboxMessages.isEmpty(); + })); + + 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 prepareOutboxMessages(count: 1, stream: stream); + async.elapse(kLocalEchoDebounceDuration); + checkNotifiedOnce(); + check(model).outboxMessages.single; + + store.removeOutboxMessage(store.outboxMessages.keys.first); + checkNotifiedOnce(); + check(model).outboxMessages.isEmpty(); + })); }); group('UserTopicEvent', () { @@ -420,7 +680,7 @@ void main() { check(model.messages.map((m) => m.id)).deepEquals(messageIds); } - 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(); @@ -434,10 +694,49 @@ void main() { ]); checkHasMessageIds([1, 2, 3, 4]); + await store.addOutboxMessages([ + 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 store.addOutboxMessages([ + 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. @@ -512,7 +811,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: [ @@ -520,10 +819,17 @@ void main() { ]); checkHasMessageIds([1]); + await store.addOutboxMessage( + 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()); @@ -533,7 +839,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 store.addOutboxMessages([ + 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()); @@ -541,10 +854,16 @@ void main() { checkNotifiedOnce(); check(model).fetched.isFalse(); checkHasMessageIds([]); + check(model).outboxMessages.single.isA().conversation + ..streamId.equals(stream.streamId) + ..topic.equals(eg.t(topic)); 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 { @@ -690,6 +1009,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()); @@ -823,6 +1174,25 @@ void main() { checkNotifiedOnce(); }); + test('channel -> new channel (with outbox messages): remove moved messages; outbox messages unaffected', () => awaitFakeAsync((async) async { + 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 outboxMessages = model.outboxMessages; + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: movedMessages, + newTopicStr: 'new', + newStreamId: otherStream.streamId, + )); + checkHasMessages(initialMessages); + check(model).outboxMessages.identicalTo(outboxMessages); + checkNotifiedOnce(); + })); + test('unrelated channel -> new channel: unaffected', () async { final thirdStream = eg.stream(); await prepareNarrow(narrow, initialMessages); @@ -1525,6 +1895,38 @@ void main() { check(model.messages.map((m) => m.id)).deepEquals(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 store.addOutboxMessages([ + 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(); + assert(!model.fetched); + async.elapse(Duration.zero); + 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')); @@ -1672,7 +2074,60 @@ 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: 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().message.id.isNull(), + (it) => it.isA(), + (it) => it.isA(), + (it) => it.isA(), + ]); + check(model.findItemWithMessageId(message.id)).equals(2); + })); + + 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: 4, stream: stream, topic: 'topic'); + async.elapse(kLocalEchoDebounceDuration); + checkNotified(count: 4); + check(model.items).deepEquals(>[ + (it) => it.isA(), + (it) => it.isA(), + (it) => it.isA(), + (it) => it.isA(), + (it) => it.isA(), + (it) => it.isA(), + (it) => it.isA(), + ]); + check(model.findItemWithMessageId(message.id)).equals(2); + })); + }); + + 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. @@ -1685,7 +2140,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); @@ -1744,6 +2199,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, @@ -1756,17 +2225,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, @@ -1774,6 +2259,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: [ @@ -1783,6 +2270,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 store.addOutboxMessages([ + 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: @@ -1796,8 +2290,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', () { @@ -1868,6 +2364,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', () { @@ -1903,6 +2409,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); } } } @@ -1926,11 +2440,14 @@ void checkInvariants(MessageListView model) { check(model).fetchOlderCoolingDown.isFalse(); } - final allMessages = >[...model.messages]; + final allMessages = [...model.messages, ...model.outboxMessages]; for (final message in allMessages) { if (message is Message) { check(model.store.messages)[message.id].isNotNull().identicalTo(message); + } else if (message is OutboxMessage) { + check(message).hidden.isFalse(); + check(model.store.outboxMessages)[message.localMessageId].isNotNull().identicalTo(message); } else { assert(false); } @@ -1990,7 +2507,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( @@ -1998,6 +2516,7 @@ void checkInvariants(MessageListView model) { ..isLastInBlock.equals( i == model.items.length || switch (model.items[i]) { MessageListMessageItem() + || MessageListOutboxMessageItem() || MessageListDateSeparatorItem() => false, MessageListRecipientHeaderItem() || MessageListHistoryStartItem() @@ -2030,6 +2549,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 contents => has((x) => x.contents, 'contents'); Subject> get items => has((x) => x.items, 'items'); Subject get fetched => has((x) => x.fetched, 'fetched'); diff --git a/test/model/message_test.dart b/test/model/message_test.dart index 802ecc1f05..5845558180 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -168,6 +168,7 @@ void main() { ..bodyFields['queue_id'].equals(store.queueId) ..bodyFields['local_id'].equals('${outboxMessage.localMessageId}'); check(outboxMessage).state.equals(OutboxMessageState.hidden); + checkNotNotified(); // Handle the message event before `future` completes, i.e. while the // message is being sent. @@ -175,12 +176,14 @@ void main() { localMessageId: outboxMessage.localMessageId)); check(store.outboxMessages).isEmpty(); check(outboxMessage).state.equals(OutboxMessageState.hidden); + checkNotifiedOnce(); // Complete the send request. The outbox message should no longer get // updated because it is not in the store any more. async.elapse(const Duration(seconds: 1)); await sendMessageFuture; check(outboxMessage).state.equals(OutboxMessageState.hidden); + checkNotNotified(); })); test('while message is being sent, message event arrives, then the send fails', () => awaitFakeAsync((async) async { @@ -188,6 +191,7 @@ void main() { // the message event to arrive. await prepareSendMessageToFail(delay: const Duration(seconds: 1)); check(outboxMessage).state.equals(OutboxMessageState.hidden); + checkNotNotified(); // Handle the message event before `future` completes, i.e. while the // message is being sent. @@ -195,12 +199,14 @@ void main() { localMessageId: outboxMessage.localMessageId)); check(store.outboxMessages).isEmpty(); check(outboxMessage).state.equals(OutboxMessageState.hidden); + checkNotifiedOnce(); // Complete the send request with an error. The outbox message should no // longer be updated because it is not in the store any more. async.elapse(const Duration(seconds: 1)); await check(sendMessageFuture).throws(); check(outboxMessage).state.equals(OutboxMessageState.hidden); + checkNotNotified(); })); test('message is sent successfully, message event arrives before debounce timeout', () async { @@ -208,6 +214,7 @@ void main() { await prepareSendMessageToSucceed(); await sendMessageFuture; check(outboxMessage).state.equals(OutboxMessageState.hidden); + checkNotNotified(); // Handle the event after the message is sent but before the debounce // timeout. @@ -217,6 +224,7 @@ void main() { // The outbox message should remain hidden since the send // request was successful. check(outboxMessage).state.equals(OutboxMessageState.hidden); + checkNotifiedOnce(); }); test('DM message is sent successfully, message event arrives before debounce timeout', () async { @@ -225,6 +233,7 @@ void main() { userIds: [eg.selfUser.userId, eg.otherUser.userId])); await sendMessageFuture; check(outboxMessage).state.equals(OutboxMessageState.hidden); + checkNotNotified(); // Handle the event after the message is sent but before the debounce // timeout. @@ -235,6 +244,7 @@ void main() { // The outbox message should remain hidden since the send // request was successful. check(outboxMessage).state.equals(OutboxMessageState.hidden); + checkNotifiedOnce(); }); test('message is sent successfully, message event arrives after debounce timeout', () => awaitFakeAsync((async) async { @@ -242,12 +252,14 @@ void main() { await prepareSendMessageToSucceed(); await sendMessageFuture; check(outboxMessage).state.equals(OutboxMessageState.hidden); + checkNotNotified(); // Pass enough time without handling the message event, to expire // the debounce timer. async.elapse(kLocalEchoDebounceDuration); check(store.outboxMessages).values.single.identicalTo(outboxMessage); check(outboxMessage).state.equals(OutboxMessageState.waiting); + checkNotifiedOnce(); // Handle the event when the outbox message is in waiting state. // The outbox message should be removed without errors. @@ -257,6 +269,7 @@ void main() { // The outbox message should no longer be updated because it is not in // the store any more. check(outboxMessage).state.equals(OutboxMessageState.waiting); + checkNotifiedOnce(); })); test('message failed to send before debounce timeout', () => awaitFakeAsync((async) async { @@ -269,6 +282,7 @@ void main() { (it) => it.isA().duration.equals(kSendMessageRetryWaitPeriod), (it) => it.isA().duration.equals(Duration.zero), // timer for send-message response ]); + checkNotNotified(); // Complete the send request with an error. await check(sendMessageFuture).throws(); @@ -276,6 +290,7 @@ void main() { check(outboxMessage).state.equals(OutboxMessageState.failed); // Both the debounce timer and wait period timer should have been cancelled. check(async.pendingTimers).isEmpty(); + checkNotifiedOnce(); })); test('message failed to send after debounce timeout', () => awaitFakeAsync((async) async { @@ -283,18 +298,21 @@ void main() { await prepareSendMessageToFail( delay: kLocalEchoDebounceDuration + const Duration(milliseconds: 1)); check(outboxMessage).state.equals(OutboxMessageState.hidden); + checkNotNotified(); // Wait for just enough time for the debounce timer to expire, but not // for the send request to complete. async.elapse(kLocalEchoDebounceDuration); check(store.outboxMessages).values.single.identicalTo(outboxMessage); check(outboxMessage).state.equals(OutboxMessageState.waiting); + checkNotifiedOnce(); // Complete the send request with an error. async.elapse(const Duration(milliseconds: 1)); await check(sendMessageFuture).throws(); check(store.outboxMessages).values.single.identicalTo(outboxMessage); check(outboxMessage).state.equals(OutboxMessageState.failed); + checkNotifiedOnce(); })); test('message failed to send, message event arrives', () async { @@ -302,6 +320,7 @@ void main() { await prepareSendMessageToFail(); await check(sendMessageFuture).throws(); check(outboxMessage).state.equals(OutboxMessageState.failed); + checkNotifiedOnce(); // Handle the event when the outbox message is in failed state. // The outbox message should be removed without errors. @@ -311,6 +330,7 @@ void main() { // The outbox message should no longer be updated because it is not in // the store any more. check(outboxMessage).state.equals(OutboxMessageState.failed); + checkNotifiedOnce(); }); test('send request pending until after kSendMessageRetryWaitPeriod, completes successfully, then message event arrives', () => awaitFakeAsync((async) async { @@ -320,12 +340,14 @@ void main() { delay: kSendMessageRetryWaitPeriod + Duration(seconds: 1)); async.elapse(kLocalEchoDebounceDuration); check(outboxMessage).state.equals(OutboxMessageState.waiting); + checkNotifiedOnce(); // Wait till we reach [kSendMessageRetryWaitPeriod] after the send request // was initiated, but before it actually completes. assert(kSendMessageRetryWaitPeriod > kLocalEchoDebounceDuration); async.elapse(kSendMessageRetryWaitPeriod - kLocalEchoDebounceDuration); check(outboxMessage).state.equals(OutboxMessageState.waitPeriodExpired); + checkNotifiedOnce(); // Wait till the send request completes successfully. async.elapse(const Duration(seconds: 1)); @@ -334,6 +356,7 @@ void main() { check(store.outboxMessages).values.single.identicalTo(outboxMessage); // … and stay in the waitPeriodExpired state. check(outboxMessage).state.equals(OutboxMessageState.waitPeriodExpired); + checkNotNotified(); // Handle the message event. The outbox message should get removed // without errors. @@ -343,6 +366,7 @@ void main() { // The outbox message should no longer be updated because it is not in // the store any more. check(outboxMessage).state.equals(OutboxMessageState.waitPeriodExpired); + checkNotifiedOnce(); })); test('send request pending until after kSendMessageRetryWaitPeriod, then fails', () => awaitFakeAsync((async) async { @@ -352,12 +376,14 @@ void main() { delay: kSendMessageRetryWaitPeriod + Duration(seconds: 1)); async.elapse(kLocalEchoDebounceDuration); check(outboxMessage).state.equals(OutboxMessageState.waiting); + checkNotifiedOnce(); // Wait till we reach [kSendMessageRetryWaitPeriod] after the send request // was initiated, but before it fails. assert(kSendMessageRetryWaitPeriod > kLocalEchoDebounceDuration); async.elapse(kSendMessageRetryWaitPeriod - kLocalEchoDebounceDuration); check(outboxMessage).state.equals(OutboxMessageState.waitPeriodExpired); + checkNotifiedOnce(); // Wait till the send request fails. async.elapse(Duration(seconds: 1)); @@ -366,6 +392,7 @@ void main() { check(store.outboxMessages).values.single.identicalTo(outboxMessage); // … and transition to failed state. check(outboxMessage).state.equals(OutboxMessageState.failed); + checkNotifiedOnce(); })); test('send request completes, message event does not arrive after kSendMessageRetryWaitPeriod', () => awaitFakeAsync((async) async { @@ -373,6 +400,7 @@ void main() { await prepareSendMessageToSucceed(); async.elapse(kLocalEchoDebounceDuration); check(outboxMessage).state.equals(OutboxMessageState.waiting); + checkNotifiedOnce(); // Wait till we reach [kSendMessageRetryWaitPeriod] after the send request // was initiated. @@ -380,6 +408,7 @@ void main() { async.elapse(kSendMessageRetryWaitPeriod - kLocalEchoDebounceDuration); // The outbox message should transition to waitPeriodExpired state. check(outboxMessage).state.equals(OutboxMessageState.waitPeriodExpired); + checkNotifiedOnce(); })); test('send request fails, message event does not arrive after kSendMessageRetryWaitPeriod', () => awaitFakeAsync((async) async { @@ -387,6 +416,7 @@ void main() { await prepareSendMessageToFail(); async.elapse(kLocalEchoDebounceDuration); check(outboxMessage).state.equals(OutboxMessageState.failed); + checkNotifiedOnce(); // Wait till we reach [kSendMessageRetryWaitPeriod] after the send request // was initiated. @@ -395,6 +425,7 @@ void main() { // The outbox message should stay in failed state, // and it should not transition to waitPeriodExpired state. check(outboxMessage).state.equals(OutboxMessageState.failed); + checkNotNotified(); })); test('when sending to empty topic, interpret topic like the server does when creating outbox message', () => awaitFakeAsync((async) async { @@ -405,6 +436,7 @@ void main() { async.elapse(kLocalEchoDebounceDuration); check(outboxMessage).conversation.isA() .topic.equals(eg.t(eg.defaultRealmEmptyTopicDisplayName)); + checkNotifiedOnce(); })); test('legacy: when sending to empty topic, interpret topic like the server does when creating outbox message', () => awaitFakeAsync((async) async { @@ -415,6 +447,7 @@ void main() { async.elapse(kLocalEchoDebounceDuration); check(outboxMessage).conversation.isA() .topic.equals(eg.t('(no topic)')); + checkNotifiedOnce(); })); test('set timestamp to now when creating outbox messages', () => awaitFakeAsync( @@ -435,6 +468,7 @@ void main() { final localMessageIds = store.outboxMessages.keys.toList(); store.removeOutboxMessage(localMessageIds.removeAt(5)); check(store.outboxMessages.keys).deepEquals(localMessageIds); + checkNotNotified(); }); group('reconcileMessages', () { diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index d17d032adc..e68e728841 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -14,12 +14,14 @@ import 'package:zulip/api/model/narrow.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'; import 'package:zulip/model/typing_status.dart'; import 'package:zulip/widgets/autocomplete.dart'; import 'package:zulip/widgets/color.dart'; +import 'package:zulip/widgets/compose_box.dart'; import 'package:zulip/widgets/content.dart'; import 'package:zulip/widgets/icons.dart'; import 'package:zulip/widgets/message_list.dart'; @@ -1353,6 +1355,37 @@ void main() { }); }); + group('OutboxMessageWithPossibleSender', () { + final stream = eg.stream(); + const content = 'outbox message content'; + + final contentInputFinder = find.byWidgetPredicate( + (widget) => widget is TextField && widget.controller is ComposeContentController); + + Finder outboxMessageFinder = find.descendant( + of: find.byType(MessageItem), + matching: find.text(content, findRichText: true)).hitTestable(); + + testWidgets('sent message appear in message list after debounce timeout', (tester) async { + await setupMessageListPage(tester, + narrow: eg.topicNarrow(stream.streamId, 'topic'), streams: [stream], + messages: []); + + connection.prepare(json: SendMessageResult(id: 1).toJson()); + await tester.enterText(contentInputFinder, content); + await tester.tap(find.byIcon(ZulipIcons.send)); + await tester.pump(Duration.zero); + check(outboxMessageFinder).findsNothing(); + + await tester.pump(kLocalEchoDebounceDuration); + check(outboxMessageFinder).findsOne(); + + await store.handleEvent(eg.messageEvent( + eg.streamMessage(stream: stream, topic: 'topic'), + localMessageId: store.outboxMessages.keys.single)); + }); + }); + group('Starred messages', () { testWidgets('unstarred message', (tester) async { final message = eg.streamMessage(flags: []); From 28d47e9da2e361993a15fe7fb1d96c9734acfe15 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 1 Apr 2025 17:44:47 -0400 Subject: [PATCH 18/19] msglist: Support retrieving failed outbox message content --- assets/l10n/app_en.arb | 4 + lib/generated/l10n/zulip_localizations.dart | 6 + .../l10n/zulip_localizations_ar.dart | 3 + .../l10n/zulip_localizations_en.dart | 3 + .../l10n/zulip_localizations_ja.dart | 3 + .../l10n/zulip_localizations_nb.dart | 3 + .../l10n/zulip_localizations_pl.dart | 3 + .../l10n/zulip_localizations_ru.dart | 3 + .../l10n/zulip_localizations_sk.dart | 3 + .../l10n/zulip_localizations_uk.dart | 3 + lib/model/message.dart | 5 +- lib/widgets/message_list.dart | 94 ++++++++-- test/widgets/message_list_test.dart | 160 +++++++++++++++++- 13 files changed, 271 insertions(+), 22 deletions(-) diff --git a/assets/l10n/app_en.arb b/assets/l10n/app_en.arb index ea2e10cff3..1e00c52242 100644 --- a/assets/l10n/app_en.arb +++ b/assets/l10n/app_en.arb @@ -801,6 +801,10 @@ "@messageIsMovedLabel": { "description": "Label for a moved message. (Use ALL CAPS for cased alphabets: Latin, Greek, Cyrillic, etc.)" }, + "messageIsntSentLabel": "MESSAGE ISN'T SENT. CHECK YOUR CONNECTION.", + "@messageIsntSentLabel": { + "description": "Label for a message that isn't sent. (Use ALL CAPS for cased alphabets: Latin, Greek, Cyrillic, etc.)" + }, "pollVoterNames": "({voterNames})", "@pollVoterNames": { "description": "The list of people who voted for a poll option, wrapped in parentheses.", diff --git a/lib/generated/l10n/zulip_localizations.dart b/lib/generated/l10n/zulip_localizations.dart index e8b15440e3..eae78bd9bc 100644 --- a/lib/generated/l10n/zulip_localizations.dart +++ b/lib/generated/l10n/zulip_localizations.dart @@ -1169,6 +1169,12 @@ abstract class ZulipLocalizations { /// **'MOVED'** String get messageIsMovedLabel; + /// Label for a message that isn't sent. (Use ALL CAPS for cased alphabets: Latin, Greek, Cyrillic, etc.) + /// + /// In en, this message translates to: + /// **'MESSAGE ISN\'T SENT. CHECK YOUR CONNECTION.'** + String get messageIsntSentLabel; + /// The list of people who voted for a poll option, wrapped in parentheses. /// /// In en, this message translates to: diff --git a/lib/generated/l10n/zulip_localizations_ar.dart b/lib/generated/l10n/zulip_localizations_ar.dart index c2478f4613..6fc0f40ca1 100644 --- a/lib/generated/l10n/zulip_localizations_ar.dart +++ b/lib/generated/l10n/zulip_localizations_ar.dart @@ -625,6 +625,9 @@ class ZulipLocalizationsAr extends ZulipLocalizations { @override String get messageIsMovedLabel => 'MOVED'; + @override + String get messageIsntSentLabel => 'MESSAGE ISN\'T SENT. CHECK YOUR CONNECTION.'; + @override String pollVoterNames(String voterNames) { return '($voterNames)'; diff --git a/lib/generated/l10n/zulip_localizations_en.dart b/lib/generated/l10n/zulip_localizations_en.dart index 289ba33af2..6095ac49a3 100644 --- a/lib/generated/l10n/zulip_localizations_en.dart +++ b/lib/generated/l10n/zulip_localizations_en.dart @@ -625,6 +625,9 @@ class ZulipLocalizationsEn extends ZulipLocalizations { @override String get messageIsMovedLabel => 'MOVED'; + @override + String get messageIsntSentLabel => 'MESSAGE ISN\'T SENT. CHECK YOUR CONNECTION.'; + @override String pollVoterNames(String voterNames) { return '($voterNames)'; diff --git a/lib/generated/l10n/zulip_localizations_ja.dart b/lib/generated/l10n/zulip_localizations_ja.dart index 00537f73a2..5330bea77e 100644 --- a/lib/generated/l10n/zulip_localizations_ja.dart +++ b/lib/generated/l10n/zulip_localizations_ja.dart @@ -625,6 +625,9 @@ class ZulipLocalizationsJa extends ZulipLocalizations { @override String get messageIsMovedLabel => 'MOVED'; + @override + String get messageIsntSentLabel => 'MESSAGE ISN\'T SENT. CHECK YOUR CONNECTION.'; + @override String pollVoterNames(String voterNames) { return '($voterNames)'; diff --git a/lib/generated/l10n/zulip_localizations_nb.dart b/lib/generated/l10n/zulip_localizations_nb.dart index 3c063e91da..6e0b7b64a3 100644 --- a/lib/generated/l10n/zulip_localizations_nb.dart +++ b/lib/generated/l10n/zulip_localizations_nb.dart @@ -625,6 +625,9 @@ class ZulipLocalizationsNb extends ZulipLocalizations { @override String get messageIsMovedLabel => 'MOVED'; + @override + String get messageIsntSentLabel => 'MESSAGE ISN\'T SENT. CHECK YOUR CONNECTION.'; + @override String pollVoterNames(String voterNames) { return '($voterNames)'; diff --git a/lib/generated/l10n/zulip_localizations_pl.dart b/lib/generated/l10n/zulip_localizations_pl.dart index d4c3a033d9..19ab8bdc91 100644 --- a/lib/generated/l10n/zulip_localizations_pl.dart +++ b/lib/generated/l10n/zulip_localizations_pl.dart @@ -625,6 +625,9 @@ class ZulipLocalizationsPl extends ZulipLocalizations { @override String get messageIsMovedLabel => 'PRZENIESIONO'; + @override + String get messageIsntSentLabel => 'MESSAGE ISN\'T SENT. CHECK YOUR CONNECTION.'; + @override String pollVoterNames(String voterNames) { return '($voterNames)'; diff --git a/lib/generated/l10n/zulip_localizations_ru.dart b/lib/generated/l10n/zulip_localizations_ru.dart index cb09ca516e..f6e4222a45 100644 --- a/lib/generated/l10n/zulip_localizations_ru.dart +++ b/lib/generated/l10n/zulip_localizations_ru.dart @@ -625,6 +625,9 @@ class ZulipLocalizationsRu extends ZulipLocalizations { @override String get messageIsMovedLabel => 'ПЕРЕМЕЩЕНО'; + @override + String get messageIsntSentLabel => 'MESSAGE ISN\'T SENT. CHECK YOUR CONNECTION.'; + @override String pollVoterNames(String voterNames) { return '($voterNames)'; diff --git a/lib/generated/l10n/zulip_localizations_sk.dart b/lib/generated/l10n/zulip_localizations_sk.dart index 0cb42c3a37..285506e028 100644 --- a/lib/generated/l10n/zulip_localizations_sk.dart +++ b/lib/generated/l10n/zulip_localizations_sk.dart @@ -625,6 +625,9 @@ class ZulipLocalizationsSk extends ZulipLocalizations { @override String get messageIsMovedLabel => 'PRESUNUTÉ'; + @override + String get messageIsntSentLabel => 'MESSAGE ISN\'T SENT. CHECK YOUR CONNECTION.'; + @override String pollVoterNames(String voterNames) { return '($voterNames)'; diff --git a/lib/generated/l10n/zulip_localizations_uk.dart b/lib/generated/l10n/zulip_localizations_uk.dart index b7e6d792f2..be5670e556 100644 --- a/lib/generated/l10n/zulip_localizations_uk.dart +++ b/lib/generated/l10n/zulip_localizations_uk.dart @@ -625,6 +625,9 @@ class ZulipLocalizationsUk extends ZulipLocalizations { @override String get messageIsMovedLabel => 'ПЕРЕМІЩЕНО'; + @override + String get messageIsntSentLabel => 'MESSAGE ISN\'T SENT. CHECK YOUR CONNECTION.'; + @override String pollVoterNames(String voterNames) { return '($voterNames)'; diff --git a/lib/model/message.dart b/lib/model/message.dart index 835d85ab40..b02ec39494 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -276,9 +276,8 @@ mixin _OutboxMessageStore on PerAccountStoreBase { void _handleMessageEventOutbox(MessageEvent event) { if (event.localMessageId != null) { final localMessageId = int.parse(event.localMessageId!, radix: 10); - // The outbox message can be missing if the user removes it (to be - // implemented in #1441) before the event arrives. - // Nothing to do in that case. + // The outbox message can be missing if the user removes it before the + // event arrives. Nothing to do in that case. _outboxMessages.remove(localMessageId); _outboxMessageDebounceTimers.remove(localMessageId)?.cancel(); _outboxMessageWaitPeriodTimers.remove(localMessageId)?.cancel(); diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 8693a61ca7..f5e744b433 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -8,6 +8,7 @@ import 'package:intl/intl.dart' hide TextDirection; import '../api/model/model.dart'; import '../generated/l10n/zulip_localizations.dart'; +import '../model/message.dart'; import '../model/message_list.dart'; import '../model/narrow.dart'; import '../model/store.dart'; @@ -1478,21 +1479,86 @@ class OutboxMessageWithPossibleSender extends StatelessWidget { final MessageListOutboxMessageItem item; + // TODO should we restore the topic as well? + void _handlePress(BuildContext context) { + final content = item.message.content.endsWith('\n') + ? item.message.content : '${item.message.content}\n'; + + final composeBoxController = + MessageListPage.ancestorOf(context).composeBoxController; + composeBoxController!.content.insertPadded(content); + if (!composeBoxController.contentFocusNode.hasFocus) { + composeBoxController.contentFocusNode.requestFocus(); + } + + final store = PerAccountStoreWidget.of(context); + assert(store.outboxMessages.containsKey(item.message.localMessageId)); + store.removeOutboxMessage(item.message.localMessageId); + } + @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 adapated 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))), - ])); + final designVariables = DesignVariables.of(context); + final zulipLocalizations = ZulipLocalizations.of(context); + final isComposeBoxOffered = + MessageListPage.ancestorOf(context).composeBoxController != null; + + final GestureTapCallback? handleTap; + final double opacity; + final Widget bottom; + switch (item.message.state) { + case OutboxMessageState.hidden: + assert(false, + 'Hidden OutboxMessage messages should not appear in message lists'); + handleTap = null; + opacity = 1.0; + bottom = SizedBox.shrink(); + + case OutboxMessageState.waiting: + handleTap = null; + opacity = 1.0; + bottom = LinearProgressIndicator( + minHeight: 2, + color: designVariables.foreground.withFadedAlpha(0.5), + backgroundColor: designVariables.foreground.withFadedAlpha(0.2)); + + case OutboxMessageState.failed: + case OutboxMessageState.waitPeriodExpired: + handleTap = isComposeBoxOffered ? () => _handlePress(context) : null; + opacity = 0.6; + bottom = Text( + zulipLocalizations.messageIsntSentLabel, + textAlign: TextAlign.end, + style: TextStyle( + color: designVariables.btnLabelAttLowIntDanger, + fontSize: 12, + height: 12 / 12, + letterSpacing: proportionalLetterSpacing( + context, 0.006, baseFontSize: 12), + ).merge(weightVariableTextStyle(context, wght: 400))); + } + + return GestureDetector( + onTap: handleTap, + behavior: HitTestBehavior.opaque, + child: Opacity(opacity: opacity, child: Padding( + padding: const EdgeInsets.symmetric(vertical: 4), + child: Column(children: [ + if (item.showSender) + _SenderRow(message: item.message, showTimestamp: false), + Padding( + padding: const EdgeInsets.symmetric(horizontal: 16), + child: Column(crossAxisAlignment: CrossAxisAlignment.stretch, + children: [ + // This is adapated from [MessageContent]. + // TODO(#576): Offer InheritedMessage ancestor once we are ready + // to support local echoing images and lightbox. + DefaultTextStyle( + style: ContentTheme.of(context).textStylePlainParagraph, + child: BlockContentList(nodes: item.content.nodes)), + + bottom, + ])), + ])))); } } diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index e68e728841..1d7eefd058 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -1366,24 +1366,174 @@ void main() { of: find.byType(MessageItem), matching: find.text(content, findRichText: true)).hitTestable(); - testWidgets('sent message appear in message list after debounce timeout', (tester) async { - await setupMessageListPage(tester, - narrow: eg.topicNarrow(stream.streamId, 'topic'), streams: [stream], - messages: []); + Finder messageIsntSentErrorFinder = find.text( + 'MESSAGE ISN\'T SENT. CHECK YOUR CONNECTION.').hitTestable(); - connection.prepare(json: SendMessageResult(id: 1).toJson()); + 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); + } + + Future sendMessageAndFail(WidgetTester tester) async { + // Send a message and fail. Dismiss the error dialog as it pops up. + connection.prepare(apiException: eg.apiBadRequest()); await tester.enterText(contentInputFinder, content); await tester.tap(find.byIcon(ZulipIcons.send)); await tester.pump(Duration.zero); + await tester.tap(find.byWidget( + checkErrorDialog(tester, expectedTitle: 'Message not sent'))); + await tester.pump(); + check(outboxMessageFinder).findsOne(); + check(messageIsntSentErrorFinder).findsOne(); + } + + testWidgets('sent message appear in message list after debounce timeout', (tester) async { + await setupMessageListPage(tester, + narrow: eg.topicNarrow(stream.streamId, 'topic'), streams: [stream], + messages: []); + await sendMessageAndSucceed(tester); check(outboxMessageFinder).findsNothing(); await tester.pump(kLocalEchoDebounceDuration); check(outboxMessageFinder).findsOne(); + check(find.descendant( + of: find.byType(MessageItem), + matching: find.byType(LinearProgressIndicator))).findsOne(); await store.handleEvent(eg.messageEvent( eg.streamMessage(stream: stream, topic: 'topic'), localMessageId: store.outboxMessages.keys.single)); }); + + testWidgets('failed to send message, retrieve the content to compose box', (tester) async { + await setupMessageListPage(tester, + narrow: eg.topicNarrow(stream.streamId, 'topic'), streams: [stream], + messages: []); + await sendMessageAndFail(tester); + + final controller = tester.state(find.byType(ComposeBox)).controller; + check(controller.content).text.isNotNull().isEmpty(); + + // Tap the message. This should put its content back into the compose box + // and remove it. + await tester.tap(outboxMessageFinder); + await tester.pump(); + check(outboxMessageFinder).findsNothing(); + check(controller.content).text.equals('$content\n\n'); + + await tester.pump(kLocalEchoDebounceDuration); + }); + + testWidgets('message sent, reaches wait period time limit and fail, retrieve the content to compose box, then message event arrives', (tester) async { + await setupMessageListPage(tester, + narrow: eg.topicNarrow(stream.streamId, 'topic'), streams: [stream], + messages: []); + await sendMessageAndSucceed(tester); + await tester.pump(kSendMessageRetryWaitPeriod); + check(messageIsntSentErrorFinder).findsOne(); + final localMessageId = store.outboxMessages.keys.single; + + final controller = tester.state(find.byType(ComposeBox)).controller; + check(controller.content).text.isNotNull().isEmpty(); + + // Tap the message. This should put its content back into the compose box + // and remove it. + await tester.tap(outboxMessageFinder); + await tester.pump(); + check(outboxMessageFinder).findsNothing(); + check(controller.content).text.equals('$content\n\n'); + check(store.outboxMessages).isEmpty(); + + // While `localMessageId` is no longer in store, there should be no error + // when a message event refers to it. + await store.handleEvent(eg.messageEvent( + eg.streamMessage(stream: stream, topic: 'topic'), + localMessageId: localMessageId)); + }); + + + testWidgets('tapping does nothing if compose box is not offered', (tester) async { + final messages = [eg.streamMessage(stream: stream, topic: 'some topic')]; + await setupMessageListPage(tester, + narrow: const CombinedFeedNarrow(), streams: [stream], subscriptions: [eg.subscription(stream)], + messages: messages); + + // Navigate to a message list page in a topic narrow, + // which has a compose box. + connection.prepare(json: + eg.newestGetMessagesResult(foundOldest: true, messages: messages).toJson()); + await tester.tap(find.text('some topic')); + await tester.pump(); // handle tap + await tester.pump(); // wait for navigation + await sendMessageAndFail(tester); + + // Navigate back to the message list page without a compose box, + // where the failed to send message should still be visible. + await tester.pageBack(); + await tester.pump(); // handle tap + await tester.pump(); // wait for navigation + check(contentInputFinder).findsNothing(); + check(outboxMessageFinder).findsOne(); + check(messageIsntSentErrorFinder).findsOne(); + + // Tap the failed to send message. + // This should not remove it from the message list. + await tester.tap(outboxMessageFinder); + await tester.pump(); + check(outboxMessageFinder).findsOne(); + }); + + testWidgets('tapping does nothing if message is still being sent', (tester) async { + await setupMessageListPage(tester, + narrow: eg.topicNarrow(stream.streamId, 'topic'), streams: [stream], + messages: []); + final controller = tester.state(find.byType(ComposeBox)).controller; + + // Send a message and wait until the debounce timer expires but before + // the message is successfully sent. + await sendMessageAndSucceed(tester, + delay: kLocalEchoDebounceDuration + Duration(seconds: 1)); + await tester.pump(kLocalEchoDebounceDuration); + check(controller.content).text.isNotNull().isEmpty(); + + await tester.tap(outboxMessageFinder); + await tester.pump(); + check(outboxMessageFinder).findsOne(); + check(controller.content).text.isNotNull().isEmpty(); + + // Wait till the send request completes. The outbox message should + // remain visible because the message event didn't arrive. + await tester.pump(Duration(seconds: 1)); + check(outboxMessageFinder).findsOne(); + check(controller.content).text.isNotNull().isEmpty(); + + // Dispose pending timers from the message store. + store.dispose(); + }); + + testWidgets('tapping does nothing if message was successfully sent and before message event arrives', (tester) async { + await setupMessageListPage(tester, + narrow: eg.topicNarrow(stream.streamId, 'topic'), streams: [stream], + messages: []); + final controller = tester.state(find.byType(ComposeBox)).controller; + + // Send a message and wait until the debounce timer expires. + await sendMessageAndSucceed(tester); + await tester.pump(kLocalEchoDebounceDuration); + check(controller.content).text.isNotNull().isEmpty(); + + await tester.tap(outboxMessageFinder); + await tester.pump(); + check(outboxMessageFinder).findsOne(); + check(controller.content).text.isNotNull().isEmpty(); + + // Dispose pending timers from the message store. + store.dispose(); + }); }); group('Starred messages', () { From d0b37e4aec46cf9d41465d532b6de3356af44170 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Mon, 14 Apr 2025 20:01:20 -0400 Subject: [PATCH 19/19] msglist: Retrieve topic from failed-to-send messages Fixes: #1441 --- lib/widgets/message_list.dart | 8 +++++++- test/widgets/message_list_test.dart | 30 ++++++++++++++++++++++++++++- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index f5e744b433..12fa47e301 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -1479,7 +1479,6 @@ class OutboxMessageWithPossibleSender extends StatelessWidget { final MessageListOutboxMessageItem item; - // TODO should we restore the topic as well? void _handlePress(BuildContext context) { final content = item.message.content.endsWith('\n') ? item.message.content : '${item.message.content}\n'; @@ -1491,6 +1490,13 @@ class OutboxMessageWithPossibleSender extends StatelessWidget { composeBoxController.contentFocusNode.requestFocus(); } + if (composeBoxController case StreamComposeBoxController(:final topic)) { + final conversation = item.message.conversation; + if (conversation is StreamConversation) { + topic.setTopic(conversation.topic); + } + } + final store = PerAccountStoreWidget.of(context); assert(store.outboxMessages.containsKey(item.message.localMessageId)); store.removeOutboxMessage(item.message.localMessageId); diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 1d7eefd058..521a6c5cd5 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -11,6 +11,7 @@ import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/initial_snapshot.dart'; import 'package:zulip/api/model/model.dart'; import 'package:zulip/api/model/narrow.dart'; +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'; @@ -1359,6 +1360,8 @@ void main() { final stream = eg.stream(); const content = 'outbox message content'; + final topicInputFinder = find.byWidgetPredicate( + (widget) => widget is TextField && widget.controller is ComposeTopicController); final contentInputFinder = find.byWidgetPredicate( (widget) => widget is TextField && widget.controller is ComposeContentController); @@ -1409,7 +1412,32 @@ void main() { localMessageId: store.outboxMessages.keys.single)); }); - testWidgets('failed to send message, retrieve the content to compose box', (tester) async { + testWidgets('in channel narrow, failed to send message, retrieve both topic and content to compose box', (tester) async { + await setupMessageListPage(tester, + narrow: ChannelNarrow(stream.streamId), streams: [stream], + messages: []); + + connection.prepare(json: GetStreamTopicsResult(topics: []).toJson()); + await tester.enterText(topicInputFinder, 'test topic'); + await sendMessageAndFail(tester); + + final controller = tester.state(find.byType(ComposeBox)).controller; + controller as StreamComposeBoxController; + await tester.enterText(topicInputFinder, 'different topic'); + check(controller.content).text.isNotNull().isEmpty(); + + // Tap the message. This should put its content back into the compose box + // and remove it. + await tester.tap(outboxMessageFinder); + await tester.pump(); + check(outboxMessageFinder).findsNothing(); + check(controller.topic).text.equals('test topic'); + check(controller.content).text.equals('$content\n\n'); + + await tester.pump(kLocalEchoDebounceDuration); + }); + + testWidgets('in topic narrow, failed to send message, retrieve the content to compose box', (tester) async { await setupMessageListPage(tester, narrow: eg.topicNarrow(stream.streamId, 'topic'), streams: [stream], messages: []);