Skip to content

Commit 03c261f

Browse files
chrisbobbechrisbobbe
committed
msglist: Follow /with/ links through message moves
Fixes: #1028
1 parent 46d4a53 commit 03c261f

File tree

12 files changed

+268
-26
lines changed

12 files changed

+268
-26
lines changed

‎lib/api/model/narrow.dart

Lines changed: 35 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,20 +14,33 @@ typedef ApiNarrow = List<ApiNarrowElement>;
1414
/// reasonably be omitted will be omitted.
1515
ApiNarrow resolveApiNarrowForServer(ApiNarrow narrow, int zulipFeatureLevel) {
1616
final supportsOperatorDm = zulipFeatureLevel >= 177; // TODO(server-7)
17+
final supportsOperatorWith = zulipFeatureLevel >= 271; // TODO(server-9)
1718

1819
bool hasDmElement = false;
20+
bool hasWithElement = false;
1921
for (final element in narrow) {
2022
switch (element) {
21-
case ApiNarrowDm(): hasDmElement = true;
23+
case ApiNarrowDm(): hasDmElement = true;
24+
case ApiNarrowWith(): hasWithElement = true;
2225
default:
2326
}
2427
}
25-
if (!hasDmElement) return narrow;
28+
if (!hasDmElement && (!hasWithElement || supportsOperatorWith)) {
29+
return narrow;
30+
}
2631

27-
return narrow.map((element) => switch (element) {
28-
ApiNarrowDm() => element.resolve(legacy: !supportsOperatorDm),
29-
_ => element,
30-
}).toList();
32+
final result = <ApiNarrowElement>[];
33+
for (final element in narrow) {
34+
switch (element) {
35+
case ApiNarrowDm():
36+
result.add(element.resolve(legacy: !supportsOperatorDm));
37+
case ApiNarrowWith() when !supportsOperatorWith:
38+
break; // drop unsupported element
39+
default:
40+
result.add(element);
41+
}
42+
}
43+
return result;
3144
}
3245

3346
/// An element in the list representing a narrow in the Zulip API.
@@ -160,6 +173,22 @@ class ApiNarrowPmWith extends ApiNarrowDm {
160173
ApiNarrowPmWith._(super.operand, {super.negated});
161174
}
162175

176+
/// An [ApiNarrowElement] with the 'with' operator.
177+
///
178+
/// If part of [ApiNarrow] use [resolveApiNarrowForServer].
179+
class ApiNarrowWith extends ApiNarrowElement {
180+
@override String get operator => 'with';
181+
182+
@override final int operand;
183+
184+
ApiNarrowWith(this.operand, {super.negated});
185+
186+
factory ApiNarrowWith.fromJson(Map<String, dynamic> json) => ApiNarrowWith(
187+
json['operand'] as int,
188+
negated: json['negated'] as bool? ?? false,
189+
);
190+
}
191+
163192
class ApiNarrowIs extends ApiNarrowElement {
164193
@override String get operator => 'is';
165194

‎lib/model/internal_link.dart

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,8 @@ Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) {
8686
fragment.write('${element.operand.join(',')}-$suffix');
8787
case ApiNarrowDm():
8888
assert(false, 'ApiNarrowDm should have been resolved');
89+
case ApiNarrowWith():
90+
fragment.write(element.operand.toString());
8991
case ApiNarrowIs():
9092
fragment.write(element.operand.toString());
9193
case ApiNarrowMessageId():
@@ -160,6 +162,7 @@ Narrow? _interpretNarrowSegments(List<String> segments, PerAccountStore store) {
160162
ApiNarrowStream? streamElement;
161163
ApiNarrowTopic? topicElement;
162164
ApiNarrowDm? dmElement;
165+
ApiNarrowWith? withElement;
163166
Set<IsOperand> isElementOperands = {};
164167

165168
for (var i = 0; i < segments.length; i += 2) {
@@ -188,12 +191,17 @@ Narrow? _interpretNarrowSegments(List<String> segments, PerAccountStore store) {
188191
if (dmIds == null) return null;
189192
dmElement = ApiNarrowDm(dmIds, negated: negated);
190193

194+
case _NarrowOperator.with_:
195+
if (withElement != null) return null;
196+
final messageId = int.tryParse(operand, radix: 10);
197+
if (messageId == null) return null;
198+
withElement = ApiNarrowWith(messageId);
199+
191200
case _NarrowOperator.is_:
192201
// It is fine to have duplicates of the same [IsOperand].
193202
isElementOperands.add(IsOperand.fromRawString(operand));
194203

195204
case _NarrowOperator.near: // TODO(#82): support for near
196-
case _NarrowOperator.with_: // TODO(#683): support for with
197205
continue;
198206

199207
case _NarrowOperator.unknown:
@@ -202,7 +210,9 @@ Narrow? _interpretNarrowSegments(List<String> segments, PerAccountStore store) {
202210
}
203211

204212
if (isElementOperands.isNotEmpty) {
205-
if (streamElement != null || topicElement != null || dmElement != null) return null;
213+
if (streamElement != null || topicElement != null || dmElement != null || withElement != null) {
214+
return null;
215+
}
206216
if (isElementOperands.length > 1) return null;
207217
switch (isElementOperands.single) {
208218
case IsOperand.mentioned:
@@ -219,13 +229,14 @@ Narrow? _interpretNarrowSegments(List<String> segments, PerAccountStore store) {
219229
return null;
220230
}
221231
} else if (dmElement != null) {
222-
if (streamElement != null || topicElement != null) return null;
232+
if (streamElement != null || topicElement != null || withElement != null) return null;
223233
return DmNarrow.withUsers(dmElement.operand, selfUserId: store.selfUserId);
224234
} else if (streamElement != null) {
225235
final streamId = streamElement.operand;
226236
if (topicElement != null) {
227-
return TopicNarrow(streamId, topicElement.operand);
237+
return TopicNarrow(streamId, topicElement.operand, with_: withElement?.operand);
228238
} else {
239+
if (withElement != null) return null;
229240
return ChannelNarrow(streamId);
230241
}
231242
}

‎lib/model/message_list.dart

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -511,6 +511,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
511511
numAfter: 0,
512512
);
513513
if (this.generation > generation) return;
514+
_adjustNarrowForTopicPermalink(result.messages.firstOrNull);
514515
store.reconcileMessages(result.messages);
515516
store.recentSenders.handleMessages(result.messages); // TODO(#824)
516517
for (final message in result.messages) {
@@ -524,12 +525,47 @@ class MessageListView with ChangeNotifier, _MessageSequence {
524525
notifyListeners();
525526
}
526527

528+
/// Update [narrow] for the result of a "with" narrow (topic permalink) fetch.
529+
///
530+
/// To avoid an extra round trip, the server handles [ApiNarrowWith]
531+
/// by returning results from the indicated message's current stream/topic
532+
/// (if the user has access),
533+
/// even if that differs from the narrow's stream/topic filters
534+
/// because the message was moved.
535+
///
536+
/// If such a "redirect" happened, this helper updates the stream and topic
537+
/// in [narrow] to match the message's current conversation.
538+
/// It also removes the "with" component from [narrow]
539+
/// whether or not a redirect happened.
540+
///
541+
/// See API doc:
542+
/// https://zulip.com/api/construct-narrow#message-ids
543+
void _adjustNarrowForTopicPermalink(Message? someFetchedMessageOrNull) {
544+
final narrow = this.narrow;
545+
if (narrow is! TopicNarrow || narrow.with_ == null) return;
546+
547+
switch (someFetchedMessageOrNull) {
548+
case null:
549+
// This can't be a redirect; a redirect can't produce an empty result.
550+
// (The server only redirects if the message is accessible to the user,
551+
// and if it is, it'll appear in the result, making it non-empty.)
552+
this.narrow = narrow.sansWith();
553+
case StreamMessage():
554+
this.narrow = TopicNarrow.ofMessage(someFetchedMessageOrNull);
555+
case DmMessage(): // TODO(log)
556+
assert(false);
557+
}
558+
}
559+
527560
/// Fetch the next batch of older messages, if applicable.
528561
Future<void> fetchOlder() async {
529562
if (haveOldest) return;
530563
if (fetchingOlder) return;
531564
if (fetchOlderCoolingDown) return;
532565
assert(fetched);
566+
assert(narrow is! TopicNarrow
567+
// We only intend to send "with" in [fetchInitial]; see there.
568+
|| (narrow as TopicNarrow).with_ == null);
533569
assert(messages.isNotEmpty);
534570
_fetchingOlder = true;
535571
_updateEndMarkers();

‎lib/model/narrow.dart

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -92,14 +92,17 @@ class ChannelNarrow extends Narrow {
9292
}
9393

9494
class TopicNarrow extends Narrow implements SendableNarrow {
95-
const TopicNarrow(this.streamId, this.topic);
95+
const TopicNarrow(this.streamId, this.topic, {this.with_});
9696

9797
factory TopicNarrow.ofMessage(StreamMessage message) {
9898
return TopicNarrow(message.streamId, message.topic);
9999
}
100100

101101
final int streamId;
102102
final TopicName topic;
103+
final int? with_;
104+
105+
TopicNarrow sansWith() => TopicNarrow(streamId, topic);
103106

104107
@override
105108
bool containsMessage(Message message) {
@@ -108,22 +111,33 @@ class TopicNarrow extends Narrow implements SendableNarrow {
108111
}
109112

110113
@override
111-
ApiNarrow apiEncode() => [ApiNarrowStream(streamId), ApiNarrowTopic(topic)];
114+
ApiNarrow apiEncode() => [
115+
ApiNarrowStream(streamId),
116+
ApiNarrowTopic(topic),
117+
if (with_ != null) ApiNarrowWith(with_!),
118+
];
112119

113120
@override
114121
StreamDestination get destination => StreamDestination(streamId, topic);
115122

116123
@override
117-
String toString() => 'TopicNarrow($streamId, ${topic.displayName})';
124+
String toString() {
125+
final fields = [
126+
streamId.toString(),
127+
topic.displayName,
128+
if (with_ != null) 'with: ${with_!}',
129+
];
130+
return 'TopicNarrow(${fields.join(', ')})';
131+
}
118132

119133
@override
120134
bool operator ==(Object other) {
121135
if (other is! TopicNarrow) return false;
122-
return other.streamId == streamId && other.topic == topic;
136+
return other.streamId == streamId && other.topic == topic && other.with_ == with_;
123137
}
124138

125139
@override
126-
int get hashCode => Object.hash('TopicNarrow', streamId, topic);
140+
int get hashCode => Object.hash('TopicNarrow', streamId, topic, with_);
127141
}
128142

129143
/// The narrow for a direct-message conversation.

‎lib/widgets/message_list.dart

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -503,8 +503,11 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
503503

504504
void _modelChanged() {
505505
if (model!.narrow != widget.narrow) {
506-
// A message move event occurred, where propagate mode is
507-
// [PropagateMode.changeAll] or [PropagateMode.changeLater].
506+
// Either:
507+
// - A message move event occurred, where propagate mode is
508+
// [PropagateMode.changeAll] or [PropagateMode.changeLater]. Or:
509+
// - We fetched a "with" / topic-permalink narrow, and the response
510+
// redirected us to the new location of the operand message ID.
508511
widget.onNarrowChanged(model!.narrow);
509512
}
510513
setState(() {

‎test/api/model/narrow_test.dart

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
void main() {
2+
// resolveApiNarrowForServer is covered in test/api/route/messages_test.dart,
3+
// in the ApiNarrow.toJson test.
4+
}

‎test/api/route/messages_test.dart

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -191,15 +191,51 @@ void main() {
191191
{'operator': 'stream', 'operand': 12},
192192
{'operator': 'topic', 'operand': 'stuff'},
193193
]));
194-
194+
checkNarrow(eg.topicNarrow(12, 'stuff', with_: 1).apiEncode(), jsonEncode([
195+
{'operator': 'stream', 'operand': 12},
196+
{'operator': 'topic', 'operand': 'stuff'},
197+
{'operator': 'with', 'operand': 1},
198+
]));
195199
checkNarrow([ApiNarrowDm([123, 234])], jsonEncode([
196200
{'operator': 'dm', 'operand': [123, 234]},
197201
]));
198202

203+
// Unlikely to occur in the wild but should still be handled correctly
204+
checkNarrow([ApiNarrowDm([123, 234]), ApiNarrowWith(1)], jsonEncode([
205+
{'operator': 'dm', 'operand': [123, 234]},
206+
{'operator': 'with', 'operand': 1},
207+
]));
208+
199209
connection.zulipFeatureLevel = 176;
210+
211+
checkNarrow(eg.topicNarrow(12, 'stuff', with_: 1).apiEncode(), jsonEncode([
212+
{'operator': 'stream', 'operand': 12},
213+
{'operator': 'topic', 'operand': 'stuff'},
214+
]));
200215
checkNarrow([ApiNarrowDm([123, 234])], jsonEncode([
201216
{'operator': 'pm-with', 'operand': [123, 234]},
202217
]));
218+
219+
// Unlikely to occur in the wild but should still be handled correctly
220+
checkNarrow([ApiNarrowDm([123, 234]), ApiNarrowWith(1)], jsonEncode([
221+
{'operator': 'pm-with', 'operand': [123, 234]},
222+
]));
223+
224+
connection.zulipFeatureLevel = 270;
225+
226+
checkNarrow(eg.topicNarrow(12, 'stuff', with_: 1).apiEncode(), jsonEncode([
227+
{'operator': 'stream', 'operand': 12},
228+
{'operator': 'topic', 'operand': 'stuff'},
229+
]));
230+
checkNarrow([ApiNarrowDm([123, 234])], jsonEncode([
231+
{'operator': 'dm', 'operand': [123, 234]},
232+
]));
233+
234+
// Unlikely to occur in the wild but should still be handled correctly
235+
checkNarrow([ApiNarrowDm([123, 234]), ApiNarrowWith(1)], jsonEncode([
236+
{'operator': 'dm', 'operand': [123, 234]},
237+
]));
238+
203239
connection.zulipFeatureLevel = eg.futureZulipFeatureLevel;
204240
});
205241
});

‎test/example_data.dart

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -332,8 +332,8 @@ Subscription subscription(
332332
/// Useful in test code that mentions a lot of topics in a compact format.
333333
TopicName t(String apiName) => TopicName(apiName);
334334

335-
TopicNarrow topicNarrow(int channelId, String topicName) {
336-
return TopicNarrow(channelId, TopicName(topicName));
335+
TopicNarrow topicNarrow(int channelId, String topicName, {int? with_}) {
336+
return TopicNarrow(channelId, TopicName(topicName), with_: with_);
337337
}
338338

339339
UserTopicItem userTopicItem(

‎test/model/internal_link_test.dart

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -284,13 +284,14 @@ void main() {
284284
final testCases = [
285285
('/#narrow/stream/check/topic/test', eg.topicNarrow(1, 'test')),
286286
('/#narrow/stream/mobile/subject/topic/near/378333', eg.topicNarrow(3, 'topic')),
287-
('/#narrow/stream/mobile/subject/topic/with/1', eg.topicNarrow(3, 'topic')),
287+
('/#narrow/stream/mobile/subject/topic/with/1', eg.topicNarrow(3, 'topic', with_: 1)),
288288
('/#narrow/stream/mobile/topic/topic/', eg.topicNarrow(3, 'topic')),
289289
('/#narrow/stream/stream/topic/topic/near/1', eg.topicNarrow(5, 'topic')),
290-
('/#narrow/stream/stream/topic/topic/with/22', eg.topicNarrow(5, 'topic')),
290+
('/#narrow/stream/stream/topic/topic/with/22', eg.topicNarrow(5, 'topic', with_: 22)),
291291
('/#narrow/stream/stream/subject/topic/near/1', eg.topicNarrow(5, 'topic')),
292-
('/#narrow/stream/stream/subject/topic/with/333', eg.topicNarrow(5, 'topic')),
292+
('/#narrow/stream/stream/subject/topic/with/333', eg.topicNarrow(5, 'topic', with_: 333)),
293293
('/#narrow/stream/stream/subject/topic', eg.topicNarrow(5, 'topic')),
294+
('/#narrow/stream/stream/subject/topic/with/asdf', null), // invalid `with`
294295
];
295296
testExpectedNarrows(testCases, streams: streams);
296297
});
@@ -313,7 +314,7 @@ void main() {
313314
final testCases = [
314315
('/#narrow/dm/1,2-group', expectedNarrow),
315316
('/#narrow/dm/1,2-group/near/1', expectedNarrow),
316-
('/#narrow/dm/1,2-group/with/2', expectedNarrow),
317+
('/#narrow/dm/1,2-group/with/2', null),
317318
('/#narrow/dm/a.40b.2Ecom.2Ec.2Ed.2Ecom/near/3', null),
318319
('/#narrow/dm/a.40b.2Ecom.2Ec.2Ed.2Ecom/with/4', null),
319320
];
@@ -326,7 +327,7 @@ void main() {
326327
final testCases = [
327328
('/#narrow/pm-with/1,2-group', expectedNarrow),
328329
('/#narrow/pm-with/1,2-group/near/1', expectedNarrow),
329-
('/#narrow/pm-with/1,2-group/with/2', expectedNarrow),
330+
('/#narrow/pm-with/1,2-group/with/2', null),
330331
('/#narrow/pm-with/a.40b.2Ecom.2Ec.2Ed.2Ecom/near/3', null),
331332
('/#narrow/pm-with/a.40b.2Ecom.2Ec.2Ed.2Ecom/with/3', null),
332333
];
@@ -342,7 +343,7 @@ void main() {
342343
('/#narrow/is/$operand', narrow),
343344
('/#narrow/is/$operand/is/$operand', narrow),
344345
('/#narrow/is/$operand/near/1', narrow),
345-
('/#narrow/is/$operand/with/2', narrow),
346+
('/#narrow/is/$operand/with/2', null),
346347
('/#narrow/channel/7-test-here/is/$operand', null),
347348
('/#narrow/channel/check/topic/test/is/$operand', null),
348349
('/#narrow/topic/test/is/$operand', null),

0 commit comments

Comments
 (0)