Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 44 additions & 40 deletions lib/model/autocomplete.dart
Original file line number Diff line number Diff line change
Expand Up @@ -125,23 +125,11 @@ class AutocompleteViewManager {
assert(removed);
}

void handleRealmUserAddEvent(RealmUserAddEvent event) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. You type @Sayed M
  2. You see that your name is in the list
  3. You stop typing
  4. 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.)

for (final view in _mentionAutocompleteViews) {
view.refreshStaleUserResults();
}
}

void handleRealmUserRemoveEvent(RealmUserRemoveEvent event) {
for (final view in _mentionAutocompleteViews) {
view.refreshStaleUserResults();
}
autocompleteDataCache.invalidateUser(event.userId);
}

void handleRealmUserUpdateEvent(RealmUserUpdateEvent event) {
for (final view in _mentionAutocompleteViews) {
view.refreshStaleUserResults();
}
autocompleteDataCache.invalidateUser(event.userId);
}

Expand Down Expand Up @@ -176,17 +164,54 @@ class AutocompleteViewManager {
/// * When the object will no longer be used, call [dispose] to free
/// resources on the [PerAccountStore].
class MentionAutocompleteView extends ChangeNotifier {
MentionAutocompleteView._({required this.store, required this.narrow});
MentionAutocompleteView._({
required this.store,
required this.narrow,
required this.sortedUsers,
});

factory MentionAutocompleteView.init({
required PerAccountStore store,
required Narrow narrow,
}) {
final view = MentionAutocompleteView._(store: store, narrow: narrow);
final view = MentionAutocompleteView._(
store: store,
narrow: narrow,
sortedUsers: _usersByRelevance(store: store, narrow: narrow),
);
store.autocompleteViewManager.registerMentionAutocomplete(view);
return view;
}

static List<User> _usersByRelevance({
required PerAccountStore store,
required Narrow narrow,
}) {
assert(narrow is! CombinedFeedNarrow);
return store.users.values.toList()
..sort((userA, userB) => compareByDms(userA, userB, store: store));
}

/// Determines which of the two users is more recent in DM conversations.
///
/// Returns a negative number if [userA] is more recent than [userB],
/// 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.
@visibleForTesting
static int compareByDms(User userA, User userB, {required PerAccountStore store}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

final recentDms = store.recentDmConversationsView;
final aLatestMessageId = recentDms.latestMessagesByRecipient[userA.userId];
final bLatestMessageId = recentDms.latestMessagesByRecipient[userB.userId];

return switch((aLatestMessageId, bLatestMessageId)) {
(int a, int b) => -a.compareTo(b),
(int(), _) => -1,
(_, int()) => 1,
_ => 0,
};
}

@override
void dispose() {
store.autocompleteViewManager.unregisterMentionAutocomplete(this);
Expand All @@ -198,6 +223,7 @@ class MentionAutocompleteView extends ChangeNotifier {

final PerAccountStore store;
final Narrow narrow;
final List<User> sortedUsers;

MentionAutocompleteQuery? get query => _query;
MentionAutocompleteQuery? _query;
Expand All @@ -208,15 +234,6 @@ class MentionAutocompleteView extends ChangeNotifier {
}
}

/// Recompute user results for the current query, if any.
///
/// Called in particular when we get a [RealmUserEvent].
void refreshStaleUserResults() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit in commit message:

(To read more about this decision, check out https://github.com/zulip/zulip-flutter/pull/693#pullrequestreview-2091696195)

That last line is so long that it'll almost always wrap. To mitigate it, put the URL on its own line, like:

To read more about this decision, see:
  https://github.com/zulip/zulip-flutter/pull/693#pullrequestreview-2091696195

if (_query != null) {
_startSearch(_query!);
}
}

Comment on lines -211 to -219
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

autocomplete: Remove the refreshStaleUserResults method

One test in autocomplete_test.dart doesn't pass at this commit:

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

Let's fix it by adding these lines after the await store.addUser:

    check(done).isFalse();
    await Future(() {});

as I suggested in #693 (review) .

This test does get removed by the next commit in the series, when the behavior it's intended to test is removed. But a failure here would break our invariant that no commits have failing tests. That invariant helps make commits easier to read, both in review and when reading the project's history. In particular, it lets the reader use a consistent standard of broken vs. non-broken behavior when understanding what a commit does. Sometimes we need to read many commits at a time, to investigate the origin of some mysterious bug, and in that case we save a lot of mental energy by not having to decide if we agree that the test failures in a (maybe months-old) commit are OK, or if they're actually a sign that something went wrong.

/// Called when the app is reassembled during debugging, e.g. for hot reload.
///
/// This will redo the search from scratch for the current query, if any.
Expand All @@ -230,18 +247,7 @@ class MentionAutocompleteView extends ChangeNotifier {
List<MentionAutocompleteResult> _results = [];

Future<void> _startSearch(MentionAutocompleteQuery query) async {
List<MentionAutocompleteResult>? newResults;

while (true) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'.

try {
newResults = await _computeResults(query);
break;
} on ConcurrentModificationError {
// Retry
// TODO backoff?
}
}

final newResults = await _computeResults(query);
if (newResults == null) {
// Query was old; new search is in progress. Or, no listeners to notify.
return;
Expand All @@ -253,9 +259,7 @@ class MentionAutocompleteView extends ChangeNotifier {

Future<List<MentionAutocompleteResult>?> _computeResults(MentionAutocompleteQuery query) async {
final List<MentionAutocompleteResult> results = [];
final Iterable<User> users = store.users.values;

final iterator = users.iterator;
final iterator = sortedUsers.iterator;
bool isDone = false;
while (!isDone) {
// CPU perf: End this task; enqueue a new one for resuming this work
Expand All @@ -266,7 +270,7 @@ class MentionAutocompleteView extends ChangeNotifier {
}

for (int i = 0; i < 1000; i++) {
if (!iterator.moveNext()) { // Can throw ConcurrentModificationError
if (!iterator.moveNext()) {
isDone = true;
break;
}
Expand All @@ -277,7 +281,7 @@ class MentionAutocompleteView extends ChangeNotifier {
}
}
}
return results; // TODO(#228) sort for most relevant first
return results;
Comment on lines -280 to +284
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?).

}
}

Expand Down
38 changes: 37 additions & 1 deletion lib/model/recent_dm_conversations.dart
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import 'dart:math';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)


import 'package:collection/collection.dart';
import 'package:flutter/foundation.dart';

Expand All @@ -19,16 +21,29 @@ class RecentDmConversationsView extends ChangeNotifier {
DmNarrow.ofRecentDmConversation(conversation, selfUserId: selfUserId),
conversation.maxMessageId,
)).toList()..sort((a, b) => -a.value.compareTo(b.value));

final latestMessagesByRecipient = <int, int>{};
for (final entry in entries) {
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.
latestMessagesByRecipient.putIfAbsent(userId, () => maxMessageId);
}
}

return RecentDmConversationsView._(
map: Map.fromEntries(entries),
sorted: QueueList.from(entries.map((e) => e.key)),
latestMessagesByRecipient: latestMessagesByRecipient,
selfUserId: selfUserId,
);
}

RecentDmConversationsView._({
required this.map,
required this.sorted,
required this.latestMessagesByRecipient,
required this.selfUserId,
});

Expand All @@ -38,6 +53,15 @@ class RecentDmConversationsView extends ChangeNotifier {
/// The [DmNarrow] keys of [map], sorted by latest message descending.
final QueueList<DmNarrow> sorted;

/// 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> latestMessagesByRecipient;

final int selfUserId;

/// Insert the key at the proper place in [sorted].
Expand All @@ -58,7 +82,7 @@ class RecentDmConversationsView extends ChangeNotifier {
}
}

/// Handle [MessageEvent], updating [map] and [sorted].
/// Handle [MessageEvent], updating [map], [sorted], and [latestMessagesByRecipient].
///
/// Can take linear time in general. That sounds inefficient...
/// but it's what the webapp does, so must not be catastrophic. 🤷
Expand Down Expand Up @@ -89,6 +113,8 @@ class RecentDmConversationsView extends ChangeNotifier {
return;
}
final key = DmNarrow.ofMessage(message, selfUserId: selfUserId);

// Update [map] and [sorted].
final prev = map[key];
if (prev == null) {
// The conversation is new. Add to both `map` and `sorted`.
Expand Down Expand Up @@ -117,6 +143,16 @@ class RecentDmConversationsView extends ChangeNotifier {
_insertSorted(key, message.id);
}
}

// Update [latestMessagesByRecipient].
for (final recipient in key.otherRecipientIds) {
latestMessagesByRecipient.update(
recipient,
(latestMessageId) => max(message.id, latestMessageId),
ifAbsent: () => message.id,
);
}

notifyListeners();
}

Expand Down
1 change: 0 additions & 1 deletion lib/model/store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,6 @@ class PerAccountStore extends ChangeNotifier with StreamStore, MessageStore {
} else if (event is RealmUserAddEvent) {
assert(debugLog("server event: realm_user/add"));
users[event.person.userId] = event.person;
autocompleteViewManager.handleRealmUserAddEvent(event);
notifyListeners();
} else if (event is RealmUserRemoveEvent) {
assert(debugLog("server event: realm_user/remove"));
Expand Down
Loading
Loading