diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 8022ba8a7d..2617f18b68 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 @@ -64,6 +66,25 @@ class MessageListMessageItem extends MessageListMessageBaseItem { }); } +/// The status of outstanding or recent fetch requests from a [MessageListView]. +enum FetchingStatus { + /// The model has not made any fetch requests (since its last reset, if any). + 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. + idle, + + /// The model has an active `fetchOlder` or `fetchNewer` request. + fetchingMore, + + /// 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. /// /// This comprises much of the guts of [MessageListView]. @@ -75,7 +96,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. @@ -95,42 +116,39 @@ 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 => switch (_status) { + FetchingStatus.unstarted || FetchingStatus.fetchInitial => false, + _ => true, + }; /// 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 are currently fetching the next batch of older messages. + /// Whether we know we have the newest messages for this narrow. /// - /// 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 => _fetchingOlder; - bool _fetchingOlder = false; + /// See also [haveOldest]. + bool get haveNewest => _haveNewest; + bool _haveNewest = false; - /// 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. + /// Whether this message list is currently busy when it comes to + /// fetching more messages. /// - /// "Recently" is decided by a [BackoffMachine] that resets - /// when a [fetchOlder] request succeeds. - /// - /// See also [fetchingOlder]. - bool get fetchOlderCoolingDown => _fetchOlderCoolingDown; - bool _fetchOlderCoolingDown = false; + /// 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.fetchingMore || FetchingStatus.backoff => true, + _ => false, + }; + + FetchingStatus _status = FetchingStatus.unstarted; - BackoffMachine? _fetchOlderCooldownBackoffMachine; + BackoffMachine? _fetchBackoffMachine; /// The parsed message contents, as a list parallel to [messages]. /// @@ -147,7 +165,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], [haveNewest], and [busyFetchingMore]. /// It exists as an optimization, to memoize that computation. /// /// See also [middleItem], an index which divides this list @@ -303,11 +321,10 @@ mixin _MessageSequence { generation += 1; messages.clear(); middleMessage = 0; - _fetched = false; _haveOldest = false; - _fetchingOlder = false; - _fetchOlderCoolingDown = false; - _fetchOlderCooldownBackoffMachine = null; + _haveNewest = false; + _status = FetchingStatus.unstarted; + _fetchBackoffMachine = null; contents.clear(); items.clear(); middleItem = 0; @@ -430,13 +447,42 @@ 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, + required Anchor anchor, + }) { + return MessageListView._(store: store, narrow: narrow, anchor: anchor) + .._register(); + } + + MessageListView._({ + required this.store, + required Narrow narrow, + required Anchor anchor, + }) : _narrow = narrow, _anchor = anchor; - factory MessageListView.init( - {required PerAccountStore store, required Narrow narrow}) { - final view = MessageListView._(store: store, narrow: narrow); - store.registerMessageList(view); - return view; + final PerAccountStore store; + + /// 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; + + /// 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); } @override @@ -445,9 +491,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. /// @@ -514,19 +557,25 @@ 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 && !fetchingOlder && !fetchOlderCoolingDown); + assert(!fetched && !haveOldest && !haveNewest && !busyFetchingMore); assert(messages.isEmpty && contents.isEmpty); + _setStatus(FetchingStatus.fetchInitial, was: FetchingStatus.unstarted); // TODO schedule all this in another isolate final generation = this.generation; final result = await getMessages(store.connection, narrow: narrow.apiEncode(), - anchor: AnchorCode.newest, + anchor: anchor, numBefore: kMessageListFetchBatchSize, - numAfter: 0, + numAfter: kMessageListFetchBatchSize, allowEmptyTopicName: true, ); if (this.generation > generation) return; @@ -536,16 +585,24 @@ 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). } - _fetched = true; _haveOldest = result.foundOldest; - notifyListeners(); + _haveNewest = result.foundNewest; + _setStatus(FetchingStatus.idle, was: FetchingStatus.fetchInitial); } /// Update [narrow] for the result of a "with" narrow (topic permalink) fetch. @@ -572,26 +629,96 @@ 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); } } /// 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(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; + }); + } + + /// 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, + 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); - _fetchingOlder = true; - notifyListeners(); + _setStatus(FetchingStatus.fetchingMore, was: FetchingStatus.idle); final generation = this.generation; bool hasFetchError = false; try { @@ -599,10 +726,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) { @@ -611,37 +738,20 @@ 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) { - _fetchingOlder = false; if (hasFetchError) { - assert(!fetchOlderCoolingDown); - _fetchOlderCoolingDown = true; - unawaited((_fetchOlderCooldownBackoffMachine ??= BackoffMachine()) + _setStatus(FetchingStatus.backoff, was: FetchingStatus.fetchingMore); + unawaited((_fetchBackoffMachine ??= BackoffMachine()) .wait().then((_) { if (this.generation != generation) return; - _fetchOlderCoolingDown = false; - notifyListeners(); + _setStatus(FetchingStatus.idle, was: FetchingStatus.backoff); })); } else { - _fetchOlderCooldownBackoffMachine = null; + _setStatus(FetchingStatus.idle, was: FetchingStatus.fetchingMore); + _fetchBackoffMachine = null; } - notifyListeners(); } } } @@ -697,8 +807,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 [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, + // 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 @@ -747,7 +865,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { switch (propagateMode) { case PropagateMode.changeAll: case PropagateMode.changeLater: - narrow = newNarrow; + _narrow = newNarrow; _reset(); fetchInitial(); case PropagateMode.changeOne: diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index e25445e514..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(); } @@ -686,15 +690,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]; @@ -731,16 +729,26 @@ class _MessageListState extends State with PerAccountStoreAwareStat } Widget _buildStartCap() { - // These assertions are invariants of [MessageListView]. - assert(!(model.fetchingOlder && model.fetchOlderCoolingDown)); - final effectiveFetchingOlder = - model.fetchingOlder || model.fetchOlderCoolingDown; - assert(!(model.haveOldest && effectiveFetchingOlder)); - return switch ((effectiveFetchingOlder, model.haveOldest)) { - (true, _) => const _MessageListLoadingMore(), - (_, true) => const _MessageListHistoryStart(), - (_, _) => const SizedBox.shrink(), - }; + // 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. + return model.haveOldest ? const _MessageListHistoryStart() + : model.busyFetchingMore ? const _MessageListLoadingMore() + : const SizedBox.shrink(); + } + + 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) { diff --git a/test/example_data.dart b/test/example_data.dart index b87cbb6dc8..79b92bdda8 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -589,20 +589,81 @@ DmMessage dmMessage({ }) as Map); } -/// A GetMessagesResult the server might return on an `anchor=newest` request. +/// 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, +}) { + 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( - // 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, + 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({ + 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, ); @@ -626,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 11f2b3056b..d58de664d8 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -26,7 +26,9 @@ 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; +const newerResult = eg.newerGetMessagesResult; void main() { // Arrange for errors caught within the Flutter framework to be printed @@ -66,7 +68,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(); @@ -74,7 +79,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++; @@ -87,11 +92,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(); } @@ -150,12 +162,13 @@ void main() { checkNotifiedOnce(); check(model) ..messages.length.equals(kMessageListFetchBatchSize) - ..haveOldest.isFalse(); + ..haveOldest.isFalse() + ..haveNewest.isTrue(); checkLastRequest( narrow: narrow.apiEncode(), anchor: 'newest', numBefore: kMessageListFetchBatchSize, - numAfter: 0, + numAfter: kMessageListFetchBatchSize, allowEmptyTopicName: true, ); } @@ -180,7 +193,22 @@ void main() { checkNotifiedOnce(); check(model) ..messages.length.equals(30) - ..haveOldest.isTrue(); + ..haveOldest.isTrue() + ..haveNewest.isTrue(); + }); + + test('early in history', () async { + 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)), + ).toJson()); + await model.fetchInitial(); + checkNotifiedOnce(); + check(model) + ..messages.length.equals(111) + ..haveOldest.isTrue() + ..haveNewest.isFalse(); }); test('no messages found', () async { @@ -194,7 +222,28 @@ void main() { check(model) ..fetched.isTrue() ..messages.isEmpty() - ..haveOldest.isTrue(); + ..haveOldest.isTrue() + ..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 @@ -243,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, @@ -256,12 +305,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(), @@ -273,42 +322,102 @@ void main() { ); }); - test('nop when already fetching', () async { + test('fetchNewer smoke', () async { const narrow = CombinedFeedNarrow(); - await prepare(narrow: narrow); - await prepareMessages(foundOldest: false, + 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(); - 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(); + 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) - ..fetchingOlder.isFalse() - ..messages.length.equals(200); + check(model)..busyFetchingMore.isFalse()..messages.length.equals(301); }); - test('nop when already haveOldest true', () async { - await prepare(narrow: const CombinedFeedNarrow()); - 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 @@ -316,45 +425,73 @@ 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(narrow: const CombinedFeedNarrow()); - 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()); 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(); + + 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).fetchOlderCoolingDown.isFalse(); + 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 { - const narrow = CombinedFeedNarrow(); - await prepare(narrow: narrow); + test('fetchOlder handles servers not understanding includeAnchor', () async { + await prepare(); await prepareMessages(foundOldest: false, messages: List.generate(100, (i) => eg.streamMessage(id: 1000 + i))); @@ -366,14 +503,30 @@ void main() { await model.fetchOlder(); checkNotified(count: 2); check(model) - ..fetchingOlder.isFalse() + ..busyFetchingMore.isFalse() ..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 { - const narrow = CombinedFeedNarrow(); - await prepare(narrow: narrow); + 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); @@ -390,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', () { @@ -418,6 +592,19 @@ void main() { check(model).messages.length.equals(30); }); + test('while in mid-history', () async { + final stream = eg.stream(); + 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)); + checkNotNotified(); + check(model).messages.length.equals(30); + }); + test('before fetch', () async { final stream = eg.stream(); await prepare(narrow: ChannelNarrow(stream.streamId)); @@ -1068,7 +1255,7 @@ void main() { messages: olderMessages, ).toJson()); final fetchFuture = model.fetchOlder(); - check(model).fetchingOlder.isTrue(); + check(model).busyFetchingMore.isTrue(); checkHasMessages(initialMessages); checkNotifiedOnce(); @@ -1081,7 +1268,7 @@ void main() { origStreamId: otherStream.streamId, newMessages: movedMessages, )); - check(model).fetchingOlder.isFalse(); + check(model).busyFetchingMore.isFalse(); checkHasMessages([]); checkNotifiedOnce(); @@ -1104,7 +1291,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 +1304,7 @@ void main() { newMessages: movedMessages, )); checkHasMessages([]); - check(model).fetchingOlder.isFalse(); + check(model).busyFetchingMore.isFalse(); checkNotifiedOnce(); async.elapse(const Duration(seconds: 1)); @@ -1138,7 +1325,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 +1343,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 +1454,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 +1467,7 @@ void main() { newMessages: movedMessages, )); checkHasMessages([]); - check(model).fetchingOlder.isFalse(); + check(model).busyFetchingMore.isFalse(); checkNotifiedOnce(); async.elapse(const Duration(seconds: 1)); @@ -1293,19 +1480,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(); })); }); @@ -1321,12 +1508,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]) { @@ -1366,7 +1555,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: @@ -1674,8 +1864,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]); @@ -1698,6 +1889,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, @@ -2140,15 +2359,11 @@ void checkInvariants(MessageListView model) { check(model) ..messages.isEmpty() ..haveOldest.isFalse() - ..fetchingOlder.isFalse() - ..fetchOlderCoolingDown.isFalse(); - } - if (model.haveOldest) { - check(model).fetchingOlder.isFalse(); - check(model).fetchOlderCoolingDown.isFalse(); + ..haveNewest.isFalse() + ..busyFetchingMore.isFalse(); } - if (model.fetchingOlder) { - check(model).fetchOlderCoolingDown.isFalse(); + if (model.haveOldest && model.haveNewest) { + check(model).busyFetchingMore.isFalse(); } for (final message in model.messages) { @@ -2292,6 +2507,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 fetchingOlder => has((x) => x.fetchingOlder, 'fetchingOlder'); - Subject get fetchOlderCoolingDown => has((x) => x.fetchOlderCoolingDown, 'fetchOlderCoolingDown'); + Subject get haveNewest => has((x) => x.haveNewest, 'haveNewest'); + Subject get busyFetchingMore => has((x) => x.busyFetchingMore, 'busyFetchingMore'); } 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++; }); 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', }); });