Skip to content

msglist: Fetch newer messages despite previous muted batch #1989

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

Open
sm-sayedi wants to merge 5 commits into
base: main
Choose a base branch
from
+733 −221
Open

msglist: Fetch newer messages despite previous muted batch #1989

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
542996e
msglist [nfc]: Mention fetchNewer in the MessageListView dartdoc
sm-sayedi Nov 8, 2025
ce56456
msglist [nfc]: Add a new point to _MessageSequence.messages dartdoc
sm-sayedi Nov 8, 2025
3c10bed
msglist [nfc]: Remove one nested `try` block in `_fetchMore`
sm-sayedi Nov 15, 2025
03d61ca
msglist: Fetch newer messages despite previous muted batch
sm-sayedi Nov 13, 2025
127f690
msglist: Track three fetch request statuses separately
sm-sayedi Nov 13, 2025
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
120 changes: 80 additions & 40 deletions lib/model/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,26 @@ mixin _MessageSequence {
/// The corresponding item index is [middleItem].
int middleMessage = 0;

/// The ID of the oldest known message so far in this narrow.
///
/// This will be `null` if no messages of this narrow are fetched yet.
/// Having a non-null value for this doesn't always mean [haveOldest] is `true`.
///
/// The related message may not appear in [messages] because it
/// is muted in one way or another.
int? get oldMessageId => _oldMessageId;
Comment on lines +159 to +166
Copy link
Collaborator

@chrisbobbe chrisbobbe Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ID of the oldest known message so far in this narrow.

There isn't necessarily a message with this ID in the narrow, though, right? In a quick skim, I don't see event-handling code to update _oldMessageId when the corresponding message is deleted or moved out of the narrow. We should avoid saying the message is in the narrow when it might not be.

int? _oldMessageId;

/// The ID of the newest known message so far in this narrow.
///
/// This will be `null` if no messages of this narrow are fetched yet.
/// Having a non-null value for this doesn't always mean [haveNewest] is `true`.
///
/// The related message may not appear in [messages] because it
/// is muted in one way or another.
int? get newMessageId => _newMessageId;
int? _newMessageId;

/// Whether [messages] and [items] represent the results of a fetch.
///
/// This allows the UI to distinguish "still working on fetching messages"
Expand Down Expand Up @@ -409,6 +429,8 @@ mixin _MessageSequence {
generation += 1;
messages.clear();
middleMessage = 0;
_oldMessageId = null;
_newMessageId = null;
outboxMessages.clear();
_haveOldest = false;
_haveNewest = false;
Expand Down Expand Up @@ -775,6 +797,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
Future<void> fetchInitial() async {
assert(!fetched && !haveOldest && !haveNewest && !busyFetchingMore);
assert(messages.isEmpty && contents.isEmpty);
assert(oldMessageId == null && newMessageId == null);

if (narrow case KeywordSearchNarrow(keyword: '')) {
// The server would reject an empty keyword search; skip the request.
Expand All @@ -798,6 +821,9 @@ class MessageListView with ChangeNotifier, _MessageSequence {
);
if (this.generation > generation) return;

_oldMessageId = result.messages.firstOrNull?.id;
_newMessageId = result.messages.lastOrNull?.id;

_adjustNarrowForTopicPermalink(result.messages.firstOrNull);

store.reconcileMessages(result.messages);
Expand Down Expand Up @@ -869,25 +895,32 @@ class MessageListView with ChangeNotifier, _MessageSequence {
/// That makes this method suitable to call frequently, e.g. every frame,
/// whenever it looks likely to be useful to have more messages.
Future<void> fetchOlder() async {
if (haveOldest) return;
if (busyFetchingMore) return;
assert(fetched);
assert(messages.isNotEmpty);
await _fetchMore(
anchor: NumericAnchor(messages[0].id),
numBefore: kMessageListFetchBatchSize,
numAfter: 0,
processResult: (result) {
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(0, fetchedMessages);
_haveOldest = result.foundOldest;
});
final generation = this.generation;
int visibleMessageCount = 0;
do {
if (haveOldest) return;
if (busyFetchingMore) return;
assert(fetched);
assert(oldMessageId != null);
await _fetchMore(
anchor: NumericAnchor(oldMessageId!),
numBefore: kMessageListFetchBatchSize,
numAfter: 0,
processResult: (result) {
_oldMessageId = result.messages.firstOrNull?.id ?? oldMessageId;
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(0, fetchedMessages);
_haveOldest = result.foundOldest;
visibleMessageCount += fetchedMessages.length;
});
} while (visibleMessageCount < kMessageListFetchBatchSize / 2
&& this.generation == generation);
Comment on lines +967 to +968
Copy link
Member

@gnprice gnprice Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have the widget state react in the way I suggested in my last comment just now, does that also let us skip these loops? We'd have the state retain responsibility for taking the initiative to call fetchOlder and fetchNewer when potentially needed.

}

/// Fetch the next batch of newer messages, if applicable.
Expand All @@ -899,29 +932,36 @@ class MessageListView with ChangeNotifier, _MessageSequence {
/// That makes this method suitable to call frequently, e.g. every frame,
/// whenever it looks likely to be useful to have more messages.
Future<void> fetchNewer() async {
if (haveNewest) return;
if (busyFetchingMore) return;
assert(fetched);
assert(messages.isNotEmpty);
await _fetchMore(
anchor: NumericAnchor(messages.last.id),
numBefore: 0,
numAfter: kMessageListFetchBatchSize,
processResult: (result) {
store.reconcileMessages(result.messages);
store.recentSenders.handleMessages(result.messages); // TODO(#824)

for (final message in result.messages) {
if (_messageVisible(message)) {
_addMessage(message);
final generation = this.generation;
int visibleMessageCount = 0;
do {
if (haveNewest) return;
if (busyFetchingMore) return;
assert(fetched);
assert(newMessageId != null);
await _fetchMore(
anchor: NumericAnchor(newMessageId!),
numBefore: 0,
numAfter: kMessageListFetchBatchSize,
processResult: (result) {
_newMessageId = result.messages.lastOrNull?.id ?? newMessageId;
store.reconcileMessages(result.messages);
store.recentSenders.handleMessages(result.messages); // TODO(#824)

for (final message in result.messages) {
if (_messageVisible(message)) {
_addMessage(message);
visibleMessageCount++;
}
}
}
_haveNewest = result.foundNewest;
_haveNewest = result.foundNewest;

if (haveNewest) {
_syncOutboxMessagesFromStore();
}
});
if (haveNewest) {
_syncOutboxMessagesFromStore();
}
});
} while (visibleMessageCount < kMessageListFetchBatchSize / 2
&& this.generation == generation);
}

Future<void> _fetchMore({
Expand Down
8 changes: 3 additions & 5 deletions lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,8 +838,6 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
model.fetchInitial();
}

bool _prevFetched = false;

void _modelChanged() {
// When you're scrolling quickly, our mark-as-read requests include the
// messages *between* _messagesRecentlyInViewport and the messages currently
Expand All @@ -866,14 +864,13 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
// This method was called because that just changed.
});

if (!_prevFetched && model.fetched && model.messages.isEmpty) {
if (model.messages.isEmpty && model.haveOldest && model.haveNewest) {
// If the fetch came up empty, there's nothing to read,
// so opening the keyboard won't be bothersome and could be helpful.
// It's definitely helpful if we got here from the new-DM page.
MessageListPage.ancestorOf(context)
.composeBoxState?.controller.requestFocusIfUnfocused();
}
_prevFetched = model.fetched;
}

/// Find the range of message IDs on screen, as a (first, last) tuple,
Expand Down Expand Up @@ -1222,7 +1219,8 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
return Column(crossAxisAlignment: CrossAxisAlignment.stretch, children: [
TypingStatusWidget(narrow: widget.narrow),
// TODO perhaps offer mark-as-read even when not done fetching?
MarkAsReadWidget(narrow: widget.narrow),
if (model.messages.isNotEmpty)
MarkAsReadWidget(narrow: widget.narrow),
Comment on lines 1219 to +1221
Copy link
Member

@gnprice gnprice Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the relationship of this change to the main changes happening in this commit? What's the user-visible change in behavior it causes?

Copy link
Collaborator Author

@sm-sayedi sm-sayedi Nov 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So with the new changes in the commit, if the initial newest batch is all muted (model.messages.isEmpty), and then when older messages are being fetched, the "Mark as read" widget will be shown along with a progress indicator and no messages, so I thought it may be a good idea to hide "Mark as read" until there are visible messages populated.

Before (this one-line change) After (this one-line change)
Screenshot 2025-11-26 at 7 30 45 AM Screenshot 2025-11-26 at 7 32 13 AM

Copy link
Member

@gnprice gnprice Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I see.

What does that situation look like before this branch / before even the first batch is fetched? I believe we show a different progress indicator (centered), and don't show the mark-read button.

Can we have that behavior continue when we've had some fetch requests finish, but haven't yet actually found any messages? I think I'd like to think of that state as equivalent to when the initial fetch is still going.

IOW I'd like to think of it as "still working on fetching messages", like in the doc comment on MessageListView.fetched (in main):

  /// 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 => switch (_status) {
    FetchingStatus.unstarted || FetchingStatus.fetchInitial => false,
    _ => true,
  };

Copy link
Collaborator Author

@sm-sayedi sm-sayedi Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that will be an improvement. It will also avoid the abrupt change from a centered progress indicator to a bottom-aligned indicator.

// To reinforce that the end of the feed has been reached:
// https://chat.zulip.org/#narrow/channel/48-mobile/topic/space.20at.20end.20of.20thread/near/2203391
const SizedBox(height: 12),
Expand Down
Loading