diff --git a/lib/model/internal_link.dart b/lib/model/internal_link.dart index 749f60698c..a552f1679f 100644 --- 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 segments, PerAccountStore store) { +NarrowLink? _interpretNarrowSegments(List segments, PerAccountStore store) { assert(segments.isNotEmpty); assert(segments.length.isEven); @@ -164,6 +185,7 @@ Narrow? _interpretNarrowSegments(List segments, PerAccountStore store) { ApiNarrowDm? dmElement; ApiNarrowWith? withElement; Set isElementOperands = {}; + int? nearMessageId; for (var i = 0; i < segments.length; i += 2) { final (operator, negated) = _parseOperator(segments[i]); @@ -201,14 +223,18 @@ Narrow? _interpretNarrowSegments(List 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; + final messageId = int.tryParse(operand, radix: 10); + if (messageId == null) return null; + nearMessageId = messageId; case _NarrowOperator.unknown: return null; } } + final Narrow? narrow; if (isElementOperands.isNotEmpty) { if (streamElement != null || topicElement != null || dmElement != null || withElement != null) { return null; @@ -216,9 +242,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 +256,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, nearMessageId, realmUrl: store.realmUrl); } @JsonEnum(fieldRename: FieldRename.kebab, alwaysCreate: true) 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/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 62801ab867..44411434fd 100644 --- 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 index aa9684b7f2..e1c24249c5 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); @@ -537,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. @@ -568,6 +586,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() { @@ -618,6 +639,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat // MessageList's dartdoc. child: SafeArea( child: ScrollToBottomButton( + model: model, scrollController: scrollController, visible: _scrollToBottomVisible))), ]))))); @@ -753,13 +775,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) { @@ -809,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/internal_link_test.dart b/test/model/internal_link_test.dart index 611cc3ece0..824def8cc2 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); + } }); } } @@ -370,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 @@ -564,3 +576,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'); +} 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/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, diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 3b3b01b323..c8928a13ad 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, @@ -368,6 +374,10 @@ 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(); const someTopic = 'some topic'; @@ -445,6 +455,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; @@ -656,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; @@ -692,6 +708,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(