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

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

Still a work in progress. Just wanted to get early feedback on the overall approach and code structure!

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.

This is the icon Web uses; which is a Font Awesome 4.7.0 icon (fa-bullhorn). This icon is changed in the newer versions. I couldn't find its SVG from their website nor from their GitHub repository, but from another repository: https://github.com/Rush/Font-Awesome-SVG-PNG/blob/master/black/svg/bullhorn.svg

Fixes: zulip#234

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.

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.

gnprice reviewed Aug 19, 2024

View reviewed changes

gnprice mentioned this pull request Aug 19, 2024

autocomplete [nfc]: Give subclasses control of computeResults #896

Merged

sm-sayedi force-pushed the issue-234-wildcard-@-mention-autocomplete branch from 76fd1ac to 1664a2d Compare August 22, 2024 20:36

gnprice mentioned this pull request Aug 22, 2024

autocomplete [nfc]: Cut disused CandidateT type parameters #908

Merged

sm-sayedi added 2 commits November 7, 2024 15:16

autocomplete: Support @-wildcard in user-mention autocomplete

a24bddb

Fixes: zulip#234

gnprice reviewed Nov 7, 2024

View reviewed changes

gnprice force-pushed the issue-234-wildcard-@-mention-autocomplete branch from 1664a2d to a24bddb Compare November 8, 2024 00:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sm-sayedi commented Aug 16, 2024 •

edited

Loading

gnprice left a comment

gnprice Aug 19, 2024

sm-sayedi Aug 22, 2024

gnprice Aug 22, 2024

gnprice Aug 19, 2024

gnprice Aug 19, 2024

gnprice Aug 19, 2024

sm-sayedi Aug 22, 2024

gnprice Aug 22, 2024

gnprice Aug 19, 2024

gnprice Aug 19, 2024

sm-sayedi Aug 22, 2024 •

edited

Loading

gnprice Aug 22, 2024

gnprice commented Aug 19, 2024

sm-sayedi commented Aug 22, 2024

alya commented Aug 22, 2024

gnprice left a comment

gnprice Nov 7, 2024

gnprice Nov 7, 2024

gnprice Nov 7, 2024

gnprice Nov 7, 2024

gnprice Nov 7, 2024

gnprice commented Nov 8, 2024

		@@ -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>

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

Are you sure you want to change the base?

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

Conversation

sm-sayedi commented Aug 16, 2024 • edited Loading

Screenshot

Screen recording

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sm-sayedi Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Aug 19, 2024

sm-sayedi commented Aug 22, 2024

alya commented Aug 22, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Nov 8, 2024

sm-sayedi commented Aug 16, 2024 •

edited

Loading

sm-sayedi Aug 22, 2024 •

edited

Loading