msglist [nfc]: Unify fetch/cooldown status as busyFetchingMore

Now the distinction between these two states exists only for asserts.

Author: gnprice

lib/model/message_list.dart

@@ -125,28 +125,18 @@ mixin _MessageSequence {
   bool get haveOldest => _haveOldest;
   bool _haveOldest = false;
 
-  /// Whether we are currently fetching the next batch of older messages.
+  /// Whether this message list is currently busy when it comes to
+  /// fetching more 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;
+  /// 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;
 
@@ -167,7 +157,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
@@ -507,7 +497,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
   Future<void> 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;
@@ -573,10 +563,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<void> 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.

lib/widgets/message_list.dart

@@ -691,13 +691,8 @@ class _MessageListState extends State<MessageList> 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();
   }
 

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,20 +285,20 @@ 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;
       // We must not have made another request, because we didn't
       // 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<ZulipApiException>();
       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<ZulipApiException>();
         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<ZulipApiException>();
         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) {
@@ -2281,6 +2275,5 @@ extension MessageListViewChecks on Subject<MessageListView> {
   Subject<int> get middleItem => has((x) => x.middleItem, 'middleItem');
   Subject<bool> get fetched => has((x) => x.fetched, 'fetched');
   Subject<bool> get haveOldest => has((x) => x.haveOldest, 'haveOldest');
-  Subject<bool> get fetchingOlder => has((x) => x.fetchingOlder, 'fetchingOlder');
-  Subject<bool> get fetchOlderCoolingDown => has((x) => x.fetchOlderCoolingDown, 'fetchOlderCoolingDown');
+  Subject<bool> get busyFetchingMore => has((x) => x.busyFetchingMore, 'busyFetchingMore');
 }