Skip to content
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 14 commits into from
Aug 14, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 65 additions & 4 deletions lib/model/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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].
Expand All @@ -35,14 +50,42 @@ mixin _MessageSequence {
/// 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].
/// It exists as an optimization, to memoize that computation.
final List<MessageListItem> items = [];

int _findMessageWithId(int messageId) {
return binarySearchByKey(messages, messageId,
(message, messageId) => message.id.compareTo(messageId));
}

int _findItemWithMessageId(int messageId) {
return binarySearchByKey(items, messageId, _compareItemToMessageId);
}
Comment on lines +95 to +97
Copy link
Collaborator

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 to MessageListMessageItem), this method will need to be replaced, right, because it assumes there is just one item per message ID.

Copy link
Member Author

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

  • it's really looking for the message item for the message ID;
  • there'll still be exactly one of those per message;
  • and the other items with a given message ID will all be in a predictable direction relative to the message item (specifically, date separator and recipient header will be before the message, and if we later dream up some item that goes after its associated message then that'll fit into this scheme too).

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.


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.
Expand All @@ -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].
Expand All @@ -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);
}
}
}

Expand Down
17 changes: 11 additions & 6 deletions lib/widgets/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -221,8 +221,7 @@ class _MessageListState extends State<MessageList> 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:
Expand All @@ -244,10 +243,16 @@ class _MessageListState extends State<MessageList> 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)
};
});
}
}

Expand Down
13 changes: 13 additions & 0 deletions test/model/message_list_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -381,13 +381,26 @@ 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<MessageListMessageItem>()
..message.identicalTo(model.messages[i])
..content.identicalTo(model.contents[i]);
}
}

extension MessageListMessageItemChecks on Subject<MessageListMessageItem> {
Subject<Message> get message => has((x) => x.message, 'message');
Subject<ZulipContent> get content => has((x) => x.content, 'content');
}

extension MessageListViewChecks on Subject<MessageListView> {
Subject<PerAccountStore> get store => has((x) => x.store, 'store');
Subject<Narrow> get narrow => has((x) => x.narrow, 'narrow');
Subject<List<Message>> get messages => has((x) => x.messages, 'messages');
Subject<List<ZulipContent>> get contents => has((x) => x.contents, 'contents');
Subject<List<MessageListItem>> get items => has((x) => x.items, 'items');
Subject<bool> get fetched => has((x) => x.fetched, 'fetched');
}

Expand Down