msglist: Fix the first and third buggy behaviors in #1798!

lib/model/message.dart

@@ -519,7 +519,8 @@ class MessageStoreImpl extends HasChannelStore with MessageStore, _OutboxMessage
         content: newContent,
         prevContentSha256: sha256.convert(utf8.encode(originalRawContent)).toString());
       // On success, we'll clear the status from _editMessageRequests
-      // when we get the event.
+      // when we get the event (or below, if in an unsubscribed channel).
+      if (_disposed) return;
     } catch (e) {
       // TODO(log) if e is something unexpected
 
@@ -536,6 +537,15 @@ class MessageStoreImpl extends HasChannelStore with MessageStore, _OutboxMessage
       _notifyMessageListViewsForOneMessage(messageId);
       rethrow;
     }
+
+    final message = messages[messageId];
+    if (message is StreamMessage && subscriptions[message.streamId] == null) {
+      // The message is in an unsubscribed channel. We don't expect an event
+      // (see "third buggy behavior" in #1798) but we know the edit request
+      // succeeded, so, clear the pending-edit state.
+      _editMessageRequests.remove(messageId);
+      _notifyMessageListViewsForOneMessage(messageId);
+    }
   }
 
   @override
@@ -889,13 +899,18 @@ const kSendMessageOfferRestoreWaitPeriod = Duration(seconds: 10);  // TODO(#1441
 ///         timed out.   not finished when
 ///                      wait period timed out.
 ///
-///              Event received.
-/// (any state) ─────────────────► (delete)
+///              Event received, or [sendMessage]
+///              request succeeds and we're sending to
+///              an unsubscribed channel.
+/// (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.
+/// If we're sending to an unsubscribed channel, we don't expect an event
+/// (see "third buggy behavior" in #1798) so in that case
+/// the outbox message is deleted when the [sendMessage] request succeeds.
 enum OutboxMessageState {
   /// The [sendMessage] HTTP request has started but the resulting
   /// [MessageEvent] hasn't arrived, and nor has the request failed.  In this
@@ -1148,6 +1163,19 @@ mixin _OutboxMessageStore on HasChannelStore {
       // The message event already arrived; nothing to do.
       return;
     }
+
+    if (destination is StreamDestination && subscriptions[destination.streamId] == null) {
+      // We don't expect an event (we're sending to an unsubscribed channel);
+      // clear the loading spinner.
+      _outboxMessages.remove(localMessageId);
+      _outboxMessageDebounceTimers.remove(localMessageId)?.cancel();
+      _outboxMessageWaitPeriodTimers.remove(localMessageId)?.cancel();
+      for (final view in _messageListViews) {
+        view.notifyListenersIfOutboxMessagePresent(localMessageId);
+      }
+      return;
+    }
+
     // The send request succeeded, so the message was definitely sent.
     // Cancel the timer that would have had us start presuming that the
     // send might have failed.

lib/widgets/compose_box.dart

@@ -1324,13 +1324,14 @@ class _SendButtonState extends State<_SendButton> {
       return;
     }
 
-    final store = PerAccountStoreWidget.of(context);
+    PerAccountStore store = PerAccountStoreWidget.of(context);
     final content = controller.content.textNormalized;
 
     controller.content.clear();
 
     try {
       await store.sendMessage(destination: widget.getDestination(), content: content);
+      if (!mounted) return;
     } on ApiRequestException catch (e) {
       if (!mounted) return;
       final zulipLocalizations = ZulipLocalizations.of(context);
@@ -1343,6 +1344,20 @@ class _SendButtonState extends State<_SendButton> {
         message: message);
       return;
     }
+
+    store = PerAccountStoreWidget.of(context);
+    final destination = widget.getDestination();
+    if (
+      destination is StreamDestination
+      && store.subscriptions[destination.streamId] == null
+    ) {
+      // We don't get new-message events for unsubscribed channels,
+      // but we can refresh the view when a send-message request succeeds,
+      // so the user will at least see their own messages without having to
+      // exit and re-enter. See the "first buggy behavior" in
+      //   https://github.com/zulip/zulip-flutter/issues/1798 .
+      MessageListPage.ancestorOf(context).refresh(AnchorCode.newest);
+    }
   }
 
   @override
@@ -1854,7 +1869,7 @@ class _EditMessageBannerTrailing extends StatelessWidget {
     // disappears, which may be long after the banner disappears.)
     final pageContext = PageRoot.contextOf(context);
 
-    final store = PerAccountStoreWidget.of(pageContext);
+    PerAccountStore store = PerAccountStoreWidget.of(pageContext);
     final controller = composeBoxState.controller;
     if (controller is! EditMessageComposeBoxController) return; // TODO(log)
     final zulipLocalizations = ZulipLocalizations.of(pageContext);
@@ -1885,6 +1900,7 @@ class _EditMessageBannerTrailing extends StatelessWidget {
         messageId: messageId,
         originalRawContent: originalRawContent,
         newContent: newContent);
+      if (!pageContext.mounted) return;
     } on ApiRequestException catch (e) {
       if (!pageContext.mounted) return;
       final zulipLocalizations = ZulipLocalizations.of(pageContext);
@@ -1897,6 +1913,18 @@ class _EditMessageBannerTrailing extends StatelessWidget {
         message: message);
       return;
     }
+
+    store = PerAccountStoreWidget.of(pageContext);
+    final messageListPageState = MessageListPage.ancestorOf(pageContext);
+    final narrow = messageListPageState.narrow;
+    if (narrow is ChannelNarrow && store.subscriptions[narrow.streamId] == null) {
+      // We don't get edit-message events for unsubscribed channels,
+      // but we can refresh the view when an edit-message request succeeds,
+      // so the user will at least see their updated message without having to
+      // exit and re-enter. See the "first buggy behavior" in
+      //   https://github.com/zulip/zulip-flutter/issues/1798 .
+      messageListPageState.refresh(NumericAnchor(messageId));
+    }
   }
 
   @override

test/model/message_test.dart

@@ -53,6 +53,7 @@ void main() {
 
   /// Initialize [store] and the rest of the test state.
   Future<void> prepare({
+    Narrow narrow = const CombinedFeedNarrow(),
     ZulipStream? stream,
     bool isChannelSubscribed = true,
     int? zulipFeatureLevel,
@@ -71,7 +72,7 @@ void main() {
     connection = store.connection as FakeApiConnection;
     notifiedCount = 0;
     messageList = MessageListView.init(store: store,
-        narrow: const CombinedFeedNarrow(),
+        narrow: narrow,
         anchor: AnchorCode.newest)
       ..addListener(() {
         notifiedCount++;
@@ -172,11 +173,17 @@ void main() {
       check(store.outboxMessages).values.single.state;
 
     Future<void> prepareOutboxMessage({
+      Narrow narrow = const CombinedFeedNarrow(),
       MessageDestination? destination,
+      bool isChannelSubscribed = true,
       int? zulipFeatureLevel,
     }) async {
       message = eg.streamMessage(stream: stream);
-      await prepare(stream: stream, zulipFeatureLevel: zulipFeatureLevel);
+      await prepare(
+        narrow: narrow,
+        stream: stream,
+        isChannelSubscribed: isChannelSubscribed,
+        zulipFeatureLevel: zulipFeatureLevel);
       await prepareMessages([eg.streamMessage(stream: stream)]);
       connection.prepare(json: SendMessageResult(id: 1).toJson());
       await store.sendMessage(
@@ -368,6 +375,16 @@ void main() {
         checkNotNotified();
       }));
 
+      test('hidden -> (delete) because send succeeded in unsubscribed channel', () => awaitFakeAsync((async) async {
+        await prepareOutboxMessage(
+          // Not CombinedFeedNarrow,
+          // which always hides outbox messages in unsubscribed channels.
+          narrow: ChannelNarrow(stream.streamId),
+          isChannelSubscribed: false);
+        check(store.outboxMessages).isEmpty();
+        checkNotNotified();
+      }));
+
       test('waiting -> (delete) because event received', () => awaitFakeAsync((async) async {
         await prepareOutboxMessage();
         async.elapse(kLocalEchoDebounceDuration);
@@ -400,6 +417,28 @@ void main() {
         checkNotNotified();
       }));
 
+      test('waiting -> (delete) because send succeeded in unsubscribed channel', () => awaitFakeAsync((async) async {
+        await prepare(
+          // Not CombinedFeedNarrow,
+          // which always hides outbox messages in unsubscribed channels.
+          narrow: ChannelNarrow(stream.streamId),
+          stream: stream,
+          isChannelSubscribed: false);
+        await prepareMessages([eg.streamMessage(stream: stream)]);
+        connection.prepare(json: SendMessageResult(id: 1).toJson(),
+          delay: kLocalEchoDebounceDuration + Duration(seconds: 1));
+        final future = store.sendMessage(
+          destination: streamDestination, content: 'content');
+        async.elapse(kLocalEchoDebounceDuration);
+        checkState().equals(OutboxMessageState.waiting);
+        checkNotifiedOnce();
+
+        async.elapse(Duration(seconds: 1));
+        await check(future).completes();
+        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.
@@ -435,6 +474,27 @@ void main() {
         checkNotifiedOnce();
       }));
 
+      test('waitPeriodExpired -> (delete) because send succeeded in unsubscribed channel', () => awaitFakeAsync((async) async {
+        await prepare(
+          // Not CombinedFeedNarrow,
+          // which always hides outbox messages in unsubscribed channels.
+          narrow: ChannelNarrow(stream.streamId),
+          isChannelSubscribed: false);
+        await prepareMessages([eg.streamMessage(stream: stream)]);
+        connection.prepare(json: SendMessageResult(id: 1).toJson(),
+          delay: kSendMessageOfferRestoreWaitPeriod + Duration(seconds: 1));
+        final future = store.sendMessage(
+          destination: streamDestination, content: 'content');
+        async.elapse(kSendMessageOfferRestoreWaitPeriod);
+        checkState().equals(OutboxMessageState.waitPeriodExpired);
+        checkNotified(count: 2);
+
+        async.elapse(Duration(seconds: 1));
+        await check(future).completes();
+        check(store.outboxMessages).isEmpty();
+        checkNotifiedOnce();
+      }));
+
       test('failed -> (delete) because event received', () => awaitFakeAsync((async) async {
         await prepareOutboxMessageToFailAfterDelay(Duration.zero);
         await check(outboxMessageFailFuture).throws();
@@ -993,6 +1053,30 @@ void main() {
       check(store.getEditMessageErrorStatus(message.id)).isNull();
       checkNotNotified();
     }));
+
+    test('request succeeds, in unsubscribed channel', () => awaitFakeAsync((async) async {
+      final channel = eg.stream();
+      message = eg.streamMessage(stream: channel, sender: eg.selfUser);
+      await prepare(
+        narrow: ChannelNarrow(channel.streamId),
+        stream: channel,
+        isChannelSubscribed: false,
+      );
+      await prepareMessages([message]);
+      check(connection.takeRequests()).length.equals(1); // message-list fetchInitial
+
+      connection.prepare(
+        json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1));
+      unawaited(store.editMessage(messageId: message.id,
+        originalRawContent: 'old content', newContent: 'new content'));
+      checkRequest(message.id, prevContent: 'old content', content: 'new content');
+      checkNotifiedOnce();
+
+      async.elapse(Duration(seconds: 1));
+      // Outbox status cleared already (we don't expect an edit-message event).
+      check(store.getEditMessageErrorStatus(message.id)).isNull();
+      checkNotifiedOnce();
+    }));
   });
 
   group('selfCanDeleteMessage', () {