diff --git a/assets/l10n/app_en.arb b/assets/l10n/app_en.arb index d11bf43eda..a384632a02 100644 --- a/assets/l10n/app_en.arb +++ b/assets/l10n/app_en.arb @@ -864,6 +864,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 75b70a0377..b0eb24d549 100644 --- a/lib/generated/l10n/zulip_localizations.dart +++ b/lib/generated/l10n/zulip_localizations.dart @@ -1274,6 +1274,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 98dd9a7af6..b8b34d433b 100644 --- a/lib/generated/l10n/zulip_localizations_ar.dart +++ b/lib/generated/l10n/zulip_localizations_ar.dart @@ -704,6 +704,10 @@ 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 0a746ff634..1870a16932 100644 --- a/lib/generated/l10n/zulip_localizations_en.dart +++ b/lib/generated/l10n/zulip_localizations_en.dart @@ -704,6 +704,10 @@ 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 74a2d4bedb..bf101a17d7 100644 --- a/lib/generated/l10n/zulip_localizations_ja.dart +++ b/lib/generated/l10n/zulip_localizations_ja.dart @@ -704,6 +704,10 @@ 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 02913278b8..ee4a52ee26 100644 --- a/lib/generated/l10n/zulip_localizations_nb.dart +++ b/lib/generated/l10n/zulip_localizations_nb.dart @@ -704,6 +704,10 @@ 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 657e1757d1..cfa2ffc3d2 100644 --- a/lib/generated/l10n/zulip_localizations_pl.dart +++ b/lib/generated/l10n/zulip_localizations_pl.dart @@ -713,6 +713,10 @@ 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 5d8899290d..ae084695f6 100644 --- a/lib/generated/l10n/zulip_localizations_ru.dart +++ b/lib/generated/l10n/zulip_localizations_ru.dart @@ -717,6 +717,10 @@ 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 3ff534eca5..8592d3e9a3 100644 --- a/lib/generated/l10n/zulip_localizations_sk.dart +++ b/lib/generated/l10n/zulip_localizations_sk.dart @@ -706,6 +706,10 @@ 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 a756bdba6a..8f3c615090 100644 --- a/lib/generated/l10n/zulip_localizations_uk.dart +++ b/lib/generated/l10n/zulip_localizations_uk.dart @@ -716,6 +716,10 @@ 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 2573cfadc6..95f0eba74a 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -1,21 +1,331 @@ +import 'dart:async'; +import 'dart:collection'; import 'dart:convert'; import 'package:crypto/crypto.dart'; +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: 500); // TODO(#1441) find the right value for this +const kSendMessageOfferRestoreWaitPeriod = Duration(seconds: 10); // TODO(#1441) find the right value for this + +/// States of an [OutboxMessage] since its creation from a +/// [MessageStore.sendMessage] call and before its eventual deletion. +/// +/// ``` +/// ┌─────────────────────┬──► failed ──────────┐ +/// │ │ │ +/// │ 4xx or other │ User restores ├────► (delete) +/// │ error. │ the draft. │ +/// │ │ │ +/// (create) ─► hidden waiting ─────┴─ waitPeriodExpired ─┘ +/// │ ▲ │ ▲ +/// └─────────┘ └──────────┘ +/// 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 + /// [kSendMessageOfferRestoreWaitPeriod] 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; + + /// 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 fails 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 fails 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; + + /// As in [MessageStoreImpl._messageListViews]. + Set get _messageListViews; + + /// As in [MessageStoreImpl._disposed]. + bool get _disposed; + + /// Update the state of the [OutboxMessage] with the given [localMessageId], + /// and notify listeners if necessary. + /// + /// The outbox message with [localMessageId] must exist. + void _updateOutboxMessage(int localMessageId, { + required OutboxMessageState newState, + }) { + assert(!_disposed); + final outboxMessage = outboxMessages[localMessageId]; + if (outboxMessage == null) { + assert(false); + return; + } + final oldState = outboxMessage.state; + // See [OutboxMessageState] for valid state transitions. + assert(newState != oldState); + switch (newState) { + case OutboxMessageState.hidden: + assert(false); + case OutboxMessageState.waiting: + assert(oldState == OutboxMessageState.hidden); + case OutboxMessageState.waitPeriodExpired: + assert(oldState == OutboxMessageState.waiting); + case OutboxMessageState.failed: + assert(oldState == OutboxMessageState.hidden + || oldState == OutboxMessageState.waiting + || oldState == OutboxMessageState.waitPeriodExpired); + } + outboxMessage._state = newState; + for (final view in _messageListViews) { + if (oldState == OutboxMessageState.hidden) { + 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 { + assert(!_disposed); + final localMessageId = _nextLocalMessageId++; + assert(!outboxMessages.containsKey(localMessageId)); + + final now = ZulipBinding.instance.utcNow().millisecondsSinceEpoch ~/ 1000; + _outboxMessages[localMessageId] = switch (destination) { + StreamDestination(:final streamId, :final topic) => StreamOutboxMessage( + localMessageId: localMessageId, + selfUserId: selfUserId, + timestamp: now, + conversation: StreamConversation( + streamId, + topic.processLikeServer( + // Processing this just once on creating the outbox message + // allows an uncommon bug, because either of these values can change. + // During the outbox message's life, a topic processed from + // "(no topic)" could become stale/wrong when zulipFeatureLevel + // changes; a topic processed from "general chat" could become + // stale/wrong when realmEmptyTopicDisplayName changes. + // + // Shrug. The same effect is caused by an unavoidable race: + // 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. + 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(!_disposed); + assert(outboxMessages.containsKey(localMessageId), + 'The timer should have been canceled when the outbox message was removed.'); + _outboxMessageDebounceTimers.remove(localMessageId); + _updateOutboxMessage(localMessageId, newState: OutboxMessageState.waiting); + }); + + _outboxMessageWaitPeriodTimers[localMessageId] = Timer(kSendMessageOfferRestoreWaitPeriod, () { + assert(!_disposed); + assert(outboxMessages.containsKey(localMessageId), + 'The timer should have been canceled when the outbox message was removed.'); + assert(!_outboxMessageDebounceTimers.containsKey(localMessageId), + 'The debounce timer should have been removed before the wait period timer expires.'); + _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) { + if (_disposed) return; + if (!_outboxMessages.containsKey(localMessageId)) { + // The message event already arrived; the failure is probably due to + // networking issues. Don't rethrow; the send succeeded + // (we got the event) so we don't want to show an error dialog. + return; + } + _outboxMessageDebounceTimers.remove(localMessageId)?.cancel(); + _outboxMessageWaitPeriodTimers.remove(localMessageId)?.cancel(); + _updateOutboxMessage(localMessageId, newState: OutboxMessageState.failed); + rethrow; + } + } + + OutboxMessage takeOutboxMessage(int localMessageId) { + assert(!_disposed); + final removed = _outboxMessages.remove(localMessageId); + _outboxMessageDebounceTimers.remove(localMessageId)?.cancel(); + _outboxMessageWaitPeriodTimers.remove(localMessageId)?.cancel(); + if (removed == null) { + throw StateError( + 'Removing unknown outbox message with localMessageId: $localMessageId'); + } + if (removed.state != OutboxMessageState.failed + && removed.state != OutboxMessageState.waitPeriodExpired + ) { + throw StateError('Unexpected state when restoring draft: ${removed.state}'); + } + for (final view in _messageListViews) { + view.removeOutboxMessage(removed); + } + return 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 before the + // event arrives. Nothing to do in that case. + _outboxMessages.remove(localMessageId); + _outboxMessageDebounceTimers.remove(localMessageId)?.cancel(); + _outboxMessageWaitPeriodTimers.remove(localMessageId)?.cancel(); + } + } + + /// Cancel [_OutboxMessageStore]'s timers. + void _disposeOutboxMessages() { + assert(!_disposed); + for (final timer in _outboxMessageDebounceTimers.values) { + timer.cancel(); + } + for (final timer in _outboxMessageWaitPeriodTimers.values) { + timer.cancel(); + } + } +} /// The portion of [PerAccountStore] for messages and message lists. mixin MessageStore { /// All known messages, indexed by [Message.id]. Map get messages; + /// [OutboxMessage]s sent by the user, indexed by [OutboxMessage.localMessageId]. + Map get outboxMessages; + Set get debugMessageListViews; void registerMessageList(MessageListView view); @@ -26,6 +336,15 @@ mixin MessageStore { required String content, }); + /// Remove from [outboxMessages] given the [localMessageId], and return + /// the removed [OutboxMessage]. + /// + /// The outbox message to be taken must exist. + /// + /// The state of the outbox message must be either [OutboxMessageState.failed] + /// or [OutboxMessageState.waitPeriodExpired]. + OutboxMessage takeOutboxMessage(int localMessageId); + /// Reconcile a batch of just-fetched messages with the store, /// mutating the list. /// @@ -78,28 +397,49 @@ class _EditMessageRequestStatus { final String newContent; } -class MessageStoreImpl extends PerAccountStoreBase with MessageStore { - MessageStoreImpl({required super.core}) - // There are no messages in InitialSnapshot, so we don't have - // a use case for initializing MessageStore with nonempty [messages]. - : messages = {}; +class MessageStoreImpl extends PerAccountStoreBase with MessageStore, _OutboxMessageStore { + MessageStoreImpl({required super.core, required String? realmEmptyTopicDisplayName}) + : _realmEmptyTopicDisplayName = realmEmptyTopicDisplayName, + // There are no messages in InitialSnapshot, so we don't have + // a use case for initializing MessageStore with nonempty [messages]. + messages = {}; + + /// The display name to use for empty topics. + /// + /// This should only be accessed when FL >= 334, since topics cannot + /// be empty otherwise. + // TODO(server-10) simplify this + String get realmEmptyTopicDisplayName { + assert(zulipFeatureLevel >= 334); + assert(_realmEmptyTopicDisplayName != null); // TODO(log) + return _realmEmptyTopicDisplayName ?? 'general chat'; + } + final String? _realmEmptyTopicDisplayName; // TODO(#668): update this realm setting @override final Map messages; + @override final Set _messageListViews = {}; @override Set get debugMessageListViews => _messageListViews; + @override + bool _disposed = false; + @override void registerMessageList(MessageListView view) { + assert(!_disposed); final added = _messageListViews.add(view); assert(added); } @override void unregisterMessageList(MessageListView view) { + // TODO: Add `assert(!_disposed);` here once we ensure [PerAccountStore] is + // only disposed after those with references to it are disposed. See + // [disposed] for details. final removed = _messageListViews.remove(view); assert(removed); } @@ -137,21 +477,29 @@ 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 + + assert(!_disposed); + _disposeOutboxMessages(); + _disposed = true; } @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, - ); + assert(!_disposed); + if (!debugOutboxEnable) { + return _apiSendMessage(connection, + destination: destination, + content: content, + readBySender: true); + } + return _outboxSendMessage( + destination: destination, content: content, + realmEmptyTopicDisplayName: realmEmptyTopicDisplayName); } @override void reconcileMessages(List messages) { + assert(!_disposed); // What to do when some of the just-fetched messages are already known? // This is common and normal: in particular it happens when one message list // overlaps another, e.g. a stream and a topic within it. @@ -185,6 +533,7 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { required String originalRawContent, required String newContent, }) async { + assert(!_disposed); if (_editMessageRequests.containsKey(messageId)) { throw StateError('an edit request is already in progress'); } @@ -202,6 +551,8 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { } catch (e) { // TODO(log) if e is something unexpected + if (_disposed) return; + final status = _editMessageRequests[messageId]; if (status == null) { // The event actually arrived before this request failed @@ -216,6 +567,7 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { @override ({String originalRawContent, String newContent}) takeFailedMessageEdit(int messageId) { + assert(!_disposed); final status = _editMessageRequests.remove(messageId); _notifyMessageListViewsForOneMessage(messageId); if (status == null) { @@ -242,6 +594,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); } @@ -341,6 +695,8 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { } } + // TODO predict outbox message moves using propagateMode + for (final view in _messageListViews) { view.messagesMoved(messageMove: messageMove, messageIds: event.messageIds); } @@ -435,4 +791,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 2f8777d3ed..ad6c494b05 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'; @@ -63,6 +64,22 @@ class MessageListMessageItem extends MessageListMessageBaseItem { }); } +/// An [OutboxMessage] to show in the message list. +class MessageListOutboxMessageItem extends MessageListMessageBaseItem { + @override + final OutboxMessage message; + @override + final ZulipContent content; + + MessageListOutboxMessageItem( + this.message, { + required super.showSender, + required super.isLastInBlock, + }) : content = ZulipContent(nodes: [ + ParagraphNode(links: [], nodes: [TextNode(message.content)]), + ]); +} + /// The sequence of messages in a message list, and how to display them. /// /// This comprises much of the guts of [MessageListView]. @@ -124,14 +141,25 @@ mixin _MessageSequence { /// It exists as an optimization, to memoize the work of parsing. final List contents = []; + /// The messages sent by the self-user, retrieved from + /// [MessageStore.outboxMessages]. + /// + /// 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 = []; + /// 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], then a [MessageListOutboxMessageItem] corresponding to each + /// element of [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(); @@ -148,9 +176,10 @@ mixin _MessageSequence { switch (item) { case MessageListRecipientHeaderItem(:var message): case MessageListDateSeparatorItem(:var message): - if (message.id == null) return 1; // TODO(#1441): test + if (message.id == null) return 1; return message.id! <= messageId ? -1 : 1; case MessageListMessageItem(:var message): return message.id.compareTo(messageId); + case MessageListOutboxMessageItem(): return 1; } } @@ -254,10 +283,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] 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; @@ -276,24 +341,28 @@ mixin _MessageSequence { _reprocessAll(); } - /// Append to [items] based on the index-th message and its content. + /// Append to [items] based on [message] and [prevMessage]. /// - /// 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)) { + /// This appends a recipient header or a date separator to [items], + /// depending on how [prevMessage] relates to [message], + /// and then the result of [buildItem]. + /// + /// [prevMessage] should be the message that visually appears before [message]. + /// + /// The caller must ensure that [prevMessage] and all messages before it + /// have been processed. + void _addItemsForMessage(MessageBase message, { + required MessageBase? prevMessage, + required MessageListMessageBaseItem Function(bool canShareSender) buildItem, + }) { + final bool canShareSender; + if (prevMessage == null || !haveSameRecipient(prevMessage, message)) { items.add(MessageListRecipientHeaderItem(message)); canShareSender = false; } else { - assert(items.last is MessageListMessageItem); - final prevMessageItem = items.last as MessageListMessageItem; - assert(identical(prevMessageItem.message, messages[index - 1])); + assert(items.last is MessageListMessageBaseItem); + final prevMessageItem = items.last as MessageListMessageBaseItem; + assert(identical(prevMessageItem.message, prevMessage)); assert(prevMessageItem.isLastInBlock); prevMessageItem.isLastInBlock = false; @@ -301,19 +370,90 @@ mixin _MessageSequence { items.add(MessageListDateSeparatorItem(message)); canShareSender = false; } else { - canShareSender = (prevMessageItem.message.senderId == message.senderId); + canShareSender = prevMessage.senderId == message.senderId; } } - items.add(MessageListMessageItem(message, content, - showSender: !canShareSender, isLastInBlock: true)); + final item = buildItem(canShareSender); + assert(identical(item.message, message)); + assert(item.showSender == !canShareSender); + assert(item.isLastInBlock); + items.add(item); + } + + /// 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) { + assert(items.lastOrNull is! MessageListOutboxMessageItem); + final prevMessage = index == 0 ? null : messages[index - 1]; + final message = messages[index]; + final content = contents[index]; + + _addItemsForMessage(message, + prevMessage: prevMessage, + buildItem: (bool canShareSender) => MessageListMessageItem( + message, content, showSender: !canShareSender, 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]; + + _addItemsForMessage(message, + prevMessage: prevMessage, + buildItem: (bool canShareSender) => MessageListOutboxMessageItem( + message, showSender: !canShareSender, isLastInBlock: true)); } - /// 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; + } + } + + /// 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); + } } } @@ -357,7 +497,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; } @@ -456,11 +598,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; notifyListeners(); @@ -564,16 +708,72 @@ 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) { + 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. + /// + /// This should only be called from [MessageStore.takeOutboxMessage]. + void removeOutboxMessage(OutboxMessage outboxMessage) { + if (_removeOutboxMessage(outboxMessage)) { + notifyListeners(); + } + } + void handleUserTopicEvent(UserTopicEvent event) { switch (_canAffectVisibility(event)) { case VisibilityEffect.none: 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(); } @@ -586,6 +786,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); + } + } } } @@ -599,14 +811,34 @@ 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; } + if (outboxMessages.isEmpty) { + assert(items.none((item) => item is MessageListOutboxMessageItem)); + _addMessage(message); + notifyListeners(); + 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(); } @@ -638,6 +870,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { // TODO in cases where we do have data to do better, do better. _reset(); notifyListeners(); + _syncOutboxMessages(); fetchInitial(); } @@ -653,6 +886,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { case PropagateMode.changeLater: narrow = newNarrow; _reset(); + _syncOutboxMessages(); fetchInitial(); case PropagateMode.changeOne: } @@ -725,6 +959,15 @@ class MessageListView with ChangeNotifier, _MessageSequence { } } + /// Notify listeners if the given outbox message is present in this view. + void notifyListenersIfOutboxMessagePresent(int localMessageId) { + final isAnyPresent = + outboxMessages.any((message) => message.localMessageId == localMessageId); + if (isAnyPresent) { + notifyListeners(); + } + } + /// 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 5f5b19128b..2eea6aacd2 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, @@ -733,6 +734,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 @@ -744,6 +747,9 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor return _messages.sendMessage(destination: destination, content: content); } @override + OutboxMessage takeOutboxMessage(int localMessageId) => + _messages.takeOutboxMessage(localMessageId); + @override void reconcileMessages(List messages) { _messages.reconcileMessages(messages); // TODO(#649) notify [unreads] of the just-fetched messages diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index d629e69029..4fbe503179 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1288,15 +1288,8 @@ class _SendButtonState extends State<_SendButton> { final content = controller.content.textNormalized; controller.content.clear(); - // The following `stoppedComposing` call is currently redundant, - // because clearing input sends a "typing stopped" notice. - // It will be necessary once we resolve #720. - store.typingNotifier.stoppedComposing(); try { - // TODO(#720) clear content input only on success response; - // while waiting, put input(s) and send button into a disabled - // "working on it" state (letting input text be selected for copying). await store.sendMessage(destination: widget.getDestination(), content: content); } on ApiRequestException catch (e) { if (!mounted) return; @@ -1388,7 +1381,6 @@ class _ComposeBoxContainer extends StatelessWidget { border: Border(top: BorderSide(color: designVariables.borderBar)), boxShadow: ComposeBoxTheme.of(context).boxShadow, ), - // TODO(#720) try a Stack for the overlaid linear progress indicator child: Material( color: designVariables.composeBoxBg, child: Column( @@ -1724,10 +1716,6 @@ class _ErrorBanner extends _Banner { @override Widget? buildTrailing(context) { - // TODO(#720) "x" button goes here. - // 24px square with 8px touchable padding in all directions? - // and `bool get padEnd => false`; see Figma: - // https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4031-17029&m=dev return null; } } @@ -2061,11 +2049,6 @@ class _ComposeBoxState extends State with PerAccountStoreAwareStateM } } - // TODO(#720) dismissable message-send error, maybe something like: - // if (controller.sendMessageError.value != null) { - // errorBanner = _ErrorBanner(label: - // ZulipLocalizations.of(context).errorSendMessageTimeout); - // } return ComposeBoxInheritedWidget.fromComposeBoxState(this, child: _ComposeBoxContainer(body: body, banner: banner)); } diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 72f646cc65..7ee1355d6b 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -5,6 +5,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'; @@ -719,7 +720,11 @@ class _MessageListState extends State with PerAccountStoreAwareStat return MessageItem( key: ValueKey(data.message.id), header: header, - trailingWhitespace: 11, + item: data); + case MessageListOutboxMessageItem(): + final header = RecipientHeader(message: data.message, narrow: widget.narrow); + return MessageItem( + header: header, item: data); } } @@ -1018,12 +1023,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) { @@ -1035,8 +1038,8 @@ class MessageItem extends StatelessWidget { child: Column(children: [ switch (item) { MessageListMessageItem() => MessageWithPossibleSender(item: item), + MessageListOutboxMessageItem() => OutboxMessageWithPossibleSender(item: item), }, - if (trailingWhitespace != null && item.isLastInBlock) SizedBox(height: trailingWhitespace!), ])); if (item case MessageListMessageItem(:final message)) { child = _UnreadMarker( @@ -1378,7 +1381,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 @@ -1408,7 +1411,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), @@ -1602,3 +1607,123 @@ class _RestoreEditMessageGestureDetector extends StatelessWidget { child: child); } } + +/// 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; + + void _handlePress(BuildContext context) { + final store = PerAccountStoreWidget.of(context); + assert(store.outboxMessages.containsKey(item.message.localMessageId)); + final message = store.takeOutboxMessage(item.message.localMessageId); + + final content = message.content.endsWith('\n') + ? message.content : '${message.content}\n'; + + final composeBoxController = + MessageListPage.ancestorOf(context).composeBoxState!.controller; + composeBoxController.content.insertPadded(content); + if (!composeBoxController.contentFocusNode.hasFocus) { + composeBoxController.contentFocusNode.requestFocus(); + } + + if (composeBoxController case StreamComposeBoxController(:final topic)) { + final conversation = message.conversation; + if (conversation is StreamConversation) { + topic.setTopic(conversation.topic); + } + } + } + + @override + Widget build(BuildContext context) { + final GestureTapCallback? handleTap; + final double opacity; + switch (item.message.state) { + case OutboxMessageState.hidden: + assert(false, + 'Hidden OutboxMessage messages should not appear in message lists'); + handleTap = null; + opacity = 1.0; + + case OutboxMessageState.waiting: + handleTap = null; + opacity = 1.0; + + case OutboxMessageState.failed: + case OutboxMessageState.waitPeriodExpired: + final isComposeBoxOffered = + MessageListPage.ancestorOf(context).composeBoxState != null; + handleTap = isComposeBoxOffered ? () => _handlePress(context) : null; + opacity = 0.6; + } + + return GestureDetector( + onTap: handleTap, + behavior: HitTestBehavior.opaque, + child: Padding( + padding: const EdgeInsets.only(top: 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. + Opacity(opacity: opacity, child: DefaultTextStyle( + style: ContentTheme.of(context).textStylePlainParagraph, + child: BlockContentList(nodes: item.content.nodes))), + + _OutboxMessageStatusRow(outboxMessageState: item.message.state), + ])), + ]))); + } +} + +class _OutboxMessageStatusRow extends StatelessWidget { + const _OutboxMessageStatusRow({required this.outboxMessageState}); + + final OutboxMessageState outboxMessageState; + + @override + Widget build(BuildContext context) { + switch (outboxMessageState) { + case OutboxMessageState.hidden: + assert(false, + 'Hidden OutboxMessage messages should not appear in message lists'); + return SizedBox.shrink(); + + case OutboxMessageState.waiting: + final designVariables = DesignVariables.of(context); + return Padding( + padding: const EdgeInsetsGeometry.only(top: 1.5, bottom: 0.5), + child: LinearProgressIndicator( + minHeight: 2, + color: designVariables.foreground.withFadedAlpha(0.5), + backgroundColor: designVariables.foreground.withFadedAlpha(0.2))); + + case OutboxMessageState.failed: + case OutboxMessageState.waitPeriodExpired: + final designVariables = DesignVariables.of(context); + final zulipLocalizations = ZulipLocalizations.of(context); + return Padding( + padding: const EdgeInsets.only(bottom: 4), + child: Text( + zulipLocalizations.messageIsntSentLabel, + textAlign: TextAlign.end, + style: TextStyle( + color: designVariables.btnLabelAttLowIntDanger, + fontSize: 12, + height: 12 / 12, + letterSpacing: proportionalLetterSpacing( + context, 0.05, baseFontSize: 12)))); + } + } +} diff --git a/test/api/model/model_checks.dart b/test/api/model/model_checks.dart index 1a17f70f60..f7ec8c0544 100644 --- a/test/api/model/model_checks.dart +++ b/test/api/model/model_checks.dart @@ -30,9 +30,15 @@ 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'); } +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/example_data.dart b/test/example_data.dart index e0f44f9ddc..eab514fe06 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -12,6 +12,7 @@ import 'package:zulip/api/route/realm.dart'; import 'package:zulip/api/route/channels.dart'; import 'package:zulip/model/binding.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'; @@ -567,6 +568,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, @@ -637,8 +676,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_list_test.dart b/test/model/message_list_test.dart index bb85556ae9..98b1996301 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, @@ -97,6 +116,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'; @@ -169,6 +207,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(); @@ -397,6 +498,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(kSendMessageOfferRestoreWaitPeriod); + checkNotified(count: 10); + check(model).outboxMessages.length.equals(5); + + store.takeOutboxMessage(store.outboxMessages.keys.first); + checkNotifiedOnce(); + check(model).outboxMessages.length.equals(4); + })); + + test('not in narrow', () => awaitFakeAsync((async) async { + await prepare(narrow: eg.topicNarrow(stream.streamId, 'topic'), stream: stream); + await prepareMessages(foundOldest: true, messages: + List.generate(30, (i) => eg.streamMessage(stream: stream, topic: 'topic'))); + await prepareOutboxMessages(count: 5, stream: stream, topic: 'other'); + async.elapse(kSendMessageOfferRestoreWaitPeriod); + checkNotNotified(); + check(model).outboxMessages.isEmpty(); + + store.takeOutboxMessage(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(kSendMessageOfferRestoreWaitPeriod); + checkNotified(count: 2); + check(model).outboxMessages.single; + + store.takeOutboxMessage(store.outboxMessages.keys.first); + checkNotifiedOnce(); + check(model).outboxMessages.isEmpty(); + })); }); group('UserTopicEvent', () { @@ -424,7 +684,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(); @@ -438,10 +698,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. @@ -516,7 +815,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: [ @@ -524,10 +823,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()); @@ -537,7 +843,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()); @@ -545,10 +858,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 { @@ -694,6 +1013,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()); @@ -827,6 +1178,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); @@ -1529,6 +1899,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')); @@ -1676,7 +2078,55 @@ void main() { }); }); - test('recipient headers are maintained consistently', () async { + group('findItemWithMessageId', () { + test('has MessageListDateSeparatorItem with null message ID', () => awaitFakeAsync((async) async { + final stream = eg.stream(); + final message = eg.streamMessage(stream: stream, topic: 'topic', + timestamp: eg.utcTimestamp(clock.daysAgo(1))); + await prepare(narrow: ChannelNarrow(stream.streamId)); + await prepareMessages(foundOldest: true, messages: [message]); + + // `findItemWithMessageId` uses binary search. Set up just enough + // outbox message items, so that a [MessageListDateSeparatorItem] for + // the outbox messages is right in the middle. + await prepareOutboxMessages(count: 2, stream: stream, topic: 'topic'); + async.elapse(kLocalEchoDebounceDuration); + checkNotified(count: 2); + check(model.items).deepEquals(>[ + (it) => it.isA(), + (it) => it.isA(), + (it) => it.isA().message.id.isNull(), + (it) => it.isA(), + (it) => it.isA(), + ]); + check(model.findItemWithMessageId(message.id)).equals(1); + })); + + test('has MessageListOutboxMessageItem', () => awaitFakeAsync((async) async { + final stream = eg.stream(); + final message = eg.streamMessage(stream: stream, topic: 'topic', + timestamp: eg.utcTimestamp(clock.now())); + await prepare(narrow: ChannelNarrow(stream.streamId)); + await prepareMessages(foundOldest: true, messages: [message]); + + // `findItemWithMessageId` uses binary search. Set up just enough + // outbox message items, so that a [MessageListOutboxMessageItem] + // is right in the middle. + await prepareOutboxMessages(count: 3, stream: stream, topic: 'topic'); + async.elapse(kLocalEchoDebounceDuration); + checkNotified(count: 3); + check(model.items).deepEquals(>[ + (it) => it.isA(), + (it) => it.isA(), + (it) => it.isA(), + (it) => it.isA(), + (it) => it.isA(), + ]); + check(model.findItemWithMessageId(message.id)).equals(1); + })); + }); + + test('recipient headers are maintained consistently', () => awaitFakeAsync((async) async { // TODO test date separators are maintained consistently too // This tests the code that maintains the invariant that recipient headers // are present just where they're required. @@ -1689,7 +2139,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); @@ -1748,6 +2198,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, @@ -1760,17 +2224,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, @@ -1778,6 +2258,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: [ @@ -1787,6 +2269,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: @@ -1799,8 +2288,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', () { @@ -1871,6 +2362,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', () { @@ -1906,6 +2407,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); } } } @@ -1929,17 +2438,27 @@ 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, ...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); + } 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(): @@ -1963,26 +2482,33 @@ void checkInvariants(MessageListView model) { } int i = 0; - 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 { + check(model.items[i]).isA() + .message.identicalTo(model.outboxMessages[j-model.messages.length]); + } + 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() + || MessageListOutboxMessageItem() || MessageListDateSeparatorItem() => false, MessageListRecipientHeaderItem() => true, }); @@ -2013,6 +2539,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 1809f0888b..cd223b2b52 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -1,14 +1,17 @@ +import 'dart:async'; import 'dart:convert'; import 'dart:io'; import 'package:checks/checks.dart'; import 'package:crypto/crypto.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'; @@ -18,12 +21,17 @@ 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; @@ -42,10 +50,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 selfAccount = eg.selfAccount.copyWith(zulipFeatureLevel: zulipFeatureLevel); + store = eg.store(account: selfAccount, + initialSnapshot: eg.initialSnapshot(zulipFeatureLevel: zulipFeatureLevel)); await store.addStream(stream); await store.addSubscription(subscription); connection = store.connection as FakeApiConnection; @@ -54,8 +68,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]. @@ -76,6 +94,334 @@ 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)); + + await store.addOutboxMessage( + StreamDestination(stream.streamId, eg.t('topic'))); + check(async.pendingTimers).deepEquals(>[ + (it) => it.isA().duration.equals(kLocalEchoDebounceDuration), + (it) => it.isA().duration.equals(kSendMessageOfferRestoreWaitPeriod), + ]); + + store.dispose(); + check(async.pendingTimers).isEmpty(); + })); + + group('sendMessage', () { + final stream = eg.stream(); + final streamDestination = StreamDestination(stream.streamId, eg.t('some topic')); + late StreamMessage message; + + test('outbox messages get unique localMessageId', () async { + await prepare(stream: stream); + await prepareMessages([]); + + 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); + }); + + late OutboxMessage outboxMessage; + + Future prepareSendMessageToSucceed({ + MessageDestination? destination, + int? zulipFeatureLevel, + }) async { + message = eg.streamMessage(stream: stream); + await prepare(stream: stream, zulipFeatureLevel: zulipFeatureLevel); + await prepareMessages([eg.streamMessage(stream: stream)]); + connection.prepare(json: SendMessageResult(id: 1).toJson()); + await store.sendMessage( + destination: destination ?? streamDestination, content: 'content'); + outboxMessage = store.outboxMessages.values.single; + check(outboxMessage).state.equals(OutboxMessageState.hidden); + } + + late Future failToSendMessageFuture; + Future prepareSendMessageToFail({ + Duration delay = Duration.zero, + }) async { + message = eg.streamMessage(stream: stream); + await prepare(stream: stream); + await prepareMessages([eg.streamMessage(stream: stream)]); + connection.prepare(apiException: eg.apiBadRequest(), delay: delay); + failToSendMessageFuture = store.sendMessage( + destination: streamDestination, content: 'content'); + outboxMessage = store.outboxMessages.values.single; + check(outboxMessage).state.equals(OutboxMessageState.hidden); + } + + test('smoke DM: hidden -> waiting -> waitPeriodExpired -> (delete)', () => awaitFakeAsync((async) async { + await prepareSendMessageToSucceed(destination: DmDestination( + userIds: [eg.selfUser.userId, eg.otherUser.userId])); + check(outboxMessage).state.equals(OutboxMessageState.hidden); + checkNotNotified(); + + async.elapse(kLocalEchoDebounceDuration); + check(outboxMessage).state.equals(OutboxMessageState.waiting); + checkNotifiedOnce(); + + async.elapse(kSendMessageOfferRestoreWaitPeriod - kLocalEchoDebounceDuration); + check(outboxMessage).state.equals(OutboxMessageState.waitPeriodExpired); + checkNotifiedOnce(); + + await store.handleEvent(eg.messageEvent( + eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]), + localMessageId: outboxMessage.localMessageId)); + check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); + })); + + test('smoke stream message: hidden -> waiting -> waitPeriodExpired -> (delete)', () => awaitFakeAsync((async) async { + await prepareSendMessageToSucceed(); + check(outboxMessage).state.equals(OutboxMessageState.hidden); + checkNotNotified(); + + async.elapse(kLocalEchoDebounceDuration); + check(outboxMessage).state.equals(OutboxMessageState.waiting); + checkNotifiedOnce(); + + async.elapse(kSendMessageOfferRestoreWaitPeriod - kLocalEchoDebounceDuration); + check(outboxMessage).state.equals(OutboxMessageState.waitPeriodExpired); + checkNotifiedOnce(); + + await store.handleEvent(eg.messageEvent(message, + localMessageId: outboxMessage.localMessageId)); + check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); + })); + + group('… -> failed', () { + test('hidden -> failed', () => awaitFakeAsync((async) async { + await prepareSendMessageToFail(); + await check(failToSendMessageFuture).throws(); + check(outboxMessage).state.equals(OutboxMessageState.failed); + checkNotifiedOnce(); + + // Wait till we reach at least [kSendMessageOfferRestoreWaitPeriod] after + // the send request was initiated. + async.elapse(kSendMessageOfferRestoreWaitPeriod); + async.flushTimers(); + // The outbox message should stay in the failed state; + // it should not transition to waiting or waitPeriodExpired. + check(outboxMessage).state.equals(OutboxMessageState.failed); + checkNotNotified(); + })); + + test('waiting -> failed', () => awaitFakeAsync((async) async { + await prepareSendMessageToFail( + delay: kLocalEchoDebounceDuration + Duration(seconds: 1)); + async.elapse(kLocalEchoDebounceDuration); + check(outboxMessage).state.equals(OutboxMessageState.waiting); + checkNotifiedOnce(); + + await check(failToSendMessageFuture).throws(); + async.elapse(Duration(seconds: 1)); + check(outboxMessage).state.equals(OutboxMessageState.failed); + checkNotifiedOnce(); + })); + + test('waitPeriodExpired -> failed', () => awaitFakeAsync((async) async { + await prepareSendMessageToFail( + delay: kSendMessageOfferRestoreWaitPeriod + Duration(seconds: 1)); + async.elapse(kSendMessageOfferRestoreWaitPeriod); + check(outboxMessage).state.equals(OutboxMessageState.waitPeriodExpired); + checkNotified(count: 2); + + await check(failToSendMessageFuture).throws(); + async.elapse(Duration(seconds: 1)); + check(outboxMessage).state.equals(OutboxMessageState.failed); + checkNotifiedOnce(); + })); + }); + + group('… -> (delete)', () { + test('hidden -> (delete) because event received', () => awaitFakeAsync((async) async { + await prepareSendMessageToSucceed(); + check(outboxMessage).state.equals(OutboxMessageState.hidden); + checkNotNotified(); + + await store.handleEvent(eg.messageEvent(message, + localMessageId: outboxMessage.localMessageId)); + check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); + })); + + test('hidden -> (delete) when event arrives before send request 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); + checkNotNotified(); + + // Handle the message event while the message is being sent. + await store.handleEvent(eg.messageEvent(message, + localMessageId: outboxMessage.localMessageId)); + check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); + + // Complete the send request. There should be no error despite + // the send request failure, because the outbox message is not + // in the store any more. + await check(failToSendMessageFuture).completes(); + async.elapse(const Duration(seconds: 1)); + checkNotNotified(); + })); + + test('waiting -> (delete) because event received', () => awaitFakeAsync((async) async { + await prepareSendMessageToSucceed(); + async.elapse(kLocalEchoDebounceDuration); + check(outboxMessage).state.equals(OutboxMessageState.waiting); + checkNotifiedOnce(); + + await store.handleEvent(eg.messageEvent(message, + localMessageId: outboxMessage.localMessageId)); + check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); + })); + + test('waiting -> (delete) when event arrives before send request 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: kLocalEchoDebounceDuration + Duration(seconds: 1)); + async.elapse(kLocalEchoDebounceDuration); + check(outboxMessage).state.equals(OutboxMessageState.waiting); + checkNotifiedOnce(); + + // Handle the message event while the message is being sent. + await store.handleEvent(eg.messageEvent(message, + localMessageId: outboxMessage.localMessageId)); + check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); + + // Complete the send request. There should be no error despite + // the send request failure, because the outbox message is not + // in the store any more. + await check(failToSendMessageFuture).completes(); + checkNotNotified(); + })); + + test('waitPeriodExpired -> (delete) because event received', () => awaitFakeAsync((async) async { + await prepareSendMessageToSucceed(); + async.elapse(kSendMessageOfferRestoreWaitPeriod); + check(outboxMessage).state.equals(OutboxMessageState.waitPeriodExpired); + checkNotified(count: 2); + + await store.handleEvent(eg.messageEvent(message, + localMessageId: outboxMessage.localMessageId)); + check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); + })); + + test('waitPeriodExpired -> (delete) when event arrives before send request 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: kSendMessageOfferRestoreWaitPeriod + Duration(seconds: 1)); + async.elapse(kSendMessageOfferRestoreWaitPeriod); + check(outboxMessage).state.equals(OutboxMessageState.waitPeriodExpired); + checkNotified(count: 2); + + // Handle the message event while the message is being sent. + await store.handleEvent(eg.messageEvent(message, + localMessageId: outboxMessage.localMessageId)); + check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); + + // Complete the send request. There should be no error despite + // the send request failure, because the outbox message is not + // in the store any more. + await check(failToSendMessageFuture).completes(); + checkNotNotified(); + })); + + test('waitPeriodExpired -> (delete) because outbox message was taken', () => awaitFakeAsync((async) async { + await prepareSendMessageToSucceed(); + async.elapse(kSendMessageOfferRestoreWaitPeriod); + check(outboxMessage).state.equals(OutboxMessageState.waitPeriodExpired); + checkNotified(count: 2); + + store.takeOutboxMessage(outboxMessage.localMessageId); + check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); + })); + + test('failed -> (delete) because event received', () => awaitFakeAsync((async) async { + await prepareSendMessageToFail(); + await check(failToSendMessageFuture).throws(); + check(outboxMessage).state.equals(OutboxMessageState.failed); + checkNotifiedOnce(); + + await store.handleEvent(eg.messageEvent(message, + localMessageId: outboxMessage.localMessageId)); + check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); + })); + + test('failed -> (delete) because outbox message was taken', () => awaitFakeAsync((async) async { + await prepareSendMessageToFail(); + await check(failToSendMessageFuture).throws(); + check(outboxMessage).state.equals(OutboxMessageState.failed); + checkNotifiedOnce(); + + store.takeOutboxMessage(outboxMessage.localMessageId); + check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); + })); + }); + + test('when sending to "(no topic)", process topic like the server does when creating outbox message', () => awaitFakeAsync((async) async { + await prepareSendMessageToSucceed( + destination: StreamDestination(stream.streamId, TopicName('(no topic)')), + zulipFeatureLevel: 370); + async.elapse(kLocalEchoDebounceDuration); + check(outboxMessage).conversation.isA() + .topic.equals(eg.t('')); + })); + + test('legacy: when sending to "(no topic)", process topic like the server does when creating outbox message', () => awaitFakeAsync((async) async { + await prepareSendMessageToSucceed( + destination: StreamDestination(stream.streamId, TopicName('(no topic)')), + zulipFeatureLevel: 369); + 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('takeOutboxMessage', () async { + final stream = eg.stream(); + await prepare(stream: stream); + await prepareMessages([]); + + for (int i = 0; i < 10; i++) { + connection.prepare(apiException: eg.apiBadRequest()); + await check(store.sendMessage( + destination: StreamDestination(stream.streamId, eg.t('topic')), + content: 'content')).throws(); + checkNotifiedOnce(); + } + + final localMessageIds = store.outboxMessages.keys.toList(); + store.takeOutboxMessage(localMessageIds.removeAt(5)); + check(store.outboxMessages.keys).deepEquals(localMessageIds); + checkNotifiedOnce(); + }); + group('reconcileMessages', () { test('from empty', () async { await prepare(); diff --git a/test/model/narrow_test.dart b/test/model/narrow_test.dart index 06c82ed117..dff0a6e179 100644 --- a/test/model/narrow_test.dart +++ b/test/model/narrow_test.dart @@ -7,32 +7,6 @@ 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() { group('SendableNarrow', () { test('ofMessage: stream message', () { @@ -61,11 +35,11 @@ void main() { eg.streamMessage(stream: stream, topic: 'topic'))).isTrue(); check(narrow.containsMessage( - _TestDmMessage(allRecipientIds: [1]))).isFalse(); + eg.dmOutboxMessage(from: eg.selfUser, to: [eg.otherUser]))).isFalse(); check(narrow.containsMessage( - _TestStreamMessage(stream: otherStream, topic: 'topic'))).isFalse(); + eg.streamOutboxMessage(stream: otherStream, topic: 'topic'))).isFalse(); check(narrow.containsMessage( - _TestStreamMessage(stream: stream, topic: 'topic'))).isTrue(); + eg.streamOutboxMessage(stream: stream, topic: 'topic'))).isTrue(); }); }); @@ -91,13 +65,13 @@ void main() { eg.streamMessage(stream: stream, topic: 'topic'))).isTrue(); check(narrow.containsMessage( - _TestDmMessage(allRecipientIds: [1]))).isFalse(); + eg.dmOutboxMessage(from: eg.selfUser, to: [eg.otherUser]))).isFalse(); check(narrow.containsMessage( - _TestStreamMessage(stream: otherStream, topic: 'topic'))).isFalse(); + eg.streamOutboxMessage(stream: otherStream, topic: 'topic'))).isFalse(); check(narrow.containsMessage( - _TestStreamMessage(stream: stream, topic: 'topic2'))).isFalse(); + eg.streamOutboxMessage(stream: stream, topic: 'topic2'))).isFalse(); check(narrow.containsMessage( - _TestStreamMessage(stream: stream, topic: 'topic'))).isTrue(); + eg.streamOutboxMessage(stream: stream, topic: 'topic'))).isTrue(); }); }); @@ -220,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( - _TestStreamMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); + eg.streamOutboxMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); check(narrow.containsMessage( - _TestDmMessage(allRecipientIds: [2]))).isFalse(); + eg.dmOutboxMessage(from: user2, to: []))).isFalse(); check(narrow.containsMessage( - _TestDmMessage(allRecipientIds: [2, 3]))).isFalse(); + eg.dmOutboxMessage(from: user2, to: [user3]))).isFalse(); check(narrow.containsMessage( - _TestDmMessage(allRecipientIds: [1, 2]))).isTrue(); + eg.dmOutboxMessage(from: user1, to: [user2]))).isTrue(); }); }); @@ -245,9 +222,9 @@ void main() { eg.streamMessage(flags: [MessageFlag.wildcardMentioned]))).isTrue(); check(narrow.containsMessage( - _TestStreamMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); + eg.streamOutboxMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); check(narrow.containsMessage( - _TestDmMessage(allRecipientIds: [eg.selfUser.userId]))).isFalse(); + eg.dmOutboxMessage(from: eg.selfUser, to: []))).isFalse(); }); }); @@ -261,9 +238,9 @@ void main() { eg.streamMessage(flags:[MessageFlag.starred]))).isTrue(); check(narrow.containsMessage( - _TestStreamMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); + eg.streamOutboxMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); check(narrow.containsMessage( - _TestDmMessage(allRecipientIds: [eg.selfUser.userId]))).isFalse(); + eg.dmOutboxMessage(from: eg.selfUser, to: []))).isFalse(); }); }); } diff --git a/test/model/store_test.dart b/test/model/store_test.dart index ef0a7a72be..d5eb4c739f 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/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); + } + } } diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart index 679f4de190..11467cea7d 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -15,6 +15,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'; @@ -295,6 +296,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]); @@ -332,6 +335,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]); @@ -723,6 +728,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); @@ -829,6 +836,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'), @@ -883,6 +892,8 @@ void main() { }) async { TypingNotifier.debugEnable = false; addTearDown(TypingNotifier.debugReset); + MessageStoreImpl.debugOutboxEnable = false; + addTearDown(MessageStoreImpl.debugReset); channel = eg.stream(); final narrow = ChannelNarrow(channel.streamId); @@ -1419,6 +1430,8 @@ void main() { int msgIdInNarrow(Narrow narrow) => msgInNarrow(narrow).id; Future prepareEditMessage(WidgetTester tester, {required Narrow narrow}) async { + MessageStoreImpl.debugOutboxEnable = false; + addTearDown(MessageStoreImpl.debugReset); await prepareComposeBox(tester, narrow: narrow, streams: [channel]); diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 69a7204827..46f9b70f8f 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -11,9 +11,11 @@ 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'; +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'; @@ -905,7 +907,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') @@ -914,8 +917,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 { @@ -1412,6 +1419,214 @@ void main() { }); }); + group('OutboxMessageWithPossibleSender', () { + 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); + + Finder outboxMessageFinder = find.descendant( + of: find.byType(MessageItem), + matching: find.text(content, findRichText: true)).hitTestable(); + + Finder messageIsntSentErrorFinder = find.text( + 'MESSAGE ISN\'T SENT. CHECK YOUR CONNECTION.').hitTestable(); + + 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('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: []); + 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(kSendMessageOfferRestoreWaitPeriod); + 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', () { testWidgets('unstarred message', (tester) async { final message = eg.streamMessage(flags: []);