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

sm-sayedi · 2024-07-29T19:26:25Z

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

Recent activity in the current topic/stream.
Recent DM conversations.
Human vs. Bot users (human users come first).
Alphabetical order.

Note: This is the fourth (last) PR in the series of PRs #608 is divided into, coming after #828.

Fixes: #228

rajveermalviya

Thanks for working on this @sm-sayedi!

This look good to me, just one nit, moving on to the mentor review @hackerkid.

rajveermalviya · 2024-07-30T17:21:15Z

lib/model/autocomplete.dart

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


nit: Could be a small helper function like compareByRecency and compareByDms.

sm-sayedi · 2024-07-31T06:40:00Z

Thanks @rajveermalviya for the review and the tip. Changes applied.

chrisbobbe

Thanks! One small comment below, and I'll mark this for Greg's review.

chrisbobbe · 2024-07-31T20:58:29Z

lib/model/autocomplete.dart

+      .normalizedNameForUser(userA);
+    final userBName = store.autocompleteViewManager.autocompleteDataCache
+      .normalizedNameForUser(userB);
+    return userAName.compareTo(userBName);


Let's add a TODO like we have in SubscriptionListPage:

// TODO(i18n): add locale-aware sorting

For example, this would sort the list of names ['c', 'b', 'a', 'à'] as ['a', 'b', 'c', 'à'], which unfortunately separates the 'a' and 'à' when it seems like they should appear next to each other.

You can try on dartpad.dev:

void main() { final list = ['c', 'b', 'a', 'à']; list.sort((a, b) => a.compareTo(b)); print(list); // [a, b, c, à] }

Could add a test that fails with the current code, with skip: true, but passes with locale-aware sorting.

sm-sayedi · 2024-08-01T06:38:13Z

Thanks @chrisbobbe for the review.

chrisbobbe · 2024-08-05T20:27:15Z

(I see my comment has been resolved, so it's Greg's turn now.)

In @-mention autocomplete, users are suggested based on: 1. Recent activity in the current topic/stream. 2. Recent DM conversations. 3. Human vs. Bot users (human users come first).

…pleteDataCache`

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

The details of negative vs. positive vs. zero return values are part of the interface of comparators in general. The details of 1 vs. any other positive value, or -1 vs. any other negative value, are implementation details of the methods -- callers should only care about negative vs. positive vs. zero.

The way these tests were meant to work wasn't particularly explicit; and the recent changes adding more signals didn't take that intended structure into account, so that it became hard to see if the tests checked what they're intended to check. Bring them back to that structure, and add comments explaining it.

The recent changes that added two new ranking criteria added four new users to this test case. Two is enough to make the test's point, because it's enough for adding users that differ on the new criteria while tying on the criteria that have higher priority in the ranking.

gnprice

Thanks @sm-sayedi, and thanks @rajveermalviya and @chrisbobbe for the previous reviews!

This looks good; merging, with some additional commits on top adjusting the tests and comments:
b48e54b autocomplete [nfc]: Simplify and clarify docs on comparator methods
093eaba autocomplete test: Fix up cross-signal tests; add comments to clarify
e51d2f2 autocomplete test: Tighten up "final results" test

So please take a look at those.

gnprice · 2024-08-08T23:40:00Z

test/model/autocomplete_test.dart

+      test('DmNarrow: DM recency > this-conversation recency or stream recency '
+          'or human vs. bot user or alphabetical order', () async {


This seems wrong — human-vs-bot should come before names, and the latter comes before those latter kinds of recency (because those don't count at all).

This was referenced Jul 29, 2024

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

Merged

autocomplete: Sort user-mention autocomplete results #608

Closed

sm-sayedi added the buddy review GSoC buddy review needed. label Jul 29, 2024

sm-sayedi requested a review from rajveermalviya July 29, 2024 19:32

rajveermalviya approved these changes Jul 30, 2024

View reviewed changes

rajveermalviya added mentor review GSoC mentor review needed. and removed buddy review GSoC buddy review needed. labels Jul 30, 2024

rajveermalviya requested a review from hackerkid July 30, 2024 17:27

sm-sayedi force-pushed the issue-228-sort-@-mention-results branch from 24f3ea5 to fefaf31 Compare July 31, 2024 06:23

sm-sayedi added the maintainer review PR ready for review by Zulip maintainers label Jul 31, 2024

sm-sayedi requested a review from chrisbobbe July 31, 2024 06:40

chrisbobbe reviewed Jul 31, 2024

View reviewed changes

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Jul 31, 2024

sm-sayedi force-pushed the issue-228-sort-@-mention-results branch from fefaf31 to a70e75f Compare August 1, 2024 06:28

sm-sayedi requested a review from gnprice August 1, 2024 06:38

sm-sayedi force-pushed the issue-228-sort-@-mention-results branch from a70e75f to d1c4539 Compare August 4, 2024 04:28

sm-sayedi and others added 7 commits August 8, 2024 17:19

autocomplete: Add "human vs. bot users" criterion

e44f9e8

In @-mention autocomplete, users are suggested based on: 1. Recent activity in the current topic/stream. 2. Recent DM conversations. 3. Human vs. Bot users (human users come first).

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

b9c048b

…pleteDataCache`

autocomplete: Add "alphabetical order" criterion

cef1769

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

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

b36ba88

gnprice force-pushed the issue-228-sort-@-mention-results branch from d1c4539 to e51d2f2 Compare August 9, 2024 00:31

gnprice merged commit e51d2f2 into zulip:main Aug 9, 2024

gnprice reviewed Aug 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sm-sayedi commented Jul 29, 2024 •

edited

Loading

rajveermalviya left a comment

rajveermalviya Jul 30, 2024

sm-sayedi commented Jul 31, 2024

chrisbobbe left a comment •

edited

Loading

chrisbobbe Jul 31, 2024 •

edited

Loading

chrisbobbe Jul 31, 2024

sm-sayedi commented Aug 1, 2024

chrisbobbe commented Aug 5, 2024

gnprice left a comment

gnprice Aug 8, 2024

		test('DmNarrow: DM recency > this-conversation recency or stream recency '
		'or human vs. bot user or alphabetical order', () async {

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

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

Conversation

sm-sayedi commented Jul 29, 2024 • edited Loading

rajveermalviya left a comment

Choose a reason for hiding this comment

rajveermalviya Jul 30, 2024

Choose a reason for hiding this comment

sm-sayedi commented Jul 31, 2024

chrisbobbe left a comment • edited Loading

Choose a reason for hiding this comment

chrisbobbe Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

chrisbobbe Jul 31, 2024

Choose a reason for hiding this comment

sm-sayedi commented Aug 1, 2024

chrisbobbe commented Aug 5, 2024

gnprice left a comment

Choose a reason for hiding this comment

gnprice Aug 8, 2024

Choose a reason for hiding this comment

sm-sayedi commented Jul 29, 2024 •

edited

Loading

chrisbobbe left a comment •

edited

Loading

chrisbobbe Jul 31, 2024 •

edited

Loading