Implement mark-as-read-on-scroll, v1 #1574

chrisbobbe · 2025-06-13T06:58:20Z

(edited)

Fixes #81. 🎉

Still to do, but can be soon-after-launch:

gnprice · 2025-06-13T07:13:51Z

Great, glad to see this progress.

Can the "range hull" logic end up naively applying its arithmetic to two ranges that actually don't have anything to do with each other, because e.g. the narrow changed, and cause hundreds of messages to get marked as read incorrectly? (when is _modelChanged called?)

If the model's narrow changes, that will always cause its listeners to be notified. (It'd be a bug in the model if data like that could change without notifying listeners.) So in particular _modelChanged will get called.

To verify that listeners do always get called, look at the references to _narrow to see where it's mutated. (This is an example of where it comes in handy to have a different name for the field from the name for the getter.) They each… hmm, well, it looks like _handlePropagateMode may have a bug where it lacks that call. At the other sites, though, listeners get notified at the end of fetchInitial, via _setStatus.

gnprice · 2025-06-13T07:22:40Z

lib/widgets/message_list.dart

+    // TODO this frequently ends up walking through a few hundred elements,
+    //   only 5-10 of which end up being MessageItem ones.
+    scrollViewElement.visitChildElements(visit);


Yeah, it should be possible to make this more focused so that it only goes down the relevant paths of the tree — only to the list items (be they MessageItem or date separator or recipient header), not to any of their descendants or to other parts of the tree. We can take a look together.

chrisbobbe · 2025-06-13T07:23:05Z

Ah I do trust that _modelChanged will get called when the model's narrow changes, I'm just not totally sure the _messagesRecentlyInViewport = null; I added in _modelChanged will do what I want it to do. Like maybe: what if that runs, but a scroll event arrives and _messagesRecentlyInViewport gets set to some messages from the old narrow (because layout/etc. for the new narrow hasn't finished yet); then on the next scroll event, _messagesRecentlyInViewport won't have anything to do with the messages currently in view, and convex-hulling the two ranges will be problematic and mark some messages read incorrectly.

I might think more clearly about this in the office tomorrow 😛

gnprice · 2025-06-13T07:25:40Z

lib/widgets/message_list.dart

+        result.add(message.id);
+      }
+    }
+    // TODO why isn't it sorted already?


This visits the items in tree order. Within each list sliver, the tree order of the items (their order as children of the sliver) is in the sliver's growth direction. So for the top sliver, the items appear from bottom to top, the reverse of their order as Zulip messages.

gnprice · 2025-06-13T07:26:37Z

lib/widgets/message_list.dart

+  /// A message is considered onscreen if
+  /// - it covers the full height of the viewport
+  ///   (i.e. its top is above the viewport top
+  ///   and its bottom is below the viewport bottom) or
+  /// - its bottom is in the viewport (i.e. between the viewport top and bottom)


Hmm, in the first case I'd think we'd rather wait to mark the message read until the user reaches the bottom of the message.

With the new "Open message feeds at" setting, It'll be more common than before to approach unread messages from below. There's also the point about how we'd like there to be at least one message in the result, so we can do the convex-hull thing from one result to the next result.

Hmm, in the first case I'd think we'd rather wait to mark the message read until the user reaches the bottom of the message.

My more-awake brain today has come around to agreeing with this :)

gnprice · 2025-06-13T07:37:12Z

lib/model/message.dart

+      } catch (e) {
+        // do nothing
+      }


This part goes better if the messages only get removed from the queue after the request succeeds. (I believe that's what the legacy app does.)

That way if there's a network hiccup or something, those messages effectively get retried and eventually marked as read.

timabbott · 2025-06-14T00:06:39Z

When we merge this, we should revert the hunk from zulip/zulip#34887 (comment).

chrisbobbe · 2025-06-14T02:20:09Z

Revision pushed; Greg, PTAL!

chrisbobbe · 2025-06-14T02:26:40Z

lib/model/message.dart

+        i++;
+        final message = messages[messageId];
+        if (message == null) continue;
+        if (!message.flags.contains(MessageFlag.read)) {
+          toSend.add(message.id);
+        }


Equivalent and probably better, right:

final message = messages[messageId]; if (message != null && !message.flags.contains(MessageFlag.read)) { toSend.add(message.id); } i++;

gnprice

Exciting! Here's comments from a closer read just now, of the model/message.dart and model/message_list.dart changes.

gnprice · 2025-06-14T03:01:25Z

lib/model/message.dart

+        if (toSend.length == _markReadOnScrollBatchSize) {
+          break;
+        }
+        i++;


nit: a more descriptive name is probably helpful for this; perhaps numFromQueue?

gnprice · 2025-06-14T03:09:09Z

lib/model/message.dart

+
+      await Future<void>.delayed(_markReadOnScrollDebounceDuration);
+      if (_disposed) return;
+    } while (!_markReadOnScrollQueue.isEmpty);


nit: isNotEmpty

Suggested change

} while (!_markReadOnScrollQueue.isEmpty);

} while (_markReadOnScrollQueue.isNotEmpty);

gnprice · 2025-06-14T03:12:54Z

lib/model/message.dart

+    final anyAdded = _markReadOnScrollQueue.add(messageIds);
+    if (!anyAdded) return;
+    if (_markReadOnScrollBusy) return;


Is the anyAdded condition needed? If the queue is nonempty but the "busy" flag is false, then nothing's working on the queue; so then it seems good to start doing so, even if this call didn't happen to add anything new to the queue.

(Also it seems like a bug if that situation happens in the first place.)

Without that condition, the add implementation can also get a bit simpler.

gnprice · 2025-06-14T03:15:07Z

lib/model/message.dart

+    //   (in Unreads and on the message objects)
+    try {
+      await updateMessageFlags(connection,
+        messages: toSend.toList(),


Looks like this method could take type List for toSend, and then no need to copy here. (The argument it gets passed is already a List, and isn't getting reused to be modified by anything else so there's no semantic need to copy it.)

gnprice · 2025-06-14T03:24:08Z

lib/model/message.dart

+    } while (!_markReadOnScrollQueue.isEmpty);
+    _markReadOnScrollBusy = false;


Oh, speaking of the possibility of bugs causing surprising states. This clearing of the busy flag should be in a finally:

Suggested change

} while (!_markReadOnScrollQueue.isEmpty);

_markReadOnScrollBusy = false;

} while (!_markReadOnScrollQueue.isEmpty);

} finally {

if (!_disposed) {

_markReadOnScrollBusy = false;

}

}

with the try starting immediately after setting the flag:

_markReadOnScrollBusy = true; try { do {

That way if this loop stops its work for any reason — in particular an unexpected exception — it'll always clear the flag, so that a future call can still try again.

gnprice · 2025-06-14T03:33:48Z

lib/model/message_list.dart

+      // TODO(log)
+      return null;
+    }
+    return messages.sublist(firstIndex, lastIndex + 1);


This is making a new List by copying the given range of elements. But it looks like the one caller immediately consumes it as effectively an Iterable. So this could avoid the copy by instead using List.getRange.

chrisbobbe · 2025-06-14T04:11:39Z

Thanks!! Revision pushed.

gnprice · 2025-06-14T04:10:24Z

lib/model/message.dart

+  ///
+  /// Returns true if some were added (i.e. weren't already in the queue),
+  /// false otherwise.
+  bool add(Iterable<int> messageIds) {


nit: containers like List and Set have methods add to add one element and addAll to add from an iterable, so probably clearest to match that naming:

Suggested change

bool add(Iterable<int> messageIds) {

bool addAll(Iterable<int> messageIds) {

gnprice · 2025-06-14T04:14:02Z

lib/model/message_list.dart

@@ -222,6 +222,17 @@ mixin _MessageSequence {
    return binarySearchByKey(items, messageId, _compareItemToMessageId);
  }

+  Iterable<Message>? getMessagesSublist(int firstMessageId, int lastMessageId) {


nit: rename to match new semantics:

Suggested change

Iterable<Message>? getMessagesSublist(int firstMessageId, int lastMessageId) {

Iterable<Message>? getMessagesRange(int firstMessageId, int lastMessageId) {

gnprice

OK, and here's comments on the widgets/message_list.dart changes. All the suggested changes here are NFC.

gnprice · 2025-06-14T04:30:37Z

lib/main.dart


 void main() {
  assert(() {
    debugLogEnabled = true;
+    MessageListPage.debugEnableMarkReadOnScroll = true;


It doesn't feel great to me to have to set something like this in the main function, vs. having tests disable it.

Trying flutter test with the default flipped, it looks like there are just four test files with failing tests: autocomplete, action_sheet, compose_box, message_list (all in test/widgets/). So maybe just set it to false at the top of each of those?

If we did want to do it globally across all tests, there's a handy file test/flutter_test_config.dart for that. That's probably cleaner than here, because it means the default where the flag is declared is the default that applies in the live app.

(Potentially debugLogEnabled should go there too; I think I added that flag very early and may not have known about flutter_test_config at the time. But also it bothers me less to have it here than this flag would, because it's not in the user-facing functionality of the app.)

it looks like there are just four test files with failing tests: autocomplete, action_sheet, compose_box, message_list (all in test/widgets/)

(I also get failures in lightbox and emoji_reaction tests; those are easy to treat the same way)

Cool. Yeah, what I did was:

Run flutter test.

Scan the 158 resulting test failures by eye as they scroll past (each of which is like 20 lines of output, designed to be enough information to actually investigate the failure), and try to spot all the different filenames that appear.

So fairly error-prone.

(I'm not sure flutter test / dart test has a better way out of the box for collecting more of a summary of which tests failed, without all that detail about each failure. There are some verbosity controls, but I think only for turning up the verbosity on non-failing tests.)

gnprice · 2025-06-14T04:37:48Z

lib/widgets/message_list.dart

+  /// This view's decision whether to mark read on scroll,
+  /// overriding [GlobalSettings.markReadOnScroll].
+  ///
+  /// For example, this is set to false after pressing
+  /// "Mark as unread from here" in the message action sheet.
+  bool? get thisViewMarksReadOnScroll;
+  set thisViewMarksReadOnScroll(bool? value);


Given that this has essentially the same semantics when not null as the global markReadOnScroll, just overrides the latter, I actually like the idea of giving it the same name markReadOnScroll.

That way matches the pattern foo ?? theme.foo ?? _defaults.foo that we use in some other places. It also means fewer names for essentially the same concept.

gnprice · 2025-06-14T04:38:31Z

lib/widgets/message_list.dart

+  ///
+  /// Defaults to false, so we don't have to set to false all over our tests --
+  /// but main.dart sets `true` so the feature is enabled when you run the app.
+  static bool get debugEnableMarkReadOnScroll {


For this one, on the other hand, it's good the name is different (with "enable") — a true here has quite a different meaning from in the other variables, and in particular true and false are asymmetric.

gnprice · 2025-06-14T04:42:35Z

lib/widgets/message_list.dart

+    if (
+      widget.thisViewMarksReadOnScroll
+      ?? GlobalStoreWidget.settingsOf(context).markReadOnScrollForNarrow(widget.narrow)
+    ) {


Likely clearer if the logic that gives effect to debugEnableMarkReadOnScroll is consolidated here too, so they're all next to each other to make their relationships clear.

Perhaps as a method (or getter?) named like _effectiveMarkReadOnScroll.

gnprice · 2025-06-14T04:45:41Z

lib/widgets/message_list.dart

@@ -576,7 +713,44 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
    _prevFetched = model.fetched;
  }

+  void _markReadOnScroll() {


Hmmm but possibly confusing that this method (and the one on MessageStore) have the same name as the setting.

I think "on" isn't quite the right preposition here anyway — calling this method is a way of saying "please go and mark read, because scrolling happened", not "please mark read if and when scrolling happens".

Perhaps _markReadFromScroll? And similarly for the MessageStore method.

gnprice · 2025-06-14T04:47:06Z

lib/widgets/message_list.dart

  MessageListView get model => _model!;
  MessageListView? _model;

  final MessageListScrollController scrollController = MessageListScrollController();

  final ValueNotifier<bool> _scrollToBottomVisible = ValueNotifier<bool>(false);

+  (int, int)? _messagesRecentlyInViewport;


nit: move this next to _markReadOnScroll, as that's the code that's really responsible for maintaining it (and that consumes it)

gnprice · 2025-06-14T04:48:45Z

lib/widgets/message_list.dart

+  /// A message is considered onscreen if its bottom edge is in the viewport.
+  ///
+  /// Ignores outbox messages.
+  (int, int)? _findMessagesInViewport() {


nit: move this and the other new method _isMessageItemInViewport down below the lifecycle methods, and below their close partner _modelChanged; I guess that puts them right near _markReadOnScroll and the existing scroll-handling methods

gnprice · 2025-06-14T04:53:10Z

lib/widgets/message_list.dart

+      }
+      element.visitChildElements(visit);


nit: this is equivalent, right?

Suggested change

}

element.visitChildElements(visit);

default:

element.visitChildElements(visit);

}

Then it's more immediately clear that the recursion is only for widgets not among the cases above.

gnprice · 2025-06-14T04:54:28Z

lib/widgets/message_list.dart

+      final widget = element.widget;
+
+      switch (widget) {


nit: these seem closely tied together

Suggested change

final widget = element.widget;

switch (widget) {

final widget = element.widget;

switch (widget) {

gnprice · 2025-06-14T05:02:45Z

lib/widgets/message_list.dart

+        case MessageItem():
+          if (widget.item is! MessageListMessageItem) return; // ignore outbox
+


How about:

Suggested change

case MessageItem():

if (widget.item is! MessageListMessageItem) return; // ignore outbox

case MessageItem(item: MessageListOutboxMessageItem()):

return; // ignore outbox

case MessageItem(item: MessageListMessageItem(:final message)):

Then the later line

final message = (widget.item as MessageListMessageItem).message;

can be dropped.

gnprice

OK, and read the settings changes. I think that covers it!

Just one nontrivial comment here (which is still an NFC suggestion), to split the setting to a separate commit.

gnprice · 2025-06-14T05:21:06Z

lib/widgets/settings.dart

+          RadioListTile.adaptive(
+            title: Text(_valueDisplayName(value,
+              zulipLocalizations: zulipLocalizations)),
+            subtitle: () {
+              final result = _valueDisplayDescription(value,


nit: "description" seems enough to make "display" redundant:

Suggested change

RadioListTile.adaptive(

title: Text(_valueDisplayName(value,

zulipLocalizations: zulipLocalizations)),

subtitle: () {

final result = _valueDisplayDescription(value,

RadioListTile.adaptive(

title: Text(_valueDisplayName(value,

zulipLocalizations: zulipLocalizations)),

subtitle: () {

final result = _valueDescription(value,

(by contrast "name" could mean an identifier-style name used in the code, or the name displayed to the user)

gnprice · 2025-06-14T05:28:05Z

lib/widgets/settings.dart

@@ -150,6 +151,86 @@ class VisitFirstUnreadSettingPage extends StatelessWidget {
  }
 }

+class _MarkReadOnScrollSetting extends StatelessWidget {


I think this main commit could productively be split in two:

A final commit adds the setting.

A preceding commit adds all the logic in model/message and model/message_list and widgets/message_list and so on which the setting will enable, but behaves as if the setting were always false/never.

If we were writing tests for this synchronously, then after the first commit the logic would get exercised by the tests, just never in the live app. Potentially that would require a small extra mechanism, deleted in the second commit, to allow turning the behavior on. (I think not even that, though — the tests could set thisViewMarksReadOnScroll to true.)

The main value of this split is that the setting is a fair amount of code, and all of a quite different character from most of the rest of the changes.

chrisbobbe · 2025-06-14T06:05:38Z

Thanks! Revision pushed, and it's time for bed for me :)

gnprice

Thanks for the revision! Those changes all look good.

One comment below, after I looked a bit more at the addAutomaticKeepAlives question.

gnprice · 2025-06-14T06:30:08Z

lib/widgets/message_list.dart

      delegate: SliverChildBuilderDelegate(
+        // None of the children ever need to keep themselves alive;
+        // opt out of adding AutomaticKeepAlive widgets that we don't need.
+        // (This helps keep _findMessagesInViewport efficient.)
+        addAutomaticKeepAlives: false,


Looking into this a bit, I'm nearly convinced we never currently need keep-alive in these list items.

I'm less sure we'll never want it in the future — for example, being able to select text in a message is a repeatedly-requested feature, and selecting text would cause the child to try to keep itself alive. And I'm not entirely sure what the consequences would be of a widget getting discarded after all when it tried to keep itself alive. So this seems like potentially a pitfall for future changes, when we might not be thinking about this feature.

The way I looked for things that would invoke this is: it seems it's all about receiving KeepAliveNotification instances. I looked for call sites of that class's constructor; there's only one, in AutomaticKeepAliveClientMixin. So then I looked for subclasses of that mixin, and there are 5 of them:
_DismissibleState
EditableTextState
_InkResponseState
_SelectionKeepAliveState
_CupertinoTextFieldState

So selection, text fields (plain, Material which uses plain, and Cupertino), InkWell, and Dismissible which is about items the user can swipe out of a list.

Some of those are unlikely to ever appear, like Dismissible. But for InkWell, I'm not 100% sure we don't already have something in the message list that uses it.

When we measured the timing, the walk of the element tree took a very short amount of time even in debug mode. With this flag true (the default), it'll have roughly double as many elements to walk through, but that'll still make a negligible amount of time. So let's leave this commit out for launch.

When we add the setting for this, coming up, this long-awaited feature will become active. Hooray! This still needs tests. We're tracking that as zulip#1583 for early post-launch. (The launch is coming up very soon.)

This still needs tests. We're tracking this as zulip#1583 for early post-launch. (The launch is coming up very soon.) Fixes: zulip#81

gnprice · 2025-06-14T06:36:11Z

And merging! After dropping that addAutomaticKeepAlives: false commit:
2cbb71d msglist: Opt out of adding AutomaticKeepAlive widgets to the tree

As zulip/zulip-flutter#1574 was merged for this feature, we add back the instructions for automatically marking messages as read on scroll for the mobile app. Part of zulip#34748.

chrisbobbe requested a review from gnprice June 13, 2025 06:58

chrisbobbe force-pushed the pr-mark-as-read-on-scroll branch from 2e287d5 to 4016b74 Compare June 13, 2025 07:09

gnprice reviewed Jun 13, 2025

View reviewed changes

gnprice mentioned this pull request Jun 13, 2025

logo: Switch app icons to non-"beta" versions #1577

Merged

chrisbobbe mentioned this pull request Jun 13, 2025

Write tests for mark-read-on-scroll #1583

Open

chrisbobbe force-pushed the pr-mark-as-read-on-scroll branch from 4016b74 to a372d91 Compare June 14, 2025 00:22

chrisbobbe marked this pull request as ready for review June 14, 2025 00:22

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Jun 14, 2025

chrisbobbe assigned gnprice Jun 14, 2025

gnprice mentioned this pull request Jun 14, 2025

Try latency compensation on marking messages as read #685

Open

chrisbobbe force-pushed the pr-mark-as-read-on-scroll branch from a372d91 to 7280e57 Compare June 14, 2025 02:18

chrisbobbe changed the title ~~WIP mark-as-read-on-scroll~~ Implement mark-as-read-on-scroll, v1 Jun 14, 2025

chrisbobbe force-pushed the pr-mark-as-read-on-scroll branch from 7280e57 to 1f2367b Compare June 14, 2025 02:22

chrisbobbe commented Jun 14, 2025

View reviewed changes

gnprice reviewed Jun 14, 2025

View reviewed changes

chrisbobbe force-pushed the pr-mark-as-read-on-scroll branch from 1f2367b to a90d7e0 Compare June 14, 2025 04:11

gnprice reviewed Jun 14, 2025

View reviewed changes

chrisbobbe force-pushed the pr-mark-as-read-on-scroll branch from a90d7e0 to 8b5d0d4 Compare June 14, 2025 06:05

gnprice reviewed Jun 14, 2025

View reviewed changes

chrisbobbe added 2 commits June 13, 2025 23:31

msglist: Implement mark-read-on-scroll, without yet enabling

46671b4

When we add the setting for this, coming up, this long-awaited feature will become active. Hooray! This still needs tests. We're tracking that as zulip#1583 for early post-launch. (The launch is coming up very soon.)

msglist: Enable mark-read-on-scroll feature, with new global setting!

301384c

This still needs tests. We're tracking this as zulip#1583 for early post-launch. (The launch is coming up very soon.) Fixes: zulip#81

gnprice force-pushed the pr-mark-as-read-on-scroll branch from 8b5d0d4 to 301384c Compare June 14, 2025 06:36

gnprice merged commit 301384c into zulip:main Jun 14, 2025
1 check passed

gnprice mentioned this pull request Jun 14, 2025

help: Remove mobile instructions for features not in Flutter app launch. zulip/zulip#34887

Merged

12 tasks

chrisbobbe deleted the pr-mark-as-read-on-scroll branch June 14, 2025 07:30

	} while (!_markReadOnScrollQueue.isEmpty);
	} while (_markReadOnScrollQueue.isNotEmpty);

		} while (!_markReadOnScrollQueue.isEmpty);
		_markReadOnScrollBusy = false;

	bool add(Iterable<int> messageIds) {
	bool addAll(Iterable<int> messageIds) {

	Iterable<Message>? getMessagesSublist(int firstMessageId, int lastMessageId) {
	Iterable<Message>? getMessagesRange(int firstMessageId, int lastMessageId) {

		case MessageItem():
		if (widget.item is! MessageListMessageItem) return; // ignore outbox

Implement mark-as-read-on-scroll, v1 #1574

Implement mark-as-read-on-scroll, v1 #1574

Uh oh!

Conversation

chrisbobbe commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice commented Jun 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timabbott commented Jun 14, 2025

Uh oh!

chrisbobbe commented Jun 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jun 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jun 13, 2025 •

edited

Loading

chrisbobbe commented Jun 13, 2025 •

edited

Loading