From 5f9253ba38db2a573d6240da2da019f4d2ea97c7 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 7 Aug 2023 18:11:12 -0700 Subject: [PATCH 01/14] msglist [nfc]: Assimilate findMessageWithId to our style Also make the comment a bit more specific and more concise. --- lib/model/message_list.dart | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 9ba6d30ab5..866aaaf148 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -110,18 +110,17 @@ 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. + // Based on binarySearchBy in package:collection/src/algorithms.dart . + // (The package:collection version expects to be passed a whole element, + // not just a key -- so here, a whole [Message] rather than a message ID.) @visibleForTesting int findMessageWithId(int messageId) { - var min = 0; - var max = messages.length; + int min = 0; + int max = messages.length; while (min < max) { - var mid = min + ((max - min) >> 1); + final mid = min + ((max - min) >> 1); final message = messages[mid]; - var comp = message.id.compareTo(messageId); + final comp = message.id.compareTo(messageId); if (comp == 0) return mid; if (comp < 0) { min = mid + 1; From 353071e02d5bd31e613026c264e74ff9968599a1 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 7 Aug 2023 18:27:00 -0700 Subject: [PATCH 02/14] msglist [nfc]: Mark _applyChangesToMessage static; return void, not dynamic This doesn't use any data beyond what it's passed, so marking as `static` makes that explicit for the reader. That will also be helpful when we potentially go to move this logic elsewhere in the future. --- lib/model/message_list.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 866aaaf148..c970642f5b 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -87,7 +87,7 @@ class MessageListView extends ChangeNotifier { 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) { From 38abb1f18442318f2e9fde98a7d5fa811269a171 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 11 Jul 2023 23:22:27 -0700 Subject: [PATCH 03/14] msglist: Use zero for no-op numAfter in fetch This change is NFC if you include a well-behaved server in the system. That is, this request should produce the exact same response as before. --- lib/model/message_list.dart | 2 +- test/model/message_list_test.dart | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index c970642f5b..32b47f858d 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -62,7 +62,7 @@ class MessageListView extends ChangeNotifier { narrow: narrow.apiEncode(), anchor: AnchorCode.newest, // TODO(#80): switch to firstUnread numBefore: 100, - numAfter: 10, + numAfter: 0, ); messages.addAll(result.messages); contents.addAll(_contentsOfMessages(result.messages)); diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index d0bb1c587f..a1fbff010a 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -101,7 +101,7 @@ void main() async { narrow: narrow.apiEncode(), anchor: 'newest', numBefore: 100, - numAfter: 10, + numAfter: 0, ); }); From e69eb1f0d96851c86f8bad9d5ec9df363454f97c Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 11 Jul 2023 23:22:52 -0700 Subject: [PATCH 04/14] msglist [nfc]: Pull out constant for fetch-batch size --- lib/model/message_list.dart | 5 ++++- test/model/message_list_test.dart | 7 ++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 32b47f858d..0c1c17d526 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -7,6 +7,9 @@ import 'content.dart'; import 'narrow.dart'; import 'store.dart'; +/// The number of messages to fetch in each request. +const kMessageListFetchBatchSize = 100; // TODO tune + /// A view-model for a message list. /// /// The owner of one of these objects must call [dispose] when the object @@ -61,7 +64,7 @@ class MessageListView extends ChangeNotifier { final result = await getMessages(store.connection, narrow: narrow.apiEncode(), anchor: AnchorCode.newest, // TODO(#80): switch to firstUnread - numBefore: 100, + numBefore: kMessageListFetchBatchSize, numAfter: 0, ); messages.addAll(result.messages); diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index a1fbff010a..8db99cb413 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -87,7 +87,8 @@ void main() async { 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(); check(model).fetched.isFalse(); @@ -96,11 +97,11 @@ void main() async { checkNotNotified(); await fetchFuture; checkNotifiedOnce(); - check(model).messages.length.equals(100); + check(model).messages.length.equals(kMessageListFetchBatchSize); checkLastRequest( narrow: narrow.apiEncode(), anchor: 'newest', - numBefore: 100, + numBefore: kMessageListFetchBatchSize, numAfter: 0, ); }); From 6634beeaf330c1dfe37b310531b5b4722805ddd4 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 12 Jul 2023 16:18:03 -0700 Subject: [PATCH 05/14] msglist [nfc]: Cut done todos; add link for another --- lib/model/message_list.dart | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 0c1c17d526..097a3ee85d 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -24,8 +24,7 @@ const kMessageListFetchBatchSize = 100; // TODO tune /// * 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 +/// TODO support fetching another batch class MessageListView extends ChangeNotifier { MessageListView._({required this.store, required this.narrow}); @@ -57,13 +56,15 @@ class MessageListView extends ChangeNotifier { bool _fetched = false; Future fetch() async { + // TODO(#80): fetch from anchor firstUnread, instead of newest + // TODO(#82): fetch from a given message ID as anchor assert(!fetched); assert(messages.isEmpty); assert(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 + anchor: AnchorCode.newest, numBefore: kMessageListFetchBatchSize, numAfter: 0, ); From b91de7d048a85d24dff016e077e0a05828575b89 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 11 Jul 2023 23:12:23 -0700 Subject: [PATCH 06/14] msglist [nfc]: Pull out _MessageSequence from MessageListView --- lib/model/message_list.dart | 135 ++++++++++++++++++++++-------------- 1 file changed, 82 insertions(+), 53 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 097a3ee85d..56fab1f3fc 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -10,6 +10,79 @@ import 'store.dart'; /// The number of messages to fetch in each request. const kMessageListFetchBatchSize = 100; // TODO tune +/// 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; + + /// 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 = []; + + // Based on binarySearchBy in package:collection/src/algorithms.dart . + // (The package:collection version expects to be passed a whole element, + // not just a key -- so here, a whole [Message] rather than a message ID.) + @visibleForTesting + int findMessageWithId(int messageId) { + int min = 0; + int max = messages.length; + while (min < max) { + final mid = min + ((max - min) >> 1); + final message = messages[mid]; + final comp = message.id.compareTo(messageId); + if (comp == 0) return mid; + if (comp < 0) { + min = mid + 1; + } else { + max = mid; + } + } + return -1; + } + + /// Update data derived from the content of the index-th message. + void _reparseContent(int index) { + contents[index] = parseContent(messages[index].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); + // 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. + } + + /// 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); + } +} + /// A view-model for a message list. /// /// The owner of one of these objects must call [dispose] when the object @@ -25,7 +98,7 @@ const kMessageListFetchBatchSize = 100; // TODO tune /// resources on the [PerAccountStore]. /// /// TODO support fetching another batch -class MessageListView extends ChangeNotifier { +class MessageListView with ChangeNotifier, _MessageSequence { MessageListView._({required this.store, required this.narrow}); factory MessageListView.init( @@ -44,23 +117,11 @@ 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 { // TODO(#80): fetch from anchor firstUnread, instead of newest // TODO(#82): fetch from a given message ID as anchor assert(!fetched); - assert(messages.isEmpty); - assert(contents.isEmpty); + assert(messages.isEmpty && contents.isEmpty); // TODO schedule all this in another isolate final result = await getMessages(store.connection, narrow: narrow.apiEncode(), @@ -68,8 +129,9 @@ class MessageListView extends ChangeNotifier { numBefore: kMessageListFetchBatchSize, numAfter: 0, ); - messages.addAll(result.messages); - contents.addAll(_contentsOfMessages(result.messages)); + for (final message in result.messages) { + _addMessage(message); + } _fetched = true; notifyListeners(); } @@ -86,8 +148,7 @@ class MessageListView extends ChangeNotifier { return; } // TODO insert in middle instead, when appropriate - messages.add(message); - contents.add(parseContent(message.content)); + _addMessage(message); notifyListeners(); } @@ -114,27 +175,6 @@ class MessageListView extends ChangeNotifier { } } - // Based on binarySearchBy in package:collection/src/algorithms.dart . - // (The package:collection version expects to be passed a whole element, - // not just a key -- so here, a whole [Message] rather than a message ID.) - @visibleForTesting - int findMessageWithId(int messageId) { - int min = 0; - int max = messages.length; - while (min < max) { - final mid = min + ((max - min) >> 1); - final message = messages[mid]; - final 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 @@ -147,10 +187,8 @@ class MessageListView extends ChangeNotifier { return; } - final message = messages[idx]; - _applyChangesToMessage(event, message); - - contents[idx] = parseContent(message.content); + _applyChangesToMessage(event, messages[idx]); + _reparseContent(idx); notifyListeners(); } @@ -185,16 +223,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)); - } } From e9b0579615f7ebca7c2a27ce9bb2897e7bbdd9db Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 7 Aug 2023 21:42:56 -0700 Subject: [PATCH 07/14] algorithm [nfc]: Pull out a generic binarySearchByKey --- lib/model/algorithms.dart | 43 +++++++++++++++++++++++++++++++ lib/model/message_list.dart | 27 +++++-------------- test/model/algorithms_test.dart | 39 ++++++++++++++++++++++++++++ test/model/message_list_test.dart | 18 ------------- 4 files changed, 88 insertions(+), 39 deletions(-) create mode 100644 lib/model/algorithms.dart create mode 100644 test/model/algorithms_test.dart 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 56fab1f3fc..11bd556407 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -3,6 +3,7 @@ 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'; @@ -34,25 +35,9 @@ mixin _MessageSequence { /// It exists as an optimization, to memoize the work of parsing. final List contents = []; - // Based on binarySearchBy in package:collection/src/algorithms.dart . - // (The package:collection version expects to be passed a whole element, - // not just a key -- so here, a whole [Message] rather than a message ID.) - @visibleForTesting - int findMessageWithId(int messageId) { - int min = 0; - int max = messages.length; - while (min < max) { - final mid = min + ((max - min) >> 1); - final message = messages[mid]; - final comp = message.id.compareTo(messageId); - if (comp == 0) return mid; - if (comp < 0) { - min = mid + 1; - } else { - max = mid; - } - } - return -1; + int _findMessageWithId(int messageId) { + return binarySearchByKey(messages, messageId, + (message, messageId) => message.id.compareTo(messageId)); } /// Update data derived from the content of the index-th message. @@ -182,7 +167,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { /// /// TODO(#150): Handle message moves. void maybeUpdateMessage(UpdateMessageEvent event) { - final idx = findMessageWithId(event.messageId); + final idx = _findMessageWithId(event.messageId); if (idx == -1) { return; } @@ -193,7 +178,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { } void maybeUpdateMessageReactions(ReactionEvent event) { - final index = findMessageWithId(event.messageId); + final index = _findMessageWithId(event.messageId); if (index == -1) { return; } 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 8db99cb413..fbcd2e198d 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -164,24 +164,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, From 60273ac65cf1904e0669e5595b9cae151dd6bd23 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 12 Jul 2023 00:29:49 -0700 Subject: [PATCH 08/14] msglist [nfc]: Introduce MessageListItem --- lib/model/message_list.dart | 69 +++++++++++++++++++++++++++++-- lib/widgets/message_list.dart | 17 +++++--- test/model/message_list_test.dart | 13 ++++++ 3 files changed, 89 insertions(+), 10 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 11bd556407..4dea2b1434 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -11,6 +11,21 @@ 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); +} + /// The sequence of messages in a message list, and how to display them. /// /// This comprises much of the guts of [MessageListView]. @@ -35,14 +50,42 @@ mixin _MessageSequence { /// 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]. + /// It exists as an optimization, to memoize that computation. + final List items = []; + 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 MessageListMessageItem(:var message): return message.id.compareTo(messageId); + } + } + /// Update data derived from the content of the index-th message. void _reparseContent(int index) { - contents[index] = parseContent(messages[index].content); + 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. @@ -54,9 +97,7 @@ mixin _MessageSequence { messages.add(message); contents.add(parseContent(message.content)); assert(contents.length == messages.length); - // 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. + _processMessage(messages.length - 1); } /// Redo all computations from scratch, based on [messages]. @@ -65,6 +106,26 @@ mixin _MessageSequence { 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])); + } + + /// Recompute [items] from scratch, based on [messages] and [contents]. + void _reprocessAll() { + items.clear(); + for (var i = 0; i < messages.length; i++) { + _processMessage(i); + } } } diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 2779e26d22..8dc419fa30 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -221,8 +221,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 +243,16 @@ 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) { + 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/message_list_test.dart b/test/model/message_list_test.dart index fbcd2e198d..41cbcf614f 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -381,6 +381,18 @@ void checkInvariants(MessageListView model) { check(model.contents[i]) .equalsNode(parseContent(model.messages[i].content)); } + + check(model).items.length.equals(model.messages.length); + for (int i = 0; i < model.items.length; i++) { + check(model.items[i]).isA() + ..message.identicalTo(model.messages[i]) + ..content.identicalTo(model.contents[i]); + } +} + +extension MessageListMessageItemChecks on Subject { + Subject get message => has((x) => x.message, 'message'); + Subject get content => has((x) => x.content, 'content'); } extension MessageListViewChecks on Subject { @@ -388,6 +400,7 @@ 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'); } From 961c889985072f1aefac2743a616f226008993b9 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 12 Jul 2023 13:21:26 -0700 Subject: [PATCH 09/14] msglist: Show when we've reached the oldest message in the history --- lib/model/message_list.dart | 37 +++++++++++++++++++++++++++---- lib/widgets/message_list.dart | 5 +++++ test/model/message_list_test.dart | 32 ++++++++++++++++++-------- 3 files changed, 61 insertions(+), 13 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 4dea2b1434..ba1c5b871a 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -1,3 +1,4 @@ +import 'package:collection/collection.dart'; import 'package:flutter/foundation.dart'; import '../api/model/events.dart'; @@ -26,6 +27,11 @@ class MessageListMessageItem extends MessageListItem { MessageListMessageItem(this.message, this.content); } +/// 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]. @@ -42,6 +48,13 @@ mixin _MessageSequence { 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; + /// 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]. @@ -56,9 +69,10 @@ mixin _MessageSequence { /// of [messages], in order. It may have additional items interspersed /// before, between, or after the messages. /// - /// This information is completely derived from [messages]. + /// This information is completely derived from [messages] and + /// the flag [haveOldest]. /// It exists as an optimization, to memoize that computation. - final List items = []; + final QueueList items = QueueList(); int _findMessageWithId(int messageId) { return binarySearchByKey(messages, messageId, @@ -71,6 +85,7 @@ mixin _MessageSequence { static int _compareItemToMessageId(MessageListItem item, int messageId) { switch (item) { + case MessageListHistoryStartItem(): return -1; case MessageListMessageItem(:var message): return message.id.compareTo(messageId); } } @@ -120,12 +135,24 @@ mixin _MessageSequence { items.add(MessageListMessageItem(messages[index], contents[index])); } - /// Recompute [items] from scratch, based on [messages] and [contents]. + /// Update [items] to include markers at start and end as appropriate. + void _updateEndMarkers() { + switch ((items.firstOrNull, haveOldest)) { + case (MessageListHistoryStartItem(), true): break; + case (MessageListHistoryStartItem(), _ ): items.removeFirst(); + + case (_, true): items.addFirst(const MessageListHistoryStartItem()); + 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(); } } @@ -166,7 +193,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { Future fetch() async { // TODO(#80): fetch from anchor firstUnread, instead of newest // TODO(#82): fetch from a given message ID as anchor - assert(!fetched); + assert(!fetched && !haveOldest); assert(messages.isEmpty && contents.isEmpty); // TODO schedule all this in another isolate final result = await getMessages(store.connection, @@ -179,6 +206,8 @@ class MessageListView with ChangeNotifier, _MessageSequence { _addMessage(message); } _fetched = true; + _haveOldest = result.foundOldest; + _updateEndMarkers(); notifyListeners(); } diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 8dc419fa30..aeb727c12c 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -246,6 +246,11 @@ class _MessageListState extends State with PerAccountStoreAwareStat 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 MessageListMessageItem(:var message, :var content) => MessageItem( trailing: i == 0 ? const SizedBox(height: 8) : const SizedBox(height: 11), diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 41cbcf614f..f7818dd143 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -97,7 +97,9 @@ void main() async { checkNotNotified(); await fetchFuture; checkNotifiedOnce(); - check(model).messages.length.equals(kMessageListFetchBatchSize); + check(model) + ..messages.length.equals(kMessageListFetchBatchSize) + ..haveOldest.isFalse(); checkLastRequest( narrow: narrow.apiEncode(), anchor: 'newest', @@ -114,7 +116,9 @@ void main() async { ).toJson()); await model.fetch(); checkNotifiedOnce(); - check(model).messages.length.equals(30); + check(model) + ..messages.length.equals(30) + ..haveOldest.isTrue(); }); test('fetch, no messages found', () async { @@ -127,7 +131,8 @@ void main() async { checkNotifiedOnce(); check(model) ..fetched.isTrue() - ..messages.isEmpty(); + ..messages.isEmpty() + ..haveOldest.isTrue(); }); test('maybeAddMessage', () async { @@ -369,7 +374,9 @@ void main() async { void checkInvariants(MessageListView model) { if (!model.fetched) { - check(model).messages.isEmpty(); + check(model) + ..messages.isEmpty() + ..haveOldest.isFalse(); } for (int i = 0; i < model.messages.length - 1; i++) { @@ -382,11 +389,17 @@ void checkInvariants(MessageListView model) { .equalsNode(parseContent(model.messages[i].content)); } - check(model).items.length.equals(model.messages.length); - for (int i = 0; i < model.items.length; i++) { - check(model.items[i]).isA() - ..message.identicalTo(model.messages[i]) - ..content.identicalTo(model.contents[i]); + check(model).items.length.equals( + (model.haveOldest ? 1 : 0) + + model.messages.length); + int i = 0; + if (model.haveOldest) { + 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]); } } @@ -402,6 +415,7 @@ extension MessageListViewChecks on Subject { 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'); } /// A GetMessagesResult the server might return on an `anchor=newest` request. From 3665af8788794a22f06000279c85c47b6681a12b Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 10 Aug 2023 16:41:29 -0700 Subject: [PATCH 10/14] msglist test: Use AllMessagesNarrow, so that all messages are consistent These tests were relying, for the consistency of their data, on the test messages lying within the test stream they created. That's currently true but might not remain so. Instead, use the all-messages narrow, so that there's no such constraint. As a bonus the test code gets slightly simpler. --- test/widgets/message_list_test.dart | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 7341c01ca5..8f498ebb08 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -20,7 +20,7 @@ import '../model/test_store.dart'; import 'content_checks.dart'; Future setupMessageListPage(WidgetTester tester, { - required Narrow narrow, + Narrow narrow = const AllMessagesNarrow(), }) async { addTearDown(TestZulipBinding.instance.reset); addTearDown(tester.view.resetPhysicalSize); @@ -72,8 +72,7 @@ void main() { } testWidgets('scrolling changes visibility', (WidgetTester tester) async { - final stream = eg.stream(); - await setupMessageListPage(tester, narrow: StreamNarrow(stream.streamId)); + await setupMessageListPage(tester); final scrollController = findMessageListScrollController(tester)!; @@ -90,8 +89,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); final scrollController = findMessageListScrollController(tester)!; @@ -112,8 +110,7 @@ void main() { }); testWidgets('button functionality', (WidgetTester tester) async { - final stream = eg.stream(); - await setupMessageListPage(tester, narrow: StreamNarrow(stream.streamId)); + await setupMessageListPage(tester); final scrollController = findMessageListScrollController(tester)!; @@ -165,7 +162,7 @@ void main() { ..statusCode = HttpStatus.ok ..content = kSolidBlueAvatar; - await setupMessageListPage(tester, narrow: const AllMessagesNarrow()); + await setupMessageListPage(tester); checkResultForSender(eg.selfUser.avatarUrl); await handleNewAvatarEventAndPump(tester, '/foo.png'); From 7a7d80cada2d10103b9dfb9c4abc49c5bf3e9b2e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 10 Aug 2023 16:52:56 -0700 Subject: [PATCH 11/14] msglist test: Use realistic API responses in widget tests, too --- test/widgets/message_list_test.dart | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 8f498ebb08..17d99ed3ca 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -5,7 +5,6 @@ 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/widgets/content.dart'; import 'package:zulip/widgets/message_list.dart'; @@ -13,10 +12,11 @@ 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, { @@ -36,14 +36,8 @@ Future setupMessageListPage(WidgetTester tester, { 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()); + connection.prepare(json: + newestResult(foundOldest: true, messages: messages).toJson()); await tester.pumpWidget( MaterialApp( From d46523e5dc06d6bbe39f95448da56401141badc7 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 10 Aug 2023 16:56:36 -0700 Subject: [PATCH 12/14] msglist test [nfc]: Use shared store/connection variables in widget tests In particular this will allow individual test cases to call `connection.prepare` for setting up further API responses. --- test/widgets/message_list_test.dart | 62 +++++++++++++++-------------- 1 file changed, 33 insertions(+), 29 deletions(-) diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 17d99ed3ca..6b282d19d6 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -6,6 +6,7 @@ import 'package:flutter_test/flutter_test.dart'; import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/model.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'; @@ -19,39 +20,42 @@ import '../model/test_store.dart'; import '../test_images.dart'; import 'content_checks.dart'; -Future setupMessageListPage(WidgetTester tester, { - Narrow narrow = const AllMessagesNarrow(), -}) 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; - 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; + Future setupMessageListPage(WidgetTester tester, { + Narrow narrow = const AllMessagesNarrow(), + }) async { + addTearDown(TestZulipBinding.instance.reset); + addTearDown(tester.view.resetPhysicalSize); - // 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: - newestResult(foundOldest: true, 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(); -} + tester.view.physicalSize = const Size(600, 800); -void main() { - TestZulipBinding.ensureInitialized(); + 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); + final List messages = List.generate(10, (index) { + return eg.streamMessage(id: index, sender: eg.selfUser); + }); + connection.prepare(json: + newestResult(foundOldest: true, 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(); + } group('ScrollToBottomButton interactions', () { ScrollController? findMessageListScrollController(WidgetTester tester) { From 18bb0c69740bb516b91ecc9a8d0f689d81314024 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 10 Aug 2023 17:02:18 -0700 Subject: [PATCH 13/14] msglist test [nfc]: Make tests more explicit about the messages in list We'll soon want to add some tests where this detail varies. --- test/widgets/message_list_test.dart | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 6b282d19d6..7dc5d22d5f 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -28,6 +28,9 @@ void main() { Future setupMessageListPage(WidgetTester tester, { Narrow narrow = const AllMessagesNarrow(), + bool foundOldest = true, + int? messageCount, + List? messages, }) async { addTearDown(TestZulipBinding.instance.reset); addTearDown(tester.view.resetPhysicalSize); @@ -40,11 +43,12 @@ void main() { // prepare message list data store.addUser(eg.selfUser); - final List messages = List.generate(10, (index) { + assert((messageCount == null) != (messages == null)); + messages ??= List.generate(messageCount!, (index) { return eg.streamMessage(id: index, sender: eg.selfUser); }); connection.prepare(json: - newestResult(foundOldest: true, messages: messages).toJson()); + newestResult(foundOldest: foundOldest, messages: messages).toJson()); await tester.pumpWidget( MaterialApp( @@ -70,7 +74,7 @@ void main() { } testWidgets('scrolling changes visibility', (WidgetTester tester) async { - await setupMessageListPage(tester); + await setupMessageListPage(tester, messageCount: 10); final scrollController = findMessageListScrollController(tester)!; @@ -87,7 +91,7 @@ void main() { }); testWidgets('dimension updates changes visibility', (WidgetTester tester) async { - await setupMessageListPage(tester); + await setupMessageListPage(tester, messageCount: 10); final scrollController = findMessageListScrollController(tester)!; @@ -108,7 +112,7 @@ void main() { }); testWidgets('button functionality', (WidgetTester tester) async { - await setupMessageListPage(tester); + await setupMessageListPage(tester, messageCount: 10); final scrollController = findMessageListScrollController(tester)!; @@ -160,7 +164,7 @@ void main() { ..statusCode = HttpStatus.ok ..content = kSolidBlueAvatar; - await setupMessageListPage(tester); + await setupMessageListPage(tester, messageCount: 10); checkResultForSender(eg.selfUser.avatarUrl); await handleNewAvatarEventAndPump(tester, '/foo.png'); From bccaead94ef357cbccb9854418f6f224f11897b6 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 11 Jul 2023 23:27:38 -0700 Subject: [PATCH 14/14] msglist: Fetch older messages on scrolling up Fixes: #78 --- lib/model/message_list.dart | 98 +++++++++++++++--- lib/widgets/message_list.dart | 31 +++++- test/model/message_list_test.dart | 148 +++++++++++++++++++++++++--- test/widgets/message_list_test.dart | 59 ++++++++++- 4 files changed, 303 insertions(+), 33 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index ba1c5b871a..b2a9b22ce4 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -27,6 +27,15 @@ class MessageListMessageItem extends MessageListItem { 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(); @@ -55,6 +64,10 @@ mixin _MessageSequence { 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]. @@ -70,7 +83,7 @@ mixin _MessageSequence { /// before, between, or after the messages. /// /// This information is completely derived from [messages] and - /// the flag [haveOldest]. + /// the flags [haveOldest] and [fetchingOlder]. /// It exists as an optimization, to memoize that computation. final QueueList items = QueueList(); @@ -86,6 +99,11 @@ mixin _MessageSequence { 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); } } @@ -115,6 +133,19 @@ mixin _MessageSequence { _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); @@ -137,12 +168,22 @@ mixin _MessageSequence { /// Update [items] to include markers at start and end as appropriate. void _updateEndMarkers() { - switch ((items.firstOrNull, haveOldest)) { - case (MessageListHistoryStartItem(), true): break; - case (MessageListHistoryStartItem(), _ ): items.removeFirst(); - - case (_, true): items.addFirst(const MessageListHistoryStartItem()); - case (_, _): break; + 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; } } @@ -164,13 +205,12 @@ mixin _MessageSequence { /// 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 support fetching another batch class MessageListView with ChangeNotifier, _MessageSequence { MessageListView._({required this.store, required this.narrow}); @@ -190,10 +230,11 @@ class MessageListView with ChangeNotifier, _MessageSequence { final PerAccountStore store; final Narrow narrow; - Future fetch() async { + /// 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); + assert(!fetched && !haveOldest && !fetchingOlder); assert(messages.isEmpty && contents.isEmpty); // TODO schedule all this in another isolate final result = await getMessages(store.connection, @@ -211,6 +252,39 @@ class MessageListView with ChangeNotifier, _MessageSequence { 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. /// /// Called in particular when we get a [MessageEvent]. diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index aeb727c12c..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() { @@ -251,6 +275,11 @@ class _MessageListState extends State with PerAccountStoreAwareStat 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), diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index f7818dd143..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,7 +80,7 @@ void main() async { }); } - test('fetch', () async { + test('fetchInitial', () async { const narrow = AllMessagesNarrow(); prepare(narrow: narrow); connection.prepare(json: newestResult( @@ -90,7 +88,7 @@ void main() async { 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); @@ -108,26 +106,26 @@ void main() async { ); }); - 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) ..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() @@ -135,6 +133,100 @@ void main() async { ..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 { final stream = eg.stream(); prepare(narrow: StreamNarrow(stream.streamId)); @@ -376,7 +468,11 @@ void checkInvariants(MessageListView model) { if (!model.fetched) { check(model) ..messages.isEmpty() - ..haveOldest.isFalse(); + ..haveOldest.isFalse() + ..fetchingOlder.isFalse(); + } + if (model.haveOldest) { + check(model).fetchingOlder.isFalse(); } for (int i = 0; i < model.messages.length - 1; i++) { @@ -390,12 +486,15 @@ void checkInvariants(MessageListView model) { } check(model).items.length.equals( - (model.haveOldest ? 1 : 0) + ((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]) @@ -416,6 +515,7 @@ extension MessageListViewChecks on Subject { 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. @@ -436,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 7dc5d22d5f..57241407f3 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -61,12 +61,61 @@ void main() { await tester.pumpAndSettle(); } - group('ScrollToBottomButton interactions', () { - ScrollController? findMessageListScrollController(WidgetTester tester) { - final stickyHeaderListView = tester.widget(find.byType(StickyHeaderListView)); - return stickyHeaderListView.controller; - } + 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(); + + // ... 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. + + // Now we have more messages. + check(itemCount(tester)).equals(130); + }); + + 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); + + // 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),