From 7bc258b6015725dba4202701aea83cbffd39951b Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 1 May 2025 20:11:29 -0700 Subject: [PATCH 1/8] msglist: Call fetchNewer when near bottom --- lib/widgets/message_list.dart | 3 +++ test/widgets/message_list_test.dart | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index aa9684b7f2..66fdec5b5c 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -568,6 +568,9 @@ class _MessageListState extends State with PerAccountStoreAwareStat // still not yet updated to account for the newly-added messages. model.fetchOlder(); } + if (scrollMetrics.extentAfter < kFetchMessagesBufferPixels) { + model.fetchNewer(); + } } void _scrollChanged() { diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 3b3b01b323..bd7403928f 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -445,6 +445,10 @@ void main() { }); group('fetch older messages on scroll', () { + // TODO(#1569): test fetch newer messages on scroll, too; + // in particular test it happens even when near top as well as bottom + // (because may have haveOldest true but haveNewest false) + int? itemCount(WidgetTester tester) => findScrollView(tester).semanticChildCount; From c02451f4d6b8cbd279cefbec186a24822ed25280 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 13 May 2025 17:22:10 -0700 Subject: [PATCH 2/8] msglist: Show loading indicator at bottom as well as top --- lib/widgets/message_list.dart | 22 ++++++--- test/widgets/message_list_test.dart | 75 ++++++++++++++++++++++++++--- 2 files changed, 84 insertions(+), 13 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 66fdec5b5c..1c2d177664 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -756,13 +756,21 @@ class _MessageListState extends State 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) { diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index bd7403928f..8cc216077a 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -59,6 +59,7 @@ void main() { bool foundOldest = true, int? messageCount, List? messages, + GetMessagesResult? fetchResult, List? streams, List? users, List? subscriptions, @@ -83,12 +84,17 @@ void main() { // prepare message list data await store.addUser(eg.selfUser); await store.addUsers(users ?? []); - assert((messageCount == null) != (messages == null)); - messages ??= List.generate(messageCount!, (index) { - return eg.streamMessage(sender: eg.selfUser); - }); - connection.prepare(json: - eg.newestGetMessagesResult(foundOldest: foundOldest, messages: messages).toJson()); + if (fetchResult != null) { + assert(foundOldest && messageCount == null && messages == null); + } else { + assert((messageCount == null) != (messages == null)); + messages ??= List.generate(messageCount!, (index) { + return eg.streamMessage(sender: eg.selfUser); + }); + fetchResult = eg.newestGetMessagesResult( + foundOldest: foundOldest, messages: messages); + } + connection.prepare(json: fetchResult.toJson()); await tester.pumpWidget(TestZulipApp(accountId: selfAccount.id, skipAssertAccountExists: skipAssertAccountExists, @@ -696,6 +702,63 @@ void main() { }); }); + // TODO test markers at start of list (`_buildStartCap`) + + group('markers at end of list', () { + final findLoadingIndicator = find.byType(CircularProgressIndicator); + + testWidgets('spacer when have newest', (tester) async { + final messages = List.generate(10, + (i) => eg.streamMessage(content: '

message $i

')); + await setupMessageListPage(tester, narrow: CombinedFeedNarrow(), + fetchResult: eg.nearGetMessagesResult(anchor: messages.last.id, + foundOldest: true, foundNewest: true, messages: messages)); + check(findMessageListScrollController(tester)!.position) + .extentAfter.equals(0); + + // There's no loading indicator. + check(findLoadingIndicator).findsNothing(); + // The last message is spaced above the bottom of the viewport. + check(tester.getRect(find.text('message 9'))) + .bottom..isGreaterThan(400)..isLessThan(570); + }); + + testWidgets('loading indicator displaces spacer etc.', (tester) async { + await setupMessageListPage(tester, narrow: CombinedFeedNarrow(), + skipPumpAndSettle: true, + // TODO(#1569) fix realism of this data: foundNewest false should mean + // some messages found after anchor (and then we might need to scroll + // to cause fetching newer messages). + fetchResult: eg.nearGetMessagesResult(anchor: 1000, + foundOldest: true, foundNewest: false, + messages: List.generate(10, + (i) => eg.streamMessage(id: 100 + i, content: '

message $i

')))); + await tester.pump(); + + // The message list will immediately start fetching newer messages. + connection.prepare(json: eg.newerGetMessagesResult( + anchor: 109, foundNewest: true, messages: List.generate(100, + (i) => eg.streamMessage(id: 110 + i))).toJson()); + await tester.pump(Duration(milliseconds: 10)); + await tester.pump(); + + // There's a loading indicator. + check(findLoadingIndicator).findsOne(); + // It's at the bottom. + check(findMessageListScrollController(tester)!.position) + .extentAfter.equals(0); + final loadingIndicatorRect = tester.getRect(findLoadingIndicator); + check(loadingIndicatorRect).bottom.isGreaterThan(575); + // The last message is shortly above it; no spacer or anything else. + check(tester.getRect(find.text('message 9'))) + .bottom.isGreaterThan(loadingIndicatorRect.top - 36); // TODO(#1569) where's this space going? + await tester.pumpAndSettle(); + }); + + // TODO(#1569) test no typing status or mark-read button when not haveNewest + // (even without loading indicator) + }); + group('TypingStatusWidget', () { final users = [eg.selfUser, eg.otherUser, eg.thirdUser, eg.fourthUser]; final finder = find.descendant( From 48abb5c257c340d974b87cb6f865b68a5ffed72a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 12 May 2025 20:19:30 -0700 Subject: [PATCH 3/8] msglist: Accept anchor on MessageListPage This is NFC as to the live app, because nothing yet passes this argument. --- lib/widgets/message_list.dart | 39 +++++++++++++++++++++-------- test/widgets/message_list_test.dart | 2 ++ 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 1c2d177664..1c43db9583 100644 --- 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 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 createState() => _MessageListPageState(); @@ -240,6 +246,10 @@ class _MessageListPageState extends State 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 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 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 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 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); diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 8cc216077a..a1851f8001 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -374,6 +374,8 @@ void main() { }); group('fetch initial batch of messages', () { + // TODO(#1569): test effect of initAnchorMessageId + group('topic permalink', () { final someStream = eg.stream(); const someTopic = 'some topic'; From b1730aeb142453312a884050a6babdeda1a2649b Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 8 May 2025 17:09:43 -0700 Subject: [PATCH 4/8] msglist: Jump, not scroll, to end when it might be far When the message list is truly far back in history -- 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 -- trying to scroll smoothly to the bottom is hopeless. The only way to get to the newest messages in any reasonable amount of time is to jump there. So, do that. --- lib/model/message_list.dart | 16 ++++++++++++- lib/widgets/message_list.dart | 35 ++++++++++++++++++++++++++--- test/model/message_list_test.dart | 2 ++ test/widgets/message_list_test.dart | 4 ++++ 4 files changed, 53 insertions(+), 4 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 2617f18b68..458478f248 100644 --- 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/widgets/message_list.dart b/lib/widgets/message_list.dart index 1c43db9583..e1c24249c5 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -554,6 +554,7 @@ class _MessageListState extends State 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. @@ -638,6 +639,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat // MessageList's dartdoc. child: SafeArea( child: ScrollToBottomButton( + model: model, scrollController: scrollController, visible: _scrollToBottomVisible))), ]))))); @@ -837,13 +839,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 visible; + final MessageListView model; final MessageListScrollController scrollController; + final ValueNotifier 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/message_list_test.dart b/test/model/message_list_test.dart index d58de664d8..0eb30c1cbb 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -566,6 +566,8 @@ void main() { }); }); + // TODO(#1569): test jumpToEnd + group('MessageEvent', () { test('in narrow', () async { final stream = eg.stream(); diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index a1851f8001..c8928a13ad 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -375,6 +375,8 @@ void main() { group('fetch initial batch of messages', () { // TODO(#1569): test effect of initAnchorMessageId + // TODO(#1569): test that after jumpToEnd, then new store causing new fetch, + // new post-jump anchor prevails over initAnchorMessageId group('topic permalink', () { final someStream = eg.stream(); @@ -668,6 +670,8 @@ void main() { check(isButtonVisible(tester)).equals(false); }); + // TODO(#1569): test choice of jumpToEnd vs. scrollToEnd + testWidgets('scrolls at reasonable, constant speed', (tester) async { const maxSpeed = 8000.0; const distance = 40000.0; From 17d822f41f9fe7fbbc1f0002920a636555f7f7dd Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 12 May 2025 20:00:30 -0700 Subject: [PATCH 5/8] internal_link [nfc]: Introduce NarrowLink type This will give us room to start returning additional information from parsing the URL, in particular the /near/ operand. --- lib/model/internal_link.dart | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/lib/model/internal_link.dart b/lib/model/internal_link.dart index 749f60698c..8169ccc915 100644 --- a/lib/model/internal_link.dart +++ b/lib/model/internal_link.dart @@ -109,6 +109,22 @@ Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) { return result; } +/// 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, {required super.realmUrl}); + + final Narrow narrow; +} + /// A [Narrow] from a given URL, on `store`'s realm. /// /// `url` must already be a result from [PerAccountStore.tryResolveUrl] @@ -131,7 +147,7 @@ Narrow? parseInternalLink(Uri url, PerAccountStore store) { switch (category) { case 'narrow': if (segments.isEmpty || !segments.length.isEven) return null; - return _interpretNarrowSegments(segments, store); + return _interpretNarrowSegments(segments, store)?.narrow; } return null; } @@ -155,7 +171,7 @@ bool _isInternalLink(Uri url, Uri realmUrl) { return (category, segments); } -Narrow? _interpretNarrowSegments(List segments, PerAccountStore store) { +NarrowLink? _interpretNarrowSegments(List segments, PerAccountStore store) { assert(segments.isNotEmpty); assert(segments.length.isEven); @@ -209,6 +225,7 @@ Narrow? _interpretNarrowSegments(List segments, PerAccountStore store) { } } + final Narrow? narrow; if (isElementOperands.isNotEmpty) { if (streamElement != null || topicElement != null || dmElement != null || withElement != null) { return null; @@ -216,9 +233,9 @@ Narrow? _interpretNarrowSegments(List segments, PerAccountStore store) { 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 +247,20 @@ Narrow? _interpretNarrowSegments(List 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, realmUrl: store.realmUrl); } @JsonEnum(fieldRename: FieldRename.kebab, alwaysCreate: true) From cb94d4e3ad100ccfbc686170fe11cc1a3c17d7c6 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 12 May 2025 20:07:02 -0700 Subject: [PATCH 6/8] internal_link [nfc]: Propagate InternalLink up to caller --- lib/model/internal_link.dart | 14 +++++++++----- lib/widgets/content.dart | 18 ++++++++++-------- test/model/internal_link_test.dart | 20 +++++++++++++++++++- 3 files changed, 38 insertions(+), 14 deletions(-) diff --git a/lib/model/internal_link.dart b/lib/model/internal_link.dart index 8169ccc915..c5889fb7e5 100644 --- a/lib/model/internal_link.dart +++ b/lib/model/internal_link.dart @@ -125,29 +125,33 @@ class NarrowLink extends InternalLink { final Narrow narrow; } -/// A [Narrow] from a given URL, on `store`'s realm. +/// 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); switch (category) { case 'narrow': if (segments.isEmpty || !segments.length.isEven) return null; - return _interpretNarrowSegments(segments, store)?.narrow; + return _interpretNarrowSegments(segments, store); } return null; } diff --git a/lib/widgets/content.dart b/lib/widgets/content.dart index 62801ab867..cb8af4ce6d 100644 --- a/lib/widgets/content.dart +++ b/lib/widgets/content.dart @@ -1536,15 +1536,17 @@ 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))); + + 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/test/model/internal_link_test.dart b/test/model/internal_link_test.dart index 611cc3ece0..7f1046fee8 100644 --- a/test/model/internal_link_test.dart +++ b/test/model/internal_link_test.dart @@ -160,7 +160,14 @@ void main() { test(urlString, () async { final store = await setupStore(realmUrl: realmUrl, streams: streams, users: users); final url = store.tryResolveUrl(urlString)!; - check(parseInternalLink(url, store)).equals(expected); + final result = parseInternalLink(url, store); + if (expected == null) { + check(result).isNull(); + } else { + check(result).isA() + ..realmUrl.equals(realmUrl) + ..narrow.equals(expected); + } }); } } @@ -258,6 +265,9 @@ void main() { final url = store.tryResolveUrl(urlString)!; final result = parseInternalLink(url, store); check(result != null).equals(expected); + if (result != null) { + check(result).realmUrl.equals(realmUrl); + } }); } } @@ -564,3 +574,11 @@ void main() { }); }); } + +extension InternalLinkChecks on Subject { + Subject get realmUrl => has((x) => x.realmUrl, 'realmUrl'); +} + +extension NarrowLinkChecks on Subject { + Subject get narrow => has((x) => x.narrow, 'narrow'); +} From aab50893a4fc089cc7d282c6b69c2d0eae1c76c5 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 12 May 2025 20:11:15 -0700 Subject: [PATCH 7/8] internal_link: Parse /near/ in narrow links --- lib/model/internal_link.dart | 13 +++++++++---- test/model/internal_link_test.dart | 2 ++ 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/model/internal_link.dart b/lib/model/internal_link.dart index c5889fb7e5..a552f1679f 100644 --- a/lib/model/internal_link.dart +++ b/lib/model/internal_link.dart @@ -120,9 +120,10 @@ sealed class InternalLink { /// 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, {required super.realmUrl}); + 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. @@ -184,6 +185,7 @@ NarrowLink? _interpretNarrowSegments(List segments, PerAccountStore stor ApiNarrowDm? dmElement; ApiNarrowWith? withElement; Set isElementOperands = {}; + int? nearMessageId; for (var i = 0; i < segments.length; i += 2) { final (operator, negated) = _parseOperator(segments[i]); @@ -221,8 +223,11 @@ NarrowLink? _interpretNarrowSegments(List segments, PerAccountStore stor // 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; + final messageId = int.tryParse(operand, radix: 10); + if (messageId == null) return null; + nearMessageId = messageId; case _NarrowOperator.unknown: return null; @@ -264,7 +269,7 @@ NarrowLink? _interpretNarrowSegments(List segments, PerAccountStore stor return null; } - return NarrowLink(narrow, realmUrl: store.realmUrl); + return NarrowLink(narrow, nearMessageId, realmUrl: store.realmUrl); } @JsonEnum(fieldRename: FieldRename.kebab, alwaysCreate: true) diff --git a/test/model/internal_link_test.dart b/test/model/internal_link_test.dart index 7f1046fee8..824def8cc2 100644 --- a/test/model/internal_link_test.dart +++ b/test/model/internal_link_test.dart @@ -380,6 +380,8 @@ void main() { } }); + // TODO(#1570): test parsing /near/ operator + group('unexpected link shapes are rejected', () { final testCases = [ ('/#narrow/stream/name/topic/', null), // missing operand From 8cb2c7bdce57ae5d41cea24931a16b1e28c4dc6e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 12 May 2025 20:20:19 -0700 Subject: [PATCH 8/8] internal_link: Open /near/ links at the given anchor in msglist Fixes #82. One TODO comment for this issue referred to an aspect I'm leaving out of scope for now, namely when opening a notification rather than an internal Zulip link. So I've filed a separate issue #1565 for that, and this updates that comment to point there. --- lib/notifications/display.dart | 2 +- lib/widgets/content.dart | 3 ++- test/widgets/content_test.dart | 2 ++ 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/notifications/display.dart b/lib/notifications/display.dart index 081a2cf633..6e2585e135 100644 --- 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 index cb8af4ce6d..44411434fd 100644 --- a/lib/widgets/content.dart +++ b/lib/widgets/content.dart @@ -1542,7 +1542,8 @@ void _launchUrl(BuildContext context, String urlString) async { case NarrowLink(): unawaited(Navigator.push(context, MessageListPage.buildRoute(context: context, - narrow: internalLink.narrow))); + narrow: internalLink.narrow, + initAnchorMessageId: internalLink.nearMessageId))); case null: await PlatformActions.launchUrl(context, url); diff --git a/test/widgets/content_test.dart b/test/widgets/content_test.dart index a788225aac..b5150a54ee 100644 --- a/test/widgets/content_test.dart +++ b/test/widgets/content_test.dart @@ -1032,6 +1032,8 @@ void main() { .page.isA().initNarrow.equals(const ChannelNarrow(1)); }); + // TODO(#1570): test links with /near/ go to the specific message + testWidgets('invalid internal links are opened in browser', (tester) async { // Link is invalid due to `topic` operator missing an operand. final pushedRoutes = await prepare(tester,