Skip to content

msglist: Open near first unread #1169

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

msglist: Open near first unread #1169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion integration_test/unreadmarker_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ void main() {
final messages = List.generate(messageCount,
(i) => eg.streamMessage(flags: [MessageFlag.read]));
connection.prepare(json:
eg.newestGetMessagesResult(foundOldest: true, messages: messages).toJson());
eg.nearUnreadGetMessagesResult(foundOldest: true, foundNewest: true,
messages: messages).toJson());

await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id,
child: const MessageListPage(initNarrow: CombinedFeedNarrow())));
Expand Down
144 changes: 133 additions & 11 deletions lib/model/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import 'narrow.dart';
import 'store.dart';

/// The number of messages to fetch in each request.
const kMessageListFetchBatchSize = 100; // TODO tune
const kMessageListFetchBatchSize = 5; // TODO tune

/// A message, or one of its siblings shown in the message list.
///
Expand Down Expand Up @@ -58,7 +58,7 @@ class MessageListLoadingItem extends MessageListItem {
const MessageListLoadingItem(this.direction);
}

enum MessageListDirection { older }
enum MessageListDirection { older, newer }

/// Indicates we've reached the oldest message in the narrow.
class MessageListHistoryStartItem extends MessageListItem {
Expand All @@ -85,9 +85,6 @@ mixin _MessageSequence {
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;

Expand Down Expand Up @@ -118,6 +115,40 @@ mixin _MessageSequence {

BackoffMachine? _fetchOlderCooldownBackoffMachine;

/// Whether we know we have the newest messages for this narrow.
bool get haveNewest => _haveNewest;
bool _haveNewest = false;

/// Whether we are currently fetching the next batch of newer messages.
///
/// When this is true, [fetchNewer] is a no-op.
/// That method is called frequently by Flutter's scrolling logic,
/// and this field helps us avoid spamming the same request just to get
/// the same response each time.
///
/// See also [fetchNewerCoolingDown].
bool get fetchingNewer => _fetchingNewer;
bool _fetchingNewer = false;

/// Whether [fetchNewer] had a request error recently.
///
/// When this is true, [fetchNewer] is a no-op.
/// That method is called frequently by Flutter's scrolling logic,
/// and this field mitigates spamming the same request and getting
/// the same error each time.
///
/// "Recently" is decided by a [BackoffMachine] that resets
/// when a [fetchNewer] request succeeds.
///
/// See also [fetchingNewer].
bool get fetchNewerCoolingDown => _fetchNewerCoolingDown;
bool _fetchNewerCoolingDown = false;

BackoffMachine? _fetchNewerCooldownBackoffMachine;

int? get firstUnreadMessageId => _firstUnreadMessageId;
int? _firstUnreadMessageId;

/// 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].
Expand Down Expand Up @@ -151,6 +182,7 @@ mixin _MessageSequence {
case MessageListHistoryStartItem(): return -1;
case MessageListLoadingItem():
switch (item.direction) {
case MessageListDirection.newer: return 1;
case MessageListDirection.older: return -1;
}
case MessageListRecipientHeaderItem(:var message):
Expand Down Expand Up @@ -269,6 +301,11 @@ mixin _MessageSequence {
_fetchingOlder = false;
_fetchOlderCoolingDown = false;
_fetchOlderCooldownBackoffMachine = null;
_haveNewest = false;
_fetchingNewer = false;
_fetchNewerCoolingDown = false;
_fetchNewerCooldownBackoffMachine = null;
_firstUnreadMessageId = null;
contents.clear();
items.clear();
}
Expand Down Expand Up @@ -317,7 +354,8 @@ mixin _MessageSequence {
/// Update [items] to include markers at start and end as appropriate.
void _updateEndMarkers() {
assert(fetched);
assert(!(fetchingOlder && fetchOlderCoolingDown));
assert(!(fetchingOlder && fetchOlderCoolingDown)
|| !(fetchingNewer && fetchNewerCoolingDown));
final effectiveFetchingOlder = fetchingOlder || fetchOlderCoolingDown;
assert(!(effectiveFetchingOlder && haveOldest));
final startMarker = switch ((effectiveFetchingOlder, haveOldest)) {
Expand All @@ -336,6 +374,21 @@ mixin _MessageSequence {
case (_, true): items.removeFirst();
case (_, _ ): break;
}

final effectiveFetchingNewer = fetchingNewer || fetchNewerCoolingDown;
final endMarker = switch (effectiveFetchingNewer) {
true => const MessageListLoadingItem(MessageListDirection.newer),
false => null,
};
final hasEndMarker = switch (items.lastOrNull) {
MessageListLoadingItem() => true,
_ => false,
};
switch ((endMarker != null, hasEndMarker)) {
case (true, false): items.add(endMarker!);
case (false, true ): items.removeLast();
case (_, _ ): break;
}
}

/// Recompute [items] from scratch, based on [messages], [contents], and flags.
Expand Down Expand Up @@ -500,15 +553,17 @@ class MessageListView with ChangeNotifier, _MessageSequence {
Future<void> fetchInitial() async {
// TODO(#80): fetch from anchor firstUnread, instead of newest
// TODO(#82): fetch from a given message ID as anchor
assert(!fetched && !haveOldest && !fetchingOlder && !fetchOlderCoolingDown);
assert(!fetched && !haveOldest && !fetchingOlder && !fetchOlderCoolingDown
&& !haveNewest && !fetchingNewer && !fetchNewerCoolingDown);
assert(messages.isEmpty && contents.isEmpty);
// TODO schedule all this in another isolate
final generation = this.generation;
final result = await getMessages(store.connection,
narrow: narrow.apiEncode(),
anchor: AnchorCode.newest,
anchor: AnchorCode.firstUnread,
numBefore: kMessageListFetchBatchSize,
numAfter: 0,
// Results will include the anchor message, so fetch one less.
numAfter: kMessageListFetchBatchSize - 1,
);
if (this.generation > generation) return;
store.reconcileMessages(result.messages);
Expand All @@ -520,6 +575,8 @@ class MessageListView with ChangeNotifier, _MessageSequence {
}
_fetched = true;
_haveOldest = result.foundOldest;
_haveNewest = result.foundNewest;
_firstUnreadMessageId = result.anchor;
_updateEndMarkers();
notifyListeners();
}
Expand All @@ -541,7 +598,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
try {
result = await getMessages(store.connection,
narrow: narrow.apiEncode(),
anchor: NumericAnchor(messages[0].id),
anchor: NumericAnchor(messages.first.id),
includeAnchor: false,
numBefore: kMessageListFetchBatchSize,
numAfter: 0,
Expand All @@ -553,7 +610,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
if (this.generation > generation) return;

if (result.messages.isNotEmpty
&& result.messages.last.id == messages[0].id) {
&& result.messages.last.id == messages.first.id) {
// TODO(server-6): includeAnchor should make this impossible
result.messages.removeLast();
}
Expand Down Expand Up @@ -589,6 +646,71 @@ class MessageListView with ChangeNotifier, _MessageSequence {
}
}

/// Fetch the next batch of newer messages, if applicable.
Future<void> fetchNewer() async {
if (haveNewest) return;
if (fetchingNewer) return;
if (fetchNewerCoolingDown) return;
assert(fetched);
assert(messages.isNotEmpty);
_fetchingNewer = true;
_updateEndMarkers();
notifyListeners();
final generation = this.generation;
bool hasFetchError = false;
try {
final GetMessagesResult result;
try {
result = await getMessages(store.connection,
narrow: narrow.apiEncode(),
anchor: NumericAnchor(messages.last.id),
includeAnchor: false,
numBefore: 0,
numAfter: kMessageListFetchBatchSize,
);
} catch (e) {
hasFetchError = true;
rethrow;
}
if (this.generation > generation) return;

if (result.messages.isNotEmpty
&& result.messages.first.id == messages.last.id) {
// TODO(server-6): includeAnchor should make this impossible
result.messages.removeAt(0);
}

store.reconcileMessages(result.messages);
store.recentSenders.handleMessages(result.messages); // TODO(#824)

final fetchedMessages = _allMessagesVisible
? result.messages // Avoid unnecessarily copying the list.
: result.messages.where(_messageVisible);

_insertAllMessages(messages.length, fetchedMessages);
_haveNewest = result.foundNewest;
} finally {
if (this.generation == generation) {
_fetchingNewer = false;
if (hasFetchError) {
assert(!fetchNewerCoolingDown);
_fetchNewerCoolingDown = true;
unawaited((_fetchNewerCooldownBackoffMachine ??= BackoffMachine())
.wait().then((_) {
if (this.generation != generation) return;
_fetchNewerCoolingDown = false;
_updateEndMarkers();
notifyListeners();
}));
} else {
_fetchNewerCooldownBackoffMachine = null;
}
_updateEndMarkers();
notifyListeners();
}
}
}

void handleUserTopicEvent(UserTopicEvent event) {
switch (_canAffectVisibility(event)) {
case VisibilityEffect.none:
Expand Down
91 changes: 68 additions & 23 deletions lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,9 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
// still not yet updated to account for the newly-added messages.
model?.fetchOlder();
}
if (scrollMetrics.extentAfter < kFetchMessagesBufferPixels) {
model?.fetchNewer();
}
}

void _scrollChanged() {
Expand Down Expand Up @@ -566,10 +569,27 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
}

Widget _buildListView(BuildContext context) {
final length = model!.items.length;
const centerSliverKey = ValueKey('center sliver');

Widget sliver = SliverStickyHeaderList(
// TODO(#311) If we have a bottom nav, it will pad the bottom
// inset, and this shouldn't be necessary
final hasComposeBox = ComposeBox.hasComposeBox(widget.narrow);

// TODO make sticky headers work
final List<MessageListItem> olderItems, newerItems;

final firstUnreadItemIndex = model!.firstUnreadMessageId! != 10000000000000000
? model!.findItemWithMessageId(model!.firstUnreadMessageId!)
: -1;
if (firstUnreadItemIndex == -1) {
olderItems = model!.items;
newerItems = const [];
} else {
olderItems = model!.items.slice(0, firstUnreadItemIndex);
newerItems = model!.items.slice(firstUnreadItemIndex);
}

Widget topSliver = SliverStickyHeaderList(
headerPlacement: HeaderPlacement.scrollingStart,
delegate: SliverChildBuilderDelegate(
// To preserve state across rebuilds for individual [MessageItem]
Expand All @@ -591,26 +611,56 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
final valueKey = key as ValueKey<int>;
final index = model!.findItemWithMessageId(valueKey.value);
if (index == -1) return null;
return length - 1 - (index - 3);
return olderItems.length - 1 - index;
},
childCount: length + 3,
childCount: olderItems.length,
(context, i) {
// To reinforce that the end of the feed has been reached:
// https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Mark-as-read/near/1680603
if (i == 0) return const SizedBox(height: 36);
final data = olderItems[olderItems.length - 1 - i];
return _buildItem(data, i);
}));

Widget bottomSliver = SliverStickyHeaderList(
key: hasComposeBox ? centerSliverKey : null,
headerPlacement: HeaderPlacement.scrollingStart,
delegate: SliverChildBuilderDelegate(
// To preserve state across rebuilds for individual [MessageItem]
// widgets as the size of [MessageListView.items] changes we need
// to match old widgets by their key to their new position in
// the list.
//
// The keys are of type [ValueKey] with a value of [Message.id]
// and here we use a O(log n) binary search method. This could
// be improved but for now it only triggers for materialized
// widgets. As a simple test, flinging through Combined feed in
// CZO on a Pixel 5, this only runs about 10 times per rebuild
// and the timing for each call is <100 microseconds.
//
// Non-message items (e.g., start and end markers) that do not
// have state that needs to be preserved have not been given keys
// and will not trigger this callback.
findChildIndexCallback: (Key key) {
final valueKey = key as ValueKey<int>;
final index = model!.findItemWithMessageId(valueKey.value);
if (index == -1) return null;
return index;
},
childCount: newerItems.length + 3,
(context, i) {
if (i == newerItems.length) return TypingStatusWidget(narrow: widget.narrow);

if (i == 1) return MarkAsReadWidget(narrow: widget.narrow);
if (i == newerItems.length + 1) return MarkAsReadWidget(narrow: widget.narrow);

if (i == 2) return TypingStatusWidget(narrow: widget.narrow);
// To reinforce that the end of the feed has been reached:
// https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Mark-as-read/near/1680603
if (i == newerItems.length + 2) return const SizedBox(height: 36);

final data = model!.items[length - 1 - (i - 3)];
final data = newerItems[i];
return _buildItem(data, i);
}));

if (!ComposeBox.hasComposeBox(widget.narrow)) {
// TODO(#311) If we have a bottom nav, it will pad the bottom inset,
// and this can be removed; also remove mention in MessageList dartdoc
sliver = SliverSafeArea(sliver: sliver);
if (!hasComposeBox) {
topSliver = SliverSafeArea(sliver: topSliver);
bottomSliver = SliverSafeArea(key: centerSliverKey, sliver: bottomSliver);
}

return CustomScrollView(
Expand All @@ -626,17 +676,12 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
},

controller: scrollController,
semanticChildCount: length + 2,
anchor: 1.0,
semanticChildCount: model!.items.length + 2,
anchor: 0.75,
center: centerSliverKey,

slivers: [
sliver,

// This is a trivial placeholder that occupies no space. Its purpose is
// to have the key that's passed to [ScrollView.center], and so to cause
// the above [SliverStickyHeaderList] to run from bottom to top.
const SliverToBoxAdapter(key: centerSliverKey),
topSliver,
bottomSliver,
]);
}

Expand Down
Loading