From debe99e9c1170fb58c510a5d2ba5bc4ac67639e4 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 8 May 2025 15:27:46 -0700 Subject: [PATCH 01/20] msglist [nfc]: Build end-of-feed widgets in a helper method This will give us a natural home for logic that makes these depend on whether we have the newest messages, once that becomes something that varies. --- lib/widgets/message_list.dart | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index e25445e514..218fed1a7b 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -686,15 +686,9 @@ class _MessageListState extends State with PerAccountStoreAwareStat if (childIndex < 0) return null; return childIndex; }, - childCount: bottomItems + 3, + childCount: bottomItems + 1, (context, childIndex) { - // To reinforce that the end of the feed has been reached: - // https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Mark-as-read/near/1680603 - if (childIndex == bottomItems + 2) return const SizedBox(height: 36); - - if (childIndex == bottomItems + 1) return MarkAsReadWidget(narrow: widget.narrow); - - if (childIndex == bottomItems) return TypingStatusWidget(narrow: widget.narrow); + if (childIndex == bottomItems) return _buildEndCap(); final itemIndex = topItems + childIndex; final data = model.items[itemIndex]; @@ -743,6 +737,16 @@ class _MessageListState extends State with PerAccountStoreAwareStat }; } + Widget _buildEndCap() { + return Column(crossAxisAlignment: CrossAxisAlignment.stretch, children: [ + TypingStatusWidget(narrow: widget.narrow), + MarkAsReadWidget(narrow: widget.narrow), + // To reinforce that the end of the feed has been reached: + // https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Mark-as-read/near/1680603 + const SizedBox(height: 36), + ]); + } + Widget _buildItem(MessageListItem data) { switch (data) { case MessageListRecipientHeaderItem(): From d831280afb940ab00b21ed9fcd30f82f101259e6 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 1 May 2025 18:38:31 -0700 Subject: [PATCH 02/20] msglist [nfc]: Say we'll show "loading" even when fetch is at other end This is NFC ultimately because we currently only ever fetch, or show loading indicators, at one end of the message list, namely the start. When we do start supporting a message list in the middle of history, though (#82), and consequently loading newer as well as older messages, my conclusion after thinking it through is that we'll want a "busy fetching" state at one end to mean we show a loading indicator at the other end too, if it still has more to be fetched. This would look weird if the user actually saw both at the same time -- but that shouldn't happen, because if both ends (or even either end) is still open then the original fetch should have found plenty of messages to separate them, many screenfuls' worth. And conversely, if the user does kick off a fetch at one end and then scroll swiftly to the other end and witness how that appears, we want to show them a "loading" sign. The situation is exactly like if they'd had a fetch attempt on that same end and we were backing off from failure: there's no fetch right now, but even though a fetch is needed (because the user is looking at the incomplete end of the known history), the app will hold off from starting one right now. Declining to start a fetch when one is needed means effectively that the loading is busy. --- lib/widgets/message_list.dart | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 218fed1a7b..9c990c333f 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -725,16 +725,21 @@ class _MessageListState extends State with PerAccountStoreAwareStat } Widget _buildStartCap() { - // These assertions are invariants of [MessageListView]. + // If we're done fetching older messages, show that. + // 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. + + // (This assertion is an invariant of [MessageListView].) assert(!(model.fetchingOlder && model.fetchOlderCoolingDown)); - final effectiveFetchingOlder = + final busyFetchingMore = model.fetchingOlder || model.fetchOlderCoolingDown; - assert(!(model.haveOldest && effectiveFetchingOlder)); - return switch ((effectiveFetchingOlder, model.haveOldest)) { - (true, _) => const _MessageListLoadingMore(), - (_, true) => const _MessageListHistoryStart(), - (_, _) => const SizedBox.shrink(), - }; + return model.haveOldest ? const _MessageListHistoryStart() + : busyFetchingMore ? const _MessageListLoadingMore() + : const SizedBox.shrink(); } Widget _buildEndCap() { From 3be937750d20fddc436d140e332dbcbebe340511 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 1 May 2025 18:22:11 -0700 Subject: [PATCH 03/20] msglist [nfc]: Use `fetched` getter when reading Generally this is helpful because it means that viewing references to the field will highlight specifically the places that set it. Here it's also helpful because we're about to replace the field with an enum shared across several getters. --- 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 8022ba8a7d..bf86c35088 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -697,7 +697,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { if (!narrow.containsMessage(message) || !_messageVisible(message)) { return; } - if (!_fetched) { + if (!fetched) { // TODO mitigate this fetch/event race: save message to add to list later return; } From b1f97c626730ff68df1833c2c0d5c63421e20748 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 1 May 2025 18:29:10 -0700 Subject: [PATCH 04/20] msglist [nfc]: Use an enum for fetched/fetching/backoff state This makes the relationships between these flags clearer. It will also simplify some upcoming refactors that change their semantics. --- lib/model/message_list.dart | 48 +++++++++++++++++++++++++------------ 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index bf86c35088..0a53234f24 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -64,6 +64,23 @@ class MessageListMessageItem extends MessageListMessageBaseItem { }); } +/// The status of outstanding or recent fetch requests from a [MessageListView]. +enum FetchingStatus { + /// The model hasn't successfully completed a `fetchInitial` request + /// (since its last reset, if any). + unfetched, + + /// The model made a successful `fetchInitial` request, + /// and has no outstanding requests or backoff. + idle, + + /// The model has an active `fetchOlder` request. + fetchOlder, + + /// The model is in a backoff period from a failed `fetchOlder` request. + fetchOlderCoolingDown, +} + /// The sequence of messages in a message list, and how to display them. /// /// This comprises much of the guts of [MessageListView]. @@ -95,8 +112,7 @@ mixin _MessageSequence { /// /// This allows the UI to distinguish "still working on fetching messages" /// from "there are in fact no messages here". - bool get fetched => _fetched; - bool _fetched = false; + bool get fetched => _status != FetchingStatus.unfetched; /// Whether we know we have the oldest messages for this narrow. /// @@ -113,8 +129,7 @@ mixin _MessageSequence { /// the same response each time. /// /// See also [fetchOlderCoolingDown]. - bool get fetchingOlder => _fetchingOlder; - bool _fetchingOlder = false; + bool get fetchingOlder => _status == FetchingStatus.fetchOlder; /// Whether [fetchOlder] had a request error recently. /// @@ -127,8 +142,9 @@ mixin _MessageSequence { /// when a [fetchOlder] request succeeds. /// /// See also [fetchingOlder]. - bool get fetchOlderCoolingDown => _fetchOlderCoolingDown; - bool _fetchOlderCoolingDown = false; + bool get fetchOlderCoolingDown => _status == FetchingStatus.fetchOlderCoolingDown; + + FetchingStatus _status = FetchingStatus.unfetched; BackoffMachine? _fetchOlderCooldownBackoffMachine; @@ -303,10 +319,8 @@ mixin _MessageSequence { generation += 1; messages.clear(); middleMessage = 0; - _fetched = false; _haveOldest = false; - _fetchingOlder = false; - _fetchOlderCoolingDown = false; + _status = FetchingStatus.unfetched; _fetchOlderCooldownBackoffMachine = null; contents.clear(); items.clear(); @@ -520,6 +534,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { // TODO(#82): fetch from a given message ID as anchor assert(!fetched && !haveOldest && !fetchingOlder && !fetchOlderCoolingDown); assert(messages.isEmpty && contents.isEmpty); + assert(_status == FetchingStatus.unfetched); // TODO schedule all this in another isolate final generation = this.generation; final result = await getMessages(store.connection, @@ -543,7 +558,8 @@ class MessageListView with ChangeNotifier, _MessageSequence { _addMessage(message); // Now [middleMessage] is the last message (the one just added). } - _fetched = true; + assert(_status == FetchingStatus.unfetched); + _status = FetchingStatus.idle; _haveOldest = result.foundOldest; notifyListeners(); } @@ -590,7 +606,8 @@ class MessageListView with ChangeNotifier, _MessageSequence { // We only intend to send "with" in [fetchInitial]; see there. || (narrow as TopicNarrow).with_ == null); assert(messages.isNotEmpty); - _fetchingOlder = true; + assert(_status == FetchingStatus.idle); + _status = FetchingStatus.fetchOlder; notifyListeners(); final generation = this.generation; bool hasFetchError = false; @@ -628,17 +645,18 @@ class MessageListView with ChangeNotifier, _MessageSequence { _haveOldest = result.foundOldest; } finally { if (this.generation == generation) { - _fetchingOlder = false; + assert(_status == FetchingStatus.fetchOlder); if (hasFetchError) { - assert(!fetchOlderCoolingDown); - _fetchOlderCoolingDown = true; + _status = FetchingStatus.fetchOlderCoolingDown; unawaited((_fetchOlderCooldownBackoffMachine ??= BackoffMachine()) .wait().then((_) { if (this.generation != generation) return; - _fetchOlderCoolingDown = false; + assert(_status == FetchingStatus.fetchOlderCoolingDown); + _status = FetchingStatus.idle; notifyListeners(); })); } else { + _status = FetchingStatus.idle; _fetchOlderCooldownBackoffMachine = null; } notifyListeners(); From babef41de54241a6ead2698ab2d1708f2b605b52 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 3 May 2025 23:13:22 -0700 Subject: [PATCH 05/20] msglist [nfc]: Split unfetched vs fetchInitial states, just for asserts --- lib/model/message_list.dart | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 0a53234f24..8ad33dd348 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -66,9 +66,11 @@ class MessageListMessageItem extends MessageListMessageBaseItem { /// The status of outstanding or recent fetch requests from a [MessageListView]. enum FetchingStatus { - /// The model hasn't successfully completed a `fetchInitial` request - /// (since its last reset, if any). - unfetched, + /// The model has not made any fetch requests (since its last reset, if any). + unstarted, + + /// The model has made a `fetchInitial` request, which hasn't succeeded. + fetchInitial, /// The model made a successful `fetchInitial` request, /// and has no outstanding requests or backoff. @@ -112,7 +114,10 @@ mixin _MessageSequence { /// /// This allows the UI to distinguish "still working on fetching messages" /// from "there are in fact no messages here". - bool get fetched => _status != FetchingStatus.unfetched; + bool get fetched => switch (_status) { + FetchingStatus.unstarted || FetchingStatus.fetchInitial => false, + _ => true, + }; /// Whether we know we have the oldest messages for this narrow. /// @@ -144,7 +149,7 @@ mixin _MessageSequence { /// See also [fetchingOlder]. bool get fetchOlderCoolingDown => _status == FetchingStatus.fetchOlderCoolingDown; - FetchingStatus _status = FetchingStatus.unfetched; + FetchingStatus _status = FetchingStatus.unstarted; BackoffMachine? _fetchOlderCooldownBackoffMachine; @@ -320,7 +325,7 @@ mixin _MessageSequence { messages.clear(); middleMessage = 0; _haveOldest = false; - _status = FetchingStatus.unfetched; + _status = FetchingStatus.unstarted; _fetchOlderCooldownBackoffMachine = null; contents.clear(); items.clear(); @@ -534,7 +539,8 @@ class MessageListView with ChangeNotifier, _MessageSequence { // TODO(#82): fetch from a given message ID as anchor assert(!fetched && !haveOldest && !fetchingOlder && !fetchOlderCoolingDown); assert(messages.isEmpty && contents.isEmpty); - assert(_status == FetchingStatus.unfetched); + assert(_status == FetchingStatus.unstarted); + _status = FetchingStatus.fetchInitial; // TODO schedule all this in another isolate final generation = this.generation; final result = await getMessages(store.connection, @@ -558,7 +564,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { _addMessage(message); // Now [middleMessage] is the last message (the one just added). } - assert(_status == FetchingStatus.unfetched); + assert(_status == FetchingStatus.fetchInitial); _status = FetchingStatus.idle; _haveOldest = result.foundOldest; notifyListeners(); From 5aec54d10b4e16bb65791c36e09832a4291bcab4 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 1 May 2025 18:55:12 -0700 Subject: [PATCH 06/20] msglist [nfc]: Unify fetch/cooldown status as busyFetchingMore Now the distinction between these two states exists only for asserts. --- lib/model/message_list.dart | 48 +++++++++++------------ lib/widgets/message_list.dart | 7 +--- test/model/message_list_test.dart | 65 ++++++++++++++----------------- 3 files changed, 52 insertions(+), 68 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 8ad33dd348..c295515fb9 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -126,28 +126,18 @@ mixin _MessageSequence { bool get haveOldest => _haveOldest; bool _haveOldest = false; - /// Whether we are currently fetching the next batch of older messages. - /// - /// When this is true, [fetchOlder] is a no-op. - /// That method is called frequently by Flutter's scrolling logic, - /// and this field helps us avoid spamming the same request just to get - /// the same response each time. - /// - /// See also [fetchOlderCoolingDown]. - bool get fetchingOlder => _status == FetchingStatus.fetchOlder; - - /// Whether [fetchOlder] had a request error recently. - /// - /// When this is true, [fetchOlder] is a no-op. - /// That method is called frequently by Flutter's scrolling logic, - /// and this field mitigates spamming the same request and getting - /// the same error each time. - /// - /// "Recently" is decided by a [BackoffMachine] that resets - /// when a [fetchOlder] request succeeds. - /// - /// See also [fetchingOlder]. - bool get fetchOlderCoolingDown => _status == FetchingStatus.fetchOlderCoolingDown; + /// Whether this message list is currently busy when it comes to + /// fetching more messages. + /// + /// Here "busy" means a new call to fetch more 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.fetchOlder || FetchingStatus.fetchOlderCoolingDown => true, + _ => false, + }; FetchingStatus _status = FetchingStatus.unstarted; @@ -168,7 +158,7 @@ mixin _MessageSequence { /// before, between, or after the messages. /// /// This information is completely derived from [messages] and - /// the flags [haveOldest], [fetchingOlder] and [fetchOlderCoolingDown]. + /// the flags [haveOldest] and [busyFetchingMore]. /// It exists as an optimization, to memoize that computation. /// /// See also [middleItem], an index which divides this list @@ -537,7 +527,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { Future fetchInitial() async { // TODO(#80): fetch from anchor firstUnread, instead of newest // TODO(#82): fetch from a given message ID as anchor - assert(!fetched && !haveOldest && !fetchingOlder && !fetchOlderCoolingDown); + assert(!fetched && !haveOldest && !busyFetchingMore); assert(messages.isEmpty && contents.isEmpty); assert(_status == FetchingStatus.unstarted); _status = FetchingStatus.fetchInitial; @@ -603,10 +593,16 @@ 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), + /// 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. Future fetchOlder() async { if (haveOldest) return; - if (fetchingOlder) return; - if (fetchOlderCoolingDown) return; + if (busyFetchingMore) return; assert(fetched); assert(narrow is! TopicNarrow // We only intend to send "with" in [fetchInitial]; see there. diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 9c990c333f..d0862cb1d7 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -732,13 +732,8 @@ class _MessageListState extends State with PerAccountStoreAwareStat // 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. - - // (This assertion is an invariant of [MessageListView].) - assert(!(model.fetchingOlder && model.fetchOlderCoolingDown)); - final busyFetchingMore = - model.fetchingOlder || model.fetchOlderCoolingDown; return model.haveOldest ? const _MessageListHistoryStart() - : busyFetchingMore ? const _MessageListLoadingMore() + : model.busyFetchingMore ? const _MessageListLoadingMore() : const SizedBox.shrink(); } diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 11f2b3056b..797f389989 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -256,12 +256,12 @@ void main() { ).toJson()); final fetchFuture = model.fetchOlder(); checkNotifiedOnce(); - check(model).fetchingOlder.isTrue(); + check(model).busyFetchingMore.isTrue(); await fetchFuture; checkNotifiedOnce(); check(model) - ..fetchingOlder.isFalse() + ..busyFetchingMore.isFalse() ..messages.length.equals(200); checkLastRequest( narrow: narrow.apiEncode(), @@ -285,12 +285,12 @@ void main() { ).toJson()); final fetchFuture = model.fetchOlder(); checkNotifiedOnce(); - check(model).fetchingOlder.isTrue(); + check(model).busyFetchingMore.isTrue(); // Don't prepare another response. final fetchFuture2 = model.fetchOlder(); checkNotNotified(); - check(model).fetchingOlder.isTrue(); + check(model).busyFetchingMore.isTrue(); await fetchFuture; await fetchFuture2; @@ -298,7 +298,7 @@ void main() { // prepare another response and didn't get an exception. checkNotifiedOnce(); check(model) - ..fetchingOlder.isFalse() + ..busyFetchingMore.isFalse() ..messages.length.equals(200); }); @@ -330,18 +330,17 @@ void main() { check(async.pendingTimers).isEmpty(); await check(model.fetchOlder()).throws(); checkNotified(count: 2); - check(model).fetchOlderCoolingDown.isTrue(); + check(model).busyFetchingMore.isTrue(); check(connection.takeRequests()).single; await model.fetchOlder(); checkNotNotified(); - check(model).fetchOlderCoolingDown.isTrue(); - check(model).fetchingOlder.isFalse(); + check(model).busyFetchingMore.isTrue(); check(connection.lastRequest).isNull(); // Wait long enough that a first backoff is sure to finish. async.elapse(const Duration(seconds: 1)); - check(model).fetchOlderCoolingDown.isFalse(); + check(model).busyFetchingMore.isFalse(); checkNotifiedOnce(); check(connection.lastRequest).isNull(); @@ -366,7 +365,7 @@ void main() { await model.fetchOlder(); checkNotified(count: 2); check(model) - ..fetchingOlder.isFalse() + ..busyFetchingMore.isFalse() ..messages.length.equals(200); }); @@ -1068,7 +1067,7 @@ void main() { messages: olderMessages, ).toJson()); final fetchFuture = model.fetchOlder(); - check(model).fetchingOlder.isTrue(); + check(model).busyFetchingMore.isTrue(); checkHasMessages(initialMessages); checkNotifiedOnce(); @@ -1081,7 +1080,7 @@ void main() { origStreamId: otherStream.streamId, newMessages: movedMessages, )); - check(model).fetchingOlder.isFalse(); + check(model).busyFetchingMore.isFalse(); checkHasMessages([]); checkNotifiedOnce(); @@ -1104,7 +1103,7 @@ void main() { ).toJson()); final fetchFuture = model.fetchOlder(); checkHasMessages(initialMessages); - check(model).fetchingOlder.isTrue(); + check(model).busyFetchingMore.isTrue(); checkNotifiedOnce(); connection.prepare(delay: const Duration(seconds: 1), json: newestResult( @@ -1117,7 +1116,7 @@ void main() { newMessages: movedMessages, )); checkHasMessages([]); - check(model).fetchingOlder.isFalse(); + check(model).busyFetchingMore.isFalse(); checkNotifiedOnce(); async.elapse(const Duration(seconds: 1)); @@ -1138,7 +1137,7 @@ void main() { BackoffMachine.debugDuration = const Duration(seconds: 1); await check(model.fetchOlder()).throws(); final backoffTimerA = async.pendingTimers.single; - check(model).fetchOlderCoolingDown.isTrue(); + check(model).busyFetchingMore.isTrue(); check(model).fetched.isTrue(); checkHasMessages(initialMessages); checkNotified(count: 2); @@ -1156,36 +1155,36 @@ void main() { check(model).fetched.isFalse(); checkHasMessages([]); checkNotifiedOnce(); - check(model).fetchOlderCoolingDown.isFalse(); + check(model).busyFetchingMore.isFalse(); check(backoffTimerA.isActive).isTrue(); async.elapse(Duration.zero); check(model).fetched.isTrue(); checkHasMessages(initialMessages + movedMessages); checkNotifiedOnce(); - check(model).fetchOlderCoolingDown.isFalse(); + check(model).busyFetchingMore.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).fetchOlderCoolingDown.isTrue(); + check(model).busyFetchingMore.isTrue(); check(backoffTimerA.isActive).isTrue(); check(backoffTimerB.isActive).isTrue(); checkNotified(count: 2); - // When `backoffTimerA` ends, `fetchOlderCoolingDown` remains `true` + // When `backoffTimerA` ends, `busyFetchingMore` remains `true` // because the backoff was from a previous generation. async.elapse(const Duration(seconds: 1)); - check(model).fetchOlderCoolingDown.isTrue(); + check(model).busyFetchingMore.isTrue(); check(backoffTimerA.isActive).isFalse(); check(backoffTimerB.isActive).isTrue(); checkNotNotified(); - // When `backoffTimerB` ends, `fetchOlderCoolingDown` gets reset. + // When `backoffTimerB` ends, `busyFetchingMore` gets reset. async.elapse(const Duration(seconds: 1)); - check(model).fetchOlderCoolingDown.isFalse(); + check(model).busyFetchingMore.isFalse(); check(backoffTimerA.isActive).isFalse(); check(backoffTimerB.isActive).isFalse(); checkNotifiedOnce(); @@ -1267,7 +1266,7 @@ void main() { ).toJson()); final fetchFuture1 = model.fetchOlder(); checkHasMessages(initialMessages); - check(model).fetchingOlder.isTrue(); + check(model).busyFetchingMore.isTrue(); checkNotifiedOnce(); connection.prepare(delay: const Duration(seconds: 1), json: newestResult( @@ -1280,7 +1279,7 @@ void main() { newMessages: movedMessages, )); checkHasMessages([]); - check(model).fetchingOlder.isFalse(); + check(model).busyFetchingMore.isFalse(); checkNotifiedOnce(); async.elapse(const Duration(seconds: 1)); @@ -1293,19 +1292,19 @@ void main() { ).toJson()); final fetchFuture2 = model.fetchOlder(); checkHasMessages(initialMessages + movedMessages); - check(model).fetchingOlder.isTrue(); + check(model).busyFetchingMore.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).fetchingOlder.isTrue(); + check(model).busyFetchingMore.isTrue(); checkNotNotified(); await fetchFuture2; checkHasMessages(olderMessages + initialMessages + movedMessages); - check(model).fetchingOlder.isFalse(); + check(model).busyFetchingMore.isFalse(); checkNotifiedOnce(); })); }); @@ -2140,15 +2139,10 @@ void checkInvariants(MessageListView model) { check(model) ..messages.isEmpty() ..haveOldest.isFalse() - ..fetchingOlder.isFalse() - ..fetchOlderCoolingDown.isFalse(); + ..busyFetchingMore.isFalse(); } if (model.haveOldest) { - check(model).fetchingOlder.isFalse(); - check(model).fetchOlderCoolingDown.isFalse(); - } - if (model.fetchingOlder) { - check(model).fetchOlderCoolingDown.isFalse(); + check(model).busyFetchingMore.isFalse(); } for (final message in model.messages) { @@ -2292,6 +2286,5 @@ extension MessageListViewChecks on Subject { Subject get middleItem => has((x) => x.middleItem, 'middleItem'); Subject get fetched => has((x) => x.fetched, 'fetched'); Subject get haveOldest => has((x) => x.haveOldest, 'haveOldest'); - Subject get fetchingOlder => has((x) => x.fetchingOlder, 'fetchingOlder'); - Subject get fetchOlderCoolingDown => has((x) => x.fetchOlderCoolingDown, 'fetchOlderCoolingDown'); + Subject get busyFetchingMore => has((x) => x.busyFetchingMore, 'busyFetchingMore'); } From 70c31c345f1e8504b6fc7be00d1468078eddcc28 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 1 May 2025 18:00:02 -0700 Subject: [PATCH 07/20] msglist [nfc]: Rename backoff state to share between older/newer If a fetch in one direction has recently failed, we'll want the backoff to apply to any attempt to fetch in the other direction too; after all, it's the same server. We can also drop the term "cooldown" here, which is effectively redundant with "backoff". --- lib/model/message_list.dart | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index c295515fb9..1943a99768 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -79,8 +79,8 @@ enum FetchingStatus { /// The model has an active `fetchOlder` request. fetchOlder, - /// The model is in a backoff period from a failed `fetchOlder` request. - fetchOlderCoolingDown, + /// The model is in a backoff period from a failed request. + backoff, } /// The sequence of messages in a message list, and how to display them. @@ -135,13 +135,13 @@ mixin _MessageSequence { /// 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.fetchOlder || FetchingStatus.fetchOlderCoolingDown => true, + FetchingStatus.fetchOlder || FetchingStatus.backoff => true, _ => false, }; FetchingStatus _status = FetchingStatus.unstarted; - BackoffMachine? _fetchOlderCooldownBackoffMachine; + BackoffMachine? _fetchBackoffMachine; /// The parsed message contents, as a list parallel to [messages]. /// @@ -316,7 +316,7 @@ mixin _MessageSequence { middleMessage = 0; _haveOldest = false; _status = FetchingStatus.unstarted; - _fetchOlderCooldownBackoffMachine = null; + _fetchBackoffMachine = null; contents.clear(); items.clear(); middleItem = 0; @@ -649,17 +649,17 @@ class MessageListView with ChangeNotifier, _MessageSequence { if (this.generation == generation) { assert(_status == FetchingStatus.fetchOlder); if (hasFetchError) { - _status = FetchingStatus.fetchOlderCoolingDown; - unawaited((_fetchOlderCooldownBackoffMachine ??= BackoffMachine()) + _status = FetchingStatus.backoff; + unawaited((_fetchBackoffMachine ??= BackoffMachine()) .wait().then((_) { if (this.generation != generation) return; - assert(_status == FetchingStatus.fetchOlderCoolingDown); + assert(_status == FetchingStatus.backoff); _status = FetchingStatus.idle; notifyListeners(); })); } else { _status = FetchingStatus.idle; - _fetchOlderCooldownBackoffMachine = null; + _fetchBackoffMachine = null; } notifyListeners(); } From b75f1680d5e6b3d1c0f68df772eb8c5ce0092b67 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 1 May 2025 18:36:57 -0700 Subject: [PATCH 08/20] msglist [nfc]: Rename fetchingMore status from fetchOlder This matches the symmetry expressed in the description of busyFetchingMore and at the latter's call site in widgets code: whichever direction (older or newer) we might have a fetch request active in, the consequences we draw are the same in both directions. --- lib/model/message_list.dart | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 1943a99768..6786aeed9f 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -77,7 +77,7 @@ enum FetchingStatus { idle, /// The model has an active `fetchOlder` request. - fetchOlder, + fetchingMore, /// The model is in a backoff period from a failed request. backoff, @@ -135,7 +135,7 @@ mixin _MessageSequence { /// 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.fetchOlder || FetchingStatus.backoff => true, + FetchingStatus.fetchingMore || FetchingStatus.backoff => true, _ => false, }; @@ -609,7 +609,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { || (narrow as TopicNarrow).with_ == null); assert(messages.isNotEmpty); assert(_status == FetchingStatus.idle); - _status = FetchingStatus.fetchOlder; + _status = FetchingStatus.fetchingMore; notifyListeners(); final generation = this.generation; bool hasFetchError = false; @@ -647,7 +647,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { _haveOldest = result.foundOldest; } finally { if (this.generation == generation) { - assert(_status == FetchingStatus.fetchOlder); + assert(_status == FetchingStatus.fetchingMore); if (hasFetchError) { _status = FetchingStatus.backoff; unawaited((_fetchBackoffMachine ??= BackoffMachine()) From 7558042370bd1bdf147a56df9d8164e51cbabbfc Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 1 May 2025 19:38:46 -0700 Subject: [PATCH 09/20] msglist [nfc]: Pull out a _setStatus method This tightens up a bit the logic for maintaining the fetching status, and hopefully makes it a bit easier to read. --- lib/model/message_list.dart | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 6786aeed9f..517ce06cfd 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -523,14 +523,20 @@ class MessageListView with ChangeNotifier, _MessageSequence { } } + void _setStatus(FetchingStatus value, {FetchingStatus? was}) { + assert(was == null || _status == was); + _status = value; + if (!fetched) return; + notifyListeners(); + } + /// Fetch messages, starting from scratch. Future fetchInitial() async { // TODO(#80): fetch from anchor firstUnread, instead of newest // TODO(#82): fetch from a given message ID as anchor assert(!fetched && !haveOldest && !busyFetchingMore); assert(messages.isEmpty && contents.isEmpty); - assert(_status == FetchingStatus.unstarted); - _status = FetchingStatus.fetchInitial; + _setStatus(FetchingStatus.fetchInitial, was: FetchingStatus.unstarted); // TODO schedule all this in another isolate final generation = this.generation; final result = await getMessages(store.connection, @@ -554,10 +560,8 @@ class MessageListView with ChangeNotifier, _MessageSequence { _addMessage(message); // Now [middleMessage] is the last message (the one just added). } - assert(_status == FetchingStatus.fetchInitial); - _status = FetchingStatus.idle; _haveOldest = result.foundOldest; - notifyListeners(); + _setStatus(FetchingStatus.idle, was: FetchingStatus.fetchInitial); } /// Update [narrow] for the result of a "with" narrow (topic permalink) fetch. @@ -608,9 +612,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { // We only intend to send "with" in [fetchInitial]; see there. || (narrow as TopicNarrow).with_ == null); assert(messages.isNotEmpty); - assert(_status == FetchingStatus.idle); - _status = FetchingStatus.fetchingMore; - notifyListeners(); + _setStatus(FetchingStatus.fetchingMore, was: FetchingStatus.idle); final generation = this.generation; bool hasFetchError = false; try { @@ -647,21 +649,17 @@ class MessageListView with ChangeNotifier, _MessageSequence { _haveOldest = result.foundOldest; } finally { if (this.generation == generation) { - assert(_status == FetchingStatus.fetchingMore); if (hasFetchError) { - _status = FetchingStatus.backoff; + _setStatus(FetchingStatus.backoff, was: FetchingStatus.fetchingMore); unawaited((_fetchBackoffMachine ??= BackoffMachine()) .wait().then((_) { if (this.generation != generation) return; - assert(_status == FetchingStatus.backoff); - _status = FetchingStatus.idle; - notifyListeners(); + _setStatus(FetchingStatus.idle, was: FetchingStatus.backoff); })); } else { - _status = FetchingStatus.idle; + _setStatus(FetchingStatus.idle, was: FetchingStatus.fetchingMore); _fetchBackoffMachine = null; } - notifyListeners(); } } } From 6ff889bee4b1ebd5ad7399e56df6df1c2ac86ade Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 1 May 2025 19:09:53 -0700 Subject: [PATCH 10/20] msglist [nfc]: Introduce haveNewest in model, always true for now --- lib/model/message_list.dart | 32 ++++++++++++++++++++++++------- test/model/message_list_test.dart | 17 ++++++++++++---- 2 files changed, 38 insertions(+), 11 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 517ce06cfd..f2415504fc 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -94,7 +94,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] and [haveOldest]. + /// That information is expressed in [fetched], [haveOldest], [haveNewest]. /// /// See also [middleMessage], an index which divides this list /// into a top slice and a bottom slice. @@ -121,11 +121,19 @@ mixin _MessageSequence { /// Whether we know we have the oldest messages for this narrow. /// - /// (Currently we always have the newest messages for the narrow, - /// once [fetched] is true, because we start from the newest.) + /// See also [haveNewest]. bool get haveOldest => _haveOldest; bool _haveOldest = false; + /// Whether we know we have the newest messages for this narrow. + /// + /// (Currently this is always true once [fetched] is true, + /// because we start from the newest.) + /// + /// See also [haveOldest]. + bool get haveNewest => _haveNewest; + bool _haveNewest = false; + /// Whether this message list is currently busy when it comes to /// fetching more messages. /// @@ -158,7 +166,7 @@ mixin _MessageSequence { /// before, between, or after the messages. /// /// This information is completely derived from [messages] and - /// the flags [haveOldest] and [busyFetchingMore]. + /// the flags [haveOldest], [haveNewest], and [busyFetchingMore]. /// It exists as an optimization, to memoize that computation. /// /// See also [middleItem], an index which divides this list @@ -315,6 +323,7 @@ mixin _MessageSequence { messages.clear(); middleMessage = 0; _haveOldest = false; + _haveNewest = false; _status = FetchingStatus.unstarted; _fetchBackoffMachine = null; contents.clear(); @@ -534,7 +543,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { Future fetchInitial() async { // TODO(#80): fetch from anchor firstUnread, instead of newest // TODO(#82): fetch from a given message ID as anchor - assert(!fetched && !haveOldest && !busyFetchingMore); + assert(!fetched && !haveOldest && !haveNewest && !busyFetchingMore); assert(messages.isEmpty && contents.isEmpty); _setStatus(FetchingStatus.fetchInitial, was: FetchingStatus.unstarted); // TODO schedule all this in another isolate @@ -561,6 +570,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { // Now [middleMessage] is the last message (the one just added). } _haveOldest = result.foundOldest; + _haveNewest = true; // TODO(#82) _setStatus(FetchingStatus.idle, was: FetchingStatus.fetchInitial); } @@ -715,8 +725,16 @@ class MessageListView with ChangeNotifier, _MessageSequence { if (!narrow.containsMessage(message) || !_messageVisible(message)) { return; } - if (!fetched) { - // TODO mitigate this fetch/event race: save message to add to list later + if (!haveNewest) { + // This message list's [messages] doesn't yet reach the new end + // of the narrow's message history. (Either [fetchInitial] hasn't yet + // completed, or if it has then it was in the middle of history and no + // subsequent fetch has reached the end.) + // So this still-newer message doesn't belong. + // Leave it to be found by a subsequent fetch when appropriate. + // TODO mitigate this fetch/event race: save message to add to list later, + // in case the fetch that reaches the end is already ongoing and + // didn't include this message. return; } // TODO insert in middle instead, when appropriate diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 797f389989..4dc4a4c1fb 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -150,7 +150,8 @@ void main() { checkNotifiedOnce(); check(model) ..messages.length.equals(kMessageListFetchBatchSize) - ..haveOldest.isFalse(); + ..haveOldest.isFalse() + ..haveNewest.isTrue(); checkLastRequest( narrow: narrow.apiEncode(), anchor: 'newest', @@ -180,7 +181,8 @@ void main() { checkNotifiedOnce(); check(model) ..messages.length.equals(30) - ..haveOldest.isTrue(); + ..haveOldest.isTrue() + ..haveNewest.isTrue(); }); test('no messages found', () async { @@ -194,7 +196,8 @@ void main() { check(model) ..fetched.isTrue() ..messages.isEmpty() - ..haveOldest.isTrue(); + ..haveOldest.isTrue() + ..haveNewest.isTrue(); }); // TODO(#824): move this test @@ -417,6 +420,10 @@ void main() { check(model).messages.length.equals(30); }); + test('while in mid-history', () async { + }, skip: true, // TODO(#82): not yet possible to exercise this case + ); + test('before fetch', () async { final stream = eg.stream(); await prepare(narrow: ChannelNarrow(stream.streamId)); @@ -2139,9 +2146,10 @@ void checkInvariants(MessageListView model) { check(model) ..messages.isEmpty() ..haveOldest.isFalse() + ..haveNewest.isFalse() ..busyFetchingMore.isFalse(); } - if (model.haveOldest) { + if (model.haveOldest && model.haveNewest) { check(model).busyFetchingMore.isFalse(); } @@ -2286,5 +2294,6 @@ extension MessageListViewChecks on Subject { Subject get middleItem => has((x) => x.middleItem, 'middleItem'); Subject get fetched => has((x) => x.fetched, 'fetched'); 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 77934581ceb77b223ea1128078657c4fd79361e0 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 1 May 2025 19:15:27 -0700 Subject: [PATCH 11/20] msglist: Set haveNewest from response, like haveOldest This is NFC with a correctly-behaved server: we set `anchor=newest`, so the server always sets `found_newest` to true. Conversely, this will be helpful as we generalize `fetchInitial` to work with other anchor values; we'll use the `found_newest` value given by the server, without trying to predict it from the anchor. The server behavior that makes this effectively NFC isn't quite explicit in the API docs. Those say: found_newest: boolean Whether the server promises that the messages list includes the very newest messages matching the narrow (used by clients that paginate their requests to decide whether there may be more messages to fetch). https://zulip.com/api/get-messages#response But with `anchor=newest`, the response does need to include the very newest messages in the narrow -- that's the meaning of that `anchor` value. So the server is in fact promising the list includes those, and `found_newest` is therefore required to be true. (And indeed in practice the server does set `found_newest` to true when `anchor=newest`; it has specific logic to do so.) --- lib/model/message_list.dart | 2 +- test/example_data.dart | 20 ++++++++++++++++++ test/model/message_list_test.dart | 35 +++++++++++++++++++++++++++++-- 3 files changed, 54 insertions(+), 3 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index f2415504fc..76f8402541 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -570,7 +570,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { // Now [middleMessage] is the last message (the one just added). } _haveOldest = result.foundOldest; - _haveNewest = true; // TODO(#82) + _haveNewest = result.foundNewest; _setStatus(FetchingStatus.idle, was: FetchingStatus.fetchInitial); } diff --git a/test/example_data.dart b/test/example_data.dart index b87cbb6dc8..3dd2ab5e06 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -608,6 +608,26 @@ GetMessagesResult newestGetMessagesResult({ ); } +/// A GetMessagesResult the server might return on an initial request +/// when the anchor is in the middle of history (e.g., a /near/ link). +GetMessagesResult nearGetMessagesResult({ + required int anchor, + bool foundAnchor = true, + required bool foundOldest, + required bool foundNewest, + bool historyLimited = false, + required List messages, +}) { + return GetMessagesResult( + anchor: anchor, + foundAnchor: foundAnchor, + foundOldest: foundOldest, + foundNewest: foundNewest, + historyLimited: historyLimited, + messages: messages, + ); +} + /// A GetMessagesResult the server might return when we request older messages. GetMessagesResult olderGetMessagesResult({ required int anchor, diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 4dc4a4c1fb..05c3e58550 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -26,6 +26,7 @@ import 'recent_senders_test.dart' as recent_senders_test; import 'test_store.dart'; const newestResult = eg.newestGetMessagesResult; +const nearResult = eg.nearGetMessagesResult; const olderResult = eg.olderGetMessagesResult; void main() { @@ -185,6 +186,24 @@ void main() { ..haveNewest.isTrue(); }); + test('early in history', () async { + // For now, this gets a response that isn't realistic for the + // request it sends, to simulate when we start sending requests + // that would make this response realistic. + // TODO(#82): send appropriate fetch request + await prepare(); + connection.prepare(json: nearResult( + anchor: 1000, foundOldest: true, foundNewest: false, + messages: List.generate(111, (i) => eg.streamMessage(id: 990 + i)), + ).toJson()); + await model.fetchInitial(); + checkNotifiedOnce(); + check(model) + ..messages.length.equals(111) + ..haveOldest.isTrue() + ..haveNewest.isFalse(); + }); + test('no messages found', () async { await prepare(); connection.prepare(json: newestResult( @@ -421,8 +440,20 @@ void main() { }); test('while in mid-history', () async { - }, skip: true, // TODO(#82): not yet possible to exercise this case - ); + final stream = eg.stream(); + await prepare(narrow: ChannelNarrow(stream.streamId)); + connection.prepare(json: nearResult( + anchor: 1000, foundOldest: true, foundNewest: false, + messages: List.generate(30, + (i) => eg.streamMessage(id: 1000 + i, stream: stream))).toJson()); + await model.fetchInitial(); + checkNotifiedOnce(); + + check(model).messages.length.equals(30); + await store.addMessage(eg.streamMessage(stream: stream)); + checkNotNotified(); + check(model).messages.length.equals(30); + }); test('before fetch', () async { final stream = eg.stream(); From dcaf366e3ce86dec097fed6da8a81e90c6a3bdee Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 17 May 2025 15:49:21 -0700 Subject: [PATCH 12/20] test [nfc]: Generalize a helper eg.getMessagesResult Also expand a bit of docs to reflect what happens on a request using AnchorCode.firstUnread. --- test/example_data.dart | 59 +++++++++++++++++++++++++++++++++++------- 1 file changed, 50 insertions(+), 9 deletions(-) diff --git a/test/example_data.dart b/test/example_data.dart index 3dd2ab5e06..e3316e1218 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -589,25 +589,66 @@ DmMessage dmMessage({ }) as Map); } -/// A GetMessagesResult the server might return on an `anchor=newest` request. -GetMessagesResult newestGetMessagesResult({ - required bool foundOldest, +/// A GetMessagesResult the server might return for +/// a request that sent the given [anchor]. +/// +/// The request's anchor controls the response's [GetMessagesResult.anchor], +/// affects the default for [foundAnchor], +/// and in some cases forces the value of [foundOldest] or [foundNewest]. +GetMessagesResult getMessagesResult({ + required Anchor anchor, + bool? foundAnchor, + bool? foundOldest, + bool? foundNewest, bool historyLimited = false, required List messages, }) { - return GetMessagesResult( - // These anchor, foundAnchor, and foundNewest values are what the server - // appears to always return when the request had `anchor=newest`. - anchor: 10000000000000000, // that's 16 zeros - foundAnchor: false, - foundNewest: true, + final resultAnchor = switch (anchor) { + AnchorCode.oldest => 0, + NumericAnchor(:final messageId) => messageId, + AnchorCode.firstUnread => + throw ArgumentError("firstUnread not accepted in this helper; try NumericAnchor"), + AnchorCode.newest => 10_000_000_000_000_000, // that's 16 zeros + }; + switch (anchor) { + case AnchorCode.oldest || AnchorCode.newest: + assert(foundAnchor == null); + foundAnchor = false; + case AnchorCode.firstUnread || NumericAnchor(): + foundAnchor ??= true; + } + + if (anchor == AnchorCode.oldest) { + assert(foundOldest == null); + foundOldest = true; + } else if (anchor == AnchorCode.newest) { + assert(foundNewest == null); + foundNewest = true; + } + if (foundOldest == null || foundNewest == null) throw ArgumentError(); + + return GetMessagesResult( + anchor: resultAnchor, + foundAnchor: foundAnchor, foundOldest: foundOldest, + foundNewest: foundNewest, historyLimited: historyLimited, messages: messages, ); } +/// A GetMessagesResult the server might return on an `anchor=newest` request, +/// or `anchor=first_unread` when there are no unreads. +GetMessagesResult newestGetMessagesResult({ + required bool foundOldest, + bool historyLimited = false, + required List messages, +}) { + return getMessagesResult(anchor: AnchorCode.newest, foundOldest: foundOldest, + historyLimited: historyLimited, messages: messages); +} + /// A GetMessagesResult the server might return on an initial request /// when the anchor is in the middle of history (e.g., a /near/ link). GetMessagesResult nearGetMessagesResult({ From f3fb43197424d4f25b962a7d56b5f2a4c73a6697 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 17 May 2025 17:06:08 -0700 Subject: [PATCH 13/20] msglist [nfc]: Rearrange to follow normal ordering of class members In particular this causes the handful of places where each field of MessageListView needs to appear to all be next to each other. --- lib/model/message_list.dart | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 76f8402541..716cec8b31 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -448,13 +448,19 @@ bool _sameDay(DateTime date1, DateTime date2) { /// * When the object will no longer be used, call [dispose] to free /// resources on the [PerAccountStore]. class MessageListView with ChangeNotifier, _MessageSequence { - MessageListView._({required this.store, required this.narrow}); - factory MessageListView.init( {required PerAccountStore store, required Narrow narrow}) { - final view = MessageListView._(store: store, narrow: narrow); - store.registerMessageList(view); - return view; + return MessageListView._(store: store, narrow: narrow) + .._register(); + } + + MessageListView._({required this.store, required this.narrow}); + + final PerAccountStore store; + Narrow narrow; + + void _register() { + store.registerMessageList(this); } @override @@ -463,9 +469,6 @@ class MessageListView with ChangeNotifier, _MessageSequence { super.dispose(); } - final PerAccountStore store; - Narrow narrow; - /// Whether [message] should actually appear in this message list, /// given that it does belong to the narrow. /// From 14a6695934bd949ca53fcf2f3cd59d86e4924dfd Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 8 May 2025 16:57:26 -0700 Subject: [PATCH 14/20] msglist [nfc]: Document narrow field; make setter private Even if the reader is already sure that the field doesn't get mutated from outside this file, giving it a different name from the getter is useful for seeing exactly where it does get mutated: now one can look at the references to `_narrow`, and see the mutation sites without having them intermingled with all the sites that just read it. --- lib/model/message_list.dart | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 716cec8b31..d9f940cb80 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -454,10 +454,16 @@ class MessageListView with ChangeNotifier, _MessageSequence { .._register(); } - MessageListView._({required this.store, required this.narrow}); + MessageListView._({required this.store, required Narrow narrow}) + : _narrow = narrow; final PerAccountStore store; - Narrow narrow; + + /// The narrow shown in this message list. + /// + /// This can change over time, notably if showing a topic that gets moved. + Narrow get narrow => _narrow; + Narrow _narrow; void _register() { store.registerMessageList(this); @@ -601,9 +607,9 @@ class MessageListView with ChangeNotifier, _MessageSequence { // This can't be a redirect; a redirect can't produce an empty result. // (The server only redirects if the message is accessible to the user, // and if it is, it'll appear in the result, making it non-empty.) - this.narrow = narrow.sansWith(); + _narrow = narrow.sansWith(); case StreamMessage(): - this.narrow = TopicNarrow.ofMessage(someFetchedMessageOrNull); + _narrow = TopicNarrow.ofMessage(someFetchedMessageOrNull); case DmMessage(): // TODO(log) assert(false); } @@ -786,7 +792,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { switch (propagateMode) { case PropagateMode.changeAll: case PropagateMode.changeLater: - narrow = newNarrow; + _narrow = newNarrow; _reset(); fetchInitial(); case PropagateMode.changeOne: From 44b49be72d982f613f5ad42ca1ea7b3e90ef0b3e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 8 May 2025 16:27:04 -0700 Subject: [PATCH 15/20] msglist: Send positive numAfter for fetchInitial This is effectively NFC given normal server behavior. In particular, the Zulip server is smart enough to skip doing any actual work to fetch later messages when the anchor is already `newest`. When we start passing anchors other than `newest`, we'll need this. --- lib/model/message_list.dart | 2 +- test/model/message_list_test.dart | 2 +- test/widgets/message_list_test.dart | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index d9f940cb80..e1334ca060 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -561,7 +561,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { narrow: narrow.apiEncode(), anchor: AnchorCode.newest, numBefore: kMessageListFetchBatchSize, - numAfter: 0, + numAfter: kMessageListFetchBatchSize, allowEmptyTopicName: true, ); if (this.generation > generation) return; diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 05c3e58550..79b5ddb180 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -157,7 +157,7 @@ void main() { narrow: narrow.apiEncode(), anchor: 'newest', numBefore: kMessageListFetchBatchSize, - numAfter: 0, + numAfter: kMessageListFetchBatchSize, allowEmptyTopicName: true, ); } diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 81cc384ef8..3b3b01b323 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -404,7 +404,7 @@ void main() { 'narrow': jsonEncode(narrow.apiEncode()), 'anchor': AnchorCode.newest.toJson(), 'num_before': kMessageListFetchBatchSize.toString(), - 'num_after': '0', + 'num_after': kMessageListFetchBatchSize.toString(), 'allow_empty_topic_name': 'true', }); }); @@ -437,7 +437,7 @@ void main() { 'narrow': jsonEncode(narrow.apiEncode()), 'anchor': AnchorCode.newest.toJson(), 'num_before': kMessageListFetchBatchSize.toString(), - 'num_after': '0', + 'num_after': kMessageListFetchBatchSize.toString(), 'allow_empty_topic_name': 'true', }); }); From ce98562167fe8b50c8063beab626882a69e86afc Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 8 May 2025 16:38:13 -0700 Subject: [PATCH 16/20] msglist: Make initial fetch from any anchor, in model This is NFC as to the live app, because we continue to always set the anchor to AnchorCode.newest there. --- lib/model/message_list.dart | 50 ++++++++++++----- lib/widgets/message_list.dart | 6 ++- test/model/message_list_test.dart | 90 ++++++++++++++++++++++++------- 3 files changed, 112 insertions(+), 34 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index e1334ca060..6b57fe39b1 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -14,6 +14,8 @@ import 'message.dart'; import 'narrow.dart'; import 'store.dart'; +export '../api/route/messages.dart' show Anchor, AnchorCode, NumericAnchor; + /// The number of messages to fetch in each request. const kMessageListFetchBatchSize = 100; // TODO tune @@ -127,9 +129,6 @@ mixin _MessageSequence { /// Whether we know we have the newest messages for this narrow. /// - /// (Currently this is always true once [fetched] is true, - /// because we start from the newest.) - /// /// See also [haveOldest]. bool get haveNewest => _haveNewest; bool _haveNewest = false; @@ -448,14 +447,20 @@ bool _sameDay(DateTime date1, DateTime date2) { /// * When the object will no longer be used, call [dispose] to free /// resources on the [PerAccountStore]. class MessageListView with ChangeNotifier, _MessageSequence { - factory MessageListView.init( - {required PerAccountStore store, required Narrow narrow}) { - return MessageListView._(store: store, narrow: narrow) + factory MessageListView.init({ + required PerAccountStore store, + required Narrow narrow, + Anchor anchor = AnchorCode.newest, // TODO(#82): make required, for explicitness + }) { + return MessageListView._(store: store, narrow: narrow, anchor: anchor) .._register(); } - MessageListView._({required this.store, required Narrow narrow}) - : _narrow = narrow; + MessageListView._({ + required this.store, + required Narrow narrow, + required Anchor anchor, + }) : _narrow = narrow, _anchor = anchor; final PerAccountStore store; @@ -465,6 +470,17 @@ class MessageListView with ChangeNotifier, _MessageSequence { Narrow get narrow => _narrow; Narrow _narrow; + /// The anchor point this message list starts from in the message history. + /// + /// This is passed to the server in the get-messages request + /// sent by [fetchInitial]. + /// That includes not only the original [fetchInitial] call made by + /// the message-list widget, but any additional [fetchInitial] calls + /// which might be made internally by this class in order to + /// fetch the messages from scratch, e.g. after certain events. + Anchor get anchor => _anchor; + final Anchor _anchor; + void _register() { store.registerMessageList(this); } @@ -550,8 +566,6 @@ class MessageListView with ChangeNotifier, _MessageSequence { /// Fetch messages, starting from scratch. Future fetchInitial() async { - // TODO(#80): fetch from anchor firstUnread, instead of newest - // TODO(#82): fetch from a given message ID as anchor assert(!fetched && !haveOldest && !haveNewest && !busyFetchingMore); assert(messages.isEmpty && contents.isEmpty); _setStatus(FetchingStatus.fetchInitial, was: FetchingStatus.unstarted); @@ -559,7 +573,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { final generation = this.generation; final result = await getMessages(store.connection, narrow: narrow.apiEncode(), - anchor: AnchorCode.newest, + anchor: anchor, numBefore: kMessageListFetchBatchSize, numAfter: kMessageListFetchBatchSize, allowEmptyTopicName: true, @@ -571,12 +585,20 @@ class MessageListView with ChangeNotifier, _MessageSequence { store.reconcileMessages(result.messages); store.recentSenders.handleMessages(result.messages); // TODO(#824) - // We'll make the bottom slice start at the last visible message, if any. + // The bottom slice will start at the "anchor message". + // This is the first visible message at or past [anchor] if any, + // else the last visible message if any. [reachedAnchor] helps track that. + bool reachedAnchor = false; for (final message in result.messages) { if (!_messageVisible(message)) continue; - middleMessage = messages.length; + if (!reachedAnchor) { + // Push the previous message into the top slice. + middleMessage = messages.length; + // We could interpret [anchor] for ourselves; but the server has already + // done that work, reducing it to an int, `result.anchor`. So use that. + reachedAnchor = message.id >= result.anchor; + } _addMessage(message); - // Now [middleMessage] is the last message (the one just added). } _haveOldest = result.foundOldest; _haveNewest = result.foundNewest; diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index d0862cb1d7..542d0a52b2 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -517,7 +517,11 @@ class _MessageListState extends State with PerAccountStoreAwareStat } void _initModel(PerAccountStore store) { - _model = MessageListView.init(store: store, narrow: widget.narrow); + // TODO(#82): get anchor as page/route argument, instead of using newest + // TODO(#80): default to anchor firstUnread, instead of newest + final anchor = AnchorCode.newest; + _model = MessageListView.init(store: store, + narrow: widget.narrow, anchor: anchor); model.addListener(_modelChanged); model.fetchInitial(); } diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 79b5ddb180..c5d1a65c2e 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -67,7 +67,10 @@ void main() { void checkNotifiedOnce() => checkNotified(count: 1); /// Initialize [model] and the rest of the test state. - Future prepare({Narrow narrow = const CombinedFeedNarrow()}) async { + Future prepare({ + Narrow narrow = const CombinedFeedNarrow(), + Anchor anchor = AnchorCode.newest, + }) async { final stream = eg.stream(streamId: eg.defaultStreamMessageStreamId); subscription = eg.subscription(stream); store = eg.store(); @@ -75,7 +78,7 @@ void main() { await store.addSubscription(subscription); connection = store.connection as FakeApiConnection; notifiedCount = 0; - model = MessageListView.init(store: store, narrow: narrow) + model = MessageListView.init(store: store, narrow: narrow, anchor: anchor) ..addListener(() { checkInvariants(model); notifiedCount++; @@ -88,11 +91,18 @@ void main() { /// /// The test case must have already called [prepare] to initialize the state. Future prepareMessages({ - required bool foundOldest, + bool? foundOldest, + bool? foundNewest, + int? anchorMessageId, required List messages, }) async { - connection.prepare(json: - newestResult(foundOldest: foundOldest, messages: messages).toJson()); + final result = eg.getMessagesResult( + anchor: model.anchor == AnchorCode.firstUnread + ? NumericAnchor(anchorMessageId!) : model.anchor, + foundOldest: foundOldest, + foundNewest: foundNewest, + messages: messages); + connection.prepare(json: result.toJson()); await model.fetchInitial(); checkNotifiedOnce(); } @@ -187,11 +197,7 @@ void main() { }); test('early in history', () async { - // For now, this gets a response that isn't realistic for the - // request it sends, to simulate when we start sending requests - // that would make this response realistic. - // TODO(#82): send appropriate fetch request - await prepare(); + await prepare(anchor: NumericAnchor(1000)); connection.prepare(json: nearResult( anchor: 1000, foundOldest: true, foundNewest: false, messages: List.generate(111, (i) => eg.streamMessage(id: 990 + i)), @@ -219,6 +225,26 @@ void main() { ..haveNewest.isTrue(); }); + group('sends proper anchor', () { + Future checkFetchWithAnchor(Anchor anchor) async { + await prepare(anchor: anchor); + // This prepared response isn't entirely realistic, depending on the anchor. + // That's OK; these particular tests don't use the details of the response. + connection.prepare(json: + newestResult(foundOldest: true, messages: []).toJson()); + await model.fetchInitial(); + checkNotifiedOnce(); + check(connection.lastRequest).isA() + .url.queryParameters['anchor'] + .equals(anchor.toJson()); + } + + test('oldest', () => checkFetchWithAnchor(AnchorCode.oldest)); + test('firstUnread', () => checkFetchWithAnchor(AnchorCode.firstUnread)); + test('newest', () => checkFetchWithAnchor(AnchorCode.newest)); + test('numeric', () => checkFetchWithAnchor(NumericAnchor(12345))); + }); + // TODO(#824): move this test test('recent senders track all the messages', () async { const narrow = CombinedFeedNarrow(); @@ -441,13 +467,10 @@ void main() { test('while in mid-history', () async { final stream = eg.stream(); - await prepare(narrow: ChannelNarrow(stream.streamId)); - connection.prepare(json: nearResult( - anchor: 1000, foundOldest: true, foundNewest: false, - messages: List.generate(30, - (i) => eg.streamMessage(id: 1000 + i, stream: stream))).toJson()); - await model.fetchInitial(); - checkNotifiedOnce(); + await prepare(narrow: ChannelNarrow(stream.streamId), + anchor: NumericAnchor(1000)); + await prepareMessages(foundOldest: true, foundNewest: false, messages: + List.generate(30, (i) => eg.streamMessage(id: 1000 + i, stream: stream))); check(model).messages.length.equals(30); await store.addMessage(eg.streamMessage(stream: stream)); @@ -1711,8 +1734,9 @@ void main() { ..middleMessage.equals(0); }); - test('on fetchInitial not empty', () async { - await prepare(narrow: const CombinedFeedNarrow()); + test('on fetchInitial, anchor past end', () async { + await prepare(narrow: const CombinedFeedNarrow(), + anchor: AnchorCode.newest); final stream1 = eg.stream(); final stream2 = eg.stream(); await store.addStreams([stream1, stream2]); @@ -1735,6 +1759,34 @@ void main() { .equals(messages[messages.length - 2].id); }); + test('on fetchInitial, anchor in middle', () async { + final s1 = eg.stream(); + final s2 = eg.stream(); + final messages = [ + eg.streamMessage(id: 1, stream: s1), eg.streamMessage(id: 2, stream: s2), + eg.streamMessage(id: 3, stream: s1), eg.streamMessage(id: 4, stream: s2), + eg.streamMessage(id: 5, stream: s1), eg.streamMessage(id: 6, stream: s2), + eg.streamMessage(id: 7, stream: s1), eg.streamMessage(id: 8, stream: s2), + ]; + final anchorId = 4; + + await prepare(narrow: const CombinedFeedNarrow(), + anchor: NumericAnchor(anchorId)); + await store.addStreams([s1, s2]); + await store.addSubscription(eg.subscription(s1)); + await store.addSubscription(eg.subscription(s2, isMuted: true)); + await prepareMessages(foundOldest: true, foundNewest: true, + messages: messages); + // The anchor message is the first visible message with ID at least anchorId… + check(model) + ..messages[model.middleMessage - 1].id.isLessThan(anchorId) + ..messages[model.middleMessage].id.isGreaterOrEqual(anchorId); + // … even though a non-visible message actually had anchorId itself. + check(messages[3].id) + ..equals(anchorId) + ..isLessThan(model.messages[model.middleMessage].id); + }); + /// Like [prepareMessages], but arrange for the given top and bottom slices. Future prepareMessageSplit(List top, List bottom, { bool foundOldest = true, From 4fc68620841d99e0a3d89b9b905524ce494015a5 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 17 May 2025 12:47:04 -0700 Subject: [PATCH 17/20] msglist [nfc]: Cut default for MessageListView.anchor There's no value that's a natural default for this at a model level: different UI scenarios will use different values. So require callers to be explicit. --- lib/model/message_list.dart | 2 +- test/model/message_list_test.dart | 9 ++++++--- test/model/message_test.dart | 5 +++-- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 6b57fe39b1..349a0f8dd5 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -450,7 +450,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { factory MessageListView.init({ required PerAccountStore store, required Narrow narrow, - Anchor anchor = AnchorCode.newest, // TODO(#82): make required, for explicitness + required Anchor anchor, }) { return MessageListView._(store: store, narrow: narrow, anchor: anchor) .._register(); diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index c5d1a65c2e..cad01baf7e 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -1381,12 +1381,14 @@ void main() { int notifiedCount1 = 0; final model1 = MessageListView.init(store: store, - narrow: ChannelNarrow(stream.streamId)) + narrow: ChannelNarrow(stream.streamId), + anchor: AnchorCode.newest) ..addListener(() => notifiedCount1++); int notifiedCount2 = 0; final model2 = MessageListView.init(store: store, - narrow: eg.topicNarrow(stream.streamId, 'hello')) + narrow: eg.topicNarrow(stream.streamId, 'hello'), + anchor: AnchorCode.newest) ..addListener(() => notifiedCount2++); for (final m in [model1, model2]) { @@ -1426,7 +1428,8 @@ void main() { await store.handleEvent(mkEvent(message)); // init msglist *after* event was handled - model = MessageListView.init(store: store, narrow: const CombinedFeedNarrow()); + model = MessageListView.init(store: store, + narrow: const CombinedFeedNarrow(), anchor: AnchorCode.newest); checkInvariants(model); connection.prepare(json: diff --git a/test/model/message_test.dart b/test/model/message_test.dart index 7dff077b1d..0d28ed7dc0 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -51,7 +51,6 @@ void main() { /// Initialize [store] and the rest of the test state. Future prepare({ - Narrow narrow = const CombinedFeedNarrow(), ZulipStream? stream, int? zulipFeatureLevel, }) async { @@ -64,7 +63,9 @@ void main() { await store.addSubscription(subscription); connection = store.connection as FakeApiConnection; notifiedCount = 0; - messageList = MessageListView.init(store: store, narrow: narrow) + messageList = MessageListView.init(store: store, + narrow: const CombinedFeedNarrow(), + anchor: AnchorCode.newest) ..addListener(() { notifiedCount++; }); From 21d0d5e0d8276249009330309ac2ae1387778d8c Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 17 May 2025 12:18:11 -0700 Subject: [PATCH 18/20] msglist test [nfc]: Simplify a bit by cutting redundant default narrow --- test/model/message_list_test.dart | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index cad01baf7e..80be2235c3 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -322,8 +322,7 @@ void main() { }); test('nop when already fetching', () async { - const narrow = CombinedFeedNarrow(); - await prepare(narrow: narrow); + await prepare(); await prepareMessages(foundOldest: false, messages: List.generate(100, (i) => eg.streamMessage(id: 1000 + i))); @@ -351,7 +350,7 @@ void main() { }); test('nop when already haveOldest true', () async { - await prepare(narrow: const CombinedFeedNarrow()); + await prepare(); await prepareMessages(foundOldest: true, messages: List.generate(30, (i) => eg.streamMessage())); check(model) @@ -370,7 +369,7 @@ 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()); - await prepare(narrow: const CombinedFeedNarrow()); + await prepare(); await prepareMessages(foundOldest: false, messages: initialMessages); check(connection.takeRequests()).single; @@ -400,8 +399,7 @@ void main() { })); test('handles servers not understanding includeAnchor', () async { - const narrow = CombinedFeedNarrow(); - await prepare(narrow: narrow); + await prepare(); await prepareMessages(foundOldest: false, messages: List.generate(100, (i) => eg.streamMessage(id: 1000 + i))); @@ -419,8 +417,7 @@ void main() { // TODO(#824): move this test test('recent senders track all the messages', () async { - const narrow = CombinedFeedNarrow(); - await prepare(narrow: narrow); + await prepare(); final initialMessages = List.generate(10, (i) => eg.streamMessage(id: 100 + i)); await prepareMessages(foundOldest: false, messages: initialMessages); From 2f69e7c2025945c9061b1bc2f8f567d686a80d0f Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 1 May 2025 19:49:35 -0700 Subject: [PATCH 19/20] msglist [nfc]: Factor out _fetchMore from fetchOlder --- lib/model/message_list.dart | 53 ++++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 19 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 349a0f8dd5..f2872170cf 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -649,10 +649,39 @@ class MessageListView with ChangeNotifier, _MessageSequence { if (haveOldest) return; if (busyFetchingMore) return; assert(fetched); + assert(messages.isNotEmpty); + await _fetchMore( + anchor: NumericAnchor(messages[0].id), + numBefore: kMessageListFetchBatchSize, + numAfter: 0, + processResult: (result) { + if (result.messages.isNotEmpty + && result.messages.last.id == messages[0].id) { + // TODO(server-6): includeAnchor should make this impossible + result.messages.removeLast(); + } + + 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; + }); + } + + Future _fetchMore({ + required Anchor anchor, + required int numBefore, + required int numAfter, + 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); - assert(messages.isNotEmpty); _setStatus(FetchingStatus.fetchingMore, was: FetchingStatus.idle); final generation = this.generation; bool hasFetchError = false; @@ -661,10 +690,10 @@ class MessageListView with ChangeNotifier, _MessageSequence { try { result = await getMessages(store.connection, narrow: narrow.apiEncode(), - anchor: NumericAnchor(messages[0].id), + anchor: anchor, includeAnchor: false, - numBefore: kMessageListFetchBatchSize, - numAfter: 0, + numBefore: numBefore, + numAfter: numAfter, allowEmptyTopicName: true, ); } catch (e) { @@ -673,21 +702,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { } if (this.generation > generation) return; - if (result.messages.isNotEmpty - && result.messages.last.id == messages[0].id) { - // TODO(server-6): includeAnchor should make this impossible - result.messages.removeLast(); - } - - 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; + processResult(result); } finally { if (this.generation == generation) { if (hasFetchError) { From 7820fd9bbfc7a1021d2fb548012377ed3943fada Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 1 May 2025 19:55:01 -0700 Subject: [PATCH 20/20] msglist: Add fetchNewer method to model This completes the model layer of #82 and #80: the message list can start at an arbitrary anchor, including a numeric message-ID anchor or AnchorCode.firstUnread, and can fetch more history from there in both directions. Still to do is to work that into the widgets layer. This change is therefore NFC as to the live app: nothing calls this method yet. --- lib/model/message_list.dart | 40 ++++++- test/example_data.dart | 18 ++++ test/model/message_list_test.dart | 174 ++++++++++++++++++++++++++---- 3 files changed, 208 insertions(+), 24 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index f2872170cf..2617f18b68 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -78,7 +78,7 @@ enum FetchingStatus { /// and has no outstanding requests or backoff. idle, - /// The model has an active `fetchOlder` request. + /// The model has an active `fetchOlder` or `fetchNewer` request. fetchingMore, /// The model is in a backoff period from a failed request. @@ -673,6 +673,42 @@ 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), + /// 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. + 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) { + if (result.messages.isNotEmpty + && result.messages.first.id == messages.last.id) { + // TODO(server-6): includeAnchor should make this impossible + result.messages.removeAt(0); + } + + store.reconcileMessages(result.messages); + store.recentSenders.handleMessages(result.messages); // TODO(#824) + + for (final message in result.messages) { + if (_messageVisible(message)) { + _addMessage(message); + } + } + _haveNewest = result.foundNewest; + }); + } + Future _fetchMore({ required Anchor anchor, required int numBefore, @@ -775,7 +811,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { // This message list's [messages] doesn't yet reach the new end // of the narrow's message history. (Either [fetchInitial] hasn't yet // completed, or if it has then it was in the middle of history and no - // subsequent fetch has reached the end.) + // subsequent [fetchNewer] has reached the end.) // So this still-newer message doesn't belong. // Leave it to be found by a subsequent fetch when appropriate. // TODO mitigate this fetch/event race: save message to add to list later, diff --git a/test/example_data.dart b/test/example_data.dart index e3316e1218..79b92bdda8 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -687,6 +687,24 @@ GetMessagesResult olderGetMessagesResult({ ); } +/// A GetMessagesResult the server might return when we request newer messages. +GetMessagesResult newerGetMessagesResult({ + required int anchor, + bool foundAnchor = false, // the value if the server understood includeAnchor false + required bool foundNewest, + bool historyLimited = false, + required List messages, +}) { + return GetMessagesResult( + anchor: anchor, + foundAnchor: foundAnchor, + foundOldest: false, + foundNewest: foundNewest, + historyLimited: historyLimited, + messages: messages, + ); +} + int _nextLocalMessageId = 1; StreamOutboxMessage streamOutboxMessage({ diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 80be2235c3..d58de664d8 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -28,6 +28,7 @@ import 'test_store.dart'; const newestResult = eg.newestGetMessagesResult; const nearResult = eg.nearGetMessagesResult; const olderResult = eg.olderGetMessagesResult; +const newerResult = eg.newerGetMessagesResult; void main() { // Arrange for errors caught within the Flutter framework to be printed @@ -291,8 +292,8 @@ void main() { }); }); - group('fetchOlder', () { - test('smoke', () async { + group('fetching more', () { + test('fetchOlder smoke', () async { const narrow = CombinedFeedNarrow(); await prepare(narrow: narrow); await prepareMessages(foundOldest: false, @@ -321,14 +322,43 @@ void main() { ); }); - test('nop when already fetching', () async { - await prepare(); - await prepareMessages(foundOldest: false, + test('fetchNewer smoke', () async { + const narrow = CombinedFeedNarrow(); + await prepare(narrow: narrow, anchor: NumericAnchor(1000)); + await prepareMessages(foundOldest: true, foundNewest: false, messages: List.generate(100, (i) => eg.streamMessage(id: 1000 + i))); + connection.prepare(json: newerResult( + anchor: 1099, foundNewest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 1100 + i)), + ).toJson()); + final fetchFuture = model.fetchNewer(); + checkNotifiedOnce(); + check(model).busyFetchingMore.isTrue(); + + await fetchFuture; + checkNotifiedOnce(); + check(model) + ..busyFetchingMore.isFalse() + ..messages.length.equals(200); + checkLastRequest( + narrow: narrow.apiEncode(), + anchor: '1099', + includeAnchor: false, + numBefore: 0, + numAfter: kMessageListFetchBatchSize, + allowEmptyTopicName: true, + ); + }); + + test('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))); + connection.prepare(json: olderResult( - anchor: 1000, foundOldest: false, - messages: List.generate(100, (i) => eg.streamMessage(id: 900 + i)), + anchor: 900, foundOldest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 800 + i)), ).toJson()); final fetchFuture = model.fetchOlder(); checkNotifiedOnce(); @@ -338,24 +368,56 @@ void main() { final fetchFuture2 = model.fetchOlder(); checkNotNotified(); check(model).busyFetchingMore.isTrue(); + final fetchFuture3 = model.fetchNewer(); + checkNotNotified(); + check(model)..busyFetchingMore.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(200); + check(model)..busyFetchingMore.isFalse()..messages.length.equals(301); }); - test('nop when already haveOldest true', () async { - await prepare(); - await prepareMessages(foundOldest: true, messages: - List.generate(30, (i) => eg.streamMessage())); + test('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))); + + 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).busyFetchingMore.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); + + 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); + }); + + test('fetchOlder nop when already haveOldest true', () async { + await prepare(anchor: NumericAnchor(1000)); + await prepareMessages(foundOldest: true, foundNewest: false, messages: + List.generate(151, (i) => eg.streamMessage(id: 950 + i))); check(model) ..haveOldest.isTrue() - ..messages.length.equals(30); + ..messages.length.equals(151); await model.fetchOlder(); // We must not have made a request, because we didn't @@ -363,14 +425,33 @@ void main() { checkNotNotified(); check(model) ..haveOldest.isTrue() - ..messages.length.equals(30); + ..messages.length.equals(151); + }); + + test('fetchNewer nop when already haveNewest true', () async { + await prepare(anchor: NumericAnchor(1000)); + await prepareMessages(foundOldest: false, foundNewest: true, messages: + List.generate(151, (i) => eg.streamMessage(id: 950 + i))); + check(model) + ..haveNewest.isTrue() + ..messages.length.equals(151); + + await model.fetchNewer(); + // We must not have made a request, because we didn't + // prepare a response and didn't get an exception. + checkNotNotified(); + check(model) + ..haveNewest.isTrue() + ..messages.length.equals(151); }); test('nop during backoff', () => awaitFakeAsync((async) async { final olderMessages = List.generate(5, (i) => eg.streamMessage()); final initialMessages = List.generate(5, (i) => eg.streamMessage()); - await prepare(); - await prepareMessages(foundOldest: false, messages: initialMessages); + final newerMessages = List.generate(5, (i) => eg.streamMessage()); + await prepare(anchor: NumericAnchor(initialMessages[2].id)); + await prepareMessages(foundOldest: false, foundNewest: false, + messages: initialMessages); check(connection.takeRequests()).single; connection.prepare(apiException: eg.apiBadRequest()); @@ -385,20 +466,31 @@ void main() { check(model).busyFetchingMore.isTrue(); check(connection.lastRequest).isNull(); + await model.fetchNewer(); + checkNotNotified(); + check(model).busyFetchingMore.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(); checkNotifiedOnce(); check(connection.lastRequest).isNull(); - connection.prepare(json: olderResult( - anchor: 1000, foundOldest: false, messages: olderMessages).toJson()); + connection.prepare(json: olderResult(anchor: initialMessages.first.id, + foundOldest: false, messages: olderMessages).toJson()); await model.fetchOlder(); checkNotified(count: 2); check(connection.takeRequests()).single; + + connection.prepare(json: newerResult(anchor: initialMessages.last.id, + foundNewest: false, messages: newerMessages).toJson()); + await model.fetchNewer(); + checkNotified(count: 2); + check(connection.takeRequests()).single; })); - test('handles servers not understanding includeAnchor', () async { + test('fetchOlder handles servers not understanding includeAnchor', () async { await prepare(); await prepareMessages(foundOldest: false, messages: List.generate(100, (i) => eg.streamMessage(id: 1000 + i))); @@ -415,8 +507,25 @@ void main() { ..messages.length.equals(200); }); + test('fetchNewer handles servers not understanding includeAnchor', () async { + await prepare(anchor: NumericAnchor(1000)); + await prepareMessages(foundOldest: true, foundNewest: false, + messages: List.generate(101, (i) => eg.streamMessage(id: 1000 + i))); + + // The old behavior is to include the anchor message regardless of includeAnchor. + connection.prepare(json: newerResult( + anchor: 1100, foundNewest: false, foundAnchor: true, + messages: List.generate(101, (i) => eg.streamMessage(id: 1100 + i)), + ).toJson()); + await model.fetchNewer(); + checkNotified(count: 2); + check(model) + ..busyFetchingMore.isFalse() + ..messages.length.equals(201); + }); + // TODO(#824): move this test - test('recent senders track all the messages', () async { + test('fetchOlder recent senders track all the messages', () async { await prepare(); final initialMessages = List.generate(10, (i) => eg.streamMessage(id: 100 + i)); await prepareMessages(foundOldest: false, messages: initialMessages); @@ -434,6 +543,27 @@ void main() { recent_senders_test.checkMatchesMessages(store.recentSenders, [...initialMessages, ...oldMessages]); }); + + // 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)); + await prepareMessages(foundOldest: true, foundNewest: false, + messages: initialMessages); + + final newMessages = List.generate(10, (i) => eg.streamMessage(id: 110 + i)) + // Not subscribed to the stream with id 10. + ..add(eg.streamMessage(id: 120, 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); + recent_senders_test.checkMatchesMessages(store.recentSenders, + [...initialMessages, ...newMessages]); + }); }); group('MessageEvent', () {