-
Notifications
You must be signed in to change notification settings - Fork 348
Channel link autocomplete #1902
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
Exciting! Thanks for building this. Just to record here what I said on the team call yesterday: for this PR, we can start the reviews in parallel with you writing the tests. So I'd suggest going ahead and adding those docs and comments next — then once you consider the PR all ready except for the tests, just mention that here and add the "maintainer review" label. |
5a8171d to
6671cfa
Compare
|
Thanks @gnprice for mentioning this. This is now ready for an initial review. (While working on the first todo, I realized that there were a few other places that needed some changes, which caused a delay 😀) |
6671cfa to
fe25a32
Compare
|
(just rebased atop main with conflicts resolved) |
fe25a32 to
480e787
Compare
6b2fb06 to
5072dce
Compare
|
@chrisbobbe Pushed a new revision with tests included. |
|
Thanks for this, and apologies for my delay in reviewing! Here's a review of the first six commits: 05c8437 channel: Add isRecentlyActive field plus some comments on later commits where I happened to see something interesting. 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no—thanks for the ping in DMs, somehow I didn't actually submit that review yesterday! Here it is.
test/example_data.dart
Outdated
| historyPublicToSubscribers: historyPublicToSubscribers ?? true, | ||
| messageRetentionDays: messageRetentionDays, | ||
| channelPostPolicy: channelPostPolicy ?? ChannelPostPolicy.any, | ||
| isRecentlyActive: isRecentlyActive ?? false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think true might be a more natural default value for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to true. The false default value was to make the tests in "ranking across signals" and "final results end-to-end" of "ChannelLinkAutocompleteView" group a little less verbose.🙂
lib/api/model/model.dart
Outdated
| @JsonKey(name: 'stream_post_policy') | ||
| ChannelPostPolicy? channelPostPolicy; // TODO(server-10) remove | ||
| // final bool isAnnouncementOnly; // deprecated for `channelPostPolicy`; ignore | ||
| bool? isRecentlyActive; // TODO(server-10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
channel: Add isRecentlyActive field
Since we're already not matching the order in the API doc (see e.g. #1894 (comment) ), I'd put this next to the related-looking field streamWeeklyTraffic, perhaps just above it without an empty line in between.
Similarly elsewhere in this commit.
lib/api/model/events.dart
Outdated
| required super.id, | ||
| required this.streams, | ||
| required this.streamIds, | ||
| }) : assert(streams != null || streamIds != null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We try to reserve assert for invariants that are our own responsibility, i.e. those that won't be broken except for some broken logic in the client. Here, the invariant exists, but it's one that can be broken by something out of our control, in particular a badly-behaving server.
Also, asserts don't run in production, so this won't work as "crunchy shell" validation. It makes sense to want such validation, so the "soft center" of the app can rely on this invariant. But let's do it in ChannelDeleteEvent.fromJson; for an example to follow, see DeleteMessageEvent.fromJson.
lib/api/model/events.dart
Outdated
|
|
||
| final List<ZulipStream> streams; | ||
| final List<ZulipStreamId>? streams; // TODO(server-10): remove | ||
| final List<int>? streamIds; // TODO(server-10): remove nullability |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we'd normally say TODO(server-10) make required or just TODO(server-10).
lib/model/channel.dart
Outdated
| streams.remove(stream.streamId); | ||
| streamsByName.remove(stream.name); | ||
| subscriptions.remove(stream.streamId); | ||
| final channelIds = event.streamIds ?? event.streams!.map((e) => e.streamId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh actually, we can make this nicer by encapsulating this conditional in the API-binding layer—ChannelDeleteEvent can have just final List<int> streamIds (maybe channelIds, as the more modern name?), and it can read its value depending on what the JSON looks like.
Something like this (untested)?:
/// A [ChannelEvent] with op `delete`: https://zulip.com/api/get-events#stream-delete
@JsonSerializable(fieldRename: FieldRename.snake)
class ChannelDeleteEvent extends ChannelEvent {
@override
@JsonKey(includeToJson: true)
String get op => 'delete';
@JsonKey(readValue: _readChannelIds)
final List<int> channelIds;
// TODO(server-10) simplify away; rely on stream_ids
static List<int> _readChannelIds(Map<dynamic, dynamic> json, String key) {
final channelIds = json['stream_ids'] as List<int>?;
if (channelIds != null) return channelIds;
final channels = json['streams'] as List<dynamic>;
return channels
.map((c) => (c as Map<String, dynamic>)['stream_id'] as int)
.toList();
}
ChannelDeleteEvent({
required super.id,
required this.channelIds,
});
factory ChannelDeleteEvent.fromJson(Map<String, dynamic> json) =>
_$ChannelDeleteEventFromJson(json);
@override
Map<String, dynamic> toJson() => _$ChannelDeleteEventToJson(this);
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(In that code, the crunchy-shell validation is done by the final channels = json['streams'] as List<dynamic>; line, which will throw if both .stream_ids and .streams are absent in the json.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to the new version. One thing that this does is that in toJson, there will be an entry with key channel_ids; not exactly what the server gives us (stream_ids or streams). Should we edit the toJson method to match the server data, or is it not that important since we don't use it to send it back to the server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, so there was a test failing in test/model/store_test.dart and the fix was to include streams in toJson.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, streams is deprecated and may be removed, so I think we should treat it as valid if streams is absent and stream_ids is present. I'd want our tests to tolerate that form without failing.
I took a look and found a bug in _readChannelIds in this revision 🙂:
diff --git lib/api/model/events.dart lib/api/model/events.dart
index 6a0d9ffa4..4d9c5121c 100644
--- lib/api/model/events.dart
+++ lib/api/model/events.dart
@@ -622,7 +622,7 @@ class ChannelDeleteEvent extends ChannelEvent {
// TODO(server-10) simplify away; rely on stream_ids
static List<int> _readChannelIds(Map<dynamic, dynamic> json, String key) {
final channelIds = json['stream_ids'] as List<dynamic>?;
- if (channelIds != null) channelIds.map((id) => id as int).toList();
+ if (channelIds != null) return channelIds.map((id) => id as int).toList();
final channels = json['streams'] as List<dynamic>;
return channels
lib/widgets/autocomplete.dart
Outdated
| overflow: TextOverflow.ellipsis, | ||
| color: designVariables.contextMenuItemMeta)), | ||
| child: BlockContentList( | ||
| nodes: parseContent(channel!.renderedDescription).nodes), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat! Unfortunately we're not ready to show the rendered channel description; some of our content widgets will break if they appear outside the context of a message, because they use InheritedMessage.of(context), and we need to address that systematically, which is #488. See related issues:
- Show channel description in channel action sheet #1896
- content: Support Zulip content outside messages (even outside per-account contexts) #488
For now let's do as I did in #1877:
- Not try to show the channel description here
- File an issue for it and leave a TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #1945. Also, looking at https://github.com/zulip/zulip-flutter/blob/main/lib/widgets/content.dart, it seems like InheritedMessage.of(context) is used in two places, MessageImagePreview and MessageInlineVideo, and by manually testing, it seems like the server is not sending the related HTML for rendering these widgets when there is an image or video in the channel description. So I think it will be safe to show the channel description now. But as it’s possible that InheritedMessage.of(context) will be used in other widgets, it's good to wait for #488 as you mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by manually testing, it seems like the server is not sending the related HTML for rendering these widgets when there is an image or video in the channel description. So I think it will be safe to show the channel description now.
For this sort of detail I'd want to rely on API guarantees rather than manual testing on a current server. It's at least plausible that we could run into different behavior with some old server we still support (like 7), or even on any server, including a modern one like CZO, for a channel that was last updated when the server version was ancient, like 3 or something.
But as it’s possible that
InheritedMessage.of(context)will be used in other widgets, it's good to wait for #488 as you mentioned.
Yep, this is also true :) it'll be nice to be systematic about it.
| // Behavior we have that web doesn't and might like to follow: | ||
| // - A "word-prefixes" match quality on channel names: | ||
| // see [NameMatchQuality.wordPrefixes], which we rank on. | ||
| // | ||
| // Behavior web has that seems undesired, which we don't plan to follow: | ||
| // - A "word-boundary" match quality on channel names: | ||
| // special rank when the whole query appears contiguously | ||
| // right after a word-boundary character. | ||
| // Our [NameMatchQuality.wordPrefixes] seems smarter. | ||
| // - Ranking some case-sensitive matches differently from case-insensitive | ||
| // matches. Users will expect a lowercase query to be adequate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I reading web's sort_streams correctly that it also considers channel descriptions in the filtering and ranking? I don't personally think we need to do that, but it probably deserves a mention here.
lib/model/autocomplete.dart
Outdated
| return switch((tryCast<Subscription>(a), tryCast<Subscription>(b))) { | ||
| (Subscription(), null) => -1, | ||
| (null, Subscription()) => 1, | ||
| (Subscription(isMuted: false), Subscription(isMuted: true)) => -1, | ||
| (Subscription(isMuted: true), Subscription(isMuted: false)) => 1, | ||
| (Subscription(pinToTop: true), Subscription(pinToTop: false)) => -1, | ||
| (Subscription(pinToTop: false), Subscription(pinToTop: true)) => 1, | ||
| _ => 0, | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to add and use tryCast for this, and instead do something like:
| return switch((tryCast<Subscription>(a), tryCast<Subscription>(b))) { | |
| (Subscription(), null) => -1, | |
| (null, Subscription()) => 1, | |
| (Subscription(isMuted: false), Subscription(isMuted: true)) => -1, | |
| (Subscription(isMuted: true), Subscription(isMuted: false)) => 1, | |
| (Subscription(pinToTop: true), Subscription(pinToTop: false)) => -1, | |
| (Subscription(pinToTop: false), Subscription(pinToTop: true)) => 1, | |
| _ => 0, | |
| }; | |
| if (a is Subscription && b is! Subscription) return -1; | |
| if (a is! Subscription && b is Subscription) return 1; | |
| return switch((a, b)) { | |
| (Subscription(isMuted: false), Subscription(isMuted: true)) => -1, | |
| (Subscription(isMuted: true), Subscription(isMuted: false)) => 1, | |
| (Subscription(pinToTop: true), Subscription(pinToTop: false)) => -1, | |
| (Subscription(pinToTop: false), Subscription(pinToTop: true)) => 1, | |
| _ => 0, | |
| }; |
which is equivalent and doesn't add a step for the reader to interpret where null comes from and what it means. It also separates the main, headline logic from the rest, corresponding to the dartdoc's choice of what goes in its first line vs. the body:
/// Comparator that puts subscribed channels before unsubscribed ones.
///
/// For subscribed channels, it puts them in the following way:
/// pinned unmuted > unpinned unmuted > pinned muted > unpinned mutedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(also nit: s/way/order/ in that dartdoc)
lib/model/autocomplete.dart
Outdated
| /// Comparator that puts channels with more weekly traffic first. | ||
| /// | ||
| /// A channel with undefined weekly traffic (`null`) is put after the channel | ||
| /// with a weekly traffic defined (even if it is zero). | ||
| /// | ||
| /// Weekly traffic is the average number of messages sent to the channel per | ||
| /// week, which is determined by [ZulipStream.streamWeeklyTraffic]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// Comparator that puts channels with more weekly traffic first. | |
| /// | |
| /// A channel with undefined weekly traffic (`null`) is put after the channel | |
| /// with a weekly traffic defined (even if it is zero). | |
| /// | |
| /// Weekly traffic is the average number of messages sent to the channel per | |
| /// week, which is determined by [ZulipStream.streamWeeklyTraffic]. | |
| /// Comparator that puts channels with more [ZulipStream.streamWeeklyTraffic] first. | |
| /// | |
| /// A channel with undefined weekly traffic (`null`) is put after the channel | |
| /// with a weekly traffic defined (even if it is zero). |
This is a very reasonable definition of "weekly traffic" 🙂 and so isn't likely to bitrot i.e. become incorrect over time. But since we're just using ZulipStream.streamWeeklyTraffic directly (no computations on it), let's leave that field's definition as the single place where we write its definition, so we only have to change that one thing if the meaning changes.
…I see that we haven't actually written down the field's meaning, which we might've done in dartdoc on the field. But that's quite normal and appropriate; by leaving it blank, we mean to defer to the API documentation, which is linked in the class ZulipStream dartdoc.
test/model/compose_test.dart
Outdated
| eg.stream(streamId: 5, name: 'UI [v2]'), | ||
| eg.stream(streamId: 6, name: r'Save $$'), | ||
| ]; | ||
| store.addStreams(channels); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be awaited; similarly in a few other places.
Thanks to the discarded_futures lint for catching this, actually; I was playing with it for #731 this morning 🙂
In the following commits, this will be used as one of the criteria for sorting channels in channel link autocomplete.
5072dce to
8cde339
Compare
There are new changes made to `stream op: delete` event in server-10:
- The `streams` field which used to be an array of the just-deleted
channel objects is now an array of objects which only contains ID
of the just-deleted channels (this would crash the app before
this commit).
- The same `streams` field is also deprecated and will be removed in a
future release.
- As a replacement to `streams`, `stream_ids` is introduced which is
an array of the just-deleted channel IDs.
These two methods were introduced but never called.
This was missed to be added in AutocompleteViewManager.reassemble.
8cde339 to
ea64c45
Compare
|
Thanks @chrisbobbe for the review. Pushed new changes, PTAL. |
Also, generalize the dartdoc of NameMatchQuality. For almost all types of autocompletes, the matching mechanism/quality to an autocomplete query seems to be the same with rare exceptions (at the time of writing this, only the emoji autocomplete matching mechanism is different).
As of this commit, it's not yet possible in the app to initiate a channel link autocomplete interaction. So in the widgets code that would consume the results of such an interaction, we just throw for now, leaving that to be implemented in a later commit.
For this commit we temporarily intercept the query at the AutocompleteField widget, to avoid invoking the widgets that are still unimplemented. That lets us defer those widgets' logic to a separate later commit.
This will make it easy to use the fragment string in several other places, such as in the next commits where we need to create a fallback markdown link for a channel.
ea64c45 to
d1abf20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This is working great; comments below, this time from reading the whole branch.
| case ChannelPropertyName.isRecentlyActive: | ||
| return value as bool?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: bump on #1902 (comment)
| case ChannelPropertyName.isRecentlyActive: | ||
| assert(value is bool?); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: bump on #1902 (comment)
lib/model/channel.dart
Outdated
| streams.remove(stream.streamId); | ||
| streamsByName.remove(stream.name); | ||
| subscriptions.remove(stream.streamId); | ||
| final channelIds = event.streamIds ?? event.streams!.map((e) => e.streamId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, streams is deprecated and may be removed, so I think we should treat it as valid if streams is absent and stream_ids is present. I'd want our tests to tolerate that form without failing.
I took a look and found a bug in _readChannelIds in this revision 🙂:
diff --git lib/api/model/events.dart lib/api/model/events.dart
index 6a0d9ffa4..4d9c5121c 100644
--- lib/api/model/events.dart
+++ lib/api/model/events.dart
@@ -622,7 +622,7 @@ class ChannelDeleteEvent extends ChannelEvent {
// TODO(server-10) simplify away; rely on stream_ids
static List<int> _readChannelIds(Map<dynamic, dynamic> json, String key) {
final channelIds = json['stream_ids'] as List<dynamic>?;
- if (channelIds != null) channelIds.map((id) => id as int).toList();
+ if (channelIds != null) return channelIds.map((id) => id as int).toList();
final channels = json['streams'] as List<dynamic>;
return channels| for (final view in _channelLinkAutocompleteViews) { | ||
| view.reassemble(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! Sounds like that would help us avoid a bug like the one fixed here.
| // Similar reasoning as in _mentionIntentRegex. | ||
| const before = r'(?<=^|\s|\p{Punctuation})'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // As Web, match both '#channel' and '#**channel'. In both cases, the raw | ||
| // query is going to be 'channel'. Matching the second case ('#**channel') | ||
| // is useful when the user selects a channel from the autocomplete list, but | ||
| // then starts pressing "backspace" to edit the query and choose another | ||
| // option, instead of clearing the entire query and starting from scratch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Looks like web also does the corresponding thing for @-mentions: https://github.com/zulip/zulip/blob/1e78447c5/web/src/composebox_typeahead.ts#L516-L531
function filter_mention_name(current_token: string): string | undefined {
if (current_token.startsWith("**")) {
current_token = current_token.slice(2);
} else if (current_token.startsWith("*")) {
current_token = current_token.slice(1);
}
if (current_token.includes("*")) {
return undefined;
}
// Don't autocomplete if there is a space following an '@'
if (current_token.startsWith(" ")) {
return undefined;
}
return current_token;
}This is maybe more important for channel/topic autocomplete, right? Because (once the topic part is implemented) you might backspace as part of figuring out how to get just a channel link without a topic. But this is a good prompt to file an issue and add a TODO for doing this with @-mention autocomplete; would you do those?
| } else { | ||
| icon = null; | ||
| iconColor = null; | ||
| label = zulipLocalizations.unknownChannelName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we normally show "(unknown channel)" in italics, to distinguish it from a potential channel with that name.
| padding: EdgeInsetsDirectional.fromSTEB(12, 4, 10, 4), | ||
| child: Row(spacing: 10, children: [ | ||
| SizedBox.square(dimension: 24, | ||
| child: Icon(size: 18, color: iconColor, icon)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rows have 44px height in the Figma, but this gives 32px height (unless increased via the device text-size setting).
To bring it up to 44px, we could structure it similarly to MentionAutocompleteItem—
| padding: EdgeInsetsDirectional.fromSTEB(12, 4, 10, 4), | |
| child: Row(spacing: 10, children: [ | |
| SizedBox.square(dimension: 24, | |
| child: Icon(size: 18, color: iconColor, icon)), | |
| padding: EdgeInsetsDirectional.fromSTEB(4, 4, 8, 4), | |
| child: Row(spacing: 6, children: [ | |
| SizedBox.square(dimension: 36, | |
| child: Center( | |
| child: Icon(size: 18, color: iconColor, icon))), |
—which could be helpful in a potential future where we made a generic AutocompleteItem widget that serves both kinds of autocomplete.
| /// | ||
| /// [fallbackMarkdownLink] will be used if the channel name includes some faulty | ||
| /// characters that will break normal #**channel** rendering. | ||
| String channelLinkSyntax(ZulipStream channel, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just channelLink, I think; the "syntax" part is implied. (As in existing functions in this file, like quoteAndReply which creates quote-and-reply syntax, or userMention which creates user-mention syntax.)
| /// Markdown link for channel, topic, or message when the channel or topic name | ||
| /// includes characters that will break normal markdown rendering. | ||
| /// | ||
| /// Refer to [_channelTopicFaultyCharsReplacements] for a complete list of | ||
| /// these characters. | ||
| // Corresponds to `topic_link_util.get_fallback_markdown_link` in Zulip web; | ||
| // https://github.com/zulip/zulip/blob/b42d3e77e/web/src/topic_link_util.ts#L96-L108 | ||
| String fallbackMarkdownLink({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
channel: Finish channel link autocomplete for compose box
Could you separate this special character-replacement logic into its own commit? I want to see if we can ground our reasoning in API documentation. As far as that goes, there's nothing "invalid" or "faulty" about these characters appearing in channel names.
TODOs:
Adding some dartdocs and commentsTestsFixes-partly: #124 (topic link autocomplete will be its own PR)
Screenshots
Screen recording
Screen.Recording.2025-10-08.at.9.08.43.PM.mov