Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

msglist: Follow /with/ links through message moves #1029

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

msglist: Follow /with/ links through message moves #1029

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
35db74d
api [nfc]: Rename resolveDmElements to resolveApiNarrowElements
chrisbobbe Oct 22, 2024
9958ae1
msglist test: Make groups for fetchInitial and fetchOlder tests
chrisbobbe Oct 22, 2024
a8dd4da
msglist test: Refactor fetchInitial smoke test, preparing for more na…
chrisbobbe Oct 30, 2024
d49d62a
msglist test: Add fetchInitial smoke test that uses a topic narrow
chrisbobbe Oct 22, 2024
9822bc5
msglist test [nfc]: Factor out findAppBarTitleWithChannelTopic helper
chrisbobbe Oct 25, 2024
ff6cc8a
msglist test [nfc]: Add feature-level param to setupMessageListPage
chrisbobbe Oct 30, 2024
0131961
msglist: Follow /with/ links through message moves
chrisbobbe Oct 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 43 additions & 9 deletions lib/api/model/narrow.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,36 @@ part 'narrow.g.dart';

typedef ApiNarrow = List<ApiNarrowElement>;

/// Resolve any [ApiNarrowDm] elements appropriately.
/// Resolve any [ApiNarrowElement]s, such as [ApiNarrowDm]s, appropriately.
///
/// This encapsulates a server-feature check.
ApiNarrow resolveDmElements(ApiNarrow narrow, int zulipFeatureLevel) {
if (!narrow.any((element) => element is ApiNarrowDm)) {
return narrow;
// TODO(server-7) remove [ApiNarrowDm] reference in dartdoc
ApiNarrow resolveApiNarrowElements(ApiNarrow narrow, int zulipFeatureLevel) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name doesn't sound right to me for the expanded scope of this function. (In particular "elements" in the name feels redundant; it doesn't seem to convey any more information than resolveApiNarrow would.)

Here's a version:

/// Adapt the given narrow to be sent to the given Zulip server version.
///
/// Any elements that take a different name on old vs. new servers
/// will be resolved to the specific name to use.
/// Any elements that are unknown to old servers and can
/// reasonably be omitted will be omitted.
ApiNarrow resolveApiNarrowForServer(ApiNarrow narrow, int zulipFeatureLevel) {

bool hasDmElement = false;
bool hasWithElement = false;
for (final element in narrow) {
switch (element) {
case ApiNarrowDm(): hasDmElement = true;
case ApiNarrowWith(): hasWithElement = true;
default:
}
}
if (!hasDmElement && !hasWithElement) return narrow;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal of the corresponding early return in the existing code is so we don't make a copy of the list in cases where we don't need to. So we should preserve that property.

In this version, I think it will end up making a no-op copy if there's a /with/ element on a recent server.


final supportsOperatorDm = zulipFeatureLevel >= 177; // TODO(server-7)
return narrow.map((element) => switch (element) {
ApiNarrowDm() => element.resolve(legacy: !supportsOperatorDm),
_ => element,
}).toList();
final supportsOperatorWith = zulipFeatureLevel >= 271; // TODO(server-9)

ApiNarrow result = narrow;
if (hasDmElement) {
result = narrow.map((element) => switch (element) {
ApiNarrowDm() => element.resolve(legacy: !supportsOperatorDm),
_ => element,
}).toList();
}
if (hasWithElement && !supportsOperatorWith) {
result.removeWhere((element) => element is ApiNarrowWith);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mutates result. If the argument narrow didn't happen to have an ApiNarrowDm in it, then that mutates the argument.

}
Comment on lines +26 to +35
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are probably cleanest as one loop that builds up result from empty, iterating through narrow.

(By the time we reach this point we should already know that the loop is going to end up producing a result that's different from narrow, so we can't avoid producing a fresh list and don't need further conditions like if (hasDmElement) as an optimization.)

return result;
}

/// An element in the list representing a narrow in the Zulip API.
Expand Down Expand Up @@ -64,13 +82,29 @@ class ApiNarrowTopic extends ApiNarrowElement {
);
}

/// An [ApiNarrowElement] with the 'with' operator.
///
/// If part of [ApiNarrow] use [resolveApiNarrowElements].
class ApiNarrowWith extends ApiNarrowElement {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: let's order this below ApiNarrowDm and its subclasses, as well as below ApiNarrowStream and ApiNarrowTopic.

This type of element is an alternative way to specify a conversation, which supersedes those three when it takes effect. So I want to think of those three jointly as one way of specifying a conversation, and this one as a different way.

@override String get operator => 'with';

@override final int operand;

ApiNarrowWith(this.operand, {super.negated});

factory ApiNarrowWith.fromJson(Map<String, dynamic> json) => ApiNarrowWith(
json['operand'] as int,
negated: json['negated'] as bool? ?? false,
);
}

/// An [ApiNarrowElement] with the 'dm', or legacy 'pm-with', operator.
///
/// An instance directly of this class must not be serialized with [jsonEncode],
/// and more generally its [operator] getter must not be called.
/// Instead, call [resolve] and use the object it returns.
///
/// If part of [ApiNarrow] use [resolveDmElements].
/// If part of [ApiNarrow] use [resolveApiNarrowElements].
class ApiNarrowDm extends ApiNarrowElement {
@override String get operator {
assert(false,
Expand Down
4 changes: 2 additions & 2 deletions lib/api/route/messages.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ Future<GetMessagesResult> getMessages(ApiConnection connection, {
// bool? useFirstUnreadAnchor // omitted because deprecated
}) {
return connection.get('getMessages', GetMessagesResult.fromJson, 'messages', {
'narrow': resolveDmElements(narrow, connection.zulipFeatureLevel!),
'narrow': resolveApiNarrowElements(narrow, connection.zulipFeatureLevel!),
'anchor': RawParameter(anchor.toJson()),
if (includeAnchor != null) 'include_anchor': includeAnchor,
'num_before': numBefore,
Expand Down Expand Up @@ -367,7 +367,7 @@ Future<UpdateMessageFlagsForNarrowResult> updateMessageFlagsForNarrow(ApiConnect
if (includeAnchor != null) 'include_anchor': includeAnchor,
'num_before': numBefore,
'num_after': numAfter,
'narrow': resolveDmElements(narrow, connection.zulipFeatureLevel!),
'narrow': resolveApiNarrowElements(narrow, connection.zulipFeatureLevel!),
'op': RawParameter(op.toJson()),
'flag': RawParameter(flag.toJson()),
});
Expand Down
12 changes: 9 additions & 3 deletions lib/model/internal_link.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ String? decodeHashComponent(String str) {
// you do so by passing the `anchor` param.
Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) {
// TODO(server-7)
final apiNarrow = resolveDmElements(
final apiNarrow = resolveApiNarrowElements(
narrow.apiEncode(), store.connection.zulipFeatureLevel!);
final fragment = StringBuffer('narrow');
for (ApiNarrowElement element in apiNarrow) {
Expand All @@ -77,6 +77,8 @@ Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) {
fragment.write('$streamId-$slugifiedName');
case ApiNarrowTopic():
fragment.write(_encodeHashComponent(element.operand));
case ApiNarrowWith():
fragment.write(element.operand.toString());
case ApiNarrowDmModern():
final suffix = element.operand.length >= 3 ? 'group' : 'dm';
fragment.write('${element.operand.join(',')}-$suffix');
Expand Down Expand Up @@ -151,6 +153,7 @@ Narrow? _interpretNarrowSegments(List<String> segments, PerAccountStore store) {

ApiNarrowStream? streamElement;
ApiNarrowTopic? topicElement;
ApiNarrowWith? withElement;
ApiNarrowDm? dmElement;
Set<IsOperand> isElementOperands = {};

Expand Down Expand Up @@ -185,9 +188,12 @@ Narrow? _interpretNarrowSegments(List<String> 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;
}
Expand Down Expand Up @@ -216,7 +222,7 @@ Narrow? _interpretNarrowSegments(List<String> segments, PerAccountStore store) {
} else if (streamElement != null) {
final streamId = streamElement.operand;
if (topicElement != null) {
return TopicNarrow(streamId, topicElement.operand);
return TopicNarrow(streamId, topicElement.operand, with_: withElement?.operand);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we find a withElement but not a stream/channel or topic, or find a withElement and also some other operator besides those?

Generally if we can't represent the exact semantics of a link with our Narrow type (i.e. with the message lists we're prepared to show in the UI), then this parsing function should return null so that we send the user to a browser.

The one case where we might want to make a hack of accepting a link even though we'll give it only approximate semantics is where it's a link that's going to be frequently generated by Zulip, i.e. either the server or one of our clients. That's basically the story of why in main we accept with operators and ignore them, and near too.

} else {
return ChannelNarrow(streamId);
}
Expand Down
36 changes: 36 additions & 0 deletions lib/model/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
numAfter: 0,
);
if (this.generation > generation) return;
_adjustTopicPermalinkNarrow(result.messages.firstOrNull);
store.reconcileMessages(result.messages);
store.recentSenders.handleMessages(result.messages); // TODO(#824)
for (final message in result.messages) {
Expand All @@ -493,11 +494,46 @@ 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 _adjustTopicPermalinkNarrow(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(:final streamId, :final topic):
this.narrow = TopicNarrow(streamId, topic);
Comment on lines +522 to +523
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
case StreamMessage(:final streamId, :final topic):
this.narrow = TopicNarrow(streamId, topic);
case StreamMessage():
this.narrow = TopicNarrow.ofMessage(someFetchedMessageOrNull);

This feels a bit more direct.

case DmMessage(): // TODO(log)
assert(false);
}
}

/// Fetch the next batch of older messages, if applicable.
Future<void> fetchOlder() async {
if (haveOldest) return;
if (fetchingOlder) 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();
Expand Down
17 changes: 12 additions & 5 deletions lib/model/narrow.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,14 +92,17 @@ 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);
}

final int streamId;
final String topic;
final int? with_;

TopicNarrow sansWith() => TopicNarrow(streamId, topic);

@override
bool containsMessage(Message message) {
Expand All @@ -108,22 +111,26 @@ 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)';
String toString() => 'TopicNarrow($streamId, $topic, with: $with_)';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: in toString, leave the optional field out when it's null (or more generally, when it's the default)

These narrow objects appear fairly often in test output, so the noise is worth a few more tokens of logic here in order to suppress.


@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.
Expand Down
16 changes: 13 additions & 3 deletions test/api/route/messages_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ void main() {
test('Narrow.toJson', () {
return FakeApiConnection.with_((connection) async {
void checkNarrow(ApiNarrow narrow, String expected) {
narrow = resolveDmElements(narrow, connection.zulipFeatureLevel!);
narrow = resolveApiNarrowElements(narrow, connection.zulipFeatureLevel!);
check(jsonEncode(narrow)).equals(expected);
}

Expand All @@ -188,6 +188,11 @@ void main() {
{'operator': 'stream', 'operand': 12},
{'operator': 'topic', 'operand': 'stuff'},
]));
checkNarrow(const 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'},
]));
Expand All @@ -203,6 +208,11 @@ void main() {
checkNarrow([ApiNarrowDm([123, 234])], jsonEncode([
{'operator': 'pm-with', 'operand': [123, 234]},
]));
connection.zulipFeatureLevel = 270;
checkNarrow(const TopicNarrow(12, 'stuff', with_: 1).apiEncode(), jsonEncode([
{'operator': 'stream', 'operand': 12},
{'operator': 'topic', 'operand': 'stuff'},
]));
connection.zulipFeatureLevel = eg.futureZulipFeatureLevel;
});
});
Expand Down Expand Up @@ -263,7 +273,7 @@ void main() {
});
});

test('narrow uses resolveDmElements to encode', () {
test('narrow uses resolveApiNarrowElements to encode', () {
return FakeApiConnection.with_(zulipFeatureLevel: 176, (connection) async {
connection.prepare(json: fakeResult.toJson());
await checkGetMessages(connection,
Expand Down Expand Up @@ -650,7 +660,7 @@ void main() {
});
});

test('narrow uses resolveDmElements to encode', () {
test('narrow uses resolveApiNarrowElements to encode', () {
return FakeApiConnection.with_(zulipFeatureLevel: 176, (connection) async {
connection.prepare(json: mkResult(foundOldest: true).toJson());
await checkUpdateMessageFlagsForNarrow(connection,
Expand Down
6 changes: 3 additions & 3 deletions test/model/internal_link_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,12 +173,12 @@ void main() {
const testCases = [
('/#narrow/stream/check/topic/test', TopicNarrow(1, 'test')),
('/#narrow/stream/mobile/subject/topic/near/378333', TopicNarrow(3, 'topic')),
('/#narrow/stream/mobile/subject/topic/with/1', TopicNarrow(3, 'topic')),
('/#narrow/stream/mobile/subject/topic/with/1', TopicNarrow(3, 'topic', with_: 1)),
('/#narrow/stream/mobile/topic/topic/', TopicNarrow(3, 'topic')),
('/#narrow/stream/stream/topic/topic/near/1', TopicNarrow(5, 'topic')),
('/#narrow/stream/stream/topic/topic/with/22', TopicNarrow(5, 'topic')),
('/#narrow/stream/stream/topic/topic/with/22', TopicNarrow(5, 'topic', with_: 22)),
('/#narrow/stream/stream/subject/topic/near/1', TopicNarrow(5, 'topic')),
('/#narrow/stream/stream/subject/topic/with/333', TopicNarrow(5, 'topic')),
('/#narrow/stream/stream/subject/topic/with/333', TopicNarrow(5, 'topic', with_: 333)),
('/#narrow/stream/stream/subject/topic', TopicNarrow(5, 'topic')),
];
testExpectedNarrows(testCases, streams: streams);
Expand Down
Loading