autocomplete: Sort user-mention autocomplete results #608

sm-sayedi · 2024-04-02T01:41:23Z

In @-mention autocomplete, users are suggested based on:

Recent activity in the current topic/stream.
Recent DM conversations.
Human vs. Bot users.
Alphabetical order.

If one option exhausts, the other is considered for the suggestions in the preceding order.

Note: This PR will no longer receive any new code pushes as it is divided into other PRs. See #693, #692, #828, #849.

sm-sayedi · 2024-04-02T01:49:30Z

The code may still have some bugs, and it is unclean, but it would be great to know if I am on the right path for this problem!

And I will be more than happy to have feedback on my draft proposal.

gnprice

Sure. Here's some high-level comments.

And I will be more than happy to have feedback on my draft proposal.

Ah indeed. 🙂 Replied there.

lib/model/store.dart

lib/model/autocomplete.dart

sm-sayedi · 2024-04-04T17:21:25Z

Thanks for the review! Revision pushed, but this time changed the whole implementation to almost match it with the Web app.

lib/model/autocomplete.dart

gnprice

Thanks! Comments below.

lib/model/autocomplete.dart

lib/model/recent_dm_conversations.dart

lib/model/store.dart

sm-sayedi · 2024-04-05T07:05:55Z

Thanks for the review! Pushed the changes.

When you think it's ready, please let me know so I proceed with writing tests.

lib/model/autocomplete.dart

gnprice

Thanks!

This strategy looks good; see comments below. I didn't cover all the points that I might comment on if doing a full review, but I tried to cover all the data structures and algorithms — I think all the data structures and algorithms here are basically what we want modulo these comments.

I'll be on vacation after today, so @chrisbobbe will review the next few revisions. There's a significant amount of complexity here, so he'll probably have plenty of comments to make about code style, dartdoc, tests, potentially also some of the data structures and algorithms, and likely also some correctness details I'd missed. He might merge the PR while I'm away, if he feels it's ready, and might flag some questions he'd like my input on. (Potentially both — we can always revise the code later.)

In any case I'll look forward to seeing when I'm back a version of this PR that's been polished up and is either merged, or nearly ready to merge 🙂

lib/model/autocomplete.dart

lib/model/recent_dm_conversations.dart

lib/model/recent_senders.dart

lib/model/autocomplete.dart

chrisbobbe · 2024-04-09T18:57:03Z

I'll be on vacation after today, so @chrisbobbe will review the next few revisions.

Yep, happy to review! 🙂 I see this PR is currently marked as a draft, so I'll plan to review it once it's marked as ready for review and you've addressed Greg's feedback above.

sm-sayedi · 2024-04-16T19:32:57Z

Thanks @gnprice for the review!

Revision pushed with the feedback addressed @chrisbobbe! Ready for your review now!

lib/model/recent_dm_conversations.dart

sm-sayedi · 2024-04-16T19:57:31Z

lib/model/recent_senders.dart

+    if (_ids.isEmpty) {
+      _ids.add(id);
+    } else {
+      int i = _ids.possibleIndexOf(id);


Instead of using List.indexOf or List.indexWhere which uses the linear search, we could take advantage of the fact this list is sorted and use binary search to find the index.

sm-sayedi · 2024-04-16T19:59:24Z

lib/model/recent_senders.dart

+      _ids.add(id);
+    } else {
+      int i = _ids.possibleIndexOf(id);
+      if (i >= 0) { // the [id] already exists, so do not add it.


I preferred this way of checking for element's presence, instead of using List.contains which again uses the linear search.

sm-sayedi · 2024-04-16T20:04:57Z

lib/model/recent_senders.dart

+        low = mid + 1;
+      }
+    }
+    return -low - 1;


Using -low - 1 instead of just -low will differentiate between the case where the element is found at index 0 and the case where the element is not found, but should be placed at the very start of the list.

Let's explain this in a code comment. 🙂 It's exactly the kind of thing that any reader will be interested in, and most readers will just be looking at the code, without also reading through this PR thread.

edit: hmm, it's not clear in the GitHub UI I'm seeing right now, but this was meant as a reply to your comment #608 (comment)

We're considering adding another `Map` field to RecentDmConversationsView; see PR #608. If we do, this dartdoc will want this added explicitness.

chrisbobbe

Thanks, @sm-sayedi! Comments below, and I see there's a test failing in CI. Does that failure reproduce when you run tools/check locally?

The new code will also need new tests.

chrisbobbe · 2024-04-16T23:45:44Z

lib/model/recent_senders.dart

+    required User userA,
+    required User userB,
+    required int streamId,
+    // TODO(#493): may turn this into a non-nullable string.


What's the connection with #493?

Removed it this time! At first I thought, if the compose box is populated with a topic narrow, we may show the user results based on that topic narrow, even if we're in the stream view. But now looking at the web, it's not the case.

chrisbobbe · 2024-04-17T20:55:18Z

lib/model/recent_senders.dart

+extension Max<T extends num> on Iterable<T> {
+  /// Finds the maximum number in an [Iterable].
+  ///
+  /// Returns null if the [Iterable] is empty.
+  T? max() => isNotEmpty ? reduce(math.max) : null;
+}


This doesn't seem to be used. Let's simplify by removing it.

Thanks for spotting this. It was used in the previous implementation, and I forgot to remove it.

chrisbobbe · 2024-04-17T22:42:05Z

lib/model/recent_dm_conversations.dart

+  // The ID of the latest messages exchanged with other users.
+  final Map<int, int> _dmRecencyData = {};


nit: use dartdoc formatting, like the others (so /// instead of //)

Also, can we find a clearer name for this field? In a class named RecentDmConversationsView, the name _dmRecencyData seems redundant, and it also seems less specific than it should be. 🙂

I think we can make the dartdoc more specific as well. The type (Map<int, int>) doesn't tell us if the map is keyed by message IDs or user IDs, since the key and value types are both int. That would be good to make clearer so the reader doesn't have to search for clues elsewhere, like the places where the field gets used. There are also a few other relevant facts that a reader would naturally wonder about.

So for example:

/// Map from user ID to the latest message ID in any conversation with the user. /// /// Both 1:1 and group DM conversations are considered. /// The self-user ID is excluded even if there is a self-DM conversation. /// /// (The identified message was not necessarily sent by the identified user; /// it might have been sent by anyone in its conversation.) final Map<int, int> _latestMessageByRecipient = {};

What do you think?

Thank you! This is a well-descriptive dartdoc. Added it.

chrisbobbe · 2024-04-18T00:04:37Z

lib/model/recent_dm_conversations.dart

+  /// The ID of the latest message exchanged with this user.
+  ///
+  /// Returns -1 if there has been no DM message exchanged ever.
+  int getRecencyIndex(final int userId) => _dmRecencyData[userId] ?? -1;


How about we pull the ?? -1 part out to callers? It seems helpful toward simplifying this function's job, which is already somewhat complex to describe accurately (see my previous comment on _dmRecencyData).

Then, I think it would be helpful to change the name to something more transparent. So for example:

/// The latest message ID in any conversation with the given user, if any. /// /// Both 1:1 and group DM conversations are considered. /// Gives null for the self-user ID even if there is a self-DM conversation. /// /// (An identified message was not necessarily sent by the given user; /// it might have been sent by anyone in its conversation.) int? latestMessageWithRecipient(final int userId) => _latestMessageByRecipient[userId];

chrisbobbe · 2024-04-18T00:16:58Z

lib/model/recent_senders.dart

+    idTracker.add(messageId);
+  }
+
+  void processStreamMessage(Message message) {


The name makes it seem like a StreamMessage is specifically expected, but the param's type is the more general type Message.

I first noticed this when look at this method's callers, which look odd because they're passing messages that are not known to be StreamMessages.

Could fix by renaming to processMessage, or by narrowing the param's type to StreamMessage instead of Message.

Please also add a dartdoc for this method, to make it clear what callers are signing up for when they call it. Maybe we can also find a more specific verb than "process"; I see it calls some helpers addStreamMessage and addTopicMessage; perhaps this should be named processMessage?

Could fix by renaming to processMessage, or by narrowing the param's type to StreamMessage instead of Message.

Went with processAsStreamMessage. I thought when using processMessage, one would think that EACH message is processed; and when narrowing the param's type to StreamMessage, each caller should always make sure they're passing the correct message type by using a check, for example:

if (message is StreamMessage) { recentSenders.processStreamMessage(message); }

I think it would be cleaner to add this check in the method itself and use a somewhat descriptive method name.
What do you think?

Maybe we can also find a more specific verb than "process".

I think the verb "process" fits good in here as it extracts some data from the message and keeps track of it in a data structure. But I would appreciate a better verb from your side!

Edited: Renamed it to handleMessage to match it with handleMessageEvent declared in model\recent_dm_conversations.dart

chrisbobbe · 2024-04-18T01:05:05Z

lib/model/recent_senders.dart

+        low = mid + 1;
+      }
+    }
+    return -low - 1;


Let's explain this in a code comment. 🙂 It's exactly the kind of thing that any reader will be interested in, and most readers will just be looking at the code, without also reading through this PR thread.

edit: hmm, it's not clear in the GitHub UI I'm seeing right now, but this was meant as a reply to your comment #608 (comment)

chrisbobbe · 2024-04-18T01:14:33Z

lib/model/recent_senders.dart

+/// A data structure to keep track of message ids.
+class IdTracker {


I think MessageIdTracker would be a more helpful name for this, since a lot of the nearby code is dealing with user IDs. 🙂

With that name, the dartdoc as written would stand out as redundant/useless, though, wouldn't it…but that could be dealt with by removing it. But if there's anything else that's interesting about this class (other than that it's a data structure that keeps track of message IDs), that could be good to put in the dartdoc.

chrisbobbe · 2024-04-18T01:21:04Z

lib/model/autocomplete.dart

+    if (!userA.isBot && userB.isBot) {
+      return -1;
+    } else if (userA.isBot && !userB.isBot) {
+      return 1;
+    }


This part doesn't seem like it belongs in a method named compareByDms, as Greg pointed out above: #608 (comment)

chrisbobbe · 2024-04-18T01:26:17Z

lib/model/autocomplete.dart

+  final Map<String, String> _namesLowercased = {};
+
+  String nameLowercased(String name) {
+    return _namesLowercased[name] ??= name.toLowerCase();
+  }


Is there a reason not to key this by user ID, like _nameWordsByUser is? (If doing that, we should make sure invalidateUser acts appropriately on the lowercased-name data.)

How about _lowercasedNameByUser and lowercasedNameForUser, on the pattern of _nameWordsByUser and nameWordsForUser?

Is there a reason not to key this by user ID, like _nameWordsByUser is?

Two reasons: a) to not pass the whole User object while we can do the work with just one of its properties. b) there can be multiple users with the same name so we can save the space a little bit by having just one key-value pair for all those user names.
However, this will bring some complexity when adding the appropriate logic in invalidateUser. Let's assume we somehow pass fullName to 'invalidateUser' besides userId to remove the lowercase name. It will remove the user name for all the users that share the same name.
So I am not sure whether to key it by userId or fullName?

Yeah, I think best to key it by user ID, the same way as _nameWordsByUser.

It's uncommon for users to have the same name, so there's minimal space savings to be had from that, which makes it not worth the complexity of worrying about interactions there.

Relatedly: in nameWordsForUser we're calling toLowerCase on user.fullName, just like this does. So let's have that step share this same cache.

chrisbobbe · 2024-04-18T01:33:54Z

lib/model/message_list.dart

@@ -381,6 +381,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
    for (final message in result.messages) {
      if (_messageVisible(message)) {
        _addMessage(message);
+        store.recentSenders.processStreamMessage(message);


Can you talk a bit about the performance considerations of this?

I am not hundred percent sure if it's an expensive task or not. I think it does relatively simple task of adding IdTracker to a map two times.

sm-sayedi · 2024-04-19T20:50:29Z

Thanks for the review! Revision pushed with some comments above. Please have a look.

The tests are failing locally for me too. I was waiting for the overall approach of this feature to be steady to start writing tests for it. I will do it after now, but at the same time, I would appreciate your feedback on the recent revision!

This field will be used to maintain a list of sorted users based on the most relevant autocomplete criteria in the upcoming commits. Co-authored-by: Greg Price <[email protected]>

In @-mention autocomplete, users are suggested based on: 1. Recent DM conversations. Fixes part of zulip#228

…ta structures These data structures are used to keep track of user messages in topics and streams.

In @-mention autocomplete, users are suggested based on: 1. Recent activity in the current stream. 2. Recent DM conversations.

In @-mention autocomplete, users are suggested based on: 1. Recent activity in the current stream. 2. Recent DM conversations. 3. Human vs. Bot users.

…pleteDataCache`

In @-mention autocomplete, users are suggested based on: 1. Recent activity in the current stream. 2. Recent DM conversations. 3. Human vs. Bot users. 4. Alphabetical order. Fixes: zulip#228

sm-sayedi · 2024-05-17T18:47:35Z

Thanks for the review! Pushed the recent DMs revision here although I wasn't sure to create a new PR for this too. Please have a look.

The other criteria will come in the following PRs.

gnprice · 2024-05-20T18:39:11Z

Thanks for the review! Pushed the recent DMs revision here although I wasn't sure to create a new PR for this too.

Thanks!

Yeah, now that I think about it: please close this PR and open a new one with the same contents. In particular that will be helpful because the last round of review above covers changes that will be part of the several different PRs in the series; by letting this PR thread end here, that review will remain accessible when looking at each of those PRs. If we continued in this same thread as the PR for the first part of the changes, then that review would get pushed up into history and potentially into the "hidden" area.

sm-sayedi · 2024-05-21T00:34:29Z

I have opened #693 as the first part of this PR. Please have a look.

gnprice · 2024-05-22T19:06:24Z

Closing this in favor of its successor PRs #693 and #692. Thanks @sm-sayedi for all your work on this so far!

sm-sayedi force-pushed the issue-228-sort-user-mention-results branch from 20fefe8 to cefaf97 Compare April 2, 2024 01:46

sm-sayedi force-pushed the issue-228-sort-user-mention-results branch from cefaf97 to 7141342 Compare April 2, 2024 01:53

gnprice reviewed Apr 2, 2024

View reviewed changes

sm-sayedi force-pushed the issue-228-sort-user-mention-results branch from 7141342 to b419e14 Compare April 4, 2024 17:17

sm-sayedi commented Apr 4, 2024

View reviewed changes

lib/model/autocomplete.dart Outdated Show resolved Hide resolved

sm-sayedi commented Apr 4, 2024

View reviewed changes

lib/model/autocomplete.dart Show resolved Hide resolved

gnprice reviewed Apr 4, 2024

View reviewed changes

sm-sayedi mentioned this pull request Apr 5, 2024

autocomplete: Give preference to subscribed users first in @-mention autocomplete #618

Open

sm-sayedi force-pushed the issue-228-sort-user-mention-results branch 2 times, most recently from 997dd5a to 981de92 Compare April 5, 2024 07:01

sm-sayedi force-pushed the issue-228-sort-user-mention-results branch from 981de92 to 22953d3 Compare April 5, 2024 07:48

sm-sayedi commented Apr 5, 2024

View reviewed changes

lib/model/autocomplete.dart Outdated Show resolved Hide resolved

gnprice reviewed Apr 5, 2024

View reviewed changes

sm-sayedi force-pushed the issue-228-sort-user-mention-results branch from 22953d3 to 804287b Compare April 16, 2024 19:21

sm-sayedi marked this pull request as ready for review April 16, 2024 19:30

sm-sayedi force-pushed the issue-228-sort-user-mention-results branch from 804287b to 5508672 Compare April 16, 2024 19:46

sm-sayedi commented Apr 16, 2024

View reviewed changes

lib/model/recent_dm_conversations.dart Outdated Show resolved Hide resolved

sm-sayedi commented Apr 16, 2024

View reviewed changes

chrisbobbe added a commit that referenced this pull request Apr 18, 2024

recent_dm_conversations [nfc]: Make sorted dartdoc more explicit

1a5425e

We're considering adding another `Map` field to RecentDmConversationsView; see PR #608. If we do, this dartdoc will want this added explicitness.

chrisbobbe reviewed Apr 18, 2024

View reviewed changes

sm-sayedi force-pushed the issue-228-sort-user-mention-results branch 2 times, most recently from 31deb52 to 6797b79 Compare April 19, 2024 20:44

sm-sayedi and others added 9 commits May 17, 2024 22:17

autocomplete [nfc]: Introduce a field for sorted users

2b825ab

This field will be used to maintain a list of sorted users based on the most relevant autocomplete criteria in the upcoming commits. Co-authored-by: Greg Price <[email protected]>

autocomplete: Add "recent DM conversations" criterion

09d3351

In @-mention autocomplete, users are suggested based on: 1. Recent DM conversations. Fixes part of zulip#228

recent-senders: Add the new MessageIdTracker and RecentSenders da…

4c93f17

…ta structures These data structures are used to keep track of user messages in topics and streams.

store: Add RecentSenders data structure to store.dart

35b3928

autocomplete: Add "recent activity in current stream" criterion

2996ce5

In @-mention autocomplete, users are suggested based on: 1. Recent activity in the current stream. 2. Recent DM conversations.

autocomplete: Add "human vs. bot users" criterion

cc715e6

In @-mention autocomplete, users are suggested based on: 1. Recent activity in the current stream. 2. Recent DM conversations. 3. Human vs. Bot users.

autocomplete [nfc]: Support caching normalized user names in `Autocom…

5448993

…pleteDataCache`

autocomplete: Add "alphabetical order" criterion

2b5711b

In @-mention autocomplete, users are suggested based on: 1. Recent activity in the current stream. 2. Recent DM conversations. 3. Human vs. Bot users. 4. Alphabetical order. Fixes: zulip#228

autocomplete [nfc]: Add TODO comments for future autocomplete criteria

c710a5c

sm-sayedi force-pushed the issue-228-sort-user-mention-results branch from 88f4872 to 5d6dc03 Compare May 17, 2024 18:22

sm-sayedi changed the title ~~autocomplete: Sort user-mention autocomplete results~~ autocomplete: In user-mention autocomplete results give priority to users in DM conversations May 18, 2024

sm-sayedi requested a review from gnprice May 18, 2024 04:02

sm-sayedi force-pushed the issue-228-sort-user-mention-results branch 3 times, most recently from 9b75e44 to 7f21d5f Compare May 20, 2024 11:21

sm-sayedi mentioned this pull request May 20, 2024

model: Introduce data structures for "recent senders criterion" of user-mention autocomplete #692

Merged

sm-sayedi changed the title ~~autocomplete: In user-mention autocomplete results give priority to users in DM conversations~~ autocomplete: Sort user-mention autocomplete results May 21, 2024

sm-sayedi mentioned this pull request May 22, 2024

autocomplete: In user-mention autocomplete results give priority to users in DM conversations #693

Merged

gnprice closed this May 22, 2024

neiljp mentioned this pull request Jun 24, 2024

Review & improve ordering of autocomplete (users/mentions) zulip/zulip-terminal#1526

Open

3 tasks

sm-sayedi mentioned this pull request Jul 19, 2024

autocomplete: Add "recent senders criterion" to user-mention autocomplete #828

Merged

sm-sayedi reopened this Jul 24, 2024

sm-sayedi force-pushed the issue-228-sort-user-mention-results branch from 7f21d5f to c710a5c Compare July 24, 2024 12:17

sm-sayedi closed this Jul 24, 2024

This was referenced Jul 25, 2024

autocomplete: Sort user-mention autocomplete results #228

Closed

autocomplete: Add "human vs. bot user" and "Alphabetical order" criteria #849

Merged

		// The ID of the latest messages exchanged with other users.
		final Map<int, int> _dmRecencyData = {};

		/// A data structure to keep track of message ids.
		class IdTracker {

autocomplete: Sort user-mention autocomplete results #608

autocomplete: Sort user-mention autocomplete results #608

Conversation

sm-sayedi commented Apr 2, 2024 • edited Loading

sm-sayedi commented Apr 2, 2024 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

sm-sayedi commented Apr 4, 2024

gnprice left a comment

Choose a reason for hiding this comment

sm-sayedi commented Apr 5, 2024

gnprice left a comment

Choose a reason for hiding this comment

chrisbobbe commented Apr 9, 2024 • edited Loading

sm-sayedi commented Apr 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

chrisbobbe 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

sm-sayedi Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

chrisbobbe Apr 18, 2024 • edited Loading

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 commented Apr 19, 2024 • edited Loading

sm-sayedi commented May 17, 2024

gnprice commented May 20, 2024

sm-sayedi commented May 21, 2024

gnprice commented May 22, 2024

sm-sayedi commented Apr 2, 2024 •

edited

Loading

sm-sayedi commented Apr 2, 2024 •

edited

Loading

chrisbobbe commented Apr 9, 2024 •

edited

Loading

chrisbobbe Apr 18, 2024 •

edited

Loading

sm-sayedi Apr 19, 2024 •

edited

Loading

chrisbobbe Apr 18, 2024 •

edited

Loading

sm-sayedi commented Apr 19, 2024 •

edited

Loading