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

sm-sayedi · 2024-05-21T00:30:33Z

In @-mention autocomplete users with whom recent DMs are exchanged are given priority.

Other criteria will be added in separate PRs.

Note: This is the first PR in the series of PRs #608 is divided into. Next PR in the series: #692.

Fixes part of: #228

chrisbobbe

Thanks! Comments below, mostly small.

chrisbobbe · 2024-05-22T01:35:46Z

test/model/recent_dm_conversations_test.dart

-          ..sorted.deepEquals(expected.sorted);
+          ..sorted.deepEquals(expected.sorted)
+          ..latestMessagesByRecipient.deepEquals(expected.latestMessagesByRecipient);
+      });


Huh, does this existing test ('existing conversation, not newest in conversation') want a check(listenersNotified).isFalse(); at the end? That sounds like it might be a nice independent improvement, if you'd like to include that in a separate commit.

Added it in a separate commit, but check(listenersNotified).isTrue(); instead of check(listenersNotified).isFalse();.

Ah, hmm. In this case it's unnecessary to notify listeners, and slightly undesirable, since nothing in the data structure actually changes, right? Quoting the comment in handleMessageEvent in the case that this test exercises:

// The conversation already has a newer message. // This should be impossible as long as we only listen for messages coming // through the event system, which sends events in order. // Anyway, do nothing.

I think it's not really bad to notify listeners. This case isn't even supposed to be exercised at all, currently, and it may rarely get exercised after #650 in certain race conditions. But I don't think we should have a check that expects the slightly undesirable behavior.

Since the test caught my attention as possibly missing something, though, it might be helpful to add something to it, so the next reader doesn't have to wonder the same thing. Maybe a commented-out check; something like this:

// (listeners are notified unnecessarily, but that's OK) // check(listenersNotified).isFalse();

Separately, let's add a ..latestMessagesByRecipient.deepEquals check to this test, in the commit that introduces latestMessagesByRecipient.

Bump on my comment at #693 (comment) :

But I don't think we should have a check that expects the slightly undesirable behavior.

chrisbobbe · 2024-05-22T01:41:28Z

lib/model/recent_dm_conversations.dart

+    final map = Map.fromEntries(entries);
+    final sorted = QueueList.from(entries.map((e) => e.key));
+
+    final msgsByUser = <int, int>{};


nit: I think my preference for this variable name is just latestMessagesByRecipient, on the pattern of map and sorted, which are named the same as the corresponding parameters of RecentDmConversationsView._. That way I don't have to think about whether "msgs" means the same thing as "latestMessages", or if "ByUser" means the same thing as "ByRecipient".

chrisbobbe · 2024-05-22T01:45:23Z

lib/model/recent_dm_conversations.dart

+    final msgsByUser = <int, int>{};
+    for (final entry in entries) {
+      final dmNarrow = entry.key;
+      final msg = entry.value;


nit: Could use a more descriptive name than msg, like maxMsgId. I think maxMessageId would also be fine; I don't mind seeing the word "message" written out.

chrisbobbe · 2024-05-22T04:21:35Z

lib/model/autocomplete.dart

@@ -267,13 +267,48 @@ class MentionAutocompleteView extends ChangeNotifier {

  List<User>? _sortedUsers;

-  List<User> sortByRelevance({required List<User> users}) {
+  /// Determines which of the two users are more recent in DM conversations.


nit "which […] is more recent"

chrisbobbe · 2024-05-22T04:49:43Z

lib/model/autocomplete.dart

+  int compareByDms(User userA, User userB) {
+    final recentDms = store.recentDmConversationsView;
+    final aLatestMessageId = recentDms.latestMessagesByRecipient[userA.userId] ?? -1;
+    final bLatestMessageId = recentDms.latestMessagesByRecipient[userB.userId] ?? -1;
+
+    return bLatestMessageId.compareTo(aLatestMessageId);
+  }


I think there are six interesting cases that could happen:

we have DMs with userA and with userB; userA is more recent

we have DMs with userA and with userB; userB is more recent

we have DMs with userA and with userB; they are equally recent

we have DMs with userA but not with userB

we have DMs with userB but not with userA

we don't have DMs with userA or userB

In our unit tests for compareByDms, let's have a test case that exercises each of these six cases.

Also, while this code is nice and compact, I noticed myself having some difficulty following it, and I think we can remove some of that difficulty without making the function much more verbose. In particular:

-1 is not a real message ID, so I found it distracting that the variables named …MessageId could take -1 as a value. (I wondered if this type mismatch might lead to a bug, and I spent some time thinking about that.)

I think I slightly prefer spelling b.compareTo(a) as -a.compareTo(b), as we've done in a few places in the code. I'm not sure exactly why. Maybe because it feels more familiar to see a before b, and it feels nicer to see something familiar plus a simple operation on it, then to think through something unfamiliar? I'm not really sure. 🤔

Here's one possible way this could look, I think (untested):

int compareByDms(User userA, User userB) { final recentDms = store.recentDmConversationsView; final aLatestMessageId = recentDms.latestMessagesByRecipient[userA.userId]; final bLatestMessageId = recentDms.latestMessagesByRecipient[userB.userId]; return switch((aLatestMessageId, bLatestMessageId)) { // negate because higher message IDs are more recent (int a, int b) => -a.compareTo(b), (int(), _ ) => -1, (_, int()) => 1, (_, _ ) => 0 }; }

chrisbobbe · 2024-05-22T04:52:16Z

test/model/autocomplete_test.dart

+      for (final user in users) {
+        store.addUser(user);
+      }


Suggested change

for (final user in users) {

store.addUser(user);

}

store.addUsers(users);

chrisbobbe · 2024-05-22T04:53:10Z

test/model/autocomplete_test.dart

+      store = eg.store(
+        initialSnapshot: eg.initialSnapshot(
+          recentPrivateConversations: dmConversations
+      ));


Can be a bit more compact:

Suggested change

store = eg.store(

initialSnapshot: eg.initialSnapshot(

recentPrivateConversations: dmConversations

));

store = eg.store(initialSnapshot: eg.initialSnapshot(

recentPrivateConversations: dmConversations));

chrisbobbe · 2024-05-22T05:11:18Z

lib/model/autocomplete.dart

+  void handleMessageEvent(MessageEvent event) {
+    _refreshStaleUserResults();
+  }
+
+  void handleOlderMessages() {
+    _refreshStaleUserResults();
+  }


handleMessageEvent makes sense, because a new message might cause RecentDmConversationsView.latestMessagesByRecipient to update. But do we need handleOlderMessages? That won't cause RecentDmConversationsView.latestMessagesByRecipient to update.

…ah, I guess this will be used when we start considering the current topic/stream, which I see you're working on in #692. Makes sense.

Also, with these additional calls to _refreshStaleUserResults, the dartdoc of MentionAutocompleteView.refreshStaleUserResults could benefit from an update. Here it is now:

/// Recompute user results for the current query, if any. /// /// Called in particular when we get a [RealmUserEvent].

It looks like it might imply that it's only called on a [RealmUserEvent], and that's not accurate anymore.

…ah, I guess this will be used when we start considering the current topic/stream, which I see you're working on in #692. Makes sense.

I removed it for this PR and will add it to the next one where it is used. Firstly, I thought this was necessary for RecentDmConversationsView.latestMessagesByRecipient.

chrisbobbe · 2024-05-22T05:20:46Z

lib/model/autocomplete.dart

-    return results; // TODO(#228) sort for most relevant first
+    return results;


Since the sorting described in #228 isn't accomplished in this commit, and in fact this commit doesn't do any sorting—that comes later—let's not remove the TODO in this commit. You could move it somewhere else, if you think there's a better place for it (maybe in _sortUsers?).

chrisbobbe · 2024-05-22T05:45:47Z

lib/model/autocomplete.dart

+      if (_sortedUsers != sortedUsers) {
+        // The list of users this loop has been working from has become stale.
+        // Abort so _startSearch can retry with the new list.
+        throw ConcurrentModificationError();
+      }


ConcurrentModificationError is convenient because that's the error that gets caught in _startSearch, but it's not the right error for what we're signaling here. Quoting its dartdoc:

/// Error occurring when a collection is modified during iteration.

That was accurate about the ConcurrentModificationError that our code was dealing with before this commit. It was thrown from iterator.moveNext(), where iterator was from a mutable collection on the store. That collection was vulnerable to being modified while the query was being processed.

Here, though, we want to trigger a retry because _sortedUsers was invalidated. That is caused by…hmm, I guess by various events that are about users. But it isn't caused by a collection being modified during iteration.

Could just throw and catch an instance of a new custom error class, instead.

Relatedly, at this commit, the description of one of our tests becomes wrong:

test('MentionAutocompleteView mutating store.users while in progress causes retry', () async {

I believe the test still passes because we invalidate _sortedUsers when a new-user event arrives. But the reason is no longer about anything being mutated, so we should change the description. Maybe '… new-user event while in progress causes retry'?

Also there's a comment later in the test that refers to mutation:

// new result sticks; no "zombie" result from `store.users` pre-mutation

It would be good to exercise all the different things that can cause a retry…hmm. Maybe we could put the bulk of this test's code into a reusable function , and swap out the store.addUser line for an action that the caller could specify by passing a function, and that passed function could do these different things in different tests:

new-user event

remove-user event

update-user event

new-message event

(handleOlderMessages?)

I think ConcurrentModificationError is fine here. Conceptually, what we are doing is iterating through a collection that got modified: _sortedUsers can be thought of as a collection which is a view (in the database sense) on store.users. Because store.users got modified, effectively _sortedUsers did too.

It's true that the way that "users sorted for autocomplete" collection is implemented is that there's a particular List<User> (so it's a "materialized view" in the database sense), and iterating through the collection iterates through that, and when the underlying "users" collection gets modified we throw away the list and replace it. That List<User> didn't get modified. But that list is an implementation detail in the "users sorted for autocomplete" collection that we're conceptually iterating through.

It would be good to exercise all the different things that can cause a retry…hmm. Maybe […]

Yeah, this would be a good change. We're now depending on those _sortedUsers = null; lines in order for this to have the right behavior; so we should extend the tests so that those are exercised. We should aim to make it so that if any one of those lines were deleted, there's some test that would fail.

@gnprice, @chrisbobbe
There's this one thing that I just noticed recently about this part and that is when store.users is modified, the _computeResults is called twice, once when _startSearch is called through refreshStaleUserResults and secondly when ConcurrentModificationError is thrown which causes a retry.

So what I recommend is the following change in _startSearch:

- while (true) { try { newResults = await _computeResults(query); - break; } on ConcurrentModificationError { // Retry // TODO backoff? + return; } - }

Hmm interesting. Is that a problem already in main?

If so, let's understand it there, and fix it in a prep commit. That reduces the amount of complexity we're debugging at a time.

If not, then what is this code doing differently from what's in main that causes this problem?

Hmm interesting. Is that a problem already in main?

Yeah, this call duplication existed from the main. Also, no ConcurrentModificationError would be thrown on RealmUserUpdateEvent. Both of them are fixed in the same commit.

Cool. In the new revision, it looks like this is the relevant commit — is that right?
7354ce7 autocomplete: Terminate the already-running search when there is a new one

That commit would definitely benefit from some more explanation in the commit message about what problem it's solving and how. See our docs on commit descriptions:
https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#commit-description

Added the description.

sm-sayedi · 2024-05-28T21:05:25Z

Thanks @chrisbobbe and @gnprice for the review! Pushed the revision. PTAL.

Also added replies to some of your comments above.

chrisbobbe

Thanks for the revision! Comments below.

In this review, I noticed an opportunity to simplify MentionAutocompleteView's responsibilities and improve the UX in some ways. (Actually Greg noticed it—we're in the office today and I asked for his thoughts on the retry.) We noticed it after you drew our attention to the unnecessary retry on ConcurrentModificationError, and in particular, it was quite helpful that you made a separate commit focused specifically on that! 🙂

In particular, let's try these things:

Remove MentionAutocompleteView.refreshStaleUserResults.
Keep that retry on ConcurrentModificationError as it was.
For the list of sorted users, calculate it just once in the lifetime of the MentionAutocompleteView instance, without recalculating it on certain events.

This does mean accepting some staleness in the results, including when the user is looking at an autocomplete menu without typing anything to change their query. However:

A MentionAutocompleteView instance corresponds to just one autocomplete interaction, and those interactions aren't expected to last for more than a few moments, from when the user starts typing a name to when they select an option from the list.
There can be a UX benefit to letting the list become slightly stale: it's nicer when menus don't jump around while we're trying to select an item, and that can happen if we're live-updating in response to events.
MentionAutocompleteView's code will be simpler.
The sorting computation is expensive and will get more expensive as we implement the rest of autocomplete: Sort user-mention autocomplete results #228. If re-sorting on various events causes dropped frames, like when the user is scrolling through the autocomplete list, it's unfortunate, and probably not offset by much benefit. (As a user, it's easy to recover from an out-of-date sorting in the list: just refine your query by typing another character or two in your query.)

chrisbobbe · 2024-05-31T20:52:56Z

test/model/recent_dm_conversations_test.dart

+        check(setupView()
+          ..addListener(() { listenersNotified = true; })
+          ..handleMessageEvent(MessageEvent(id: 1, message: message))
+        ) ..map.deepEquals({
+          key([1, 3]): 300,
+          key([1]):    200,
+          key([1, 2]): 100,
+        })
+          ..sorted.deepEquals([key([1, 3]), key([1]), key([1, 2])])
+          ..latestMessagesByRecipient.deepEquals({1: 300, 2: 100, 3: 300});


nit: I preferred the indentation in the previous revision:

Suggested change

check(setupView()

..addListener(() { listenersNotified = true; })

..handleMessageEvent(MessageEvent(id: 1, message: message))

) ..map.deepEquals({

key([1, 3]): 300,

key([1]): 200,

key([1, 2]): 100,

})

..sorted.deepEquals([key([1, 3]), key([1]), key([1, 2])])

..latestMessagesByRecipient.deepEquals({1: 300, 2: 100, 3: 300});

check(setupView()

..addListener(() { listenersNotified = true; })

..handleMessageEvent(MessageEvent(id: 1, message: message))

) ..map.deepEquals({

key([1, 3]): 300,

key([1]): 200,

key([1, 2]): 100,

})

..sorted.deepEquals([key([1, 3]), key([1]), key([1, 2])])

..latestMessagesByRecipient.deepEquals({1: 300, 2: 100, 3: 300});

, on the pattern of existing tests. I think this helps show that the ..map and the ..sorted and the ..latestMessagesByRecipient are grouped together as similar tasks. (Similarly in another test, below.)

chrisbobbe · 2024-05-31T20:58:34Z

test/model/recent_dm_conversations_test.dart

+        final message = eg.dmMessage(id: 300, from: eg.selfUser,
+            to: [eg.user(userId: 1), eg.user(userId: 3)]);


nit: keep previous revision's two-space indent (here and in the other test below)

Suggested change

final message = eg.dmMessage(id: 300, from: eg.selfUser,

to: [eg.user(userId: 1), eg.user(userId: 3)]);

final message = eg.dmMessage(id: 300, from: eg.selfUser,

to: [eg.user(userId: 1), eg.user(userId: 3)]);

chrisbobbe · 2024-05-31T21:08:19Z

lib/model/autocomplete.dart

  /// Recompute user results for the current query, if any.
  ///
-  /// Called in particular when we get a [RealmUserEvent].
+  /// Called in particular when we get a [RealmUserEvent], a [MessageEvent], or
+  /// [MessageListView.fetchOlder] is called.


autocomplete: Introduce a field for sorted users

The last part isn't accurate at this commit, right; this doesn't get called when [MessageListView.fetchOlder] is called.

chrisbobbe · 2024-05-31T23:05:10Z

lib/model/autocomplete.dart

-    while (true) {
-      try {
-        newResults = await _computeResults(query);
-        break;
-      } on ConcurrentModificationError {
-        // Retry
-        // TODO backoff?
-      }
+    try {
+      newResults = await _computeResults(query);
+    } on ConcurrentModificationError {
+      // Terminate this search as there is a new search going on
+      return;


Interesting. Yes, it does seem like the retry is unnecessary—as long as there actually is a new search happening.

The comment says there is a new search going on, and I think it's probably right. But it seems vulnerable to being wrong, especially if some code in a different area is changed, in the future, without considering whether this comment is still true after the change.

One possible approach is to add to the comment exactly why we think it's true, so at least it's more obvious if some future change makes it untrue.

But I think there's a different approach that avoids this issue and, independently of that, would simplify much of the autocomplete code and give a nicer UX in some ways. In this approach, we keep the retry and drop the new search—by removing the MentionAutocompleteView.refreshStaleUserResults method. I'll say more about this approach in my overall review comment.

chrisbobbe · 2024-05-31T23:13:46Z

lib/model/store.dart

+      users[-1] = user;
+      users.remove(-1);


I don't love the use of -1 here.

If we had a "user ID" type that excluded the value -1, if such a thing were possible in the type system, we would surely apply it to users, as something like Map<UserId, User>, and this code would be rejected.

Yeah sure, we cannot call this approach a 100% safe one, but I thought userIds are most probably positive ints, but I am not sure and I couldn't find any doc confirming this.
One other approach would be the following:

users.remove(user); users.add(user);

What do you say?

chrisbobbe · 2024-05-31T23:24:44Z

lib/model/store.dart

+      // The following two lines will eventually not change [users], but are here to
+      // trigger a modification to the collection as only changing the properties
+      // of [user] does not signal a modification of [users] where needed.
+      //
+      // One example of this is [MentionAutocompleteView._computeResults] which
+      // throws a `ConcurrentModificationError` when there is a modification done
+      // to [users].
+      users[-1] = user;
+      users.remove(-1);


I appreciate the explanation in this comment. It sounds like before this commit, an update-user event would not cause an in-progress computation to terminate (via ConcurrentModificationError), and now it does cause the computation to terminate.

This is a change in the app's observable behavior, so it doesn't belong in a foo test: […] commit. Occasionally a commit that's marked "test" will change some app code, but those changes shouldn't change user-facing behavior. (For example, you might adjust test-related comments, as in 9c8a283, or add a test-only debugFoo field to a class, or something.)

chrisbobbe · 2024-05-31T23:28:12Z

test/model/autocomplete_test.dart

+        check(results).single
+        .isA<UserMentionAutocompleteResult>()
+        .userId.equals(1);


nit:

Suggested change

check(results).single

.isA<UserMentionAutocompleteResult>()

.userId.equals(1);

check(results).single

.isA<UserMentionAutocompleteResult>()

.userId.equals(1);

(and in some other tests)

chrisbobbe · 2024-05-31T23:53:42Z

lib/model/autocomplete.dart

@@ -235,14 +235,11 @@ class MentionAutocompleteView extends ChangeNotifier {
  _startSearch(MentionAutocompleteQuery query) async {
    List<MentionAutocompleteResult>? newResults;

-    while (true) {


autocomplete: Terminate the already-running search when there is a new one Before this fix, when there was a search in progress in `_startSearch`, and then for some reason, `_startSearch` was called again […]

Nit: When the reason for the new search is that the query has changed, we actually do terminate the already-running search. See the query != _query check in _computeResults, and the test described as 'MentionAutocompleteView new query during computation replaces old'.

sm-sayedi · 2024-06-03T21:02:56Z

Thank you @chrisbobbe for the review! Pushed the revision. PTAL.

The only problem is that some tests are failing, specifically MentionAutocompleteView mutating store.users while in progress causes retry in 8aa9658: autocomplete [nfc]: Remove the refreshStaleUserResults method and the group tests under MentionAutocompleteView does retry in 2272e77: autocomplete test: Include all the cases which cause results to be recomputed. The main reason for failing these tests is removing the refreshStaleUserResults method and how await Future(() {}); line is used in tests. I don't think I could still fully understand when and how to use this line of code, so I would appreciate your guidance on making these tests not fail!

I am also curious about one thing and that is, now that we don't want to listen to live updates for the autocomplete results in a single session, why should we retry the results while we have an in-progress computation?

gnprice · 2024-06-04T02:19:21Z

I am also curious about one thing and that is, now that we don't want to listen to live updates for the autocomplete results in a single session, why should we retry the results while we have an in-progress computation?

So in the existing code, if a user gets added or removed from store.users while we're in the middle of the loop in _computeResults, that will cause the next iterator.moveNext call to throw ConcurrentModificationError. That means the loop can't finish. We still do want to produce some results for the user — we don't want the autocomplete to just hang, and we want it to include results from the potentially thousands of other users that come after the point where the moveNext call threw — so we catch that exception and restart the loop from the beginning.

That's a situation where we're forced to act in order to finish the search at all; the underlying primitives we're using don't give us the option of just brushing off the change and letting the results be slightly stale, or stale as to just the one user affected.

The idea that's new here in #693 (review) is that where we do have the choice, we'd rather just live with slightly stale results. So in particular that means we don't want to cause a ConcurrentModificationError in situations where we wouldn't already encounter one.

chrisbobbe

Thanks for the revision!

The only problem is that some tests are failing, specifically MentionAutocompleteView mutating store.users while in progress causes retry in 8aa9658: autocomplete [nfc]: Remove the refreshStaleUserResults method and […]

Interesting; yeah, I've just reproduced this test failure. Here is a proposed fix for that:

Here is the output:

00:04 +112 -1: MentionAutocompleteView mutating store.users while in progress causes retry [E]               
  Expected: a bool that:
    is true
  Actual: <false>
  package:checks/src/checks.dart 85:9             check.<fn>
  package:checks/src/checks.dart 708:12           _TestContext.expect
  package:checks/src/extensions/core.dart 108:13  BoolChecks.isTrue
  test/model/autocomplete_test.dart 295:17        main.<fn>

Looking at the test code, it looks like the test expected the search to be complete (check(done).isTrue();) but in fact it was not (done was false at that time).

It's fixed by adding another await Future(() {}); in there:

diff --git test/model/autocomplete_test.dart test/model/autocomplete_test.dart
index 41f49d144..0726fc7a6 100644
--- test/model/autocomplete_test.dart
+++ test/model/autocomplete_test.dart
@@ -292,6 +292,8 @@ void main() {
     await Future(() {});
     check(done).isFalse();
     await Future(() {});
+    check(done).isFalse();
+    await Future(() {});
     check(done).isTrue();
     check(view.results).single
       .isA<UserMentionAutocompleteResult>()

It seems like before this commit, the redundant search from refreshStaleUserResults was finishing before the retry/ConcurrentModificationError search finished. With the search from refreshStaleUserResults removed, the test now needs to wait a little longer for the retry-search to finish. Specifically, it seems like it has to wait for another iteration of the Dart event loop.

When we were writing MentionAutocompleteView, Greg recommended this article to understand Dart's event-loop architecture: https://web.archive.org/web/20170704074724/https://webdev.dartlang.org/articles/performance/event-loop

The reason for this line in _computeResults—

      // CPU perf: End this task; enqueue a new one for resuming this work
      await Future(() {});

—is to give a chance for other code, such as UI animations, to use the CPU between batches of 1000 query.testUser calls. Some Zulip orgs have very many users, and we would probably drop some animation frames if one event-loop iteration had to wait for 20,000+ query.testUser calls to be done at once. So, we do the computation asynchronously in batches of 1000.

More later; I have some thoughts after reading Greg's comment at #693 (comment).

chrisbobbe · 2024-06-04T00:44:27Z

test/model/recent_dm_conversations_test.dart

-          ..sorted.deepEquals(expected.sorted);
+          ..sorted.deepEquals(expected.sorted)
+          ..latestMessagesByRecipient.deepEquals(expected.latestMessagesByRecipient);
+      });


Bump on my comment at #693 (comment) :

But I don't think we should have a check that expects the slightly undesirable behavior.

chrisbobbe · 2024-06-04T01:11:17Z

lib/model/autocomplete.dart

@@ -125,23 +125,11 @@ class AutocompleteViewManager {
    assert(removed);
  }

-  void handleRealmUserAddEvent(RealmUserAddEvent event) {


autocomplete [nfc]: Remove the `refreshStaleUserResults` method This method was used to search for the new user results in autocomplete when the users in store were modified during an in-progress computation. Along with this, `_startSearch` would do the same thing by retrying, resulting in duplicate actions. By removing this, the unnecessary search for user results is prevented.

Since this commit changes the app's behavior, it's not [nfc].

One important change is: if a search computation isn't in progress when our user data changes, then we won't start a new search anymore. For example:

You type @Sayed M

You see that your name is in the list

You stop typing

From your computer, you make a new user named "Sayed Mahmood Sayedi (Test Account)".

What happens next is affected by the changes in this commit. Before this commit, that new user would appear in the list. At this commit, the user would not appear. That behavior change is OK, for the reasons I mentioned at #693 (review) . But in the commit message, let's emphasize that the commit is about intentionally removing some live-updating behavior. A link to that GitHub comment would give the reader helpful context on that decision, or you could summarize it.

As you've pointed out, this commit removes a redundant computation in the (rare?) case where a computation is in progress when the users collection is changed.

(It also removes a computation that isn't redundant. Before this commit, if a RealmUserUpdateEvent arrived when search was in progress, we would start a new search to replace the old one. This commit drops that new search, because, as you've found, the retry on ConcurrentModificationError isn't triggered in the update-user case. But again, that's OK; this commit is about simplifying this system by making it not care if its output is slightly outdated.)

chrisbobbe · 2024-06-04T05:22:13Z

@gnprice, it occurred to me after reading your comment above: if we compute the sorted-users list just once per MentionAutocompleteView, and the search iterates through that list instead of store.users.values, then I think we don't actually expect ConcurrentModificationErrors to be thrown anywhere. Is that right? If so, then we shouldn't need to catch ConcurrentModificationError, and I think Sayed is right that we don't need the while (true) around that catch.

If this is right, Greg, maybe this might be a good commit sequence for this PR:

e77e644 recent-dm-conversations test [nfc]: Add a missing check of listenersNotified (but edited for my review)
43d5eb6 recent-dm-conversations: Add latestMessagesByRecipient data structure
8aa9658 autocomplete [nfc]: Remove the refreshStaleUserResults method (but edited for my review)

Then a commit that computes a user list sorted by DM recency, but with this computation done only once (maybe in MentionAutocompleteView.init).

Then a commit that makes _computeResults iterate through the sorted-users list instead of store.users.values, and removes the retry on ConcurrentModificationError.

sm-sayedi · 2024-06-04T10:41:13Z

Thanks @gnprice for the comment and @chrisbobbe for the review!

if we compute the sorted-users list just once per MentionAutocompleteView, and the search iterates through that list instead of store.users.values, then I think we don't actually expect ConcurrentModificationErrors to be thrown anywhere.

Yeah, that's right, but it's not true until 4fbe8ee: autocomplete: Introduce a field for sorted users, and I think what Greg meant was before this commit, but I think we can overcome the inevitability of ConcurrentModificationError in that situation too, by using a for loop instead of an iterator, that way if store.users gets modified, we can still carry on with somewhat stale users.

Then a commit that computes a user list sorted by DM recency, but with this computation done only once (maybe in MentionAutocompleteView.init).

Yeah MentionAutocompleteView.init seems a better place for sorting the user list just one time, although now it's also sorted one time.

This method was used to search for the new user results in autocomplete when the users in store were modified during an in-progress computation. Along with this, `_startSearch` would do the same thing by retrying, resulting in duplicate actions. By removing this, the unnecessary search for user results is prevented. Besides, this commit also removes some live-updating behavior of user-mention autocomplete results in order to provide a better UX. (To read more about this decision, check out zulip#693 (review))

gnprice · 2024-06-04T20:03:30Z

if we compute the sorted-users list just once per MentionAutocompleteView, and the search iterates through that list instead of store.users.values, then I think we don't actually expect ConcurrentModificationErrors to be thrown anywhere. Is that right? If so, then we shouldn't need to catch ConcurrentModificationError, and I think Sayed is right that we don't need the while (true) around that catch.

Yes, that sounds right.

maybe this might be a good commit sequence for this PR:

I like those first three as the start of the sequence, but there's another aspect of the current revision which I'd like to preserve: it has this commit introducing a sorted-users list without yet doing any actual sorting:
215c3d7 autocomplete: Introduce a field for sorted users

and introduces the actual sorting after that. I like that structure because along with introducing the sorted-users list comes changing some of the infrastructural logic around it, and so that structure lets that logic be isolated from the changes that are concerned with things like DM recency.

So I think a good sequence would be:

those same first three
then a commit that introduces _sortedUsers, like 215c3d7 does but constructed only once per MentionAutocompleteView, and also, as you say:

makes _computeResults iterate through the sorted-users list instead of store.users.values, and removes the retry on ConcurrentModificationError.
then a commit that sorts by DM recency

along with other commits interspersed as needed for things like adding test helpers, like these existing commits in the PR:
7176adf test-store [nfc]: Add removeUser and updateUser extension methods on PerAccountStore
edcb6ea test-store [nfc]: Add addMessage extension methods on PerAccountStore

Yeah, that's right, but it's not true until 4fbe8ee: autocomplete: Introduce a field for sorted users

@sm-sayedi I think that's part of what's addressed in the commit sequence Chris proposed, and also my revised version — the change where we stop anticipating ConcurrentModificationError would happen only in the same commit where we introduce that field.

I think we can overcome the inevitability of ConcurrentModificationError in that situation too, by using a for loop instead of an iterator, that way if store.users gets modified, we can still carry on with somewhat stale users.

I don't think this works, if I'm understanding you right. If you do for (final user in store.users.values) { /* … */ }, that's basically syntactic sugar for something like (untested):

  final iterator = store.users.values.iterator;
  while (iterator.moveNext()) {
    final user = iterator.current();
    /* … */
  }

Dart has a bit of a gap in reference documentation for the language itself, so I can't point to a completely on-point piece of docs for that. But that's what I'd expect from general cross-language conventions, from the docs on for-loops:
https://dart.dev/language/loops#for-loops
and from the docs on Iterable and Iterator.

gnprice · 2024-06-04T20:05:28Z

And yeah, in MentionAutocompleteView.init is a fine place to do the sorting. Its call sites are always immediately followed by setting query, which starts a search; so there's nothing gained by deferring the sort until we start a search.

sm-sayedi · 2024-06-05T00:21:38Z

I don't think this works, if I'm understanding you right. If you do for (final user in store.users.values) { /* … */ }, that's basically syntactic sugar for something like...

You're right. For in loop is like a high level iterator, but what I meant was the for loop.

for (int i = 0; i < store.users.values.length; i++) {...}

gnprice · 2024-06-05T00:27:28Z

Ah. In that case if you try to fill in the {...} in your example, I think you will see why that way doesn't work :-)

This method was used to search for the new user results in autocomplete when the users in store were modified during an in-progress computation. Along with this, `_startSearch` would do the same thing by retrying, resulting in duplicate actions. By removing this, the unnecessary search for user results is prevented. Besides, this commit also removes some live-updating behavior of user-mention autocomplete results in order to provide a better UX. (To read more about this decision, check out zulip#693 (review))

sm-sayedi · 2024-06-05T09:43:32Z

Revision pushed @chrisbobbe. Please have a look.

chrisbobbe · 2024-06-06T16:06:06Z

Thanks for the revision! LGTM; marking for Greg's review.

gnprice

Thanks @sm-sayedi, and thanks @chrisbobbe for the previous reviews! Generally this looks good.

Various comments below. I think at this point they're all about style or tests, not the production behavior. Style and tests are important too (because they affect our future development work); but they're less likely to mean cascading changes to other parts of the PR, so that's a sign that it's closing in toward merge 🙂

gnprice · 2024-06-07T00:52:29Z

lib/model/recent_dm_conversations.dart

@@ -1,3 +1,5 @@
+import 'dart:math';


nit in commit messages:
e9eec48 recent_dm_conversations test [nfc]: Comment on a missing not-notified check
a4c640a recent-dm-conversations: Add latestMessagesByRecipient data structure

In the summary lines, "recent_dm_conversations" or "recent-dm-conversations" is pretty long for a slug — it occupies a big chunk of the 76 or so columns available. So let's make an abbreviation for it. Say, "recent-dms".

This is like how for changes to the message list, we use "msglist" for the summary-line slug, and for notifications we say "notif". Using hyphens is less common, but we have a few commits saying "action-sheet" or "choose-account". (And once it's an abbreviation and not an identifier that appears in the code, I wouldn't want to use underscores, so e.g. not "recent_dms" — that would make it look like an identifier, misleadingly so.)

gnprice · 2024-06-07T00:53:42Z

lib/model/recent_dm_conversations.dart

-      map: Map.fromEntries(entries),
-      sorted: QueueList.from(entries.map((e) => e.key)),
+      map: map,
+      sorted: sorted,


nit: these two can remain inlined here — simplifies the diff, and I think also a bit simpler to read in the resulting code

gnprice · 2024-06-07T00:55:54Z

lib/model/recent_dm_conversations.dart

+      final dmNarrow = entry.key;
+      final maxMessageId = entry.value;
+      for (final userId in dmNarrow.otherRecipientIds) {
+        // only take the latest message of a user across all the conversations.


nit: either a complete sentence with initial capital and final period, or incomplete with neither, but not a period without an initial capital

gnprice · 2024-06-07T01:03:42Z

lib/model/recent_dm_conversations.dart

    }
+    for (final recipient in key.otherRecipientIds) {


nit: as this function body gets more complex, it calls for a bit more organization. In particular let's separate it into stanzas for the different parts of its job. So here:

Suggested change

}

for (final recipient in key.otherRecipientIds) {

}

// Update [latestMessagesByRecipient].

for (final recipient in key.otherRecipientIds) {

and a blank line below before notifyListeners; and a blank line and a comment "Update [map] and [sorted]." before the final prev = line.

The most important effect of all that is to group together the map/sorted logic. That logic is fairly gnarly, so it's real helpful for the reader to be able to isolate it and not wonder about where its boundaries are or how it might intermingle with the other logic around it. Highlighting the boundaries wasn't particularly necessary when its boundaries were nearly the same as those of the function body, separated only by a few trivial lines above and below; but now that there's this new other piece of nontrivial logic, it becomes more needed.

gnprice · 2024-06-07T01:07:30Z

test/model/recent_dm_conversations_test.dart

+          ..sorted.deepEquals([key([2]), key([1]), key([1, 2])])
+          ..latestMessagesByRecipient.deepEquals({1: 200, 2: 300});


nit:

Suggested change

..sorted.deepEquals([key([2]), key([1]), key([1, 2])])

..latestMessagesByRecipient.deepEquals({1: 200, 2: 300});

..sorted.deepEquals([key([2]), key([1]), key([1, 2])])

..latestMessagesByRecipient.deepEquals({2: 300, 1: 200});

IOW, keep the expected map sorted (in the source code) by value, the same way the expected map is above. That helps keep it easy for the reader to verify that the expected data is right.

(similarly in a couple of other cases below)

gnprice · 2024-06-07T02:02:32Z

lib/model/autocomplete.dart

+  /// returns a positive number if [userB] is more recent than [userA],
+  /// and returns `0` if both [userA] and [userB] are equally recent
+  /// or there is no DM exchanged with them whatsoever.
+  static int compareByDms(User userA, User userB, {required PerAccountStore store}) {


Suggested change

static int compareByDms(User userA, User userB, {required PerAccountStore store}) {

@visibleForTesting

static int compareByDms(User userA, User userB, {required PerAccountStore store}) {

Conceptually this is a private helper — it's exposed only because that's convenient for unit testing.

(We also don't have to expose it in order to write all the tests we like; we could do a more end-to-end test by running a query "" and seeing what order the results come in. But the meaning of this is fairly crisp, so it's basically harmless to expose if it's convenient for tests.)

gnprice · 2024-06-07T02:10:25Z

test/model/autocomplete_test.dart

+
+        final userA = eg.user(userId: 1);
+        final userB = eg.user(userId: 2);
+        final compareAB = MentionAutocompleteView.compareByDms(userA, userB, store: store);
+        check(compareAB).isNegative();


These test cases have a fair number of lines that are repetitive. Let's pull those out into a local helper function, like this:

Suggested change

final userA = eg.user(userId: 1);

final userB = eg.user(userId: 2);

final compareAB = MentionAutocompleteView.compareByDms(userA, userB, store: store);

check(compareAB).isNegative();

check(compare1vs2()).isNegative();

That'll mean each test case adds a grand total of 7 or 8 lines instead of 11 or 12, which makes the whole list of test cases quite a lot more compact to look at. That's helpful for scanning through it and seeing what's covered and thinking of things that might not be.

Relatedly, it focuses each test case much more tightly on just the information that's distinctive in it, which is helpful for understanding what each individual test case is saying.

gnprice · 2024-06-07T02:16:32Z

test/model/autocomplete_test.dart

+      test('has DMs with userA and userB, latest with userB, prioritizes userB', () async {
+        await prepare(dmConversations: [
+          RecentDmConversation(userIds: [2],    maxMessageId: 200),
+          RecentDmConversation(userIds: [1, 2], maxMessageId: 100),


There's a bit of a mismatch in the naming here. The descriptions call the users "A" and "B":

test('has DMs with userA and userB, latest with userA, prioritizes userA', () async {

as do the User locals (though my suggestion above has those going away into a helper).

But then the actual data that expresses the content of the test case has "1" and "2", because it needs to be expressed as user IDs.

I think either way of naming will work; it just needs to be consistent, so that the reader doesn't have to translate back and forth in their head in order to understand the test case. If you want "A" and "B", the user IDs can be handled by having something like

const idA = 1; const idB = 2;

up at the group level.

gnprice · 2024-06-07T02:20:46Z

test/model/autocomplete_test.dart

+    test('autocomplete suggests relevant users in the following order: '
+      '1. Users most recent in the DM conversations', () async {
+      final users = [


nit: indent continuation line further to avoid looking like part of the body:

Suggested change

test('autocomplete suggests relevant users in the following order: '

'1. Users most recent in the DM conversations', () async {

final users = [

test('autocomplete suggests relevant users in the following order: '

'1. Users most recent in the DM conversations', () async {

final users = [

But taking a step back, if the test name feels like it needs to spill onto a second line, that's a sign that it's too long for a name 🙂

This test's name will take more and more lines once we add other criteria. I thought about shrinking its name, but as this test is about exercising the different criteria users are suggested based on, and to provide the reader with enough context, I couldn't come up with a shorter name. I would appreciate any clue from your side!🙂

For someone reading the source code, information is just as visible if it's in a comment as in the test name. So the most direct fix is to move the extra information from the name to a comment, e.g. replacing:

group('autocomplete suggests relevant users in the following order: ' '1. Users most recent in the DM conversations', () { Future<void> checkResultsIn(Narrow narrow, {required List<int> expected}) async {

with:

group('autocomplete suggests relevant users in the intended order', () { // The order should be: // 1. Users most recent in the DM conversations Future<void> checkResultsIn(Narrow narrow, {required List<int> expected}) async {

The role of a test name is to identify the test case. It appears in places like the status line that flutter test shows you, with the name of a currently-running test, or in a heading line when a test fails; and it can be used with flutter test --name to pick specific tests to run. If the name gets much longer than a line, it's not really readable in those formats anyway.

For example here's the names in your current revision:

(Usually I'd post command output like this as a code block, but I think for this discussion it's helpful to see how it looks on the screen.)

And here it is with the tweak above:

Still kind of long, but getting more manageable. And I think the details of the order don't come across particularly helpfully in any case.

Separately: if there's a test where you feel like it takes more than a line to name what it's about, that's also a sign that the test itself may be combining too many different things. I suspect that's also something that's going on here, but it's relatively mild at this stage and will become more important in #692 where there are more ingredients in the sorting.

When an individual test case is testing too many ideas at once, it tends to make it so that when something breaks the test, it's hard to tell what the test intended; and as a result it's hard to tell whether the test just needs to be updated to expect the new output, or the test is telling you something you broke in the actual application code and should fix. The main point of a test case is to tell you when some future change accidentally broke the functionality the test is testing; so when that becomes hard to tell, it largely defeats the purpose of the test.

gnprice · 2024-06-07T02:26:10Z

test/model/autocomplete_test.dart

+      const streamNarrow = StreamNarrow(1);
+      await checkResultsIn(streamNarrow, expected: [3, 0, 1, 2, 4]);
+
+      const topicNarrow = TopicNarrow(1, 'topic');
+      await checkResultsIn(topicNarrow, expected: [3, 0, 1, 2, 4]);
+
+      final dmNarrow = DmNarrow(allRecipientIds: [eg.selfUser.userId], selfUserId: eg.selfUser.userId);
+      await checkResultsIn(dmNarrow, expected: [3, 0, 1, 2, 4]);
+
+      const allMessagesNarrow = CombinedFeedNarrow();
+      // Results are in the original order as we do not sort them for
+      // [CombinedFeedNarrow] because we can not access autocomplete for now.
+      await checkResultsIn(allMessagesNarrow, expected: [0, 1, 2, 3, 4]);


These should be separate test cases, because each of them is logically independent of the others — checking something that doesn't need the others to have happened first. Having separate test cases is helpful because it means that if a change breaks some of them, you can immediately see which ones it breaks and doesn't break, rather than have to fiddle around editing the test in order to try each one in turn.

You can keep each of them pretty much just as compact as this by… actually it looks like you've basically already done all the work needed for that, with the checkResultsIn helper 🙂. Just move the users and dmConversations locals inside that helper, so that they stay inside (transitively) test calls rather than being at the top level of a group.

This data structure is used to keep track of the latest message of each recipient across all DM conversations.

This method was used to search for the new user results in autocomplete when the users in store were modified during an in-progress computation. Along with this, `_startSearch` would do the same thing by retrying, resulting in duplicate actions. By removing this, the unnecessary search for user results is prevented. Besides, this commit also removes some live-updating behavior of user-mention autocomplete results in order to provide a better UX. To read more about this decision, see: zulip#693 (review)

sm-sayedi · 2024-06-07T21:12:00Z

Thanks @gnprice for the review with helpful comments! Revision pushed! Please have a look.

Also, added replies to some of your comments in the previous review thread.

gnprice

Thanks for the revision! This is very close to merge now.

Comments below, most of them quite small. There are also two open subthreads above:
#693 (comment)
#693 (comment)

gnprice · 2024-06-08T01:54:15Z

test/model/autocomplete_test.dart

+  test('MentionAutocompleteView mutating store.users while in progress is not '
+      'reflected in current query results', () async {


Yeah, so this test name doesn't really capture what's being tested (continuing the thread from https://github.com/zulip/zulip-flutter/pull/693/files#r1630509271 ):

The results not reflecting the mutation is acceptable (and we've chosen to accept it), but not totally desired.

The fact that some results do eventually appear — that's important, and something that didn't have to be the case if we had a buggy implementation.

So the latter is what the test is about.

gnprice · 2024-06-08T04:02:49Z

test/model/autocomplete_test.dart

    check(done).isFalse();
-    await store.addUser(eg.user(userId: 10000, email: '[email protected]', fullName: 'User 10000'));
-    await Future(() {});
-    check(done).isFalse();
+    await store.addUser(eg.user(userId: 11000, email: '[email protected]', fullName: 'User 11000'));
    await Future(() {});
    check(done).isTrue();


This makes me a bit nervous, because it's not clear from looking at this test code that the addUser actually happened soon enough to test what this is meant to test.

The test needs the addUser to finish at a time when the search is still unfinished. But here it looks like it could also be that the addUser came too late, after the search was already complete.

To rule that out, I'd really like to have a check(done).isFalse() after the addUser (like there is in the existing test). Try to make that happen, for example by adding more users so that there's over 2000 of them.

gnprice · 2024-06-08T04:11:57Z

lib/model/autocomplete.dart

+
+    final users = store.users.values.toList();
+    return users..sort((userA, userB) => compareByRelevance(
+      userA, userB, store: store));


nit:

Suggested change

final users = store.users.values.toList();

return users..sort((userA, userB) => compareByRelevance(

userA, userB, store: store));

return store.users.values.toList()

..sort((userA, userB) => compareByRelevance(userA, userB, store: store));

gnprice · 2024-06-08T04:12:29Z

lib/model/autocomplete.dart

+  static int compareByRelevance(
+    User userA,
+    User userB, {
+    required PerAccountStore store,
+  }) {


nit:

Suggested change

static int compareByRelevance(

User userA,

User userB, {

required PerAccountStore store,

}) {

static int compareByRelevance(User userA, User userB, {

required PerAccountStore store,

}) {

gnprice · 2024-06-08T04:16:16Z

lib/model/autocomplete.dart

+      userA, userB, store: store));
+  }
+
+  static int compareByRelevance(


Oh and this should also be private — from the perspective of any other part of the app, the interface that reaches this code is only through MentionAutocompleteView.init.

(Or perhaps make this visibleForTesting and compareByDms be private; compare #693 (comment) . But that choice will become clearer when we get to #692; right now there's no real distinction between these two methods anyway.)

I removed it for this PR as we can simply replace it with compareByDms. I think we can add it when there are more criteria.

gnprice · 2024-06-08T04:18:44Z

test/model/autocomplete_test.dart

+        check(compareAB()).isGreaterThan(0);
+      });
+
+      test('doesn\'t have DMs with userA or userB -> prioritizes neither', () async {


nit:

Suggested change

test('doesn\'t have DMs with userA or userB -> prioritizes neither', () async {

test('has no DMs with userA or userB -> prioritizes neither', () async {

Shorter, and as a bonus gets rid of the need to backslash-escape 🙂

gnprice · 2024-06-08T04:21:38Z

test/model/autocomplete_test.dart

+        check(compareAB()).isNegative();
+      });
+
+      test('has DMs with userA and userB, latest with userB -> prioritizes userB', () async {
+        await prepare(dmConversations: [
+          RecentDmConversation(userIds: [idB],      maxMessageId: 200),
+          RecentDmConversation(userIds: [idA, idB], maxMessageId: 100),
+        ]);
+        check(compareAB()).isGreaterThan(0);


Huh, I guess there's no isPositive.

Let's keep these parallel to each other as far as we can, so they look different only in the ways they really meaningfully are different. So isLessThan(0) instead of isNegative().

gnprice · 2024-06-08T04:25:37Z

test/model/autocomplete_test.dart

+          DmNarrow(allRecipientIds: [eg.selfUser.userId],
+            selfUserId: eg.selfUser.userId),


nit: can tighten a bit:

Suggested change

DmNarrow(allRecipientIds: [eg.selfUser.userId],

selfUserId: eg.selfUser.userId),

DmNarrow.withUser(eg.selfUser.userId, selfUserId: eg.selfUser.userId),

gnprice · 2024-06-08T04:31:07Z

test/model/autocomplete_test.dart

+        await check(checkResultsIn(
+          const CombinedFeedNarrow(),
+          expected: [0, 1, 2, 3, 4])
+        ).throws((e) => e.isA<AssertionError>());


nit: can tighten like so:

Suggested change

).throws((e) => e.isA<AssertionError>());

).throws<AssertionError>();

Though also just .throws() would be fine — we don't need to be all that particular about how it fails.

This method was used to search for the new user results in autocomplete when the users in store were modified during an in-progress computation. Along with this, `_startSearch` would do the same thing by retrying, resulting in duplicate actions. By removing this, the unnecessary search for user results is prevented. Besides, this commit also removes some live-updating behavior of user-mention autocomplete results in order to provide a better UX. To read more about this decision, see: zulip#693 (review)

sm-sayedi · 2024-06-09T17:03:31Z

Thanks @gnprice for the review! Revision pushed. Also added two replies in previous sub-threads: #693 comment and #693 comment.

gnprice · 2024-06-10T07:39:16Z

Thanks! Generally the revisions look good. Replied above, and I think #693 (comment) is still open.

sm-sayedi · 2024-06-10T18:28:47Z

Thanks @gnprice! The comment about the test was really helpful! 😃 Revision pushed.

gnprice · 2024-06-10T19:06:43Z

test/model/autocomplete_test.dart

+    while (!done) {
      await Future(() {});


This will become an infinite loop if something goes wrong such that the search never finishes. When a test breaks, it's annoying if the behavior is that it spins forever — it makes CI take much longer to show results (as the test runs until a timeout), and it gets in the way of seeing results from other tests.

That's why it's best to have some bound on the number of iterations. See my suggestion above at https://github.com/zulip/zulip-flutter/pull/693/files#r1632735033 . Or for examples in our existing tests in the repo, see test/widgets/message_list_test.dart and search for "for (int "; there's a couple of tests there that do a fling-scroll, and don't want to hard-code an expectation for just how many frames it'll take before the scroll reaches the relevant point, but they do put an upper bound on how many they'll wait for.

sm-sayedi · 2024-06-11T00:29:55Z

Thank you @gnprice. Update pushed.

gnprice

Thanks @sm-sayedi for all your work on this! This version now LGTM with two comments below.

I'll fix the one nit, and merge.

gnprice · 2024-06-11T02:03:41Z

test/model/autocomplete_test.dart

+    for (int i = 0; i < 3; i++) {
      await Future(() {});
-      check(view.results).single
-        .isA<UserMentionAutocompleteResult>()
-        .userId.equals(10000);
    }


This is OK as is because it's only 3 iterations anyway, but if it were many more I'd want to have it break as soon as the thing is done (in addition to having an upper bound on the number of iterations). See my suggested code above, or those examples I mentioned in the message-list tests, which look like:

for (int i = 0; i < 30; i++) { // Find the point in the fling where the fetch starts. await tester.pump(const Duration(milliseconds: 100)); if (itemCount(tester)! > 101) break; // The loading indicator appeared. }

Note the upper bound on iteration count (30) as well as the break.

gnprice · 2024-06-11T02:05:37Z

test/model/autocomplete_test.dart

+  test('MentionAutocompleteView mutating store.users while in progress does not '
+      'interrupt the current query results', () async {


nit:

Suggested change

test('MentionAutocompleteView mutating store.users while in progress does not '

'interrupt the current query results', () async {

test('MentionAutocompleteView mutating store.users while in progress does not '

'prevent query from finishing', () async {

"interrupt" reads to me as more about what happens internally — e.g. the old version does get interrupted, even though it then resumes and recovers. The test is really about the ultimate output, which is why the old version passes the test (modulo the detail about checking the new user 11000 is absent).

This field will be used to maintain a list of sorted users based on the most relevant autocomplete criteria in the upcoming commits. This commit also removes the retry logic for an in-progress query computation as there is no longer the cause that would necessitate such a retry.

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

sm-sayedi · 2024-06-11T02:38:47Z

Thanks @gnprice and @chrisbobbe for all your continuous help on this!! Happy to see it finally merged. 🙂

but if it were many more I'd want to have it break as soon as the thing is done...

Sorry for this, I just forgot to include that. As you said it's not a big matter for this small for loop, but maybe I will fix it anyway in an NFC commit in #692.

This was referenced May 21, 2024

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

Merged

autocomplete: Sort user-mention autocomplete results #608

Closed

gnprice added the maintainer review PR ready for review by Zulip maintainers label May 21, 2024

chrisbobbe reviewed May 22, 2024

View reviewed changes

sm-sayedi force-pushed the issue-228-@-mention-results-with-recent-DMs-criterion branch from 9a6757b to cf3737d Compare May 28, 2024 20:55

sm-sayedi requested a review from chrisbobbe May 28, 2024 21:06

sm-sayedi force-pushed the issue-228-@-mention-results-with-recent-DMs-criterion branch from cf3737d to 05db60b Compare May 31, 2024 17:55

chrisbobbe reviewed Jun 1, 2024

View reviewed changes

sm-sayedi force-pushed the issue-228-@-mention-results-with-recent-DMs-criterion branch 2 times, most recently from 3ad170b to 783cb10 Compare June 3, 2024 20:59

sm-sayedi requested a review from chrisbobbe June 3, 2024 21:16

chrisbobbe reviewed Jun 4, 2024

View reviewed changes

sm-sayedi force-pushed the issue-228-@-mention-results-with-recent-DMs-criterion branch from 783cb10 to d65c114 Compare June 4, 2024 17:56

sm-sayedi force-pushed the issue-228-@-mention-results-with-recent-DMs-criterion branch from d65c114 to 08fd062 Compare June 5, 2024 08:10

sm-sayedi force-pushed the issue-228-@-mention-results-with-recent-DMs-criterion branch from 08fd062 to 9da1bab Compare June 5, 2024 08:20

sm-sayedi force-pushed the issue-228-@-mention-results-with-recent-DMs-criterion branch from 9da1bab to bca9c2c Compare June 5, 2024 08:22

sm-sayedi requested a review from chrisbobbe June 6, 2024 04:38

sm-sayedi force-pushed the issue-228-@-mention-results-with-recent-DMs-criterion branch from da4c9a0 to 9a1c213 Compare June 6, 2024 06:41

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Jun 6, 2024

gnprice reviewed Jun 7, 2024

View reviewed changes

sm-sayedi added 2 commits June 7, 2024 20:41

recent-dms test [nfc]: Comment on a missing not-notified check

fd08ab8

recent-dms: Add latestMessagesByRecipient data structure

177b719

This data structure is used to keep track of the latest message of each recipient across all DM conversations.

sm-sayedi force-pushed the issue-228-@-mention-results-with-recent-DMs-criterion branch 3 times, most recently from f3c7a7f to 4d5d823 Compare June 7, 2024 20:59

gnprice reviewed Jun 8, 2024

View reviewed changes

sm-sayedi force-pushed the issue-228-@-mention-results-with-recent-DMs-criterion branch from 4d5d823 to 18e7da6 Compare June 9, 2024 16:55

sm-sayedi force-pushed the issue-228-@-mention-results-with-recent-DMs-criterion branch from 18e7da6 to 3010865 Compare June 10, 2024 18:20

gnprice reviewed Jun 10, 2024

View reviewed changes

sm-sayedi force-pushed the issue-228-@-mention-results-with-recent-DMs-criterion branch from 3010865 to 16af601 Compare June 11, 2024 00:25

gnprice approved these changes Jun 11, 2024

View reviewed changes

sm-sayedi added 2 commits June 10, 2024 19:07

autocomplete: Add "recent DM conversations" criterion

37ad773

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

gnprice force-pushed the issue-228-@-mention-results-with-recent-DMs-criterion branch from 16af601 to 37ad773 Compare June 11, 2024 02:08

gnprice merged commit 37ad773 into zulip:main Jun 11, 2024
1 check passed

sm-sayedi mentioned this pull request Jun 12, 2024

autocomplete: Introduce topic input autocomplete #627

Merged

neiljp mentioned this pull request Jun 24, 2024

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

Open

3 tasks

		return results; // TODO(#228) sort for most relevant first
		return results;

		final message = eg.dmMessage(id: 300, from: eg.selfUser,
		to: [eg.user(userId: 1), eg.user(userId: 3)]);

		..sorted.deepEquals([key([2]), key([1]), key([1, 2])])
		..latestMessagesByRecipient.deepEquals({1: 200, 2: 300});

	static int compareByDms(User userA, User userB, {required PerAccountStore store}) {
	@visibleForTesting
	static int compareByDms(User userA, User userB, {required PerAccountStore store}) {

		test('MentionAutocompleteView mutating store.users while in progress is not '
		'reflected in current query results', () async {

	test('doesn\'t have DMs with userA or userB -> prioritizes neither', () async {
	test('has no DMs with userA or userB -> prioritizes neither', () async {

		DmNarrow(allRecipientIds: [eg.selfUser.userId],
		selfUserId: eg.selfUser.userId),

	DmNarrow(allRecipientIds: [eg.selfUser.userId],
	selfUserId: eg.selfUser.userId),
	DmNarrow.withUser(eg.selfUser.userId, selfUserId: eg.selfUser.userId),

	).throws((e) => e.isA<AssertionError>());
	).throws<AssertionError>();

		test('MentionAutocompleteView mutating store.users while in progress does not '
		'interrupt the current query results', () async {

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

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

Conversation

sm-sayedi commented May 21, 2024 • edited Loading

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

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 May 23, 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

sm-sayedi commented May 28, 2024 • edited Loading

chrisbobbe left a comment • 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

Choose a reason for hiding this comment

sm-sayedi commented Jun 3, 2024 • edited Loading

gnprice commented Jun 4, 2024

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

chrisbobbe commented Jun 4, 2024 • edited Loading

sm-sayedi commented Jun 4, 2024 • edited Loading

gnprice commented Jun 4, 2024

gnprice commented Jun 4, 2024

sm-sayedi commented Jun 5, 2024

gnprice commented Jun 5, 2024

sm-sayedi commented Jun 5, 2024

chrisbobbe commented Jun 6, 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

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 Jun 7, 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

sm-sayedi commented May 21, 2024 •

edited

Loading

sm-sayedi May 23, 2024 •

edited

Loading

sm-sayedi commented May 28, 2024 •

edited

Loading

chrisbobbe left a comment •

edited

Loading

sm-sayedi commented Jun 3, 2024 •

edited

Loading

chrisbobbe commented Jun 4, 2024 •

edited

Loading

sm-sayedi commented Jun 4, 2024 •

edited

Loading