From 6e195faaea7759f251aa02462f4aee71c4581ee7 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 18 Oct 2024 18:00:03 -0700 Subject: [PATCH] msglist: Follow /with/ links through message moves Fixes: #1028 --- lib/api/model/narrow.dart | 43 ++++++++++++--- lib/model/internal_link.dart | 17 ++++-- lib/model/message_list.dart | 36 +++++++++++++ lib/model/narrow.dart | 24 +++++++-- test/api/route/messages_test.dart | 10 ++++ test/example_data.dart | 4 +- test/model/internal_link_test.dart | 12 ++--- test/model/message_list_test.dart | 45 ++++++++++++++++ test/model/narrow_checks.dart | 1 + test/widgets/message_list_test.dart | 84 +++++++++++++++++++++++++++++ 10 files changed, 253 insertions(+), 23 deletions(-) diff --git a/lib/api/model/narrow.dart b/lib/api/model/narrow.dart index d5c0a6c9ae..d1ec678e0d 100644 --- a/lib/api/model/narrow.dart +++ b/lib/api/model/narrow.dart @@ -14,20 +14,35 @@ typedef ApiNarrow = List; /// reasonably be omitted will be omitted. ApiNarrow resolveApiNarrowForServer(ApiNarrow narrow, int zulipFeatureLevel) { final supportsOperatorDm = zulipFeatureLevel >= 177; // TODO(server-7) + final supportsOperatorWith = zulipFeatureLevel >= 271; // TODO(server-9) bool hasDmElement = false; + bool hasWithElement = false; for (final element in narrow) { switch (element) { - case ApiNarrowDm(): hasDmElement = true; + case ApiNarrowDm(): hasDmElement = true; + case ApiNarrowWith(): hasWithElement = true; default: } } - if (!hasDmElement) return narrow; + if (!hasDmElement && (!hasWithElement || supportsOperatorWith)) { + return narrow; + } - return narrow.map((element) => switch (element) { - ApiNarrowDm() => element.resolve(legacy: !supportsOperatorDm), - _ => element, - }).toList(); + final result = []; + for (final element in narrow) { + switch (element) { + case ApiNarrowDm(): + result.add(element.resolve(legacy: !supportsOperatorDm)); + case ApiNarrowWith(): + assert(!supportsOperatorWith); + // drop element; it's unsupported + break; + default: + result.add(element); + } + } + return result; } /// An element in the list representing a narrow in the Zulip API. @@ -160,6 +175,22 @@ class ApiNarrowPmWith extends ApiNarrowDm { ApiNarrowPmWith._(super.operand, {super.negated}); } +/// An [ApiNarrowElement] with the 'with' operator. +/// +/// If part of [ApiNarrow] use [resolveApiNarrowElements]. +class ApiNarrowWith extends ApiNarrowElement { + @override String get operator => 'with'; + + @override final int operand; + + ApiNarrowWith(this.operand, {super.negated}); + + factory ApiNarrowWith.fromJson(Map json) => ApiNarrowWith( + json['operand'] as int, + negated: json['negated'] as bool? ?? false, + ); +} + class ApiNarrowIs extends ApiNarrowElement { @override String get operator => 'is'; diff --git a/lib/model/internal_link.dart b/lib/model/internal_link.dart index fe4545ca5e..011f6163a0 100644 --- a/lib/model/internal_link.dart +++ b/lib/model/internal_link.dart @@ -78,6 +78,8 @@ Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) { fragment.write('$streamId-$slugifiedName'); case ApiNarrowTopic(): fragment.write(_encodeHashComponent(element.operand.apiName)); + case ApiNarrowWith(): + fragment.write(element.operand.toString()); case ApiNarrowDmModern(): final suffix = element.operand.length >= 3 ? 'group' : 'dm'; fragment.write('${element.operand.join(',')}-$suffix'); @@ -159,6 +161,7 @@ Narrow? _interpretNarrowSegments(List segments, PerAccountStore store) { ApiNarrowStream? streamElement; ApiNarrowTopic? topicElement; + ApiNarrowWith? withElement; ApiNarrowDm? dmElement; Set isElementOperands = {}; @@ -193,16 +196,21 @@ Narrow? _interpretNarrowSegments(List segments, PerAccountStore store) { isElementOperands.add(IsOperand.fromRawString(operand)); case _NarrowOperator.near: // TODO(#82): support for near - case _NarrowOperator.with_: // TODO(#683): support for with continue; + case _NarrowOperator.with_: + if (withElement != null) return null; + withElement = ApiNarrowWith(int.parse(operand, radix: 10)); + case _NarrowOperator.unknown: return null; } } if (isElementOperands.isNotEmpty) { - if (streamElement != null || topicElement != null || dmElement != null) return null; + if (streamElement != null || topicElement != null || dmElement != null || withElement != null) { + return null; + } if (isElementOperands.length > 1) return null; switch (isElementOperands.single) { case IsOperand.mentioned: @@ -219,13 +227,14 @@ Narrow? _interpretNarrowSegments(List segments, PerAccountStore store) { return null; } } else if (dmElement != null) { - if (streamElement != null || topicElement != null) return null; + if (streamElement != null || topicElement != null || withElement != null) return null; return DmNarrow.withUsers(dmElement.operand, selfUserId: store.selfUserId); } else if (streamElement != null) { final streamId = streamElement.operand; if (topicElement != null) { - return TopicNarrow(streamId, topicElement.operand); + return TopicNarrow(streamId, topicElement.operand, with_: withElement?.operand); } else { + if (withElement != null) return null; return ChannelNarrow(streamId); } } diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 670785ac4e..ea120cd72e 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -511,6 +511,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { numAfter: 0, ); if (this.generation > generation) return; + _adjustNarrowForTopicPermalink(result.messages.firstOrNull); store.reconcileMessages(result.messages); store.recentSenders.handleMessages(result.messages); // TODO(#824) for (final message in result.messages) { @@ -524,12 +525,47 @@ class MessageListView with ChangeNotifier, _MessageSequence { notifyListeners(); } + /// Update [narrow] for the result of a "with" narrow (topic permalink) fetch. + /// + /// To avoid an extra round trip, the server handles [ApiNarrowWith] + /// by returning results from the indicated message's current stream/topic + /// (if the user has access), + /// even if that differs from the narrow's stream/topic filters + /// because the message was moved. + /// + /// If such a "redirect" happened, this helper updates the stream and topic + /// in [narrow] to match the message's current conversation. + /// It also removes the "with" component from [narrow] + /// whether or not a redirect happened. + /// + /// See API doc: + /// https://zulip.com/api/construct-narrow#message-ids + void _adjustNarrowForTopicPermalink(Message? someFetchedMessageOrNull) { + final narrow = this.narrow; + if (narrow is! TopicNarrow || narrow.with_ == null) return; + + switch (someFetchedMessageOrNull) { + case null: + // This can't be a redirect; a redirect can't produce an empty result. + // (The server only redirects if the message is accessible to the user, + // and if it is, it'll appear in the result, making it non-empty.) + this.narrow = narrow.sansWith(); + case StreamMessage(): + this.narrow = TopicNarrow.ofMessage(someFetchedMessageOrNull); + case DmMessage(): // TODO(log) + assert(false); + } + } + /// Fetch the next batch of older messages, if applicable. Future fetchOlder() async { if (haveOldest) return; if (fetchingOlder) return; if (fetchOlderCoolingDown) return; assert(fetched); + assert(narrow is! TopicNarrow + // We only intend to send "with" in [fetchInitial]; see there. + || (narrow as TopicNarrow).with_ == null); assert(messages.isNotEmpty); _fetchingOlder = true; _updateEndMarkers(); diff --git a/lib/model/narrow.dart b/lib/model/narrow.dart index 9e29808ceb..de2b0b2671 100644 --- a/lib/model/narrow.dart +++ b/lib/model/narrow.dart @@ -92,7 +92,7 @@ class ChannelNarrow extends Narrow { } class TopicNarrow extends Narrow implements SendableNarrow { - const TopicNarrow(this.streamId, this.topic); + const TopicNarrow(this.streamId, this.topic, {this.with_}); factory TopicNarrow.ofMessage(StreamMessage message) { return TopicNarrow(message.streamId, message.topic); @@ -100,6 +100,9 @@ class TopicNarrow extends Narrow implements SendableNarrow { final int streamId; final TopicName topic; + final int? with_; + + TopicNarrow sansWith() => TopicNarrow(streamId, topic); @override bool containsMessage(Message message) { @@ -108,22 +111,33 @@ class TopicNarrow extends Narrow implements SendableNarrow { } @override - ApiNarrow apiEncode() => [ApiNarrowStream(streamId), ApiNarrowTopic(topic)]; + ApiNarrow apiEncode() => [ + ApiNarrowStream(streamId), + ApiNarrowTopic(topic), + if (with_ != null) ApiNarrowWith(with_!), + ]; @override StreamDestination get destination => StreamDestination(streamId, topic); @override - String toString() => 'TopicNarrow($streamId, ${topic.displayName})'; + String toString() { + final fields = [ + streamId.toString(), + topic.displayName, + if (with_ != null) with_!, + ]; + return 'TopicNarrow(${fields.join(', ')})'; + } @override bool operator ==(Object other) { if (other is! TopicNarrow) return false; - return other.streamId == streamId && other.topic == topic; + return other.streamId == streamId && other.topic == topic && other.with_ == with_; } @override - int get hashCode => Object.hash('TopicNarrow', streamId, topic); + int get hashCode => Object.hash('TopicNarrow', streamId, topic, with_); } /// The narrow for a direct-message conversation. diff --git a/test/api/route/messages_test.dart b/test/api/route/messages_test.dart index e7ff588272..f0a9ca69bf 100644 --- a/test/api/route/messages_test.dart +++ b/test/api/route/messages_test.dart @@ -188,6 +188,11 @@ void main() { {'operator': 'stream', 'operand': 12}, {'operator': 'topic', 'operand': 'stuff'}, ])); + checkNarrow(eg.topicNarrow(12, 'stuff', with_: 1).apiEncode(), jsonEncode([ + {'operator': 'stream', 'operand': 12}, + {'operator': 'topic', 'operand': 'stuff'}, + {'operator': 'with', 'operand': 1}, + ])); checkNarrow(const MentionsNarrow().apiEncode(), jsonEncode([ {'operator': 'is', 'operand': 'mentioned'}, ])); @@ -203,6 +208,11 @@ void main() { checkNarrow([ApiNarrowDm([123, 234])], jsonEncode([ {'operator': 'pm-with', 'operand': [123, 234]}, ])); + connection.zulipFeatureLevel = 270; + checkNarrow(eg.topicNarrow(12, 'stuff', with_: 1).apiEncode(), jsonEncode([ + {'operator': 'stream', 'operand': 12}, + {'operator': 'topic', 'operand': 'stuff'}, + ])); connection.zulipFeatureLevel = eg.futureZulipFeatureLevel; }); }); diff --git a/test/example_data.dart b/test/example_data.dart index 6b84bf185c..6d6b2d55dd 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -289,8 +289,8 @@ Subscription subscription( /// Useful in test code that mentions a lot of topics in a compact format. TopicName t(String apiName) => TopicName(apiName); -TopicNarrow topicNarrow(int channelId, String topicName) { - return TopicNarrow(channelId, TopicName(topicName)); +TopicNarrow topicNarrow(int channelId, String topicName, {int? with_}) { + return TopicNarrow(channelId, TopicName(topicName), with_: with_); } UserTopicItem userTopicItem( diff --git a/test/model/internal_link_test.dart b/test/model/internal_link_test.dart index eb91e35855..72d7aa2964 100644 --- a/test/model/internal_link_test.dart +++ b/test/model/internal_link_test.dart @@ -284,12 +284,12 @@ void main() { final testCases = [ ('/#narrow/stream/check/topic/test', eg.topicNarrow(1, 'test')), ('/#narrow/stream/mobile/subject/topic/near/378333', eg.topicNarrow(3, 'topic')), - ('/#narrow/stream/mobile/subject/topic/with/1', eg.topicNarrow(3, 'topic')), + ('/#narrow/stream/mobile/subject/topic/with/1', eg.topicNarrow(3, 'topic', with_: 1)), ('/#narrow/stream/mobile/topic/topic/', eg.topicNarrow(3, 'topic')), ('/#narrow/stream/stream/topic/topic/near/1', eg.topicNarrow(5, 'topic')), - ('/#narrow/stream/stream/topic/topic/with/22', eg.topicNarrow(5, 'topic')), + ('/#narrow/stream/stream/topic/topic/with/22', eg.topicNarrow(5, 'topic', with_: 22)), ('/#narrow/stream/stream/subject/topic/near/1', eg.topicNarrow(5, 'topic')), - ('/#narrow/stream/stream/subject/topic/with/333', eg.topicNarrow(5, 'topic')), + ('/#narrow/stream/stream/subject/topic/with/333', eg.topicNarrow(5, 'topic', with_: 333)), ('/#narrow/stream/stream/subject/topic', eg.topicNarrow(5, 'topic')), ]; testExpectedNarrows(testCases, streams: streams); @@ -313,7 +313,7 @@ void main() { final testCases = [ ('/#narrow/dm/1,2-group', expectedNarrow), ('/#narrow/dm/1,2-group/near/1', expectedNarrow), - ('/#narrow/dm/1,2-group/with/2', expectedNarrow), + ('/#narrow/dm/1,2-group/with/2', null), ('/#narrow/dm/a.40b.2Ecom.2Ec.2Ed.2Ecom/near/3', null), ('/#narrow/dm/a.40b.2Ecom.2Ec.2Ed.2Ecom/with/4', null), ]; @@ -326,7 +326,7 @@ void main() { final testCases = [ ('/#narrow/pm-with/1,2-group', expectedNarrow), ('/#narrow/pm-with/1,2-group/near/1', expectedNarrow), - ('/#narrow/pm-with/1,2-group/with/2', expectedNarrow), + ('/#narrow/pm-with/1,2-group/with/2', null), ('/#narrow/pm-with/a.40b.2Ecom.2Ec.2Ed.2Ecom/near/3', null), ('/#narrow/pm-with/a.40b.2Ecom.2Ec.2Ed.2Ecom/with/3', null), ]; @@ -342,7 +342,7 @@ void main() { ('/#narrow/is/$operand', narrow), ('/#narrow/is/$operand/is/$operand', narrow), ('/#narrow/is/$operand/near/1', narrow), - ('/#narrow/is/$operand/with/2', narrow), + ('/#narrow/is/$operand/with/2', null), ('/#narrow/channel/7-test-here/is/$operand', null), ('/#narrow/channel/check/topic/test/is/$operand', null), ('/#narrow/topic/test/is/$operand', null), diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index ded9006289..01370c67e9 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -99,6 +99,9 @@ void main() { final someChannel = eg.stream(); const someTopic = 'some topic'; + final otherStream = eg.stream(); + const otherTopic = 'other topic'; + group('smoke', () { Future smoke( Narrow narrow, @@ -134,6 +137,22 @@ void main() { await smoke(TopicNarrow(someChannel.streamId, eg.t(someTopic)), (i) => eg.streamMessage(stream: someChannel, topic: someTopic)); }); + + test('topic permalink, message was not moved', () async { + await smoke(TopicNarrow(someChannel.streamId, eg.t(someTopic), with_: 1), + (int i) => eg.streamMessage( + id: i == 0 ? 1 : null, + stream: someChannel, + topic: someTopic)); + }); + + test('topic permalink, message was moved', () async { + await smoke(TopicNarrow(someChannel.streamId, eg.t(someTopic), with_: 1), + (int i) => eg.streamMessage( + id: i == 0 ? 1 : null, + stream: otherStream, + topic: otherTopic)); + }); }); test('short history', () async { @@ -181,6 +200,32 @@ void main() { check(model).messages.length.equals(1); recent_senders_test.checkMatchesMessages(store.recentSenders, messages); }); + + group('topic permalinks', () { + test('if redirect, we follow it and remove "with" element', () async { + await prepare(narrow: TopicNarrow(someChannel.streamId, eg.t(someTopic), with_: 1)); + connection.prepare(json: newestResult( + foundOldest: false, + messages: [eg.streamMessage(id: 1, stream: otherStream, topic: otherTopic)], + ).toJson()); + await model.fetchInitial(); + checkNotifiedOnce(); + check(model).narrow + .equals(TopicNarrow(otherStream.streamId, eg.t(otherTopic))); + }); + + test('if no redirect, we still remove "with" element', () async { + await prepare(narrow: TopicNarrow(someChannel.streamId, eg.t(someTopic), with_: 1)); + connection.prepare(json: newestResult( + foundOldest: false, + messages: [eg.streamMessage(id: 1, stream: someChannel, topic: someTopic)], + ).toJson()); + await model.fetchInitial(); + checkNotifiedOnce(); + check(model).narrow + .equals(TopicNarrow(someChannel.streamId, eg.t(someTopic))); + }); + }); }); group('fetchOlder', () { diff --git a/test/model/narrow_checks.dart b/test/model/narrow_checks.dart index ce65de854d..df141654dd 100644 --- a/test/model/narrow_checks.dart +++ b/test/model/narrow_checks.dart @@ -16,4 +16,5 @@ extension DmNarrowChecks on Subject { extension TopicNarrowChecks on Subject { Subject get streamId => has((x) => x.streamId, 'streamId'); Subject get topic => has((x) => x.topic, 'topic'); + Subject get with_ => has((x) => x.with_, 'with_'); } diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 3a24109af8..d172d1b0f2 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -13,6 +13,7 @@ import 'package:zulip/api/model/model.dart'; import 'package:zulip/api/model/narrow.dart'; import 'package:zulip/api/route/messages.dart'; import 'package:zulip/model/localizations.dart'; +import 'package:zulip/model/message_list.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/model/typing_status.dart'; @@ -57,6 +58,7 @@ void main() { UnreadMessagesSnapshot? unreadMsgs, int? zulipFeatureLevel, List navObservers = const [], + bool skipPumpAndSettle = false, }) async { TypingNotifier.debugEnable = false; addTearDown(TypingNotifier.debugReset); @@ -84,6 +86,7 @@ void main() { navigatorObservers: navObservers, child: MessageListPage(initNarrow: narrow))); + if (skipPumpAndSettle) return; // global store, per-account store, and message list get loaded await tester.pumpAndSettle(); } @@ -235,6 +238,87 @@ void main() { check(backgroundColor()).isSameColorAs(MessageListTheme.dark.streamMessageBgDefault); }); + group('fetch initial batch of messages', () { + group('topic permalink', () { + final someStream = eg.stream(); + const someTopic = 'some topic'; + + final otherStream = eg.stream(); + const otherTopic = 'other topic'; + + testWidgets('with message move', (tester) async { + final narrow = TopicNarrow(someStream.streamId, eg.t(someTopic), with_: 1); + await setupMessageListPage(tester, + narrow: narrow, + // server sends the /with/ message in its current, different location + messages: [eg.streamMessage(id: 1, stream: otherStream, topic: otherTopic)], + streams: [someStream, otherStream], + subscriptions: [eg.subscription(someStream), eg.subscription(otherStream)], + skipPumpAndSettle: true); + await tester.pump(); // global store loaded + await tester.pump(); // per-account store loaded + + // Until we learn the conversation was moved, + // we put the link's stream/topic in the app bar. + checkAppBarChannelTopic(someStream.name, someTopic); + + await tester.pumpAndSettle(); // initial message fetch plus anything else + + // When we learn the conversation was moved, + // we put the new stream/topic in the app bar. + checkAppBarChannelTopic(otherStream.name, otherTopic); + + // We followed the move in just one fetch. + check(connection.takeRequests()).single.isA() + ..method.equals('GET') + ..url.path.equals('/api/v1/messages') + ..url.queryParameters.deepEquals({ + 'narrow': jsonEncode(narrow.apiEncode()), + 'anchor': AnchorCode.newest.toJson(), + 'num_before': kMessageListFetchBatchSize.toString(), + 'num_after': '0', + }); + }); + + testWidgets('without message move', (tester) async { + final narrow = TopicNarrow(someStream.streamId, eg.t(someTopic), with_: 1); + await setupMessageListPage(tester, + narrow: narrow, + // server sends the /with/ message in its current, different location + messages: [eg.streamMessage(id: 1, stream: someStream, topic: someTopic)], + streams: [someStream], + subscriptions: [eg.subscription(someStream)], + skipPumpAndSettle: true); + await tester.pump(); // global store loaded + await tester.pump(); // per-account store loaded + + // Until we learn if the conversation was moved, + // we put the link's stream/topic in the app bar. + checkAppBarChannelTopic(someStream.name, someTopic); + + await tester.pumpAndSettle(); // initial message fetch plus anything else + + // There was no move, so we're still showing the same stream/topic. + checkAppBarChannelTopic(someStream.name, someTopic); + + // We only made one fetch. + check(connection.takeRequests()).single.isA() + ..method.equals('GET') + ..url.path.equals('/api/v1/messages') + ..url.queryParameters.deepEquals({ + 'narrow': jsonEncode([ + ApiNarrowStream(someStream.streamId), + ApiNarrowTopic(eg.t(someTopic)), + ApiNarrowWith(1), + ]), + 'anchor': AnchorCode.newest.toJson(), + 'num_before': kMessageListFetchBatchSize.toString(), + 'num_after': '0', + }); + }); + }); + }); + group('fetch older messages on scroll', () { int? itemCount(WidgetTester tester) => tester.widget(find.byType(CustomScrollView)).semanticChildCount;