autocomplete: Support @-wildcard in user-mention autocomplete #889

sm-sayedi · 2024-08-16T21:48:59Z

Screenshot

Screen recording

Wildcard.mention.mp4

Fixes: #234

gnprice

Thanks @sm-sayedi! A few high-level comments below.

gnprice · 2024-08-19T21:24:53Z

assets/icons/bullhorn.svg

@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><path d="M12,8H4A2,2 0 0,0 2,10V14A2,2 0 0,0 4,16H5V20A1,1 0 0,0 6,21H8A1,1 0 0,0 9,20V16H12L17,20V4L12,8M15,15.6L13,14H4V10H13L15,8.4V15.6M21.5,12C21.5,13.71 20.54,15.26 19,16V8C20.53,8.75 21.5,10.3 21.5,12Z" /></svg>


What was the origin of this icon — is it from a particular place in Figma?

(I don't see a bullhorn.svg in Zulip web, which would be the other usual source.)

See git log --stat assets/icons/ for example previous commit messages.

That icon was from materialdesignicons.com. Updated the icon in this revision to match it with the web. The web uses a Font Awesome icon.

Cool. That sounds like a case of this issue, then:

Add Font Awesome icons #222

Given Vlad's reply there, we'll probably want some other icon for this. I think the most efficient way to resolve that is to start a thread in #mobile and ask Vlad there what icon he'd like us to use.

He did recently finish a design that's related (I still need to update the relevant issue to reflect that):
https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3859-2936&t=z2va3CAInaEg1eWy-0
but looking there, it doesn't have an example for a wildcard, only for a group or an individual user. So go ahead and ask in chat, and include a link to that Figma node for context.

gnprice · 2024-08-19T21:30:47Z

lib/api/model/model.dart

+  /// The full name of the wildcard to be shown in autocomplete suggestions.
+  ///
+  /// Ex: "all (Notify channel)" or "everyone (Notify recipients)".
+  final String fullDisplayName;


This seems more about our UI than about the Zulip server API. So let's find a home for it outside of lib/api/.

I think a good solution would look similar to MentionAutocompleteResult and its subclasses. Instead of a User being an object that knows how to behave as an @-mention autocomplete candidate, we'd have an object that describes an @-mention autocomplete candidate and refers to a User to do so.

In fact probably a good solution would be for these "autocomplete candidate" objects to be the same objects as the result objects.

I'll try doing a quick refactor in that direction.

(→ #889 (comment) below)

This seems more about our UI than about the Zulip server API. So let's find a home for it outside of lib/api/.

Now that the model class is moved outside of lib/api, is it okay for fullDisplayName to still live in the class?

Yeah. Try to find the arrangement that seems cleanest (and once the PR is ready, reviewers may have suggestions), but between lib/model/ and lib/widgets/ either way is basically fine for where that lives.

gnprice · 2024-08-19T21:37:08Z

lib/model/store.dart

+  Map<String, Wildcard> wildcardsForNarrow(Narrow narrow) => Map.fromEntries(
+    _wildcardsForNarrow(narrow).map((w) => MapEntry(w.name, w)));
+
+  List<Wildcard> _wildcardsForNarrow(Narrow narrow) {


This should live in an autocomplete-specific file — store.dart, and the PerAccountStore class definition, are fairly crowded places, so anything that can naturally go somewhere more specific should do so.

gnprice · 2024-08-19T21:38:01Z

lib/model/store.dart

+    return [
+      Wildcard(


Does this mirror the corresponding logic in zulip-mobile? (I haven't read this closely.) A link for comparison would be handy.

It doesn't follow the zulip-mobile in the exact same way regarding the code structure. Here's the zulip-mobile implementation: https://github.com/zulip/zulip-mobile/blob/main/src/autocomplete/WildcardMentionItem.js. Should I provide this link in the code itself?

OK. Let's try to make sure it gets the same results, even if the code ends up organized differently. (There should be test cases in zulip-mobile that will be helpful to translate over, to verify that.)

Then also include a link to that zulip-mobile implementation in either a code comment or commit message. For the link, make it a GitHub permalink:
#692 (comment)
#884 (comment)
That'll be helpful for reviewers, as a point of comparison.

gnprice · 2024-08-19T23:36:50Z

In fact probably a good solution would be for these "autocomplete candidate" objects to be the same objects as the result objects.

I'll try doing a quick refactor in that direction.

OK, I started down that road (generating #895 along the way), and then realized it would mean we'd allocate one of those objects for every user, in MentionAutocompleteView._usersByRelevance and so MentionAutocompleteView.init. That doesn't sound great performance-wise when there are like 30k users in a realm.

Instead, let's handle the wildcard-vs-user distinction in the control flow. Take a look at #896, and build your changes atop that. Then the existing results for an @-mention are computed here:

  Future<List<MentionAutocompleteResult>?> computeResults() async {
    final results = <MentionAutocompleteResult>[];
    if (await filterCandidates(filter: _testUser,
          candidates: sortedUsers, results: results)) {
      return null;
    }
    return results;
  }

where that filterCandidates call does the main loop and accumulates results onto that results local. To add wildcard candidates, you can just add more code next to that, calling List.add on the same results local.

sm-sayedi · 2024-08-22T20:44:39Z

Thanks @gnprice for the previous review. It was really helpful. Pushed a new revision (still draft). PTAL.

alya · 2024-08-22T23:06:12Z

We're currently thinking we should use the three-people icon for wildcard mentions, rather than a loudspeaker. Assuming we move forward with it for the web app, we should make the same change here.

gnprice

Hmm, I missed that this had a request for my review, sorry @sm-sayedi — being a draft and with neither of the review labels, it looked like it was awaiting another revision first.

In general any time there's a PR where you're awaiting review for more than a week or two, please feel free to ping on the PR thread or in chat. We generally try to turn around reviews within a day or two, so a wait of over a week is likely a sign that something fell through the cracks.

Comments below. Since this PR is still a draft, I looked only at high-level aspects of the structure.

gnprice · 2024-11-07T23:29:53Z

lib/model/autocomplete.dart

+    _isChannelWildcardIncluded = false;
+    final results = <MentionAutocompleteResult>[];
+    // give priority to wildcard mentions
+    if (await filterCandidates(filter: _testWildcard,


The fancy filterCandidates machinery isn't needed for the wildcards, because there's only a handful of them. (It's useful when there are potentially thousands of candidates.) So this can be simplified to a direct loop through the wildcard candidates.

That also lets _inChannelWildcardIncluded be a local variable, which simplifies reasoning about it.

… Then I started writing that the logic might be further clarified by unrolling the loop, akin to the logic in zulip-mobile:
https://github.com/zulip/zulip-mobile/blob/715d60a5e87fe37032bce58bd72edb99208e15be/src/autocomplete/WildcardMentionItem.js#L113-L138

But I see we discussed this previously above:
#889 (comment)
and that you have the corresponding logic living instead in the widgets code, in ComposeAutocomplete._wildcards. That structure may be fine too. Should have a link to the zulip-mobile version, though, as discussed there.

I switched back to a similar logic of what zulip-mobile does. I think it fits our current code structure well.

gnprice · 2024-11-07T23:30:46Z

lib/model/autocomplete.dart

  final List<User> sortedUsers;

-  @override
-  Future<List<MentionAutocompleteResult>?> computeResults() async {


Moving this method to lower in the file is perfectly reasonable, but should happen as its own separate NFC prep commit. That way the substantive change stands out better.

gnprice · 2024-11-07T23:47:30Z

lib/widgets/autocomplete.dart

+        fullDisplayName: 'all (${isDmNarrow
+          ? zulipLocalizations.notifyRecipients
+          : zulipLocalizations.notifyChannel(isChannelWildcardAvailable
+              ? "channel" : "stream")})',


I think it will be cleaner to move this fullDisplayName logic to where the result is actually being displayed, at build time.

Also avoid string manipulation on translated strings. See the "general background" at the end of this section:
https://github.com/zulip/zulip-mobile/blob/main/docs/howto/translations.md#all-code-contributors-wiring-our-code-for-translations

Instead, put the two parts of this into separate widgets or TextSpans. That way they can also get different text styles.

gnprice · 2024-11-07T23:50:19Z

lib/widgets/autocomplete.dart

+      case WildcardMentionAutocompleteResult(:var wildcardName):
+        avatar = const Icon(ZulipIcons.bullhorn, size: 29); // web uses 19px
+        print('wildcard name: $wildcardName');
+        label = _wildcards(context, store).singleWhere((w) => w.name == wildcardName).fullDisplayName;


Let's avoid doing a search like this, and instead have the MentionAutocompleteResult value include whatever information this needs.

(If we did do a search, then like with store.users[userId] above, we should make it so that we're just looking it up in an existing data structure rather than recomputing the whole list again.)

In fact I think I'd like to refactor UserMentionAutocompleteResult so that it carries the actual User object, rather than just a user ID. That change is orthogonal to this PR, though.

gnprice · 2024-11-08T00:03:27Z

Also just pushed a rebase of the branch. (I did the rebase in order to better review it, since some things had changed in main; having done that, I figure I might as well share the result.)

The main change is the rebase across e91694f (#908), which causes some of the changes here to disappear as having already happened.

sm-sayedi · 2024-12-06T07:50:03Z

Thanks @gnprice for the initial reviews. The PR is now ready for the maintainer review.

Note: The first two commits are actually necessary for displaying wildcard mentions based on a permission. I first created those commits and then started the CZO discussion , in which it was decided to leave the permission part for a future issue. I am not sure if we want to keep these two commits for now!

sm-sayedi · 2024-12-06T07:52:50Z

lib/widgets/autocomplete.dart

+    return Text.rich(TextSpan(text: '${wildcard.name} ', children: [
+      TextSpan(text: description, style: TextStyle(fontSize: 12,
+        color: Colors.black.withValues(alpha: 0.8)))]));


Web uses 85% of the name (i.e., all) font size for the description (i.e., Notify channel). For us, 85% of 14 is almost 12.

sm-sayedi · 2024-12-06T08:54:05Z

The CI is failing and it's related to icons. I am not sure why it is happening!

gnprice · 2024-12-06T09:46:32Z

The error output is:

Running icons...

added 162 packages in 29s
Error: there were icon updates:
 M assets/icons/ZulipIcons.ttf

Seems like that font file isn't updated to match your other changes. The icons.dart file has instructions for updating for icons.

sm-sayedi · 2024-12-06T17:40:53Z

Just transferred the CI failure discussion to CZO: https://chat.zulip.org/#narrow/channel/516-mobile-dev-help/topic/CI.20failure.20related.20to.20Zulip.20icons.20-.20.23F889/near/1994794.

chrisbobbe · 2025-01-27T22:37:01Z

Thanks, LGTM! Marking for Greg's review.

chrisbobbe · 2025-01-27T23:14:16Z

While reviewing, I noticed a new bug, but it's caused by an upstream change, not the changes here; filed as #1310.

gnprice

Thanks @sm-sayedi for carrying this through! And thanks @chrisbobbe for the previous reviews.

Generally this looks good. Some comments below. I haven't yet read the test changes, because I think a couple of these comments will lead to some simplifications there.

gnprice · 2025-01-28T00:00:52Z

lib/widgets/icons.dart

+  /// The Zulip custom icon "three_person".
+  static const IconData three_person = IconData(0xf121, fontFamily: "Zulip Icons");


nit:

assets: Add "three_person.svg" icon for wildcard mentions

The prefix we've used for this is "icons:" — see git log --oneline assets/icons.

gnprice · 2025-01-28T00:13:45Z

test/widgets/autocomplete_test.dart

-  // (hint text of compose input in a 1:1 DM)
-  final finder = find.widgetWithText(TextField, 'Message @${eg.otherUser.fullName}');
+  final finder = find.widgetWithText(TextField,
+    _composeInputHintTextFor(narrow, store: store));


This _composeInputHintTextFor helper is a fair bit of code, and seems brittle given that its details aren't something this test file is meant to be testing.

Instead, let's borrow what we have in compose_box_test.dart:

final contentInputFinder = find.byWidgetPredicate( (widget) => widget is TextField && widget.controller is ComposeContentController);

It's true that's a bit less end-to-end (the user sees the hint text and doesn't see the controller class is called "ComposeContentController"), but it should be more stable. The user doesn't care what the exact hint text is — outside of the tests for the hint text itself, what we really mean here is "the box that looks to the user like the compose box". It's tough to express that crisply, so we can settle for the ComposeContentController thing.

At least ComposeContentController isn't really an implementation detail — there are several different features of the product that all mean we'll want some kind of custom input-controller logic for the message content input, and will want to expose that to other code in the app, so it's a pretty stable assumption that we'll have something playing more or less the role of ComposeContentController indefinitely.

gnprice · 2025-01-28T00:28:02Z

lib/model/autocomplete.dart

-  AutocompleteIntent<ComposeAutocompleteQuery>? autocompleteIntent() {
+  AutocompleteIntent<ComposeAutocompleteQuery>? autocompleteIntent(ZulipLocalizations localizations) {


Hmm, I don't love having the localizations live on the AutocompleteQuery. That feels like it leads to having to route it through places that shouldn't have to know about it, like this.

Let's instead handle it the same way we handle the store: the view-model (the AutocompleteView) keeps around a reference to it, and passes that reference when needed to methods on the query that need it.

Then the autocomplete subsystem can get access to the localizations in the first place the same way it does for the store: the ComposeAutocomplete.initViewModel method, which has a build context, gets it from the context and passes it along.

In the current revision, the base AutocompleteView class holds a reference to ZulipLocalizations. With that, all the subclasses (should) retrieve the instance through FooAutocompleteView.init. What if we only have the ZulipLocalizations property in MentionAutocompleteView, not in the base class? I think for now, having localizations only makes sense in MentionAutocompleteView not the other two subclasses. Then if in the future, it makes sense for the others too, we can refactor the code and move ZulipLocalizations localizations to the base class, just like in this revision.

Yeah, I think having the localizations live either on the base class or the subclass would be fine — whichever seems easier and/or produces cleaner code right now. I'm not sure other types of autocomplete will ever need localizations; if they do, then add you say, we can refactor then.

OK, and now having read this revision: I agree, moving it to the subclass MentionAutocompleteView will make things cleaner.

gnprice · 2025-01-28T00:32:34Z

lib/model/autocomplete.dart

+    results.addAll(computeWildcardMentionResults(isComposingChannelMessage:
+      narrow is ChannelNarrow || narrow is TopicNarrow));
+
+    if (await filterCandidates(filter: _testUser,
+        candidates: sortedUsers, results: results)) {


Instead of making a fresh list and passing it to addAll, have the callee accumulate results on results directly:

Suggested change

results.addAll(computeWildcardMentionResults(isComposingChannelMessage:

narrow is ChannelNarrow || narrow is TopicNarrow));

if (await filterCandidates(filter: _testUser,

candidates: sortedUsers, results: results)) {

computeWildcardMentionResults(results: results,

isComposingChannelMessage: narrow is ChannelNarrow || narrow is TopicNarrow);

if (await filterCandidates(filter: _testUser,

candidates: sortedUsers, results: results)) {

That's a more efficient style since it involves less allocation and copying. Not a big deal for this particular helper method — there's at most a couple of these wildcard-mention results — but it's the style that computeResults is already working in, so let's follow along. (And it's important that the existing logic in computeResults works that way, because there may be a lot of individual-user results.)

gnprice · 2025-01-28T00:43:35Z

lib/model/autocomplete.dart

+    if (query.testWildcardOption(WildcardMentionOption.all)) {
+      wildcardMentions.add(WildcardMentionAutocompleteResult(
+        wildcard: WildcardMentionOption.all));
+    } else if (query.testWildcardOption(WildcardMentionOption.everyone)) {
+      wildcardMentions.add(WildcardMentionAutocompleteResult(
+        wildcard: WildcardMentionOption.everyone));


This ends up with a kind of unfortunate amount of verbosity, and it feels like it'd be easy to miss the key details that matter in it.

Here's one way to tighten it up, with a local helper function (inside this method) tryOption:

Suggested change

if (query.testWildcardOption(WildcardMentionOption.all)) {

wildcardMentions.add(WildcardMentionAutocompleteResult(

wildcard: WildcardMentionOption.all));

} else if (query.testWildcardOption(WildcardMentionOption.everyone)) {

wildcardMentions.add(WildcardMentionAutocompleteResult(

wildcard: WildcardMentionOption.everyone));

all: {

if (tryOption(WildcardMentionOption.all)) break all;

if (tryOption(WildcardMentionOption.everyone)) break all;

So tryOption would do both the query.test… call, and the results.add when the answer is yes.

The break statements there are doing a similar job to an early return, but without needing to wrap this subgroup of the logic in a function just to get early returns.

gnprice · 2025-01-28T01:04:35Z

lib/model/compose.dart

+  // Deprecated in FL 247. Empirically, current servers (FL 330)
+  // still parse "@**stream**" in messages though.
+  stream(canonicalString: 'stream'),
+  topic(canonicalString: 'topic'); // New in FL 224.


Suggested change

topic(canonicalString: 'topic'); // New in FL 224.

topic(canonicalString: 'topic'); // TODO(server-8): New in FL 224.

The TODO-server comment is how we make sure we can systematically clean out these historical comments in the future when they're no longer relevant. Basically all references to a Zulip feature level (as well as anything else that will become obsolete when we drop support for some old Zulip Server version) should have a TODO-server comment that will make sure we find it.

gnprice · 2025-01-28T01:05:49Z

lib/model/compose.dart

+  /// The string identifying this option (e.g. "all" as in "@**all**").
+  // If servers want to localize this, we can compute it
+  // dynamically from server data instead of hard-coding it.
+  final String canonicalString;


I don't 100% understand this comment (the // part), but I think it's speculating about a possible future change to the API. We can leave that out — if the API changes, we'll deal with it then.

gnprice · 2025-01-28T01:14:45Z

lib/model/compose.dart

+  final isChannelWildcardAvailable = store.account.zulipFeatureLevel >= 247; // TODO(server-9)
+  assert(isChannelWildcardAvailable || wildcardOption != WildcardMentionOption.channel);
+  final isTopicWildcardAvailable = store.account.zulipFeatureLevel >= 224; // TODO(sever-8)
+  assert(isTopicWildcardAvailable || wildcardOption != WildcardMentionOption.topic);
+
+  final name = wildcardOption == WildcardMentionOption.stream && isChannelWildcardAvailable
+    ? WildcardMentionOption.channel.canonicalString
+    : wildcardOption.canonicalString;


All these equality comparisons to different enum values feel like they'd be clearer as one switch statement that lists all the values.

Could start with String name = wildcardOption.canonicalString; and then have the switch mutate it in one case, to avoid repeating the common version and have the exception stand out.

gnprice · 2025-01-28T01:18:47Z

lib/widgets/autocomplete.dart

      ]));
  }
+
+  Widget wildcardLabel(WildcardMentionOption wildcard, {


nit: put this method before build; and mark it private

gnprice · 2025-01-28T01:21:23Z

lib/widgets/autocomplete.dart

+    };
+    return Text.rich(TextSpan(text: '${wildcard.canonicalString} ', children: [
+      TextSpan(text: description, style: TextStyle(fontSize: 12,
+        color: Colors.black.withValues(alpha: 0.8)))]));


Does this color work equally well in dark theme?

Let's move it to DesignVariables so the theme behavior is explicit.

Thanks for pointing it out. It didn't look good in the dark theme. Replaced it with the following, and now it looks good:

color: DefaultTextStyle.of(context).style.color?.withValues(alpha: 0.8)

Does it still need to be defined in DesignVariables?

Yeah, that's fine.

In principle an expression like that should still go into DesignVariables, because it still might vary by theme — for example, if in the future we add "high contrast" themes, they might want to keep the alpha at 1, or set it to 90% or something.

But that isn't a feature we have on the horizon. And these design details will get replaced soon anyway with #995 / #913. So with this solution which works for the existing themes, it's not worth the overhead to add to DesignVariables.

sm-sayedi · 2025-01-29T15:21:11Z

Thanks @gnprice for the review. Changes pushed, with two comments, one just above this comment and another in here. PTAL.

gnprice · 2025-01-29T23:35:32Z

lib/model/compose.dart

+  all(canonicalString: 'all'),
+  everyone(canonicalString: 'everyone'),
+  channel(canonicalString: 'channel'),
+  // Deprecated in FL 247. Empirically, current servers (FL 330)


needs a TODO-server comment (similar to #889 (comment) )

gnprice · 2025-01-29T23:36:29Z

lib/model/autocomplete.dart

+      }
+    }
+
+    final isTopicWildcardAvailable = store.account.zulipFeatureLevel >= 224; // TODO(sever-8)


Suggested change

final isTopicWildcardAvailable = store.account.zulipFeatureLevel >= 224; // TODO(sever-8)

final isTopicWildcardAvailable = store.account.zulipFeatureLevel >= 224; // TODO(server-8)

The spelling is important for these to serve their purpose of being findable via grep 🙂

(there's one other like this)

gnprice · 2025-01-29T23:41:41Z

lib/widgets/autocomplete.dart

-  AutocompleteIntent<ComposeAutocompleteQuery>? autocompleteIntent() => controller.autocompleteIntent();
+  AutocompleteIntent<ComposeAutocompleteQuery>? autocompleteIntent(
+    BuildContext context) => controller.autocompleteIntent();


These changes are no longer needed, since the query and intent no longer carry the localizations.

gnprice · 2025-01-29T23:44:23Z

lib/model/autocomplete.dart

-  AutocompleteIntent<ComposeAutocompleteQuery>? autocompleteIntent() {
+  AutocompleteIntent<ComposeAutocompleteQuery>? autocompleteIntent(ZulipLocalizations localizations) {


OK, and now having read this revision: I agree, moving it to the subclass MentionAutocompleteView will make things cleaner.

gnprice · 2025-01-29T23:45:39Z

Thanks for the revision! I've read through the whole lib/ changes again, and just a few comments above. I'll take a look at the tests next.

gnprice

OK, and read through the tests too. Comments below.

gnprice · 2025-01-29T23:55:13Z