WIP mark-as-read-on-scroll

TODO:

- Audit for efficiency -- I keep feeling dread when adding things
  like `.sort()` because I know this is a hot codepath :)

- Audit for correctness. Can the "range hull" logic end up naively
  applying its math 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 unpredictably? (when is
  _modelChanged called?)

- User setting

- Latency compensation (can be a followup maybe?)

- Tests

Author: chrisbobbe

lib/model/message.dart

@@ -1,7 +1,7 @@
 import 'dart:async';
-import 'dart:collection';
 import 'dart:convert';
 
+import 'package:collection/collection.dart';
 import 'package:crypto/crypto.dart';
 import 'package:flutter/foundation.dart';
 
@@ -28,6 +28,8 @@ mixin MessageStore {
   void registerMessageList(MessageListView view);
   void unregisterMessageList(MessageListView view);
 
+  void markReadOnScroll(Iterable<int> messageIds);
+
   Future<void> sendMessage({
     required MessageDestination destination,
     required String content,
@@ -180,6 +182,37 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore, _OutboxMes
     _disposed = true;
   }
 
+  final _markReadOnScrollQueue = _MarkReadOnScrollQueue();
+  bool _markReadOnScrollBusy = false;
+
+  @override
+  void markReadOnScroll(Iterable<int> messageIds) async {
+    final anyAdded = _markReadOnScrollQueue.add(messageIds);
+    if (!anyAdded) return;
+    if (_markReadOnScrollBusy) return;
+
+    _markReadOnScrollBusy = true;
+    do {
+      final batch = _markReadOnScrollQueue.take(1000)
+        .whereNot((id) => messages[id]?.flags.contains(MessageFlag.read) ?? false).toList();
+      if (batch.isEmpty) continue;
+
+      // TODO mark as read locally for latency compensation
+      //   (in Unreads and on the message objects)
+      try {
+        await updateMessageFlags(connection,
+          messages: batch,
+          op: UpdateMessageFlagsOp.add,
+          flag: MessageFlag.read);
+      } catch (e) {
+        // do nothing
+      }
+
+      await Future<void>.delayed(Duration(milliseconds: 500));
+    } while (!_markReadOnScrollQueue.isEmpty);
+    _markReadOnScrollBusy = false;
+  }
+
   @override
   Future<void> sendMessage({required MessageDestination destination, required String content}) {
     assert(!_disposed);
@@ -517,6 +550,38 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore, _OutboxMes
   }
 }
 
+class _MarkReadOnScrollQueue {
+  _MarkReadOnScrollQueue();
+
+  bool get isEmpty => _queue.isEmpty;
+
+  final _set = <int>{};
+  final _queue = QueueList<int>();
+
+  /// Add [messageIds] to the end of the queue,
+  /// if they aren't already in the queue.
+  ///
+  /// Returns true if some were added (i.e. weren't already in the queue),
+  /// false otherwise.
+  bool add(Iterable<int> messageIds) {
+    final newMessageIds = messageIds.whereNot((id) => _set.contains(id));
+    if (newMessageIds.isEmpty) return false;
+    _queue.addAll(newMessageIds);
+    _set.addAll(newMessageIds);
+    return true;
+  }
+
+  List<int> take(int count) {
+    final result = <int>[];
+    for (int i = 0; i < count; i++) {
+      if (_queue.isEmpty) break;
+      result.add(_queue.removeFirst());
+    }
+    _set.removeAll(result);
+    return result;
+  }
+}
+
 /// The duration an outbox message stays hidden to the user.
 ///
 /// See [OutboxMessageState.waiting].

lib/model/message_list.dart

@@ -222,6 +222,15 @@ mixin _MessageSequence {
     return binarySearchByKey(items, messageId, _compareItemToMessageId);
   }
 
+  List<Message> getMessagesSublist(int firstMessageId, int lastMessageId) {
+    final firstIndex = _findMessageWithId(firstMessageId);
+    final lastIndex = _findMessageWithId(lastMessageId);
+    if (firstIndex == -1 || lastIndex == -1) {
+      throw StateError('messagesSublist called with message IDs not in list');
+    }
+    return messages.sublist(firstIndex, lastIndex + 1);
+  }
+
   static int _compareItemToMessageId(MessageListItem item, int messageId) {
     switch (item) {
       case MessageListRecipientHeaderItem(:var message):

lib/model/store.dart

@@ -754,6 +754,9 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor
   void unregisterMessageList(MessageListView view) =>
     _messages.unregisterMessageList(view);
   @override
+  void markReadOnScroll(Iterable<int> messageIds) =>
+    _messages.markReadOnScroll(messageIds);
+  @override
   Future<void> sendMessage({required MessageDestination destination, required String content}) {
     assert(!_disposed);
     return _messages.sendMessage(destination: destination, content: content);

lib/widgets/action_sheet.dart

@@ -896,9 +896,11 @@ class MarkAsUnreadButton extends MessageActionSheetMenuItemButton {
   }
 
   @override void onPressed() async {
-    final narrow = findMessageListPage().narrow;
+    final messageListPage = findMessageListPage();
     unawaited(ZulipAction.markNarrowAsUnreadFromMessage(pageContext,
-      message, narrow));
+      message, messageListPage.narrow));
+    // TODO should we alert the user about this change somehow? A snackbar?
+    messageListPage.markReadOnScrollEnabled = false;
   }
 }
 

lib/widgets/message_list.dart

@@ -139,6 +139,14 @@ abstract class MessageListPageState {
   ///
   /// This is null if [MessageList] has not mounted yet.
   MessageListView? get model;
+
+  /// Whether this message-list page is marking messages as read on scroll.
+  ///
+  /// This is local for the page and can differ from
+  /// <TODO reference global setting>, for example it's turned off when
+  /// pressing "Mark as unread from here" in the message action sheet.
+  bool get markReadOnScrollEnabled;
+  set markReadOnScrollEnabled(bool value);
 }
 
 class MessageListPage extends StatefulWidget {
@@ -186,10 +194,21 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
   MessageListView? get model => _messageListKey.currentState?.model;
   final GlobalKey<_MessageListState> _messageListKey = GlobalKey();
 
+  @override
+  bool get markReadOnScrollEnabled => _markReadOnScrollEnabled;
+  late bool _markReadOnScrollEnabled;
+  @override
+  set markReadOnScrollEnabled(bool value) {
+    setState(() {
+      _markReadOnScrollEnabled = value;
+    });
+  }
+
   @override
   void initState() {
     super.initState();
     narrow = widget.initNarrow;
+    _markReadOnScrollEnabled = true; // TODO initialize from GlobalSettingsStore
   }
 
   void _narrowChanged(Narrow newNarrow) {
@@ -298,6 +317,7 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
                   narrow: narrow,
                   initAnchor: initAnchor,
                   onNarrowChanged: _narrowChanged,
+                  markReadOnScrollEnabled: markReadOnScrollEnabled,
                 ))),
             if (ComposeBox.hasComposeBox(narrow))
               ComposeBox(key: _composeBoxKey, narrow: narrow)
@@ -503,24 +523,102 @@ class MessageList extends StatefulWidget {
     required this.narrow,
     required this.initAnchor,
     required this.onNarrowChanged,
+    required this.markReadOnScrollEnabled,
   });
 
   final Narrow narrow;
   final Anchor initAnchor;
   final void Function(Narrow newNarrow) onNarrowChanged;
+  final bool markReadOnScrollEnabled;
 
   @override
   State<StatefulWidget> createState() => _MessageListState();
 }
 
 class _MessageListState extends State<MessageList> with PerAccountStoreAwareStateMixin<MessageList> {
+  final GlobalKey _scrollViewKey = GlobalKey();
+
   MessageListView get model => _model!;
   MessageListView? _model;
 
   final MessageListScrollController scrollController = MessageListScrollController();
 
   final ValueNotifier<bool> _scrollToBottomVisible = ValueNotifier<bool>(false);
 
+  List<int>? _messagesRecentlyInViewport;
+
+  /// Which messages are onscreen.
+  ///
+  /// Ignores outbox messages.
+  ///
+  /// 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)
+  ///
+  /// This definition is helpful for mark-as-read-on-scroll, because
+  /// when this method is called during a fast scroll through many messages,
+  /// the returned range (almost*) always includes at least one message.
+  /// For the mark-as-read request, we "fill in" any messages
+  /// between that message(s) and the messages currently in view.
+  /// That can be needed when the scrolling is so fast that
+  /// some messages go by without rendering in the viewport.
+  ///
+  /// (*It can be empty in the middle of the message list, rarely,
+  /// if a tall message covers most of the viewport
+  /// but its bottom isn't visible and the message above it is offscreen,
+  /// on the far side of a date separator or recipient header.)
+  List<int> _getMessagesInViewport() {
+    final messageItemElements = <Element>[];
+    void visit(Element element) {
+      final widget = element.widget;
+      if (widget is MessageItem) {
+        if (widget.item is MessageListMessageItem) {
+          messageItemElements.add(element);
+        }
+        return;
+      }
+      element.visitChildElements(visit);
+    }
+
+    final scrollViewElement = _scrollViewKey.currentContext as Element;
+    // TODO this frequently ends up walking through a few hundred elements,
+    //   only 5-10 of which end up being MessageItem ones.
+    scrollViewElement.visitChildElements(visit);
+
+    final scrollViewRenderObject = scrollViewElement.renderObject as RenderBox;
+    final viewportHeight = scrollViewRenderObject.size.height;
+
+    final result = <int>[];
+    for (final element in messageItemElements) {
+      final renderObject = element.renderObject as RenderBox;
+      final widget = element.widget as MessageItem;
+      final item = widget.item as MessageListMessageItem; // (see `visit`)
+      final message = item.message;
+
+      final messageHeight = renderObject.size.height;
+      final messageTop = renderObject.localToGlobal(
+        Offset.zero, ancestor: scrollViewRenderObject).dy;
+      final messageBottom = renderObject.localToGlobal(
+        Offset(0, messageHeight), ancestor: scrollViewRenderObject).dy;
+
+      final doesMessageSpanViewport = messageTop < 0 && messageBottom > viewportHeight;
+      if (doesMessageSpanViewport) {
+        result.add(message.id);
+        break;
+      }
+
+      final isMessageBottomVisible = messageBottom > 0 && messageBottom < viewportHeight;
+      if (isMessageBottomVisible) {
+        result.add(message.id);
+      }
+    }
+    // TODO why isn't it sorted already?
+    result.sort();
+    return result;
+  }
+
   @override
   void initState() {
     super.initState();
@@ -552,6 +650,17 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
   bool _prevFetched = false;
 
   void _modelChanged() {
+    // When you're scrolling quickly, our mark-as-read requests include the
+    // messages between _recentMessageRangeInViewport and the current range,
+    // so messages don't get left out because you were scrolling so fast
+    // that they never rendered onscreen.
+    //
+    // Here, the onscreen messages might be totally different,
+    // and not because of scrolling (e.g. because the narrow changed).
+    // Avoid "filling in" a mark-as-read request with totally wrong messages,
+    // by forgetting the old range.
+    _messagesRecentlyInViewport = null;
+
     if (model.narrow != widget.narrow) {
       // Either:
       // - A message move event occurred, where propagate mode is
@@ -576,7 +685,37 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
     _prevFetched = model.fetched;
   }
 
+  void _markReadOnScroll() {
+    final currentRange = _getMessagesInViewport();
+    if (currentRange.isEmpty) return;
+
+    final currentFirst = currentRange.first;
+    final currentLast = currentRange.last;
+    final prevFirst = _messagesRecentlyInViewport?.first;
+    final prevLast = _messagesRecentlyInViewport?.last;
+
+    // ("Hull" as in the "convex hull" around the old and new ranges.)
+    final firstOfHull = switch ((prevFirst, currentFirst)) {
+      (int previous, int current) => previous < current ? previous : current,
+      (           _, int current) => current,
+    };
+
+    final lastOfHull = switch ((prevLast, currentLast)) {
+      (int previous, int current) => previous > current ? previous : current,
+      (           _, int current) => current,
+    };
+
+    final sublist = model.getMessagesSublist(firstOfHull, lastOfHull);
+    model.store.markReadOnScroll(sublist.map((message) => message.id));
+
+    _messagesRecentlyInViewport = currentRange;
+  }
+
   void _handleScrollMetrics(ScrollMetrics scrollMetrics) {
+    if (widget.markReadOnScrollEnabled) {
+      _markReadOnScroll();
+    }
+
     if (scrollMetrics.extentAfter == 0) {
       _scrollToBottomVisible.value = false;
     } else {
@@ -745,6 +884,8 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
     }
 
     return MessageListScrollView(
+      key: _scrollViewKey,
+
       // TODO: Offer `ScrollViewKeyboardDismissBehavior.interactive` (or
       //   similar) if that is ever offered:
       //     https://github.com/flutter/flutter/issues/57609#issuecomment-1355340849