Merge remote-tracking branch 'pr/1566'

gnprice · gnprice · commit 385ee8e8a747 · 2025-06-11T21:47:39.000-07:00
diff --git a/lib/model/internal_link.dart b/lib/model/internal_link.dart
@@ -109,22 +109,43 @@ Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) {
   return result;
 }
 
-/// A [Narrow] from a given URL, on `store`'s realm.
+/// The result of parsing some URL within a Zulip realm,
+/// when the URL corresponds to some page in this app.
+sealed class InternalLink {
+  InternalLink({required this.realmUrl});
+
+  final Uri realmUrl;
+}
+
+/// The result of parsing some URL that points to a narrow on a Zulip realm,
+/// when the narrow is of a type that this app understands.
+class NarrowLink extends InternalLink {
+  NarrowLink(this.narrow, this.nearMessageId, {required super.realmUrl});
+
+  final Narrow narrow;
+  final int? nearMessageId;
+}
+
+/// Try to parse the given URL as a page in this app, on `store`'s realm.
 ///
 /// `url` must already be a result from [PerAccountStore.tryResolveUrl]
 /// on `store`.
 ///
-/// Returns `null` if any of the operator/operand pairs are invalid.
+/// Returns null if the URL isn't on this realm,
+/// or isn't a valid Zulip URL,
+/// or isn't currently supported as leading to a page in this app.
 ///
+/// In particular this will return null if `url` is a `/#narrow/…` URL
+/// and any of the operator/operand pairs are invalid.
 /// Since narrow links can combine operators in ways our [Narrow] type can't
 /// represent, this can also return null for valid narrow links.
 ///
 /// This can also return null for some valid narrow links that our Narrow
 /// type *could* accurately represent. We should try to understand these
-/// better, but some kinds will be rare, even unheard-of:
+/// better, but some kinds will be rare, even unheard-of.  For example:
 ///   #narrow/stream/1-announce/stream/1-announce (duplicated operator)
 // TODO(#252): handle all valid narrow links, returning a search narrow
-Narrow? parseInternalLink(Uri url, PerAccountStore store) {
+InternalLink? parseInternalLink(Uri url, PerAccountStore store) {
   if (!_isInternalLink(url, store.realmUrl)) return null;
 
   final (category, segments) = _getCategoryAndSegmentsFromFragment(url.fragment);
@@ -155,7 +176,7 @@ bool _isInternalLink(Uri url, Uri realmUrl) {
   return (category, segments);
 }
 
-Narrow? _interpretNarrowSegments(List<String> segments, PerAccountStore store) {
+NarrowLink? _interpretNarrowSegments(List<String> segments, PerAccountStore store) {
   assert(segments.isNotEmpty);
   assert(segments.length.isEven);
 
@@ -164,6 +185,7 @@ Narrow? _interpretNarrowSegments(List<String> segments, PerAccountStore store) {
   ApiNarrowDm? dmElement;
   ApiNarrowWith? withElement;
   Set<IsOperand> isElementOperands = {};
+  int? nearMessageId;
 
   for (var i = 0; i < segments.length; i += 2) {
     final (operator, negated) = _parseOperator(segments[i]);
@@ -201,24 +223,26 @@ Narrow? _interpretNarrowSegments(List<String> segments, PerAccountStore store) {
         // It is fine to have duplicates of the same [IsOperand].
         isElementOperands.add(IsOperand.fromRawString(operand));
 
-      case _NarrowOperator.near: // TODO(#82): support for near
-        continue;
+      case _NarrowOperator.near:
+        if (nearMessageId != null) return null;
+        nearMessageId = int.tryParse(operand, radix: 10);
 
       case _NarrowOperator.unknown:
         return null;
     }
   }
 
+  final Narrow? narrow;
   if (isElementOperands.isNotEmpty) {
     if (streamElement != null || topicElement != null || dmElement != null || withElement != null) {
       return null;
     }
     if (isElementOperands.length > 1) return null;
     switch (isElementOperands.single) {
       case IsOperand.mentioned:
-        return const MentionsNarrow();
+        narrow = const MentionsNarrow();
       case IsOperand.starred:
-        return const StarredMessagesNarrow();
+        narrow = const StarredMessagesNarrow();
       case IsOperand.dm:
       case IsOperand.private:
       case IsOperand.alerted:
@@ -230,17 +254,20 @@ Narrow? _interpretNarrowSegments(List<String> segments, PerAccountStore store) {
     }
   } else if (dmElement != null) {
     if (streamElement != null || topicElement != null || withElement != null) return null;
-    return DmNarrow.withUsers(dmElement.operand, selfUserId: store.selfUserId);
+    narrow = DmNarrow.withUsers(dmElement.operand, selfUserId: store.selfUserId);
   } else if (streamElement != null) {
     final streamId = streamElement.operand;
     if (topicElement != null) {
-      return TopicNarrow(streamId, topicElement.operand, with_: withElement?.operand);
+      narrow = TopicNarrow(streamId, topicElement.operand, with_: withElement?.operand);
     } else {
       if (withElement != null) return null;
-      return ChannelNarrow(streamId);
+      narrow = ChannelNarrow(streamId);
     }
+  } else {
+    return null;
   }
-  return null;
+
+  return NarrowLink(narrow, nearMessageId, realmUrl: store.realmUrl);
 }
 
 @JsonEnum(fieldRename: FieldRename.kebab, alwaysCreate: true)
diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart
@@ -479,7 +479,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
   /// which might be made internally by this class in order to
   /// fetch the messages from scratch, e.g. after certain events.
   Anchor get anchor => _anchor;
-  final Anchor _anchor;
+  Anchor _anchor;
 
   void _register() {
     store.registerMessageList(this);
@@ -756,6 +756,20 @@ class MessageListView with ChangeNotifier, _MessageSequence {
     }
   }
 
+  /// Reset this view to start from the newest messages.
+  ///
+  /// This will set [anchor] to [AnchorCode.newest],
+  /// and cause messages to be re-fetched from scratch.
+  void jumpToEnd() {
+    assert(fetched);
+    assert(!haveNewest);
+    assert(anchor != AnchorCode.newest);
+    _anchor = AnchorCode.newest;
+    _reset();
+    notifyListeners();
+    fetchInitial();
+  }
+
   /// Add [outboxMessage] if it belongs to the view.
   void addOutboxMessage(OutboxMessage outboxMessage) {
     // TODO(#1441) implement this
diff --git a/lib/notifications/display.dart b/lib/notifications/display.dart
@@ -513,7 +513,7 @@ class NotificationDisplayManager {
 
     return MessageListPage.buildRoute(
       accountId: account.id,
-      // TODO(#82): Open at specific message, not just conversation
+      // TODO(#1565): Open at specific message, not just conversation
       narrow: payload.narrow);
   }
 
diff --git a/lib/widgets/content.dart b/lib/widgets/content.dart
@@ -1536,15 +1536,18 @@ void _launchUrl(BuildContext context, String urlString) async {
     return;
   }
 
-  final internalNarrow = parseInternalLink(url, store);
-  if (internalNarrow != null) {
-    unawaited(Navigator.push(context,
-      MessageListPage.buildRoute(context: context,
-        narrow: internalNarrow)));
-    return;
+  final internalLink = parseInternalLink(url, store);
+  assert(internalLink == null || internalLink.realmUrl == store.realmUrl);
+  switch (internalLink) {
+    case NarrowLink():
+      unawaited(Navigator.push(context,
+        MessageListPage.buildRoute(context: context,
+          narrow: internalLink.narrow,
+          initAnchorMessageId: internalLink.nearMessageId)));
+
+    case null:
+      await PlatformActions.launchUrl(context, url);
   }
-
-  await PlatformActions.launchUrl(context, url);
 }
 
 /// Like [Image.network], but includes [authHeader] if [src] is on-realm.
diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart
@@ -141,12 +141,17 @@ abstract class MessageListPageState {
 }
 
 class MessageListPage extends StatefulWidget {
-  const MessageListPage({super.key, required this.initNarrow});
+  const MessageListPage({
+    super.key,
+    required this.initNarrow,
+    this.initAnchorMessageId,
+  });
 
   static AccountRoute<void> buildRoute({int? accountId, BuildContext? context,
-      required Narrow narrow}) {
+      required Narrow narrow, int? initAnchorMessageId}) {
     return MaterialAccountWidgetRoute(accountId: accountId, context: context,
-      page: MessageListPage(initNarrow: narrow));
+      page: MessageListPage(
+        initNarrow: narrow, initAnchorMessageId: initAnchorMessageId));
   }
 
   /// The [MessageListPageState] above this context in the tree.
@@ -162,6 +167,7 @@ class MessageListPage extends StatefulWidget {
   }
 
   final Narrow initNarrow;
+  final int? initAnchorMessageId; // TODO(#1564) highlight target upon load
 
   @override
   State<MessageListPage> createState() => _MessageListPageState();
@@ -240,6 +246,10 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
         actions.add(_TopicListButton(streamId: streamId));
     }
 
+    // TODO(#80): default to anchor firstUnread, instead of newest
+    final initAnchor = widget.initAnchorMessageId == null
+      ? AnchorCode.newest : NumericAnchor(widget.initAnchorMessageId!);
+
     // Insert a PageRoot here, to provide a context that can be used for
     // MessageListPage.ancestorOf.
     return PageRoot(child: Scaffold(
@@ -259,7 +269,8 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
       //   we matched to the Figma in 21dbae120. See another frame, which uses that:
       //     https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=147%3A9088&mode=dev
       body: Builder(
-        builder: (BuildContext context) => Column(
+        builder: (BuildContext context) {
+          return Column(
           // Children are expected to take the full horizontal space
           // and handle the horizontal device insets.
           // The bottom inset should be handled by the last child only.
@@ -279,11 +290,13 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
                 child: MessageList(
                   key: _messageListKey,
                   narrow: narrow,
+                  initAnchor: initAnchor,
                   onNarrowChanged: _narrowChanged,
                 ))),
             if (ComposeBox.hasComposeBox(narrow))
               ComposeBox(key: _composeBoxKey, narrow: narrow)
-          ]))));
+          ]);
+        })));
   }
 }
 
@@ -479,9 +492,15 @@ const kFetchMessagesBufferPixels = (kMessageListFetchBatchSize / 2) * _kShortMes
 /// When there is no [ComposeBox], also takes responsibility
 /// for dealing with the bottom inset.
 class MessageList extends StatefulWidget {
-  const MessageList({super.key, required this.narrow, required this.onNarrowChanged});
+  const MessageList({
+    super.key,
+    required this.narrow,
+    required this.initAnchor,
+    required this.onNarrowChanged,
+  });
 
   final Narrow narrow;
+  final Anchor initAnchor;
   final void Function(Narrow newNarrow) onNarrowChanged;
 
   @override
@@ -504,8 +523,9 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
 
   @override
   void onNewStore() { // TODO(#464) try to keep using old model until new one gets messages
+    final anchor = _model == null ? widget.initAnchor : _model!.anchor;
     _model?.dispose();
-    _initModel(PerAccountStoreWidget.of(context));
+    _initModel(PerAccountStoreWidget.of(context), anchor);
   }
 
   @override
@@ -516,10 +536,7 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
     super.dispose();
   }
 
-  void _initModel(PerAccountStore store) {
-    // TODO(#82): get anchor as page/route argument, instead of using newest
-    // TODO(#80): default to anchor firstUnread, instead of newest
-    final anchor = AnchorCode.newest;
+  void _initModel(PerAccountStore store, Anchor anchor) {
     _model = MessageListView.init(store: store,
       narrow: widget.narrow, anchor: anchor);
     model.addListener(_modelChanged);
@@ -535,6 +552,7 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
       //   redirected us to the new location of the operand message ID.
       widget.onNarrowChanged(model.narrow);
     }
+    // TODO when model reset, reset scroll
     setState(() {
       // The actual state lives in the [MessageListView] model.
       // This method was called because that just changed.
@@ -557,6 +575,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() {
@@ -607,6 +628,7 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
                   //   MessageList's dartdoc.
                   child: SafeArea(
                     child: ScrollToBottomButton(
+                      model: model,
                       scrollController: scrollController,
                       visible: _scrollToBottomVisible))),
               ])))));
@@ -742,13 +764,21 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
   }
 
   Widget _buildEndCap() {
-    return Column(crossAxisAlignment: CrossAxisAlignment.stretch, children: [
-      TypingStatusWidget(narrow: widget.narrow),
-      MarkAsReadWidget(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
-      const SizedBox(height: 36),
-    ]);
+    if (model.haveNewest) {
+      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),
+        // 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
+        const SizedBox(height: 36),
+      ]);
+    } else if (model.busyFetchingMore) {
+      // See [_buildStartCap] for why this condition shows a loading indicator.
+      return const _MessageListLoadingMore();
+    } else {
+      return SizedBox.shrink();
+    }
   }
 
   Widget _buildItem(MessageListItem data) {
@@ -798,13 +828,40 @@ class _MessageListLoadingMore extends StatelessWidget {
 }
 
 class ScrollToBottomButton extends StatelessWidget {
-  const ScrollToBottomButton({super.key, required this.scrollController, required this.visible});
+  const ScrollToBottomButton({
+    super.key,
+    required this.model,
+    required this.scrollController,
+    required this.visible,
+  });
 
-  final ValueNotifier<bool> visible;
+  final MessageListView model;
   final MessageListScrollController scrollController;
+  final ValueNotifier<bool> visible;
 
   void _scrollToBottom() {
-    scrollController.position.scrollToEnd();
+    if (model.haveNewest) {
+      // Scrolling smoothly from here to the bottom won't require any requests
+      // to the server.
+      // It also probably isn't *that* far away: the user must have scrolled
+      // here from there (or from near enough that a fetch reached there),
+      // so scrolling back there -- at top speed -- shouldn't take too long.
+      // Go for it.
+      scrollController.position.scrollToEnd();
+    } else {
+      // This message list doesn't have the messages for the bottom of history.
+      // There could be quite a lot of history between here and there --
+      // for example, at first unread in the combined feed or a busy channel,
+      // for a user who has some old unreads going back months and years.
+      // In that case trying to scroll smoothly to the bottom is hopeless.
+      //
+      // Given that there were at least 100 messages between this message list's
+      // initial anchor and the end of history (or else `fetchInitial` would
+      // have reached the end at the outset), that situation is very likely.
+      // Even if the end is close by, it's at least one fetch away.
+      // Instead of scrolling, jump to the end, which is always just one fetch.
+      model.jumpToEnd();
+    }
   }
 
   @override
diff --git a/test/model/internal_link_test.dart b/test/model/internal_link_test.dart
diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart

Original file line number	Diff line number	Diff line change
`@@ -513,7 +513,7 @@ class NotificationDisplayManager {`
`513`	`513`
`514`	`514`	`return MessageListPage.buildRoute(`
`515`	`515`	`accountId: account.id,`
`516`		`- // TODO(#82): Open at specific message, not just conversation`
	`516`	`+ // TODO(#1565): Open at specific message, not just conversation`
`517`	`517`	`narrow: payload.narrow);`
`518`	`518`	`}`
`519`	`519`