-
Notifications
You must be signed in to change notification settings - Fork 238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fetch older messages on scrolling up in message list #264
Merged
Merged
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
5f9253b
msglist [nfc]: Assimilate findMessageWithId to our style
gnprice 353071e
msglist [nfc]: Mark _applyChangesToMessage static; return void, not d…
gnprice 38abb1f
msglist: Use zero for no-op numAfter in fetch
gnprice e69eb1f
msglist [nfc]: Pull out constant for fetch-batch size
gnprice 6634bee
msglist [nfc]: Cut done todos; add link for another
gnprice b91de7d
msglist [nfc]: Pull out _MessageSequence from MessageListView
gnprice e9b0579
algorithm [nfc]: Pull out a generic binarySearchByKey
gnprice 60273ac
msglist [nfc]: Introduce MessageListItem
gnprice 961c889
msglist: Show when we've reached the oldest message in the history
gnprice 3665af8
msglist test: Use AllMessagesNarrow, so that all messages are consistent
gnprice 7a7d80c
msglist test: Use realistic API responses in widget tests, too
gnprice d46523e
msglist test [nfc]: Use shared store/connection variables in widget t…
gnprice 18bb0c6
msglist test [nfc]: Make tests more explicit about the messages in list
gnprice bccaead
msglist: Fetch older messages on scrolling up
gnprice File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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<E, K>( | ||
List<E> 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; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,161 @@ | ||
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 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<Message> 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; | ||
|
||
/// 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<ZulipContent> 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 flag [haveOldest]. | ||
/// It exists as an optimization, to memoize that computation. | ||
final QueueList<MessageListItem> 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 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); | ||
} | ||
|
||
/// 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() { | ||
switch ((items.firstOrNull, haveOldest)) { | ||
case (MessageListHistoryStartItem(), true): break; | ||
case (MessageListHistoryStartItem(), _ ): items.removeFirst(); | ||
|
||
case (_, true): items.addFirst(const MessageListHistoryStartItem()); | ||
case (_, _): break; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool! This is nice and compact! |
||
} | ||
|
||
/// 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 | ||
|
@@ -21,9 +170,8 @@ import 'store.dart'; | |
/// * 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 { | ||
/// TODO support fetching another batch | ||
class MessageListView with ChangeNotifier, _MessageSequence { | ||
MessageListView._({required this.store, required this.narrow}); | ||
|
||
factory MessageListView.init( | ||
|
@@ -42,31 +190,24 @@ class MessageListView extends ChangeNotifier { | |
final PerAccountStore store; | ||
final Narrow narrow; | ||
|
||
final List<Message> messages = []; | ||
|
||
/// The parsed message contents, as a list parallel to [messages]. | ||
final List<ZulipContent> 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<void> fetch() async { | ||
assert(!fetched); | ||
assert(messages.isEmpty); | ||
assert(contents.isEmpty); | ||
// TODO(#80): fetch from anchor firstUnread, instead of newest | ||
// TODO(#82): fetch from a given message ID as anchor | ||
assert(!fetched && !haveOldest); | ||
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(); | ||
} | ||
|
||
|
@@ -82,12 +223,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,49 +250,25 @@ 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 | ||
/// were changed, and ignores any changes to its stream or topic. | ||
/// | ||
/// 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 +298,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<ZulipContent> _contentsOfMessages(Iterable<Message> 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)); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to check my understanding: when
items
starts having multiple items with the same message ID (a date separator and recipient header, in addition toMessageListMessageItem
), this method will need to be replaced, right, because it assumes there is just one item per message ID.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. It'll need some adjustments and I suppose some renamings for clarification, but I think the core logic will actually continue to work fine: the key is that
So when the comparison function sees some item that has the right message ID but isn't a message item, it can just say those compare before the key.