diff --git a/lib/model/algorithms.dart b/lib/model/algorithms.dart new file mode 100644 index 0000000000..37ef1cc986 --- /dev/null +++ b/lib/model/algorithms.dart @@ -0,0 +1,43 @@ + +/// Returns the index in [sortedList] of an element matching the given [key], +/// if there is one. +/// +/// Returns -1 if there is no matching element. +/// +/// The return values of [compare] are interpreted in the same way as for +/// [Comparable.compareTo]: a negative value of `compare(element, key)` means +/// the element goes before the key, zero means the element matches the key, +/// and positive means the element goes after the key. +/// +/// The list [sortedList] must be sorted in the sense that any elements that +/// compare before [key] come before any elements that match [key] or +/// compare after it, and any elements that match [key] come before any +/// elements that compare after [key]. +/// If the list is not sorted, the return value may be arbitrary. +/// +/// Only the portion of the list from [start] to [end] (defaulting to 0 +/// and `sortedList.length` respectively, so to the entire list) is searched, +/// and only that portion need be sorted. +// Based on binarySearchBy in package:collection/src/algorithms.dart . +int binarySearchByKey( + List sortedList, + K key, + int Function(E element, K key) compare, + [int start = 0, int? end] +) { + end = RangeError.checkValidRange(start, end, sortedList.length); + int min = start; + int max = end; + while (min < max) { + final mid = min + ((max - min) >> 1); + final element = sortedList[mid]; + final comp = compare(element, key); + if (comp == 0) return mid; + if (comp < 0) { + min = mid + 1; + } else { + max = mid; + } + } + return -1; +} diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 9ba6d30ab5..b2a9b22ce4 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -1,12 +1,202 @@ +import 'package:collection/collection.dart'; import 'package:flutter/foundation.dart'; import '../api/model/events.dart'; import '../api/model/model.dart'; import '../api/route/messages.dart'; +import 'algorithms.dart'; import 'content.dart'; import 'narrow.dart'; import 'store.dart'; +/// The number of messages to fetch in each request. +const kMessageListFetchBatchSize = 100; // TODO tune + +/// A message, or one of its siblings shown in the message list. +/// +/// See [MessageListView.items], which is a list of these. +sealed class MessageListItem { + const MessageListItem(); +} + +/// A message to show in the message list. +class MessageListMessageItem extends MessageListItem { + final Message message; + final ZulipContent content; + + MessageListMessageItem(this.message, this.content); +} + +/// Indicates the app is loading more messages at the top or bottom. +class MessageListLoadingItem extends MessageListItem { + final MessageListDirection direction; + + const MessageListLoadingItem(this.direction); +} + +enum MessageListDirection { older, newer } + +/// Indicates we've reached the oldest message in the narrow. +class MessageListHistoryStartItem extends MessageListItem { + const MessageListHistoryStartItem(); +} + +/// The sequence of messages in a message list, and how to display them. +/// +/// This comprises much of the guts of [MessageListView]. +mixin _MessageSequence { + /// The messages. + /// + /// See also [contents] and [items]. + final List messages = []; + + /// Whether [messages] and [items] represent the results of a fetch. + /// + /// 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; + + /// 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.) + bool get haveOldest => _haveOldest; + bool _haveOldest = false; + + /// Whether we are currently fetching the next batch of older messages. + bool get fetchingOlder => _fetchingOlder; + bool _fetchingOlder = false; + + /// The parsed message contents, as a list parallel to [messages]. + /// + /// The i'th element is the result of parsing the i'th element of [messages]. + /// + /// This information is completely derived from [messages]. + /// It exists as an optimization, to memoize the work of parsing. + final List contents = []; + + /// The messages and their siblings in the UI, in order. + /// + /// This has a [MessageListMessageItem] corresponding to each element + /// of [messages], in order. It may have additional items interspersed + /// before, between, or after the messages. + /// + /// This information is completely derived from [messages] and + /// the flags [haveOldest] and [fetchingOlder]. + /// It exists as an optimization, to memoize that computation. + final QueueList items = QueueList(); + + int _findMessageWithId(int messageId) { + return binarySearchByKey(messages, messageId, + (message, messageId) => message.id.compareTo(messageId)); + } + + int _findItemWithMessageId(int messageId) { + return binarySearchByKey(items, messageId, _compareItemToMessageId); + } + + static int _compareItemToMessageId(MessageListItem item, int messageId) { + switch (item) { + case MessageListHistoryStartItem(): return -1; + case MessageListLoadingItem(): + switch (item.direction) { + case MessageListDirection.older: return -1; + case MessageListDirection.newer: return 1; + } + case MessageListMessageItem(:var message): return message.id.compareTo(messageId); + } + } + + /// Update data derived from the content of the index-th message. + void _reparseContent(int index) { + final message = messages[index]; + final content = parseContent(message.content); + contents[index] = content; + + final itemIndex = _findItemWithMessageId(message.id); + assert(itemIndex > -1 + && items[itemIndex] is MessageListMessageItem + && identical((items[itemIndex] as MessageListMessageItem).message, message)); + items[itemIndex] = MessageListMessageItem(message, content); + } + + /// Append [message] to [messages], and update derived data accordingly. + /// + /// The caller is responsible for ensuring this is an appropriate thing to do + /// given [narrow], our state of being caught up, and other concerns. + void _addMessage(Message message) { + assert(contents.length == messages.length); + messages.add(message); + contents.add(parseContent(message.content)); + assert(contents.length == messages.length); + _processMessage(messages.length - 1); + } + + void _insertAllMessages(int index, Iterable toInsert) { + // TODO parse/process messages in smaller batches, to not drop frames. + // On a Pixel 5, a batch of 100 messages takes ~15-20ms in _insertAllMessages. + // (Before that, ~2-5ms in jsonDecode and 0ms in fromJson, + // so skip worrying about those steps.) + assert(contents.length == messages.length); + messages.insertAll(index, toInsert); + contents.insertAll(index, toInsert.map( + (message) => parseContent(message.content))); + assert(contents.length == messages.length); + _reprocessAll(); + } + + /// Redo all computations from scratch, based on [messages]. + void _recompute() { + assert(contents.length == messages.length); + contents.clear(); + contents.addAll(messages.map((message) => parseContent(message.content))); + assert(contents.length == messages.length); + _reprocessAll(); + } + + /// Append to [items] based on the index-th message and its content. + /// + /// The previous messages in the list must already have been processed. + /// This message must already have been parsed and reflected in [contents]. + void _processMessage(int index) { + // This will get more complicated to handle the ways that messages interact + // with the display of neighboring messages: sender headings #175, + // recipient headings #174, and date separators #173. + items.add(MessageListMessageItem(messages[index], contents[index])); + } + + /// Update [items] to include markers at start and end as appropriate. + void _updateEndMarkers() { + assert(!(haveOldest && fetchingOlder)); + final startMarker = switch ((fetchingOlder, haveOldest)) { + (true, _) => const MessageListLoadingItem(MessageListDirection.older), + (_, true) => const MessageListHistoryStartItem(), + (_, _) => null, + }; + final hasStartMarker = switch (items.firstOrNull) { + MessageListLoadingItem() => true, + MessageListHistoryStartItem() => true, + _ => false, + }; + switch ((startMarker != null, hasStartMarker)) { + case (true, true): items[0] = startMarker!; + case (true, _ ): items.addFirst(startMarker!); + case (_, true): items.removeFirst(); + case (_, _ ): break; + } + } + + /// Recompute [items] from scratch, based on [messages], [contents], and flags. + void _reprocessAll() { + items.clear(); + for (var i = 0; i < messages.length; i++) { + _processMessage(i); + } + _updateEndMarkers(); + } +} + /// A view-model for a message list. /// /// The owner of one of these objects must call [dispose] when the object @@ -15,15 +205,13 @@ import 'store.dart'; /// Lifecycle: /// * Create with [init]. /// * Add listeners with [addListener]. -/// * Fetch messages with [fetch]. When the fetch completes, this object +/// * Fetch messages with [fetchInitial]. When the fetch completes, this object /// will notify its listeners (as it will any other time the data changes.) +/// * Fetch more messages as needed with [fetchOlder]. /// * On reassemble, call [reassemble]. /// * When the object will no longer be used, call [dispose] to free /// resources on the [PerAccountStore]. -/// -/// TODO: richer fetch method: use narrow, take anchor, support fetching another batch -/// TODO: update on server events -class MessageListView extends ChangeNotifier { +class MessageListView with ChangeNotifier, _MessageSequence { MessageListView._({required this.store, required this.narrow}); factory MessageListView.init( @@ -42,32 +230,59 @@ class MessageListView extends ChangeNotifier { final PerAccountStore store; final Narrow narrow; - final List messages = []; - - /// The parsed message contents, as a list parallel to [messages]. - final List contents = []; - - /// Whether [messages] represents the results of a fetch. - /// - /// TODO this bit of API will get more complex - bool get fetched => _fetched; - bool _fetched = false; - - Future fetch() async { - assert(!fetched); - assert(messages.isEmpty); - assert(contents.isEmpty); + /// 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); + assert(messages.isEmpty && contents.isEmpty); // TODO schedule all this in another isolate final result = await getMessages(store.connection, narrow: narrow.apiEncode(), - anchor: AnchorCode.newest, // TODO(#80): switch to firstUnread - numBefore: 100, - numAfter: 10, + anchor: AnchorCode.newest, + numBefore: kMessageListFetchBatchSize, + numAfter: 0, ); - messages.addAll(result.messages); - contents.addAll(_contentsOfMessages(result.messages)); + for (final message in result.messages) { + _addMessage(message); + } _fetched = true; + _haveOldest = result.foundOldest; + _updateEndMarkers(); + notifyListeners(); + } + + /// Fetch the next batch of older messages, if applicable. + Future fetchOlder() async { + if (haveOldest) return; + if (fetchingOlder) return; + assert(fetched); + assert(messages.isNotEmpty); + _fetchingOlder = true; + _updateEndMarkers(); notifyListeners(); + try { + final result = await getMessages(store.connection, + narrow: narrow.apiEncode(), + anchor: NumericAnchor(messages[0].id), + includeAnchor: false, + numBefore: kMessageListFetchBatchSize, + numAfter: 0, + ); + + if (result.messages.isNotEmpty + && result.messages.last.id == messages[0].id) { + // TODO(server-6): includeAnchor should make this impossible + result.messages.removeLast(); + } + + _insertAllMessages(0, result.messages); + _haveOldest = result.foundOldest; + } finally { + _fetchingOlder = false; + _updateEndMarkers(); + notifyListeners(); + } } /// Add [message] to this view, if it belongs here. @@ -82,12 +297,11 @@ class MessageListView extends ChangeNotifier { return; } // TODO insert in middle instead, when appropriate - messages.add(message); - contents.add(parseContent(message.content)); + _addMessage(message); notifyListeners(); } - _applyChangesToMessage(UpdateMessageEvent event, Message message) { + static void _applyChangesToMessage(UpdateMessageEvent event, Message message) { // TODO(server-5): Cut this fallback; rely on renderingOnly from FL 114 final isRenderingOnly = event.renderingOnly ?? (event.userId == null); if (event.editTimestamp != null && !isRenderingOnly) { @@ -110,28 +324,6 @@ class MessageListView extends ChangeNotifier { } } - // This is almost directly copied from package:collection/src/algorithms.dart - // The way that package was set up doesn't allow us to search - // for a message ID among a bunch of message objects - this is a quick - // modification of that method to work here for us. - @visibleForTesting - int findMessageWithId(int messageId) { - var min = 0; - var max = messages.length; - while (min < max) { - var mid = min + ((max - min) >> 1); - final message = messages[mid]; - var comp = message.id.compareTo(messageId); - if (comp == 0) return mid; - if (comp < 0) { - min = mid + 1; - } else { - max = mid; - } - } - return -1; - } - /// Update the message the given event applies to, if present in this view. /// /// This method only handles the case where the message's contents @@ -139,20 +331,18 @@ class MessageListView extends ChangeNotifier { /// /// TODO(#150): Handle message moves. void maybeUpdateMessage(UpdateMessageEvent event) { - final idx = findMessageWithId(event.messageId); + final idx = _findMessageWithId(event.messageId); if (idx == -1) { return; } - final message = messages[idx]; - _applyChangesToMessage(event, message); - - contents[idx] = parseContent(message.content); + _applyChangesToMessage(event, messages[idx]); + _reparseContent(idx); notifyListeners(); } void maybeUpdateMessageReactions(ReactionEvent event) { - final index = findMessageWithId(event.messageId); + final index = _findMessageWithId(event.messageId); if (index == -1) { return; } @@ -182,16 +372,7 @@ class MessageListView extends ChangeNotifier { /// This will redo from scratch any computations we can, such as parsing /// message contents. It won't repeat network requests. void reassemble() { - contents.clear(); - contents.addAll(_contentsOfMessages(messages)); + _recompute(); notifyListeners(); } - - static Iterable _contentsOfMessages(Iterable messages) { - // This will get more complicated to handle the ways that messages interact - // with the display of neighboring messages: sender headings #175, - // recipient headings #174, and date separators #173. - // TODO factor [messages] and [contents] into own class to encapsulate that - return messages.map((message) => parseContent(message.content)); - } } diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 2779e26d22..060caff32a 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -123,6 +123,19 @@ class MessageListAppBarTitle extends StatelessWidget { } } +/// The approximate height of a short message in the message list. +const _kShortMessageHeight = 80; + +/// The point at which we fetch more history, in pixels from the start or end. +/// +/// When the user scrolls to within this distance of the start (or end) of the +/// history we currently have, we make a request to fetch the next batch of +/// older (or newer) messages. +// +// When the user reaches this point, they're at least halfway through the +// previous batch. +const kFetchMessagesBufferPixels = (kMessageListFetchBatchSize / 2) * _kShortMessageHeight; + class MessageList extends StatefulWidget { const MessageList({super.key, required this.narrow}); @@ -160,7 +173,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat void _initModel(PerAccountStore store) { model = MessageListView.init(store: store, narrow: widget.narrow); model!.addListener(_modelChanged); - model!.fetch(); + model!.fetchInitial(); } void _modelChanged() { @@ -176,6 +189,17 @@ class _MessageListState extends State with PerAccountStoreAwareStat } else { _scrollToBottomVisibleValue.value = true; } + + final extentRemainingAboveViewport = scrollMetrics.maxScrollExtent - scrollMetrics.pixels; + if (extentRemainingAboveViewport < kFetchMessagesBufferPixels) { + // TODO: This ends up firing a second time shortly after we fetch a batch. + // The result is that each time we decide to fetch a batch, we end up + // fetching two batches in quick succession. This is basically harmless + // but makes things a bit more complicated to reason about. + // The cause seems to be that this gets called again with maxScrollExtent + // still not yet updated to account for the newly-added messages. + model?.fetchOlder(); + } } void _scrollChanged() { @@ -221,8 +245,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat } Widget _buildListView(context) { - final length = model!.messages.length; - assert(model!.contents.length == length); + final length = model!.items.length; return StickyHeaderListView.builder( // TODO: Offer `ScrollViewKeyboardDismissBehavior.interactive` (or // similar) if that is ever offered: @@ -244,10 +267,26 @@ class _MessageListState extends State with PerAccountStoreAwareStat // TODO handle scroll starting at first unread, or link anchor // TODO on new message when scrolled up, anchor scroll to what's in view reverse: true, - itemBuilder: (context, i) => MessageItem( - trailing: i == 0 ? const SizedBox(height: 8) : const SizedBox(height: 11), - message: model!.messages[length - 1 - i], - content: model!.contents[length - 1 - i])); + itemBuilder: (context, i) { + final data = model!.items[length - 1 - i]; + return switch (data) { + MessageListHistoryStartItem() => + const Center( + child: Padding( + padding: EdgeInsets.symmetric(vertical: 16.0), + child: Text("No earlier messages."))), // TODO use an icon + MessageListLoadingItem() => + const Center( + child: Padding( + padding: EdgeInsets.symmetric(vertical: 16.0), + child: CircularProgressIndicator())), // TODO perhaps a different indicator + MessageListMessageItem(:var message, :var content) => + MessageItem( + trailing: i == 0 ? const SizedBox(height: 8) : const SizedBox(height: 11), + message: message, + content: content) + }; + }); } } diff --git a/test/model/algorithms_test.dart b/test/model/algorithms_test.dart new file mode 100644 index 0000000000..ad6d4b3ed2 --- /dev/null +++ b/test/model/algorithms_test.dart @@ -0,0 +1,39 @@ + +import 'package:checks/checks.dart'; +import 'package:test/scaffolding.dart'; +import 'package:zulip/model/algorithms.dart'; + +void main() { + group('binarySearchByKey', () { + late List<({int data})> list; + + int search(int key) => binarySearchByKey(list, key, (element, key) => + element.data.compareTo(key)); + + test('empty', () { + list = []; + check(search(1)).equals(-1); + }); + + test('2 elements', () { + list = [(data: 2), (data: 4)]; + check(search(1)).equals(-1); + check(search(2)).equals(0); + check(search(3)).equals(-1); + check(search(4)).equals(1); + check(search(5)).equals(-1); + }); + + test('3 elements', () { + list = [(data: 2), (data: 4), (data: 6)]; + // Exercise the binary search before, at, and after each element of the list. + check(search(1)).equals(-1); + check(search(2)).equals(0); + check(search(3)).equals(-1); + check(search(4)).equals(1); + check(search(5)).equals(-1); + check(search(6)).equals(2); + check(search(7)).equals(-1); + }); + }); +} diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index d0bb1c587f..3c3f51460e 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -26,14 +26,12 @@ void main() async { late MessageListView model; late int notifiedCount; - void checkNotNotified() { - check(notifiedCount).equals(0); - } - - void checkNotifiedOnce() { - check(notifiedCount).equals(1); + void checkNotified({required int count}) { + check(notifiedCount).equals(count); notifiedCount = 0; } + void checkNotNotified() => checkNotified(count: 0); + void checkNotifiedOnce() => checkNotified(count: 1); /// Initialize [model] and the rest of the test state. void prepare({Narrow narrow = const AllMessagesNarrow()}) { @@ -59,7 +57,7 @@ void main() async { }) async { connection.prepare(json: newestResult(foundOldest: foundOldest, messages: messages).toJson()); - await model.fetch(); + await model.fetchInitial(); checkNotifiedOnce(); } @@ -82,51 +80,151 @@ void main() async { }); } - test('fetch', () async { + test('fetchInitial', () async { const narrow = AllMessagesNarrow(); prepare(narrow: narrow); connection.prepare(json: newestResult( foundOldest: false, - messages: List.generate(100, (i) => eg.streamMessage(id: 1000 + i)), + messages: List.generate(kMessageListFetchBatchSize, + (i) => eg.streamMessage(id: 1000 + i)), ).toJson()); - final fetchFuture = model.fetch(); + final fetchFuture = model.fetchInitial(); check(model).fetched.isFalse(); checkInvariants(model); checkNotNotified(); await fetchFuture; checkNotifiedOnce(); - check(model).messages.length.equals(100); + check(model) + ..messages.length.equals(kMessageListFetchBatchSize) + ..haveOldest.isFalse(); checkLastRequest( narrow: narrow.apiEncode(), anchor: 'newest', - numBefore: 100, - numAfter: 10, + numBefore: kMessageListFetchBatchSize, + numAfter: 0, ); }); - test('fetch, short history', () async { + test('fetchInitial, short history', () async { prepare(); connection.prepare(json: newestResult( foundOldest: true, messages: List.generate(30, (i) => eg.streamMessage(id: 1000 + i)), ).toJson()); - await model.fetch(); + await model.fetchInitial(); checkNotifiedOnce(); - check(model).messages.length.equals(30); + check(model) + ..messages.length.equals(30) + ..haveOldest.isTrue(); }); - test('fetch, no messages found', () async { + test('fetchInitial, no messages found', () async { prepare(); connection.prepare(json: newestResult( foundOldest: true, messages: [], ).toJson()); - await model.fetch(); + await model.fetchInitial(); checkNotifiedOnce(); check(model) ..fetched.isTrue() - ..messages.isEmpty(); + ..messages.isEmpty() + ..haveOldest.isTrue(); + }); + + test('fetchOlder', () async { + const narrow = AllMessagesNarrow(); + prepare(narrow: narrow); + await prepareMessages(foundOldest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 1000 + i))); + + connection.prepare(json: olderResult( + anchor: 1000, foundOldest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 900 + i)), + ).toJson()); + final fetchFuture = model.fetchOlder(); + checkNotifiedOnce(); + check(model).fetchingOlder.isTrue(); + + await fetchFuture; + checkNotifiedOnce(); + check(model) + ..fetchingOlder.isFalse() + ..messages.length.equals(200); + checkLastRequest( + narrow: narrow.apiEncode(), + anchor: '1000', + includeAnchor: false, + numBefore: kMessageListFetchBatchSize, + numAfter: 0, + ); + }); + + test('fetchOlder nop when already fetching', () async { + const narrow = AllMessagesNarrow(); + prepare(narrow: narrow); + await prepareMessages(foundOldest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 1000 + i))); + + connection.prepare(json: olderResult( + anchor: 1000, foundOldest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 900 + i)), + ).toJson()); + final fetchFuture = model.fetchOlder(); + checkNotifiedOnce(); + check(model).fetchingOlder.isTrue(); + + // Don't prepare another response. + final fetchFuture2 = model.fetchOlder(); + checkNotNotified(); + checkInvariants(model); + check(model).fetchingOlder.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() + ..messages.length.equals(200); + }); + + test('fetchOlder nop when already haveOldest true', () async { + prepare(narrow: const AllMessagesNarrow()); + await prepareMessages(foundOldest: true, messages: + List.generate(30, (i) => eg.streamMessage(id: 1000 + i))); + check(model) + ..haveOldest.isTrue() + ..messages.length.equals(30); + + await model.fetchOlder(); + // We must not have made a request, because we didn't + // prepare a response and didn't get an exception. + checkNotNotified(); + checkInvariants(model); + check(model) + ..haveOldest.isTrue() + ..messages.length.equals(30); + }); + + test('fetchOlder handles servers not understanding includeAnchor', () async { + const narrow = AllMessagesNarrow(); + prepare(narrow: narrow); + await prepareMessages(foundOldest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 1000 + i))); + + // The old behavior is to include the anchor message regardless of includeAnchor. + connection.prepare(json: olderResult( + anchor: 1000, foundOldest: false, foundAnchor: true, + messages: List.generate(101, (i) => eg.streamMessage(id: 900 + i)), + ).toJson()); + await model.fetchOlder(); + checkNotified(count: 2); + check(model) + ..fetchingOlder.isFalse() + ..messages.length.equals(200); }); test('maybeAddMessage', () async { @@ -163,24 +261,6 @@ void main() async { checkInvariants(model); }); - test('findMessageWithId', () async { - prepare(); - await prepareMessages(foundOldest: true, messages: [ - eg.streamMessage(id: 2), - eg.streamMessage(id: 4), - eg.streamMessage(id: 6), - ]); - - // Exercise the binary search before, at, and after each element of the list. - check(model.findMessageWithId(1)).equals(-1); - check(model.findMessageWithId(2)).equals(0); - check(model.findMessageWithId(3)).equals(-1); - check(model.findMessageWithId(4)).equals(1); - check(model.findMessageWithId(5)).equals(-1); - check(model.findMessageWithId(6)).equals(2); - check(model.findMessageWithId(7)).equals(-1); - }); - group('maybeUpdateMessage', () { test('update a message', () async { final originalMessage = eg.streamMessage(id: 243, @@ -386,7 +466,13 @@ void main() async { void checkInvariants(MessageListView model) { if (!model.fetched) { - check(model).messages.isEmpty(); + check(model) + ..messages.isEmpty() + ..haveOldest.isFalse() + ..fetchingOlder.isFalse(); + } + if (model.haveOldest) { + check(model).fetchingOlder.isFalse(); } for (int i = 0; i < model.messages.length - 1; i++) { @@ -398,6 +484,27 @@ void checkInvariants(MessageListView model) { check(model.contents[i]) .equalsNode(parseContent(model.messages[i].content)); } + + check(model).items.length.equals( + ((model.haveOldest || model.fetchingOlder) ? 1 : 0) + + model.messages.length); + int i = 0; + if (model.haveOldest) { + check(model.items[i++]).isA(); + } + if (model.fetchingOlder) { + check(model.items[i++]).isA(); + } + for (int j = 0; j < model.messages.length; j++) { + check(model.items[i++]).isA() + ..message.identicalTo(model.messages[j]) + ..content.identicalTo(model.contents[j]); + } +} + +extension MessageListMessageItemChecks on Subject { + Subject get message => has((x) => x.message, 'message'); + Subject get content => has((x) => x.content, 'content'); } extension MessageListViewChecks on Subject { @@ -405,7 +512,10 @@ extension MessageListViewChecks on Subject { Subject get narrow => has((x) => x.narrow, 'narrow'); Subject> get messages => has((x) => x.messages, 'messages'); Subject> get contents => has((x) => x.contents, 'contents'); + Subject> get items => has((x) => x.items, 'items'); Subject get fetched => has((x) => x.fetched, 'fetched'); + Subject get haveOldest => has((x) => x.haveOldest, 'haveOldest'); + Subject get fetchingOlder => has((x) => x.fetchingOlder, 'fetchingOlder'); } /// A GetMessagesResult the server might return on an `anchor=newest` request. @@ -426,3 +536,21 @@ GetMessagesResult newestResult({ messages: messages, ); } + +/// A GetMessagesResult the server might return when we request older messages. +GetMessagesResult olderResult({ + required int anchor, + bool foundAnchor = false, // the value if the server understood includeAnchor false + required bool foundOldest, + bool historyLimited = false, + required List messages, +}) { + return GetMessagesResult( + anchor: anchor, + foundAnchor: foundAnchor, + foundNewest: false, // empirically always this, even when anchor happens to be latest + foundOldest: foundOldest, + historyLimited: historyLimited, + messages: messages, + ); +} diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 7341c01ca5..57241407f3 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -5,66 +5,117 @@ import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/model.dart'; -import 'package:zulip/api/route/messages.dart'; import 'package:zulip/model/narrow.dart'; +import 'package:zulip/model/store.dart'; import 'package:zulip/widgets/content.dart'; import 'package:zulip/widgets/message_list.dart'; import 'package:zulip/widgets/sticky_header.dart'; import 'package:zulip/widgets/store.dart'; import '../api/fake_api.dart'; -import '../test_images.dart'; import '../example_data.dart' as eg; import '../model/binding.dart'; +import '../model/message_list_test.dart'; import '../model/test_store.dart'; +import '../test_images.dart'; import 'content_checks.dart'; -Future setupMessageListPage(WidgetTester tester, { - required Narrow narrow, -}) async { - addTearDown(TestZulipBinding.instance.reset); - addTearDown(tester.view.resetPhysicalSize); +void main() { + TestZulipBinding.ensureInitialized(); - tester.view.physicalSize = const Size(600, 800); + late PerAccountStore store; + late FakeApiConnection connection; + + Future setupMessageListPage(WidgetTester tester, { + Narrow narrow = const AllMessagesNarrow(), + bool foundOldest = true, + int? messageCount, + List? messages, + }) async { + addTearDown(TestZulipBinding.instance.reset); + addTearDown(tester.view.resetPhysicalSize); + + tester.view.physicalSize = const Size(600, 800); + + await TestZulipBinding.instance.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + store = await TestZulipBinding.instance.globalStore.perAccount(eg.selfAccount.id); + connection = store.connection as FakeApiConnection; + + // prepare message list data + store.addUser(eg.selfUser); + assert((messageCount == null) != (messages == null)); + messages ??= List.generate(messageCount!, (index) { + return eg.streamMessage(id: index, sender: eg.selfUser); + }); + connection.prepare(json: + newestResult(foundOldest: foundOldest, messages: messages).toJson()); + + await tester.pumpWidget( + MaterialApp( + home: GlobalStoreWidget( + child: PerAccountStoreWidget( + accountId: eg.selfAccount.id, + child: MessageListPage(narrow: narrow))))); + + // global store, per-account store, and message list get loaded + await tester.pumpAndSettle(); + } + + ScrollController? findMessageListScrollController(WidgetTester tester) { + final stickyHeaderListView = tester.widget(find.byType(StickyHeaderListView)); + return stickyHeaderListView.controller; + } + + group('fetch older messages on scroll', () { + int? itemCount(WidgetTester tester) => + tester.widget(find.byType(StickyHeaderListView)).semanticChildCount; + + testWidgets('basic', (tester) async { + await setupMessageListPage(tester, foundOldest: false, + messages: List.generate(30, (i) => eg.streamMessage(id: 950 + i, sender: eg.selfUser))); + check(itemCount(tester)).equals(30); + + // Fling-scroll upward... + await tester.fling(find.byType(MessageListPage), const Offset(0, 300), 8000); + await tester.pump(); - await TestZulipBinding.instance.globalStore.add(eg.selfAccount, eg.initialSnapshot()); - final store = await TestZulipBinding.instance.globalStore.perAccount(eg.selfAccount.id); - final connection = store.connection as FakeApiConnection; + // ... and we should fetch more messages as we go. + connection.prepare(json: olderResult(anchor: 950, foundOldest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 850 + i, sender: eg.selfUser))).toJson()); + await tester.pump(const Duration(seconds: 3)); // Fast-forward to end of fling. + await tester.pump(Duration.zero); // Allow a frame for the response to arrive. - // prepare message list data - store.addUser(eg.selfUser); - final List messages = List.generate(10, (index) { - return eg.streamMessage(id: index, sender: eg.selfUser); - }); - connection.prepare(json: GetMessagesResult( - anchor: messages[0].id, - foundNewest: true, - foundOldest: true, - foundAnchor: true, - historyLimited: false, - messages: messages, - ).toJson()); - - await tester.pumpWidget( - MaterialApp( - home: GlobalStoreWidget( - child: PerAccountStoreWidget( - accountId: eg.selfAccount.id, - child: MessageListPage(narrow: narrow))))); - - // global store, per-account store, and message list get loaded - await tester.pumpAndSettle(); -} + // Now we have more messages. + check(itemCount(tester)).equals(130); + }); -void main() { - TestZulipBinding.ensureInitialized(); + testWidgets('observe double-fetch glitch', (tester) async { + await setupMessageListPage(tester, foundOldest: false, + messages: List.generate(30, (i) => eg.streamMessage(id: 950 + i, sender: eg.selfUser))); + check(itemCount(tester)).equals(30); - group('ScrollToBottomButton interactions', () { - ScrollController? findMessageListScrollController(WidgetTester tester) { - final stickyHeaderListView = tester.widget(find.byType(StickyHeaderListView)); - return stickyHeaderListView.controller; - } + // Fling-scroll upward... + await tester.fling(find.byType(MessageListPage), const Offset(0, 300), 8000); + await tester.pump(); + // ... and we fetch more messages as we go. + connection.prepare(json: olderResult(anchor: 950, foundOldest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 850 + i, sender: eg.selfUser))).toJson()); + await tester.pump(const Duration(seconds: 2)); + await tester.pump(Duration.zero); + check(itemCount(tester)).equals(130); + + // But on the next frame, we promptly fetch *another* batch. + // This is a glitch and it'd be nicer if we didn't. + connection.prepare(json: olderResult(anchor: 850, foundOldest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 750 + i, sender: eg.selfUser))).toJson()); + await tester.pump(const Duration(milliseconds: 1)); + await tester.pump(Duration.zero); + check(itemCount(tester)).equals(230); + }); + }); + + group('ScrollToBottomButton interactions', () { bool isButtonVisible(WidgetTester tester) { return tester.any(find.descendant( of: find.byType(ScrollToBottomButton), @@ -72,8 +123,7 @@ void main() { } testWidgets('scrolling changes visibility', (WidgetTester tester) async { - final stream = eg.stream(); - await setupMessageListPage(tester, narrow: StreamNarrow(stream.streamId)); + await setupMessageListPage(tester, messageCount: 10); final scrollController = findMessageListScrollController(tester)!; @@ -90,8 +140,7 @@ void main() { }); testWidgets('dimension updates changes visibility', (WidgetTester tester) async { - final stream = eg.stream(); - await setupMessageListPage(tester, narrow: StreamNarrow(stream.streamId)); + await setupMessageListPage(tester, messageCount: 10); final scrollController = findMessageListScrollController(tester)!; @@ -112,8 +161,7 @@ void main() { }); testWidgets('button functionality', (WidgetTester tester) async { - final stream = eg.stream(); - await setupMessageListPage(tester, narrow: StreamNarrow(stream.streamId)); + await setupMessageListPage(tester, messageCount: 10); final scrollController = findMessageListScrollController(tester)!; @@ -165,7 +213,7 @@ void main() { ..statusCode = HttpStatus.ok ..content = kSolidBlueAvatar; - await setupMessageListPage(tester, narrow: const AllMessagesNarrow()); + await setupMessageListPage(tester, messageCount: 10); checkResultForSender(eg.selfUser.avatarUrl); await handleNewAvatarEventAndPump(tester, '/foo.png');