From 542996e5da4fe69d758eb8666d8e4efe492326ab Mon Sep 17 00:00:00 2001 From: Sayed Mahmood Sayedi Date: Sat, 8 Nov 2025 08:50:35 +0430 Subject: [PATCH 1/5] msglist [nfc]: Mention fetchNewer in the MessageListView dartdoc --- lib/model/message_list.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 613b1a712e..7aadad6225 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -593,7 +593,7 @@ bool _sameDay(DateTime date1, DateTime date2) { /// * Add listeners with [addListener]. /// * Fetch messages with [fetchInitial]. When the fetch completes, this object /// will notify its listeners (as it will any other time the data changes.) -/// * Fetch more messages as needed with [fetchOlder]. +/// * Fetch more messages as needed with [fetchOlder] and [fetchNewer]. /// * On reassemble, call [reassemble]. /// * When the object will no longer be used, call [dispose] to free /// resources on the [PerAccountStore]. From ce56456364f92bacfa6c47c60e9132e5b037661e Mon Sep 17 00:00:00 2001 From: Sayed Mahmood Sayedi Date: Sat, 8 Nov 2025 17:52:00 +0430 Subject: [PATCH 2/5] msglist [nfc]: Add a new point to _MessageSequence.messages dartdoc --- lib/model/message_list.dart | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 7aadad6225..d5720e4065 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -127,6 +127,10 @@ mixin _MessageSequence { /// conceptually belongs in this message list. /// That information is expressed in [fetched], [haveOldest], [haveNewest]. /// + /// This also may or may not represent all the message history that + /// conceptually belongs in this narrow because some messages might be + /// muted in one way or another and they may not appear in the message list. + /// /// See also [middleMessage], an index which divides this list /// into a top slice and a bottom slice. /// From 3c10bedb18438dc7bd106e49d1d36adacc5fb51c Mon Sep 17 00:00:00 2001 From: Sayed Mahmood Sayedi Date: Sat, 15 Nov 2025 10:56:05 +0430 Subject: [PATCH 3/5] msglist [nfc]: Remove one nested `try` block in `_fetchMore` The nested `try` block doesn't seem to be making any difference, so good to remove. --- lib/model/message_list.dart | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index d5720e4065..a4cb6927e7 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -937,23 +937,20 @@ class MessageListView with ChangeNotifier, _MessageSequence { final generation = this.generation; bool hasFetchError = false; try { - final GetMessagesResult result; - try { - result = await getMessages(store.connection, - narrow: narrow.apiEncode(), - anchor: anchor, - includeAnchor: false, - numBefore: numBefore, - numAfter: numAfter, - allowEmptyTopicName: true, - ); - } catch (e) { - hasFetchError = true; - rethrow; - } + final result = await getMessages(store.connection, + narrow: narrow.apiEncode(), + anchor: anchor, + includeAnchor: false, + numBefore: numBefore, + numAfter: numAfter, + allowEmptyTopicName: true, + ); if (this.generation > generation) return; processResult(result); + } catch (e) { + hasFetchError = true; + rethrow; } finally { if (this.generation == generation) { if (hasFetchError) { From 03d61ca5461888f20e42e2d3e44ab06385b2b942 Mon Sep 17 00:00:00 2001 From: Sayed Mahmood Sayedi Date: Thu, 13 Nov 2025 18:04:50 +0430 Subject: [PATCH 4/5] msglist: Fetch newer messages despite previous muted batch --- lib/model/message_list.dart | 120 +++++++---- lib/widgets/message_list.dart | 8 +- test/model/message_list_test.dart | 337 ++++++++++++++++++++++++++++-- 3 files changed, 400 insertions(+), 65 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index a4cb6927e7..6c50d13513 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -145,6 +145,26 @@ mixin _MessageSequence { /// The corresponding item index is [middleItem]. int middleMessage = 0; + /// The ID of the oldest known message so far in this narrow. + /// + /// This will be `null` if no messages of this narrow are fetched yet. + /// Having a non-null value for this doesn't always mean [haveOldest] is `true`. + /// + /// The related message may not appear in [messages] because it + /// is muted in one way or another. + int? get oldMessageId => _oldMessageId; + int? _oldMessageId; + + /// The ID of the newest known message so far in this narrow. + /// + /// This will be `null` if no messages of this narrow are fetched yet. + /// Having a non-null value for this doesn't always mean [haveNewest] is `true`. + /// + /// The related message may not appear in [messages] because it + /// is muted in one way or another. + int? get newMessageId => _newMessageId; + int? _newMessageId; + /// Whether [messages] and [items] represent the results of a fetch. /// /// This allows the UI to distinguish "still working on fetching messages" @@ -409,6 +429,8 @@ mixin _MessageSequence { generation += 1; messages.clear(); middleMessage = 0; + _oldMessageId = null; + _newMessageId = null; outboxMessages.clear(); _haveOldest = false; _haveNewest = false; @@ -775,6 +797,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { Future fetchInitial() async { assert(!fetched && !haveOldest && !haveNewest && !busyFetchingMore); assert(messages.isEmpty && contents.isEmpty); + assert(oldMessageId == null && newMessageId == null); if (narrow case KeywordSearchNarrow(keyword: '')) { // The server would reject an empty keyword search; skip the request. @@ -798,6 +821,9 @@ class MessageListView with ChangeNotifier, _MessageSequence { ); if (this.generation > generation) return; + _oldMessageId = result.messages.firstOrNull?.id; + _newMessageId = result.messages.lastOrNull?.id; + _adjustNarrowForTopicPermalink(result.messages.firstOrNull); store.reconcileMessages(result.messages); @@ -869,25 +895,32 @@ class MessageListView with ChangeNotifier, _MessageSequence { /// That makes this method suitable to call frequently, e.g. every frame, /// whenever it looks likely to be useful to have more messages. Future fetchOlder() async { - if (haveOldest) return; - if (busyFetchingMore) return; - assert(fetched); - assert(messages.isNotEmpty); - await _fetchMore( - anchor: NumericAnchor(messages[0].id), - numBefore: kMessageListFetchBatchSize, - numAfter: 0, - processResult: (result) { - store.reconcileMessages(result.messages); - store.recentSenders.handleMessages(result.messages); // TODO(#824) - - final fetchedMessages = _allMessagesVisible - ? result.messages // Avoid unnecessarily copying the list. - : result.messages.where(_messageVisible); - - _insertAllMessages(0, fetchedMessages); - _haveOldest = result.foundOldest; - }); + final generation = this.generation; + int visibleMessageCount = 0; + do { + if (haveOldest) return; + if (busyFetchingMore) return; + assert(fetched); + assert(oldMessageId != null); + await _fetchMore( + anchor: NumericAnchor(oldMessageId!), + numBefore: kMessageListFetchBatchSize, + numAfter: 0, + processResult: (result) { + _oldMessageId = result.messages.firstOrNull?.id ?? oldMessageId; + store.reconcileMessages(result.messages); + store.recentSenders.handleMessages(result.messages); // TODO(#824) + + final fetchedMessages = _allMessagesVisible + ? result.messages // Avoid unnecessarily copying the list. + : result.messages.where(_messageVisible); + + _insertAllMessages(0, fetchedMessages); + _haveOldest = result.foundOldest; + visibleMessageCount += fetchedMessages.length; + }); + } while (visibleMessageCount < kMessageListFetchBatchSize / 2 + && this.generation == generation); } /// Fetch the next batch of newer messages, if applicable. @@ -899,29 +932,36 @@ class MessageListView with ChangeNotifier, _MessageSequence { /// That makes this method suitable to call frequently, e.g. every frame, /// whenever it looks likely to be useful to have more messages. Future fetchNewer() async { - if (haveNewest) return; - if (busyFetchingMore) return; - assert(fetched); - assert(messages.isNotEmpty); - await _fetchMore( - anchor: NumericAnchor(messages.last.id), - numBefore: 0, - numAfter: kMessageListFetchBatchSize, - processResult: (result) { - store.reconcileMessages(result.messages); - store.recentSenders.handleMessages(result.messages); // TODO(#824) - - for (final message in result.messages) { - if (_messageVisible(message)) { - _addMessage(message); + final generation = this.generation; + int visibleMessageCount = 0; + do { + if (haveNewest) return; + if (busyFetchingMore) return; + assert(fetched); + assert(newMessageId != null); + await _fetchMore( + anchor: NumericAnchor(newMessageId!), + numBefore: 0, + numAfter: kMessageListFetchBatchSize, + processResult: (result) { + _newMessageId = result.messages.lastOrNull?.id ?? newMessageId; + store.reconcileMessages(result.messages); + store.recentSenders.handleMessages(result.messages); // TODO(#824) + + for (final message in result.messages) { + if (_messageVisible(message)) { + _addMessage(message); + visibleMessageCount++; + } } - } - _haveNewest = result.foundNewest; + _haveNewest = result.foundNewest; - if (haveNewest) { - _syncOutboxMessagesFromStore(); - } - }); + if (haveNewest) { + _syncOutboxMessagesFromStore(); + } + }); + } while (visibleMessageCount < kMessageListFetchBatchSize / 2 + && this.generation == generation); } Future _fetchMore({ diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 0d3038b1da..da171a5ab3 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -838,8 +838,6 @@ class _MessageListState extends State with PerAccountStoreAwareStat model.fetchInitial(); } - bool _prevFetched = false; - void _modelChanged() { // When you're scrolling quickly, our mark-as-read requests include the // messages *between* _messagesRecentlyInViewport and the messages currently @@ -866,14 +864,13 @@ class _MessageListState extends State with PerAccountStoreAwareStat // This method was called because that just changed. }); - if (!_prevFetched && model.fetched && model.messages.isEmpty) { + if (model.messages.isEmpty && model.haveOldest && model.haveNewest) { // If the fetch came up empty, there's nothing to read, // so opening the keyboard won't be bothersome and could be helpful. // It's definitely helpful if we got here from the new-DM page. MessageListPage.ancestorOf(context) .composeBoxState?.controller.requestFocusIfUnfocused(); } - _prevFetched = model.fetched; } /// Find the range of message IDs on screen, as a (first, last) tuple, @@ -1222,7 +1219,8 @@ class _MessageListState extends State with PerAccountStoreAwareStat return Column(crossAxisAlignment: CrossAxisAlignment.stretch, children: [ TypingStatusWidget(narrow: widget.narrow), // TODO perhaps offer mark-as-read even when not done fetching? - MarkAsReadWidget(narrow: widget.narrow), + if (model.messages.isNotEmpty) + MarkAsReadWidget(narrow: widget.narrow), // To reinforce that the end of the feed has been reached: // https://chat.zulip.org/#narrow/channel/48-mobile/topic/space.20at.20end.20of.20thread/near/2203391 const SizedBox(height: 12), diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index c773c2feaf..178b576dd5 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -545,6 +545,165 @@ void main() { ); }); + group('makes multiple requests', () { + final mutedUser = eg.user(); + + List streamMessages(int count, {required int fromId}) => + List.generate(count, (i) => eg.streamMessage(id: fromId + i)); + + List mutedDmMessages(int count, {required int fromId}) => + List.generate(count, (i) => eg.dmMessage(id: fromId + i, from: eg.selfUser, to: [mutedUser])); + + group('until enough visible messages', () { + // Enough visible messages are at least `kMessageListFetchBatchSize / 2`, + // which in the time of writing this (2025-11), they are `100 / 2 = 50`. + + test('fetchOlder', () async { + await prepare(users: [mutedUser], mutedUserIds: [mutedUser.userId]); + await prepareMessages(foundOldest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 1000 + i))); + + connection.prepare(json: olderResult( + anchor: 1000, foundOldest: false, + messages: streamMessages(30, fromId: 900) + mutedDmMessages(70, fromId: 930), + ).toJson()); + final fetchFuture = model.fetchOlder(); + check(model).messages.length.equals(100); + + connection.prepare(json: olderResult( + anchor: 900, foundOldest: false, + messages: streamMessages(10, fromId: 800) + mutedDmMessages(90, fromId: 810), + ).toJson()); + await Future(() {}); + check(model).messages.length.equals(130); + + connection.prepare(json: olderResult( + anchor: 800, foundOldest: false, + messages: streamMessages(9, fromId: 700) + mutedDmMessages(91, fromId: 709), + ).toJson()); + await Future(() {}); + check(model).messages.length.equals(140); + + connection.prepare(json: olderResult( + anchor: 800, foundOldest: false, + messages: streamMessages(1, fromId: 600) + mutedDmMessages(99, fromId: 601), + ).toJson()); + await Future(() {}); + check(model).messages.length.equals(149); + + await fetchFuture; + check(model).messages.length.equals(150); + }); + + test('fetchNewer', () async { + await prepare(anchor: AnchorCode.firstUnread, + users: [mutedUser], mutedUserIds: [mutedUser.userId]); + await prepareMessages( + foundOldest: true, foundNewest: false, + anchorMessageId: 950, + messages: List.generate(100, (i) => eg.streamMessage(id: 900 + i))); + + connection.prepare(json: newerResult( + anchor: 1000, foundNewest: false, + messages: streamMessages(30, fromId: 1000) + mutedDmMessages(70, fromId: 1030), + ).toJson()); + final fetchFuture = model.fetchNewer(); + check(model).messages.length.equals(100); + + connection.prepare(json: newerResult( + anchor: 900, foundNewest: false, + messages: streamMessages(10, fromId: 1100) + mutedDmMessages(90, fromId: 1110), + ).toJson()); + await Future(() {}); + check(model).messages.length.equals(130); + + connection.prepare(json: newerResult( + anchor: 800, foundNewest: false, + messages: streamMessages(9, fromId: 1200) + mutedDmMessages(91, fromId: 1209), + ).toJson()); + await Future(() {}); + check(model).messages.length.equals(140); + + connection.prepare(json: newerResult( + anchor: 800, foundNewest: false, + messages: streamMessages(1, fromId: 1300) + mutedDmMessages(99, fromId: 1301), + ).toJson()); + await Future(() {}); + check(model).messages.length.equals(149); + + await fetchFuture; + check(model).messages.length.equals(150); + }); + }); + + group('until haveOldest/haveNewest', () { + test('fetchOlder', () async { + await prepare(users: [mutedUser], mutedUserIds: [mutedUser.userId]); + await prepareMessages(foundOldest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 1000 + i))); + + connection.prepare(json: olderResult( + anchor: 1000, foundOldest: false, + messages: streamMessages(30, fromId: 900) + mutedDmMessages(70, fromId: 930), + ).toJson()); + final fetchFuture = model.fetchOlder(); + check(model).messages.length.equals(100); + + connection.prepare(json: olderResult( + anchor: 900, foundOldest: false, + messages: streamMessages(10, fromId: 800) + mutedDmMessages(90, fromId: 810), + ).toJson()); + await Future(() {}); + check(model).messages.length.equals(130); + + connection.prepare(json: olderResult( + anchor: 800, foundOldest: true, + messages: streamMessages(9, fromId: 700) + mutedDmMessages(91, fromId: 709), + ).toJson()); + await Future(() {}); + check(model).messages.length.equals(140); + + await fetchFuture; + check(model).haveOldest.isTrue(); + check(model).messages.length.equals(149); + }); + + test('fetchNewer', () async { + await prepare(anchor: AnchorCode.firstUnread, + users: [mutedUser], mutedUserIds: [mutedUser.userId]); + await prepareMessages( + foundOldest: true, foundNewest: false, + anchorMessageId: 950, + messages: List.generate(100, (i) => eg.streamMessage(id: 900 + i))); + + connection.prepare(json: newerResult( + anchor: 1000, foundNewest: false, + messages: streamMessages(30, fromId: 1000) + mutedDmMessages(70, fromId: 1030), + ).toJson()); + final fetchFuture = model.fetchNewer(); + check(model).messages.length.equals(100); + + connection.prepare(json: newerResult( + anchor: 900, foundNewest: false, + messages: streamMessages(10, fromId: 1100) + mutedDmMessages(90, fromId: 1110), + ).toJson()); + await Future(() {}); + check(model).messages.length.equals(130); + + connection.prepare(json: newerResult( + anchor: 800, foundNewest: true, + messages: streamMessages(9, fromId: 1200) + mutedDmMessages(91, fromId: 1209), + ).toJson()); + await Future(() {}); + check(model).messages.length.equals(140); + + await fetchFuture; + check(model).haveNewest.isTrue(); + check(model).messages.length.equals(149); + }); + }); + }); + test('nop when already fetching older', () async { await prepare(anchor: NumericAnchor(1000)); await prepareMessages(foundOldest: false, foundNewest: false, @@ -640,10 +799,10 @@ void main() { }); test('nop during backoff', () => awaitFakeAsync((async) async { - final olderMessages = List.generate(5, (i) => eg.streamMessage()); - final initialMessages = List.generate(5, (i) => eg.streamMessage()); - final newerMessages = List.generate(5, (i) => eg.streamMessage()); - await prepare(anchor: NumericAnchor(initialMessages[2].id)); + final olderMessages = List.generate(50, (i) => eg.streamMessage()); + final initialMessages = List.generate(50, (i) => eg.streamMessage()); + final newerMessages = List.generate(50, (i) => eg.streamMessage()); + await prepare(anchor: NumericAnchor(initialMessages[25].id)); await prepareMessages(foundOldest: false, foundNewest: false, messages: initialMessages); check(connection.takeRequests()).single; @@ -687,10 +846,10 @@ void main() { // TODO(#824): move this test test('fetchOlder recent senders track all the messages', () async { await prepare(); - final initialMessages = List.generate(10, (i) => eg.streamMessage(id: 100 + i)); + final initialMessages = List.generate(50, (i) => eg.streamMessage(id: 100 + i)); await prepareMessages(foundOldest: false, messages: initialMessages); - final oldMessages = List.generate(10, (i) => eg.streamMessage(id: 89 + i)) + final oldMessages = List.generate(50, (i) => eg.streamMessage(id: 49 + i)) // Not subscribed to the stream with id 10. ..add(eg.streamMessage(id: 99, stream: eg.stream(streamId: 10))); connection.prepare(json: olderResult( @@ -699,7 +858,7 @@ void main() { ).toJson()); await model.fetchOlder(); - check(model).messages.length.equals(20); + check(model).messages.length.equals(100); recent_senders_test.checkMatchesMessages(store.recentSenders, [...initialMessages, ...oldMessages]); }); @@ -707,25 +866,151 @@ void main() { // TODO(#824): move this test test('TODO fetchNewer recent senders track all the messages', () async { await prepare(anchor: NumericAnchor(100)); - final initialMessages = List.generate(10, (i) => eg.streamMessage(id: 100 + i)); + final initialMessages = List.generate(50, (i) => eg.streamMessage(id: 100 + i)); await prepareMessages(foundOldest: true, foundNewest: false, messages: initialMessages); - final newMessages = List.generate(10, (i) => eg.streamMessage(id: 110 + i)) + final newMessages = List.generate(50, (i) => eg.streamMessage(id: 150 + i)) // Not subscribed to the stream with id 10. - ..add(eg.streamMessage(id: 120, stream: eg.stream(streamId: 10))); + ..add(eg.streamMessage(id: 200, stream: eg.stream(streamId: 10))); connection.prepare(json: newerResult( anchor: 100, foundNewest: false, messages: newMessages, ).toJson()); await model.fetchNewer(); - check(model).messages.length.equals(20); + check(model).messages.length.equals(100); recent_senders_test.checkMatchesMessages(store.recentSenders, [...initialMessages, ...newMessages]); }); }); + group('oldMessageId, newMessageId', () { + group('fetchInitial', () { + test('visible messages', () async { + await prepare(); + check(model)..oldMessageId.isNull()..newMessageId.isNull(); + + connection.prepare(json: newestResult( + foundOldest: true, + messages: List.generate(100, (i) => eg.streamMessage(id: 100 + i)), + ).toJson()); + await model.fetchInitial(); + + checkNotifiedOnce(); + check(model) + ..messages.length.equals(100) + ..oldMessageId.equals(100)..newMessageId.equals(199); + }); + + test('invisible messages', () async { + final mutedUser = eg.user(); + await prepare(users: [mutedUser], mutedUserIds: [mutedUser.userId]); + check(model)..oldMessageId.isNull()..newMessageId.isNull(); + + connection.prepare(json: newestResult( + foundOldest: true, + messages: List.generate(100, + (i) => eg.dmMessage(id: 100 + i, from: eg.selfUser, to: [mutedUser])), + ).toJson()); + await model.fetchInitial(); + + checkNotifiedOnce(); + check(model) + ..messages.isEmpty() + ..oldMessageId.equals(100)..newMessageId.equals(199); + }); + + test('no messages found', () async { + await prepare(); + check(model)..oldMessageId.isNull()..newMessageId.isNull(); + + connection.prepare(json: newestResult( + foundOldest: true, + messages: [], + ).toJson()); + await model.fetchInitial(); + + checkNotifiedOnce(); + check(model) + ..messages.isEmpty() + ..oldMessageId.isNull()..newMessageId.isNull(); + }); + }); + + group('fetching more', () { + test('visible messages', () async { + await prepare(anchor: AnchorCode.firstUnread); + check(model)..oldMessageId.isNull()..newMessageId.isNull(); + + await prepareMessages( + foundOldest: false, foundNewest: false, + anchorMessageId: 250, + messages: List.generate(100, (i) => eg.streamMessage(id: 200 + i))); + check(model) + ..messages.length.equals(100) + ..oldMessageId.equals(200)..newMessageId.equals(299); + + connection.prepare(json: olderResult( + anchor: 200, foundOldest: true, + messages: List.generate(100, (i) => eg.streamMessage(id: 100 + i)), + ).toJson()); + await model.fetchOlder(); + checkNotified(count: 2); + check(model) + ..messages.length.equals(200) + ..oldMessageId.equals(100)..newMessageId.equals(299); + + connection.prepare(json: newerResult( + anchor: 299, foundNewest: true, + messages: List.generate(100, (i) => eg.streamMessage(id: 300 + i)), + ).toJson()); + await model.fetchNewer(); + checkNotified(count: 2); + check(model) + ..messages.length.equals(300) + ..oldMessageId.equals(100)..newMessageId.equals(399); + }); + + test('invisible messages', () async { + final mutedUser = eg.user(); + await prepare(anchor: AnchorCode.firstUnread, + users: [mutedUser], mutedUserIds: [mutedUser.userId]); + check(model)..oldMessageId.isNull()..newMessageId.isNull(); + + await prepareMessages( + foundOldest: false, foundNewest: false, + anchorMessageId: 250, + messages: List.generate(100, (i) => eg.streamMessage(id: 200 + i))); + check(model) + ..messages.length.equals(100) + ..oldMessageId.equals(200)..newMessageId.equals(299); + + connection.prepare(json: olderResult( + anchor: 200, foundOldest: true, + messages: List.generate(100, + (i) => eg.dmMessage(id: 100 + i, from: eg.selfUser, to: [mutedUser])), + ).toJson()); + await model.fetchOlder(); + checkNotified(count: 2); + check(model) + ..messages.length.equals(100) + ..oldMessageId.equals(100)..newMessageId.equals(299); + + connection.prepare(json: newerResult( + anchor: 299, foundNewest: true, + messages: List.generate(100, + (i) => eg.dmMessage(id: 300 + i, from: eg.selfUser, to: [mutedUser])), + ).toJson()); + await model.fetchNewer(); + checkNotified(count: 2); + check(model) + ..messages.length.equals(100) + ..oldMessageId.equals(100)..newMessageId.equals(399); + }); + }); + }); + // TODO(#1569): test jumpToEnd group('MessageEvent', () { @@ -2822,11 +3107,19 @@ void main() { Message dmMessage(int id) => eg.dmMessage(id: id, from: eg.selfUser, to: [], timestamp: timestamp); + List streamMessages({required int fromId, required int toId}) { + assert(fromId > 0 && fromId <= toId); + return [ + for (int id = fromId; id <= toId; id++) + streamMessage(id) + ]; + } + // First, test fetchInitial, where some headers are needed and others not. await prepare(narrow: CombinedFeedNarrow()); connection.prepare(json: newestResult( foundOldest: false, - messages: [streamMessage(10), streamMessage(11), dmMessage(12)], + messages: [streamMessage(200), streamMessage(201), dmMessage(202)], ).toJson()); await model.fetchInitial(); checkNotifiedOnce(); @@ -2835,7 +3128,7 @@ void main() { connection.prepare(json: olderResult( anchor: model.messages[0].id, foundOldest: false, - messages: [streamMessage(7), streamMessage(8), dmMessage(9)], + messages: [...streamMessages(fromId: 150, toId: 198), dmMessage(199)], ).toJson()); await model.fetchOlder(); checkNotified(count: 2); @@ -2844,17 +3137,17 @@ void main() { connection.prepare(json: olderResult( anchor: model.messages[0].id, foundOldest: false, - messages: [streamMessage(6)], + messages: streamMessages(fromId: 100, toId: 149), ).toJson()); await model.fetchOlder(); checkNotified(count: 2); // Then test MessageEvent, where a new header is needed… - await store.addMessage(streamMessage(13)); + await store.addMessage(streamMessage(203)); checkNotifiedOnce(); // … and where it's not. - await store.addMessage(streamMessage(14)); + await store.addMessage(streamMessage(204)); checkNotifiedOnce(); // Then test UpdateMessageEvent edits, where a header is and remains needed… @@ -2892,7 +3185,7 @@ void main() { connection.prepare(json: olderResult( anchor: model.messages[0].id, foundOldest: true, - messages: [streamMessage(5)], + messages: [streamMessage(99)], ).toJson()); await model.fetchOlder(); checkNotified(count: 2); @@ -2904,16 +3197,16 @@ void main() { final outboxMessageIds = store.outboxMessages.keys.toList(); // Then test removing the first outbox message… await store.handleEvent(eg.messageEvent( - dmMessage(15), localMessageId: outboxMessageIds.first)); + dmMessage(205), localMessageId: outboxMessageIds.first)); checkNotifiedOnce(); // … and handling a new non-outbox message… - await store.handleEvent(eg.messageEvent(streamMessage(16))); + await store.handleEvent(eg.messageEvent(streamMessage(206))); checkNotifiedOnce(); // … and removing the second outbox message. await store.handleEvent(eg.messageEvent( - dmMessage(17), localMessageId: outboxMessageIds.last)); + dmMessage(207), localMessageId: outboxMessageIds.last)); checkNotifiedOnce(); })); @@ -3206,6 +3499,8 @@ void checkInvariants(MessageListView model) { check(model) ..messages.isEmpty() ..outboxMessages.isEmpty() + ..oldMessageId.isNull() + ..newMessageId.isNull() ..haveOldest.isFalse() ..haveNewest.isFalse() ..busyFetchingMore.isFalse(); @@ -3394,6 +3689,8 @@ extension MessageListViewChecks on Subject { Subject> get items => has((x) => x.items, 'items'); Subject get middleItem => has((x) => x.middleItem, 'middleItem'); Subject get fetched => has((x) => x.fetched, 'fetched'); + Subject get oldMessageId => has((x) => x.oldMessageId, 'oldMessageId'); + Subject get newMessageId => has((x) => x.newMessageId, 'newMessageId'); Subject get haveOldest => has((x) => x.haveOldest, 'haveOldest'); Subject get haveNewest => has((x) => x.haveNewest, 'haveNewest'); Subject get busyFetchingMore => has((x) => x.busyFetchingMore, 'busyFetchingMore'); From 127f6906eff9f5a704b87205471fdd352a03d26a Mon Sep 17 00:00:00 2001 From: Sayed Mahmood Sayedi Date: Thu, 13 Nov 2025 17:45:36 +0430 Subject: [PATCH 5/5] msglist: Track three fetch request statuses separately This change is necessary when there is a need to fetch more messages in both directions, older and newer, and when fetching in one direction avoids fetching in another direction at the same time, because of the `if (busyFetchingMore) return` line in both `fetchOlder` and `fetchNewer`. This scenario happens when a conversation is opened in its first unread, such that the number of messages in the initial batch is so low (because they're muted in one way or another) that it's already past the certain point where the scroll metrics listener in the widget code triggers both `fetchOlder` and `fetchNewer`. In 2025-11, that code first calls `fetchOlder` then `fetchNewer`, and for the reason mentioned above, `fetchNewer` will not fetch any new messages. But that's fine, because as soon as older messages from `fetchOlder` arrives, there will be a change in the scroll metrics, so `fetchNewer` will be called again, fetching new messages. But imagine if the number of messages in the initial batch occupies less than a screenful, and then `fetchOlder` returns no messages or a few messages that combined with the initial batch messages are still less than a screenful; in that case, there will be no change in the scroll metrics to call `fetchNewer`. With the three fetch request types being separated, especially the two request types for older and newer messages, each direction can fetch more messages independently without interfering with one another. Another benefit of this change is that if there's a backoff in one direction, it will not affect the other one. Fixes: #1256 --- lib/model/message_list.dart | 141 +++++++++----- lib/widgets/message_list.dart | 10 +- test/model/message_list_test.dart | 303 +++++++++++++++++++++-------- test/model/message_test.dart | 4 +- test/widgets/compose_box_test.dart | 10 +- 5 files changed, 322 insertions(+), 146 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 6c50d13513..835646ee49 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -83,22 +83,33 @@ class MessageListOutboxMessageItem extends MessageListMessageBaseItem { ]); } -/// The status of outstanding or recent fetch requests from a [MessageListView]. -enum FetchingStatus { - /// The model has not made any fetch requests (since its last reset, if any). +/// The status of outstanding or recent `fetchInitial` request from a [MessageListView]. +enum FetchingInitialStatus { + /// The model has not made a `fetchInitial` request (since its last reset, if any). unstarted, /// The model has made a `fetchInitial` request, which hasn't succeeded. - fetchInitial, + fetching, - /// The model made a successful `fetchInitial` request, - /// and has no outstanding requests or backoff. + /// The model made a successful `fetchInitial` request. idle, +} + +/// The status of outstanding or recent "fetch more" request from a [MessageListView]. +/// +/// By a "fetch more" request we mean a `fetchOlder` or a `fetchNewer` request. +enum FetchingMoreStatus { + /// The model has not made the "fetch more" request (since its last reset, if any). + unstarted, + + /// The model has made the "fetch more" request, which hasn't succeeded. + fetching, - /// The model has an active `fetchOlder` or `fetchNewer` request. - fetchingMore, + /// The model made a successful "fetch more" request, + /// and has no outstanding request of the same kind or backoff. + idle, - /// The model is in a backoff period from a failed request. + /// The model is in a backoff period from the failed "fetch more" request. backoff, } @@ -125,7 +136,7 @@ mixin _MessageSequence { /// /// This may or may not represent all the message history that /// conceptually belongs in this message list. - /// That information is expressed in [fetched], [haveOldest], [haveNewest]. + /// That information is expressed in [initialFetched], [haveOldest], [haveNewest]. /// /// This also may or may not represent all the message history that /// conceptually belongs in this narrow because some messages might be @@ -165,12 +176,15 @@ mixin _MessageSequence { int? get newMessageId => _newMessageId; int? _newMessageId; - /// Whether [messages] and [items] represent the results of a fetch. + /// Whether the first batch of messages for this narrow is fetched yet. /// - /// This allows the UI to distinguish "still working on fetching messages" - /// from "there are in fact no messages here". - bool get fetched => switch (_status) { - FetchingStatus.unstarted || FetchingStatus.fetchInitial => false, + /// Some or all of the fetched messages may not make it to [messages] + /// and [items] if they're muted in one way or another. + /// + /// This allows the UI to distinguish "still working on fetching first batch + /// of messages" from "there are in fact no messages here". + bool get initialFetched => switch (_fetchInitialStatus) { + .unstarted || .fetching => false, _ => true, }; @@ -187,19 +201,34 @@ mixin _MessageSequence { bool _haveNewest = false; /// Whether this message list is currently busy when it comes to - /// fetching more messages. + /// fetching older messages. + /// + /// Here "busy" means a new call to fetch older messages would do nothing, + /// rather than make any request to the server, + /// as a result of an existing recent request. + /// This is true both when the recent request is still outstanding, + /// and when it failed and the backoff from that is still in progress. + bool get busyFetchingOlder => switch(_fetchOlderStatus) { + .fetching || .backoff => true, + _ => false, + }; + + /// Whether this message list is currently busy when it comes to + /// fetching newer messages. /// - /// Here "busy" means a new call to fetch more messages would do nothing, + /// Here "busy" means a new call to fetch older messages would do nothing, /// rather than make any request to the server, /// as a result of an existing recent request. /// This is true both when the recent request is still outstanding, /// and when it failed and the backoff from that is still in progress. - bool get busyFetchingMore => switch (_status) { - FetchingStatus.fetchingMore || FetchingStatus.backoff => true, + bool get busyFetchingNewer => switch(_fetchNewerStatus) { + .fetching || .backoff => true, _ => false, }; - FetchingStatus _status = FetchingStatus.unstarted; + FetchingInitialStatus _fetchInitialStatus = .unstarted; + FetchingMoreStatus _fetchOlderStatus = .unstarted; + FetchingMoreStatus _fetchNewerStatus = .unstarted; BackoffMachine? _fetchBackoffMachine; @@ -228,7 +257,7 @@ mixin _MessageSequence { /// [MessageListOutboxMessageItem]s corresponding to [outboxMessages]. /// /// This information is completely derived from [messages], [outboxMessages], - /// and the flags [haveOldest], [haveNewest], and [busyFetchingMore]. + /// and the flags [haveOldest], [haveNewest], [busyFetchingNewer], and [busyFetchingOlder]. /// It exists as an optimization, to memoize that computation. /// /// See also [middleItem], an index which divides this list @@ -434,7 +463,9 @@ mixin _MessageSequence { outboxMessages.clear(); _haveOldest = false; _haveNewest = false; - _status = FetchingStatus.unstarted; + _fetchInitialStatus = .unstarted; + _fetchOlderStatus = .unstarted; + _fetchNewerStatus = .unstarted; _fetchBackoffMachine = null; contents.clear(); items.clear(); @@ -786,16 +817,29 @@ class MessageListView with ChangeNotifier, _MessageSequence { } } - void _setStatus(FetchingStatus value, {FetchingStatus? was}) { - assert(was == null || _status == was); - _status = value; - if (!fetched) return; + void _setInitialStatus(FetchingInitialStatus value, {FetchingInitialStatus? was}) { + assert(was == null || _fetchInitialStatus == was); + _fetchInitialStatus = value; + if (!initialFetched) return; + notifyListeners(); + } + + void _setOlderStatus(FetchingMoreStatus value, {FetchingMoreStatus? was}) { + assert(was == null || _fetchOlderStatus == was); + _fetchOlderStatus = value; + notifyListeners(); + } + + void _setNewerStatus(FetchingMoreStatus value, {FetchingMoreStatus? was}) { + assert(was == null || _fetchNewerStatus == was); + _fetchNewerStatus = value; notifyListeners(); } /// Fetch messages, starting from scratch. Future fetchInitial() async { - assert(!fetched && !haveOldest && !haveNewest && !busyFetchingMore); + assert(!initialFetched && !haveOldest && !haveNewest + && !busyFetchingOlder && !busyFetchingNewer); assert(messages.isEmpty && contents.isEmpty); assert(oldMessageId == null && newMessageId == null); @@ -805,11 +849,11 @@ class MessageListView with ChangeNotifier, _MessageSequence { // probably better if the UI code doesn't take it to this point. _haveOldest = true; _haveNewest = true; - _setStatus(FetchingStatus.idle, was: FetchingStatus.unstarted); + _setInitialStatus(.idle, was: .unstarted); return; } - _setStatus(FetchingStatus.fetchInitial, was: FetchingStatus.unstarted); + _setInitialStatus(.fetching, was: .unstarted); // TODO schedule all this in another isolate final generation = this.generation; final result = await getMessages(store.connection, @@ -851,7 +895,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { _syncOutboxMessagesFromStore(); } - _setStatus(FetchingStatus.idle, was: FetchingStatus.fetchInitial); + _setInitialStatus(.idle, was: .fetching); } /// Update [narrow] for the result of a "with" narrow (topic permalink) fetch. @@ -889,23 +933,24 @@ class MessageListView with ChangeNotifier, _MessageSequence { /// Fetch the next batch of older messages, if applicable. /// /// If there are no older messages to fetch (i.e. if [haveOldest]), - /// or if this message list is already busy fetching more messages - /// (i.e. if [busyFetchingMore], which includes backoff from failed requests), + /// or if this message list is already busy fetching older messages + /// (i.e. if [busyFetchingOlder], which includes backoff from failed requests), /// then this method does nothing and immediately returns. /// That makes this method suitable to call frequently, e.g. every frame, - /// whenever it looks likely to be useful to have more messages. + /// whenever it looks likely to be useful to have older messages. Future fetchOlder() async { final generation = this.generation; int visibleMessageCount = 0; do { if (haveOldest) return; - if (busyFetchingMore) return; - assert(fetched); + if (busyFetchingOlder) return; + assert(initialFetched); assert(oldMessageId != null); await _fetchMore( anchor: NumericAnchor(oldMessageId!), numBefore: kMessageListFetchBatchSize, numAfter: 0, + setStatus: _setOlderStatus, processResult: (result) { _oldMessageId = result.messages.firstOrNull?.id ?? oldMessageId; store.reconcileMessages(result.messages); @@ -926,23 +971,24 @@ class MessageListView with ChangeNotifier, _MessageSequence { /// Fetch the next batch of newer messages, if applicable. /// /// If there are no newer messages to fetch (i.e. if [haveNewest]), - /// or if this message list is already busy fetching more messages - /// (i.e. if [busyFetchingMore], which includes backoff from failed requests), + /// or if this message list is already busy fetching newer messages + /// (i.e. if [busyFetchingNewer], which includes backoff from failed requests), /// then this method does nothing and immediately returns. /// That makes this method suitable to call frequently, e.g. every frame, - /// whenever it looks likely to be useful to have more messages. + /// whenever it looks likely to be useful to have newer messages. Future fetchNewer() async { final generation = this.generation; int visibleMessageCount = 0; do { if (haveNewest) return; - if (busyFetchingMore) return; - assert(fetched); + if (busyFetchingNewer) return; + assert(initialFetched); assert(newMessageId != null); await _fetchMore( anchor: NumericAnchor(newMessageId!), numBefore: 0, numAfter: kMessageListFetchBatchSize, + setStatus: _setNewerStatus, processResult: (result) { _newMessageId = result.messages.lastOrNull?.id ?? newMessageId; store.reconcileMessages(result.messages); @@ -968,12 +1014,13 @@ class MessageListView with ChangeNotifier, _MessageSequence { required Anchor anchor, required int numBefore, required int numAfter, + required void Function(FetchingMoreStatus value, {FetchingMoreStatus? was}) setStatus, required void Function(GetMessagesResult) processResult, }) async { assert(narrow is! TopicNarrow // We only intend to send "with" in [fetchInitial]; see there. || (narrow as TopicNarrow).with_ == null); - _setStatus(FetchingStatus.fetchingMore, was: FetchingStatus.idle); + setStatus(.fetching); final generation = this.generation; bool hasFetchError = false; try { @@ -994,14 +1041,14 @@ class MessageListView with ChangeNotifier, _MessageSequence { } finally { if (this.generation == generation) { if (hasFetchError) { - _setStatus(FetchingStatus.backoff, was: FetchingStatus.fetchingMore); + setStatus(.backoff, was: .fetching); unawaited((_fetchBackoffMachine ??= BackoffMachine()) .wait().then((_) { if (this.generation != generation) return; - _setStatus(FetchingStatus.idle, was: FetchingStatus.backoff); + setStatus(.idle, was: .backoff); })); } else { - _setStatus(FetchingStatus.idle, was: FetchingStatus.fetchingMore); + setStatus(.idle, was: .fetching); _fetchBackoffMachine = null; } } @@ -1013,7 +1060,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { /// This will set [anchor] to [AnchorCode.newest], /// and cause messages to be re-fetched from scratch. void jumpToEnd() { - assert(fetched); + assert(initialFetched); assert(!haveNewest); assert(anchor != AnchorCode.newest); _anchor = AnchorCode.newest; @@ -1095,7 +1142,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { // TODO get the newly-unmuted messages from the message store // For now, we simplify the task by just refetching this message list // from scratch. - if (fetched) { + if (initialFetched) { _reset(); notifyListeners(); fetchInitial(); @@ -1123,7 +1170,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { // TODO get the newly-unmuted messages from the message store // For now, we simplify the task by just refetching this message list // from scratch. - if (fetched) { + if (initialFetched) { _reset(); notifyListeners(); fetchInitial(); diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index da171a5ab3..f7e2de1a55 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -1026,7 +1026,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat Widget build(BuildContext context) { final zulipLocalizations = ZulipLocalizations.of(context); - if (!model.fetched) return const Center(child: CircularProgressIndicator()); + if (!model.initialFetched) return const Center(child: CircularProgressIndicator()); if (model.items.isEmpty && model.haveNewest && model.haveOldest) { final String header; @@ -1206,11 +1206,9 @@ class _MessageListState extends State with PerAccountStoreAwareStat // Else if we're busy with fetching, then show a loading indicator. // // This applies even if the fetch is over, but failed, and we're still - // in backoff from it; and even if the fetch is/was for the other direction. - // The loading indicator really means "busy, working on it"; and that's the - // right summary even if the fetch is internally queued behind other work. + // in backoff from it. return model.haveOldest ? const _MessageListHistoryStart() - : model.busyFetchingMore ? const _MessageListLoadingMore() + : model.busyFetchingOlder ? const _MessageListLoadingMore() : const SizedBox.shrink(); } @@ -1225,7 +1223,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat // https://chat.zulip.org/#narrow/channel/48-mobile/topic/space.20at.20end.20of.20thread/near/2203391 const SizedBox(height: 12), ]); - } else if (model.busyFetchingMore) { + } else if (model.busyFetchingNewer) { // See [_buildStartCap] for why this condition shows a loading indicator. return const _MessageListLoadingMore(); } else { diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 178b576dd5..b9234ff779 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -101,7 +101,7 @@ void main() { checkInvariants(model); notifiedCount++; }); - check(model).fetched.isFalse(); + check(model).initialFetched.isFalse(); checkNotNotified(); } @@ -192,7 +192,7 @@ void main() { messages: List.generate(kMessageListFetchBatchSize, generateMessages), ).toJson()); final fetchFuture = model.fetchInitial(); - check(model).fetched.isFalse(); + check(model).initialFetched.isFalse(); checkNotNotified(); await fetchFuture; @@ -257,7 +257,7 @@ void main() { await model.fetchInitial(); checkNotifiedOnce(); check(model) - ..fetched.isTrue() + ..initialFetched.isTrue() ..messages.isEmpty() ..haveOldest.isTrue() ..haveNewest.isTrue(); @@ -292,7 +292,7 @@ void main() { async.elapse(kLocalEchoDebounceDuration); checkNotNotified(); check(model) - ..fetched.isFalse() + ..initialFetched.isFalse() ..outboxMessages.isEmpty(); connection.prepare( @@ -300,7 +300,7 @@ void main() { await model.fetchInitial(); checkNotifiedOnce(); check(model) - ..fetched.isTrue() + ..initialFetched.isTrue() ..outboxMessages.length.equals(1); })); @@ -313,7 +313,7 @@ void main() { async.elapse(kLocalEchoDebounceDuration); checkNotNotified(); check(model) - ..fetched.isFalse() + ..initialFetched.isFalse() ..outboxMessages.isEmpty(); connection.prepare(json: newestResult(foundOldest: true, @@ -321,7 +321,7 @@ void main() { await model.fetchInitial(); checkNotifiedOnce(); check(model) - ..fetched.isTrue() + ..initialFetched.isTrue() ..outboxMessages.length.equals(1); })); @@ -335,7 +335,7 @@ void main() { await prepareOutboxMessages(count: 1, stream: stream, topic: 'topic'); async.elapse(kLocalEchoDebounceDuration); checkNotNotified(); - check(model)..fetched.isFalse()..outboxMessages.isEmpty(); + check(model)..initialFetched.isFalse()..outboxMessages.isEmpty(); final message = eg.streamMessage(stream: stream, topic: 'topic'); connection.prepare(json: nearResult( @@ -345,7 +345,7 @@ void main() { messages: [message]).toJson()); await model.fetchInitial(); checkNotifiedOnce(); - check(model)..fetched.isTrue()..haveNewest.isFalse()..outboxMessages.isEmpty(); + check(model)..initialFetched.isTrue()..haveNewest.isFalse()..outboxMessages.isEmpty(); connection.prepare(json: newerResult(anchor: message.id, foundNewest: true, messages: [eg.streamMessage(stream: stream, topic: 'topic')]).toJson()); @@ -463,7 +463,7 @@ void main() { model.renarrowAndFetch(newNarrow, newAnchor); checkNotifiedOnce(); check(model) - ..fetched.isFalse() + ..initialFetched.isFalse() ..narrow.equals(newNarrow) ..anchor.equals(newAnchor) ..messages.isEmpty(); @@ -472,14 +472,14 @@ void main() { // pending; check that the list is still empty despite the fetchOlder. async.elapse(Duration(milliseconds: 750)); check(model) - ..fetched.isFalse() + ..initialFetched.isFalse() ..narrow.equals(newNarrow) ..messages.isEmpty(); // Elapse until the renarrowAndFetch completes. async.elapse(Duration(seconds: 250)); check(model) - ..fetched.isTrue() + ..initialFetched.isTrue() ..narrow.equals(newNarrow) ..anchor.equals(newAnchor) ..messages.length.equals(2); @@ -499,12 +499,12 @@ void main() { ).toJson()); final fetchFuture = model.fetchOlder(); checkNotifiedOnce(); - check(model).busyFetchingMore.isTrue(); + check(model).busyFetchingOlder.isTrue(); await fetchFuture; checkNotifiedOnce(); check(model) - ..busyFetchingMore.isFalse() + ..busyFetchingOlder.isFalse() ..messages.length.equals(200); checkLastRequest( narrow: narrow.apiEncode(), @@ -528,12 +528,12 @@ void main() { ).toJson()); final fetchFuture = model.fetchNewer(); checkNotifiedOnce(); - check(model).busyFetchingMore.isTrue(); + check(model).busyFetchingNewer.isTrue(); await fetchFuture; checkNotifiedOnce(); check(model) - ..busyFetchingMore.isFalse() + ..busyFetchingNewer.isFalse() ..messages.length.equals(200); checkLastRequest( narrow: narrow.apiEncode(), @@ -704,7 +704,7 @@ void main() { }); }); - test('nop when already fetching older', () async { + test('fetchOlder nop when already fetching older', () async { await prepare(anchor: NumericAnchor(1000)); await prepareMessages(foundOldest: false, foundNewest: false, messages: List.generate(201, (i) => eg.streamMessage(id: 900 + i))); @@ -715,26 +715,54 @@ void main() { ).toJson()); final fetchFuture = model.fetchOlder(); checkNotifiedOnce(); - check(model).busyFetchingMore.isTrue(); + check(model).busyFetchingOlder.isTrue(); // Don't prepare another response. final fetchFuture2 = model.fetchOlder(); checkNotNotified(); - check(model).busyFetchingMore.isTrue(); - final fetchFuture3 = model.fetchNewer(); - checkNotNotified(); - check(model)..busyFetchingMore.isTrue()..messages.length.equals(201); + check(model)..busyFetchingOlder.isTrue()..messages.length.equals(201); await fetchFuture; await fetchFuture2; - await fetchFuture3; // We must not have made another request, because we didn't // prepare another response and didn't get an exception. checkNotifiedOnce(); - check(model)..busyFetchingMore.isFalse()..messages.length.equals(301); + check(model)..busyFetchingOlder.isFalse()..messages.length.equals(301); + }); + + test('fetchNewer fetches when already fetching older', () async { + await prepare(anchor: NumericAnchor(1000)); + await prepareMessages(foundOldest: false, foundNewest: false, + messages: List.generate(201, (i) => eg.streamMessage(id: 900 + i))); + + connection.prepare(json: olderResult( + anchor: 900, foundOldest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 800 + i)), + ).toJson()); + final fetchFuture = model.fetchOlder(); + checkNotifiedOnce(); + check(model).busyFetchingOlder.isTrue(); + check(model).busyFetchingNewer.isFalse(); + + connection.prepare(json: newerResult( + anchor: 1100, foundNewest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 1101 + i)), + ).toJson()); + final fetchFuture2 = model.fetchNewer(); + checkNotifiedOnce(); + check(model).busyFetchingOlder.isTrue(); + check(model).busyFetchingNewer.isTrue(); + check(model).messages.length.equals(201); + + await fetchFuture; + await fetchFuture2; + checkNotified(count: 2); + check(model).busyFetchingOlder.isFalse(); + check(model).busyFetchingNewer.isFalse(); + check(model).messages.length.equals(401); }); - test('nop when already fetching newer', () async { + test('fetchNewer nop when already fetching newer', () async { await prepare(anchor: NumericAnchor(1000)); await prepareMessages(foundOldest: false, foundNewest: false, messages: List.generate(201, (i) => eg.streamMessage(id: 900 + i))); @@ -745,23 +773,51 @@ void main() { ).toJson()); final fetchFuture = model.fetchNewer(); checkNotifiedOnce(); - check(model).busyFetchingMore.isTrue(); + check(model).busyFetchingNewer.isTrue(); // Don't prepare another response. - final fetchFuture2 = model.fetchOlder(); - checkNotNotified(); - check(model).busyFetchingMore.isTrue(); - final fetchFuture3 = model.fetchNewer(); + final fetchFuture2 = model.fetchNewer(); checkNotNotified(); - check(model)..busyFetchingMore.isTrue()..messages.length.equals(201); + check(model)..busyFetchingNewer.isTrue()..messages.length.equals(201); await fetchFuture; await fetchFuture2; - await fetchFuture3; // We must not have made another request, because we didn't // prepare another response and didn't get an exception. checkNotifiedOnce(); - check(model)..busyFetchingMore.isFalse()..messages.length.equals(301); + check(model)..busyFetchingNewer.isFalse()..messages.length.equals(301); + }); + + test('fetchOlder fetches when already fetching newer', () async { + await prepare(anchor: NumericAnchor(1000)); + await prepareMessages(foundOldest: false, foundNewest: false, + messages: List.generate(201, (i) => eg.streamMessage(id: 900 + i))); + + connection.prepare(json: newerResult( + anchor: 1100, foundNewest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 1101 + i)), + ).toJson()); + final fetchFuture = model.fetchNewer(); + checkNotifiedOnce(); + check(model).busyFetchingNewer.isTrue(); + check(model).busyFetchingOlder.isFalse(); + + connection.prepare(json: olderResult( + anchor: 900, foundOldest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 800 + i)), + ).toJson()); + final fetchFuture2 = model.fetchOlder(); + checkNotifiedOnce(); + check(model).busyFetchingNewer.isTrue(); + check(model).busyFetchingOlder.isTrue(); + check(model).messages.length.equals(201); + + await fetchFuture; + await fetchFuture2; + checkNotified(count: 2); + check(model).busyFetchingNewer.isFalse(); + check(model).busyFetchingOlder.isFalse(); + check(model).messages.length.equals(401); }); test('fetchOlder nop when already haveOldest true', () async { @@ -798,10 +854,9 @@ void main() { ..messages.length.equals(151); }); - test('nop during backoff', () => awaitFakeAsync((async) async { + test('fetchOlder nop during fetchOlder backoff', () => awaitFakeAsync((async) async { final olderMessages = List.generate(50, (i) => eg.streamMessage()); final initialMessages = List.generate(50, (i) => eg.streamMessage()); - final newerMessages = List.generate(50, (i) => eg.streamMessage()); await prepare(anchor: NumericAnchor(initialMessages[25].id)); await prepareMessages(foundOldest: false, foundNewest: false, messages: initialMessages); @@ -811,22 +866,17 @@ void main() { check(async.pendingTimers).isEmpty(); await check(model.fetchOlder()).throws(); checkNotified(count: 2); - check(model).busyFetchingMore.isTrue(); + check(model).busyFetchingOlder.isTrue(); check(connection.takeRequests()).single; await model.fetchOlder(); checkNotNotified(); - check(model).busyFetchingMore.isTrue(); - check(connection.lastRequest).isNull(); - - await model.fetchNewer(); - checkNotNotified(); - check(model).busyFetchingMore.isTrue(); + check(model).busyFetchingOlder.isTrue(); check(connection.lastRequest).isNull(); // Wait long enough that a first backoff is sure to finish. async.elapse(const Duration(seconds: 1)); - check(model).busyFetchingMore.isFalse(); + check(model).busyFetchingOlder.isFalse(); checkNotifiedOnce(); check(connection.lastRequest).isNull(); @@ -835,10 +885,89 @@ void main() { await model.fetchOlder(); checkNotified(count: 2); check(connection.takeRequests()).single; + })); + + test('fetchNewer fetches during fetchOlder backoff', () => awaitFakeAsync((async) async { + final initialMessages = List.generate(50, (i) => eg.streamMessage()); + final newerMessages = List.generate(50, (i) => eg.streamMessage()); + await prepare(anchor: NumericAnchor(initialMessages[25].id)); + await prepareMessages(foundOldest: false, foundNewest: false, + messages: initialMessages); + check(connection.takeRequests()).single; + + connection.prepare(apiException: eg.apiBadRequest()); + check(async.pendingTimers).isEmpty(); + await check(model.fetchOlder()).throws(); + checkNotified(count: 2); + check(model).busyFetchingOlder.isTrue(); + check(connection.takeRequests()).single; connection.prepare(json: newerResult(anchor: initialMessages.last.id, foundNewest: false, messages: newerMessages).toJson()); + final fetchNewerFuture = model.fetchNewer(); + check(model).busyFetchingOlder.isTrue(); + check(model).busyFetchingNewer.isTrue(); + await fetchNewerFuture; + check(model).busyFetchingNewer.isFalse(); + checkNotified(count: 2); + check(connection.takeRequests()).single; + })); + + test('fetchNewer nop during fetchNewer backoff', () => awaitFakeAsync((async) async { + final initialMessages = List.generate(50, (i) => eg.streamMessage()); + final newerMessages = List.generate(50, (i) => eg.streamMessage()); + await prepare(anchor: NumericAnchor(initialMessages[25].id)); + await prepareMessages(foundOldest: false, foundNewest: false, + messages: initialMessages); + check(connection.takeRequests()).single; + + connection.prepare(apiException: eg.apiBadRequest()); + check(async.pendingTimers).isEmpty(); + await check(model.fetchNewer()).throws(); + checkNotified(count: 2); + check(model).busyFetchingNewer.isTrue(); + check(connection.takeRequests()).single; + await model.fetchNewer(); + checkNotNotified(); + check(model).busyFetchingNewer.isTrue(); + check(connection.lastRequest).isNull(); + + // Wait long enough that a first backoff is sure to finish. + async.elapse(const Duration(seconds: 1)); + check(model).busyFetchingNewer.isFalse(); + checkNotifiedOnce(); + check(connection.lastRequest).isNull(); + + connection.prepare(json: newerResult(anchor: initialMessages.last.id, + foundNewest: false, messages: newerMessages).toJson()); + await model.fetchNewer(); + checkNotified(count: 2); + check(connection.takeRequests()).single; + })); + + test('fetchOlder fetches during fetchNewer backoff', () => awaitFakeAsync((async) async { + final olderMessages = List.generate(50, (i) => eg.streamMessage()); + final initialMessages = List.generate(50, (i) => eg.streamMessage()); + await prepare(anchor: NumericAnchor(initialMessages[25].id)); + await prepareMessages(foundOldest: false, foundNewest: false, + messages: initialMessages); + check(connection.takeRequests()).single; + + connection.prepare(apiException: eg.apiBadRequest()); + check(async.pendingTimers).isEmpty(); + await check(model.fetchNewer()).throws(); + checkNotified(count: 2); + check(model).busyFetchingNewer.isTrue(); + check(connection.takeRequests()).single; + + connection.prepare(json: olderResult(anchor: initialMessages.last.id, + foundOldest: false, messages: olderMessages).toJson()); + final fetchOlderFuture = model.fetchOlder(); + check(model).busyFetchingNewer.isTrue(); + check(model).busyFetchingOlder.isTrue(); + await fetchOlderFuture; + check(model).busyFetchingOlder.isFalse(); checkNotified(count: 2); check(connection.takeRequests()).single; })); @@ -1057,7 +1186,7 @@ void main() { await prepare(narrow: ChannelNarrow(stream.streamId)); await store.addMessage(eg.streamMessage(stream: stream)); checkNotNotified(); - check(model).fetched.isFalse(); + check(model).initialFetched.isFalse(); }); test('when there are outbox messages', () => awaitFakeAsync((async) async { @@ -1185,13 +1314,13 @@ void main() { await prepare(narrow: ChannelNarrow(stream.streamId)); await prepareOutboxMessages(count: 5, stream: stream); check(model) - ..fetched.isFalse() + ..initialFetched.isFalse() ..outboxMessages.isEmpty(); async.elapse(kLocalEchoDebounceDuration); checkNotNotified(); check(model) - ..fetched.isFalse() + ..initialFetched.isFalse() ..outboxMessages.isEmpty(); })); }); @@ -1447,7 +1576,7 @@ void main() { json: newestResult(foundOldest: true, messages: messages).toJson()); await setVisibility(UserTopicVisibilityPolicy.unmuted); checkNotifiedOnce(); - check(model).fetched.isFalse(); + check(model).initialFetched.isFalse(); checkHasMessageIds([]); check(model).outboxMessages.isEmpty(); @@ -1591,7 +1720,7 @@ void main() { json: newestResult(foundOldest: true, messages: messages).toJson()); await store.setMutedUsers([]); checkNotifiedOnce(); - check(model).fetched.isFalse(); + check(model).initialFetched.isFalse(); checkHasMessageIds([]); async.elapse(Duration.zero); @@ -1880,7 +2009,7 @@ void main() { origStreamId: otherStream.streamId, newMessages: movedMessages, )); - check(model).fetched.isFalse(); + check(model).initialFetched.isFalse(); checkHasMessages([]); checkNotifiedOnce(); @@ -2005,7 +2134,7 @@ void main() { origTopicStr: origTopic, newMessages: movedMessages, )); - check(model).fetched.isFalse(); + check(model).initialFetched.isFalse(); checkHasMessages([]); checkNotifiedOnce(); @@ -2046,7 +2175,7 @@ void main() { origTopicStr: 'other', newMessages: otherTopicMovedMessages, )); - check(model).fetched.isTrue(); + check(model).initialFetched.isTrue(); checkHasMessages(initialMessages); checkNotNotified(); })); @@ -2058,7 +2187,7 @@ void main() { origStreamId: 200, newMessages: otherChannelMovedMessages, )); - check(model).fetched.isTrue(); + check(model).initialFetched.isTrue(); checkHasMessages(initialMessages); checkNotNotified(); })); @@ -2119,7 +2248,7 @@ void main() { messages: [followedMessage], ).toJson()); - check(model).fetched.isFalse(); + check(model).initialFetched.isFalse(); checkHasMessages([]); await store.handleEvent(eg.updateMessageEventMoveTo( origTopicStr: 'topic', @@ -2148,7 +2277,7 @@ void main() { messages: olderMessages, ).toJson()); final fetchFuture = model.fetchOlder(); - check(model).busyFetchingMore.isTrue(); + check(model).busyFetchingOlder.isTrue(); checkHasMessages(initialMessages); checkNotifiedOnce(); @@ -2161,7 +2290,7 @@ void main() { origStreamId: otherStream.streamId, newMessages: movedMessages, )); - check(model).busyFetchingMore.isFalse(); + check(model).busyFetchingOlder.isFalse(); checkHasMessages([]); checkNotifiedOnce(); @@ -2184,7 +2313,7 @@ void main() { ).toJson()); final fetchFuture = model.fetchOlder(); checkHasMessages(initialMessages); - check(model).busyFetchingMore.isTrue(); + check(model).busyFetchingOlder.isTrue(); checkNotifiedOnce(); connection.prepare(delay: const Duration(seconds: 1), json: newestResult( @@ -2197,7 +2326,7 @@ void main() { newMessages: movedMessages, )); checkHasMessages([]); - check(model).busyFetchingMore.isFalse(); + check(model).busyFetchingOlder.isFalse(); checkNotifiedOnce(); async.elapse(const Duration(seconds: 1)); @@ -2218,8 +2347,8 @@ void main() { BackoffMachine.debugDuration = const Duration(seconds: 1); await check(model.fetchOlder()).throws(); final backoffTimerA = async.pendingTimers.single; - check(model).busyFetchingMore.isTrue(); - check(model).fetched.isTrue(); + check(model).busyFetchingOlder.isTrue(); + check(model).initialFetched.isTrue(); checkHasMessages(initialMessages); checkNotified(count: 2); @@ -2233,39 +2362,39 @@ void main() { newMessages: movedMessages, )); // Check that _reset was called. - check(model).fetched.isFalse(); + check(model).initialFetched.isFalse(); checkHasMessages([]); checkNotifiedOnce(); - check(model).busyFetchingMore.isFalse(); + check(model).busyFetchingOlder.isFalse(); check(backoffTimerA.isActive).isTrue(); async.elapse(Duration.zero); - check(model).fetched.isTrue(); + check(model).initialFetched.isTrue(); checkHasMessages(initialMessages + movedMessages); checkNotifiedOnce(); - check(model).busyFetchingMore.isFalse(); + check(model).busyFetchingOlder.isFalse(); check(backoffTimerA.isActive).isTrue(); connection.prepare(apiException: eg.apiBadRequest()); BackoffMachine.debugDuration = const Duration(seconds: 2); await check(model.fetchOlder()).throws(); final backoffTimerB = async.pendingTimers.last; - check(model).busyFetchingMore.isTrue(); + check(model).busyFetchingOlder.isTrue(); check(backoffTimerA.isActive).isTrue(); check(backoffTimerB.isActive).isTrue(); checkNotified(count: 2); - // When `backoffTimerA` ends, `busyFetchingMore` remains `true` + // When `backoffTimerA` ends, `busyFetchingOlder` remains `true` // because the backoff was from a previous generation. async.elapse(const Duration(seconds: 1)); - check(model).busyFetchingMore.isTrue(); + check(model).busyFetchingOlder.isTrue(); check(backoffTimerA.isActive).isFalse(); check(backoffTimerB.isActive).isTrue(); checkNotNotified(); - // When `backoffTimerB` ends, `busyFetchingMore` gets reset. + // When `backoffTimerB` ends, `busyFetchingOlder` gets reset. async.elapse(const Duration(seconds: 1)); - check(model).busyFetchingMore.isFalse(); + check(model).busyFetchingOlder.isFalse(); check(backoffTimerA.isActive).isFalse(); check(backoffTimerB.isActive).isFalse(); checkNotifiedOnce(); @@ -2280,7 +2409,7 @@ void main() { ).toJson()); final fetchFuture = model.fetchInitial(); checkHasMessages([]); - check(model).fetched.isFalse(); + check(model).initialFetched.isFalse(); connection.prepare(delay: const Duration(seconds: 2), json: newestResult( foundOldest: false, @@ -2292,12 +2421,12 @@ void main() { newMessages: movedMessages, )); checkHasMessages([]); - check(model).fetched.isFalse(); + check(model).initialFetched.isFalse(); checkNotifiedOnce(); await fetchFuture; checkHasMessages([]); - check(model).fetched.isFalse(); + check(model).initialFetched.isFalse(); checkNotNotified(); async.elapse(const Duration(seconds: 1)); @@ -2314,7 +2443,7 @@ void main() { ).toJson()); final fetchFuture = model.fetchInitial(); checkHasMessages([]); - check(model).fetched.isFalse(); + check(model).initialFetched.isFalse(); connection.prepare(delay: const Duration(seconds: 1), json: newestResult( foundOldest: false, @@ -2326,11 +2455,11 @@ void main() { newMessages: movedMessages, )); checkHasMessages([]); - check(model).fetched.isFalse(); + check(model).initialFetched.isFalse(); async.elapse(const Duration(seconds: 1)); checkHasMessages(initialMessages + movedMessages); - check(model).fetched.isTrue(); + check(model).initialFetched.isTrue(); await fetchFuture; checkHasMessages(initialMessages + movedMessages); @@ -2347,7 +2476,7 @@ void main() { ).toJson()); final fetchFuture1 = model.fetchOlder(); checkHasMessages(initialMessages); - check(model).busyFetchingMore.isTrue(); + check(model).busyFetchingOlder.isTrue(); checkNotifiedOnce(); connection.prepare(delay: const Duration(seconds: 1), json: newestResult( @@ -2360,7 +2489,7 @@ void main() { newMessages: movedMessages, )); checkHasMessages([]); - check(model).busyFetchingMore.isFalse(); + check(model).busyFetchingOlder.isFalse(); checkNotifiedOnce(); async.elapse(const Duration(seconds: 1)); @@ -2373,19 +2502,19 @@ void main() { ).toJson()); final fetchFuture2 = model.fetchOlder(); checkHasMessages(initialMessages + movedMessages); - check(model).busyFetchingMore.isTrue(); + check(model).busyFetchingOlder.isTrue(); checkNotifiedOnce(); await fetchFuture1; checkHasMessages(initialMessages + movedMessages); // The older fetchOlder call should not override fetchingOlder set by // the new fetchOlder call, nor should it notify the listeners. - check(model).busyFetchingMore.isTrue(); + check(model).busyFetchingOlder.isTrue(); checkNotNotified(); await fetchFuture2; checkHasMessages(olderMessages + initialMessages + movedMessages); - check(model).busyFetchingMore.isFalse(); + check(model).busyFetchingOlder.isFalse(); checkNotifiedOnce(); })); }); @@ -2654,9 +2783,9 @@ void main() { newMessages: messages, origStreamId: eg.stream().streamId)); checkNotifiedOnce(); - check(model).fetched.isFalse(); + check(model).initialFetched.isFalse(); async.elapse(Duration.zero); - check(model).fetched.isTrue(); + check(model).initialFetched.isTrue(); check(model.outboxMessages).single.isA() .conversation.topic.equals(eg.t('not muted')); })); @@ -3495,7 +3624,7 @@ List? _lastMessages; int? _lastMiddleMessage; void checkInvariants(MessageListView model) { - if (!model.fetched) { + if (!model.initialFetched) { check(model) ..messages.isEmpty() ..outboxMessages.isEmpty() @@ -3503,10 +3632,11 @@ void checkInvariants(MessageListView model) { ..newMessageId.isNull() ..haveOldest.isFalse() ..haveNewest.isFalse() - ..busyFetchingMore.isFalse(); + ..busyFetchingOlder.isFalse() + ..busyFetchingNewer.isFalse(); } if (model.haveOldest && model.haveNewest) { - check(model).busyFetchingMore.isFalse(); + check(model)..busyFetchingOlder.isFalse()..busyFetchingNewer.isFalse(); } for (final message in model.messages) { @@ -3688,10 +3818,11 @@ extension MessageListViewChecks on Subject { Subject> get contents => has((x) => x.contents, 'contents'); Subject> get items => has((x) => x.items, 'items'); Subject get middleItem => has((x) => x.middleItem, 'middleItem'); - Subject get fetched => has((x) => x.fetched, 'fetched'); + Subject get initialFetched => has((x) => x.initialFetched, 'initialFetched'); Subject get oldMessageId => has((x) => x.oldMessageId, 'oldMessageId'); Subject get newMessageId => has((x) => x.newMessageId, 'newMessageId'); Subject get haveOldest => has((x) => x.haveOldest, 'haveOldest'); Subject get haveNewest => has((x) => x.haveNewest, 'haveNewest'); - Subject get busyFetchingMore => has((x) => x.busyFetchingMore, 'busyFetchingMore'); + Subject get busyFetchingOlder => has((x) => x.busyFetchingOlder, 'busyFetchingOlder'); + Subject get busyFetchingNewer => has((x) => x.busyFetchingNewer, 'busyFetchingNewer'); } diff --git a/test/model/message_test.dart b/test/model/message_test.dart index 96d95c0c04..de12dd3b6b 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -77,7 +77,7 @@ void main() { notifiedCount++; }); addTearDown(messageList.dispose); - check(messageList).fetched.isFalse(); + check(messageList).initialFetched.isFalse(); checkNotNotified(); // This cleans up possibly pending timers from [MessageStoreImpl]. @@ -99,7 +99,7 @@ void main() { Future addMessages(Iterable messages) async { await store.addMessages(messages); - checkNotified(count: messageList.fetched ? messages.length : 0); + checkNotified(count: messageList.initialFetched ? messages.length : 0); } test('dispose cancels pending timers', () => awaitFakeAsync((async) async { diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart index d27e374e3b..f6a775541e 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -1504,7 +1504,7 @@ void main() { bannerLabel: zulipLocalizations.composeBoxBannerLabelUnsubscribedWhenCannotSend); final model = MessageListPage.ancestorOf(state.context).model!; check(model) - ..fetched.isTrue()..messages.length.equals(100); + ..initialFetched.isTrue()..messages.length.equals(100); connection.prepare(json: eg.newestGetMessagesResult(foundOldest: true, messages: messages).toJson(), @@ -1512,10 +1512,10 @@ void main() { await tester.tap(find.widgetWithText(ZulipWebUiKitButton, 'Refresh')); await tester.pump(); check(model) - ..fetched.isFalse()..messages.length.equals(0); + ..initialFetched.isFalse()..messages.length.equals(0); await tester.pump(Duration(seconds: 1)); check(model) - ..fetched.isTrue()..messages.length.equals(100); + ..initialFetched.isTrue()..messages.length.equals(100); connection.takeRequests(); @@ -1536,10 +1536,10 @@ void main() { }); await tester.pump(Duration(milliseconds: 500)); check(model) - ..fetched.isFalse()..messages.length.equals(0); + ..initialFetched.isFalse()..messages.length.equals(0); await tester.pump(Duration(seconds: 1)); check(model) - ..fetched.isTrue()..messages.length.equals(100); + ..initialFetched.isTrue()..messages.length.equals(100); }); }