Skip to content

Commit 706f3c6

Browse files
committed
autocomplete: Sort user-mention autocomplete results
The @-mention autocomplete shows relevant results in the following order of priority: * Recent conversation users * Recent DM users * Alphabetical order Fixes: zulip#228
1 parent 8619e8e commit 706f3c6

11 files changed

+506
-23
lines changed

lib/model/autocomplete.dart

Lines changed: 119 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,18 @@ class AutocompleteViewManager {
145145
autocompleteDataCache.invalidateUser(event.userId);
146146
}
147147

148+
void handleMessageEvent(MessageEvent event) {
149+
for (final view in _mentionAutocompleteViews) {
150+
view.refreshStaleUserResults();
151+
}
152+
}
153+
154+
void handleOlderMessages() {
155+
for (final view in _mentionAutocompleteViews) {
156+
view.refreshStaleUserResults();
157+
}
158+
}
159+
148160
/// Called when the app is reassembled during debugging, e.g. for hot reload.
149161
///
150162
/// Calls [MentionAutocompleteView.reassemble] for all that are registered.
@@ -190,6 +202,7 @@ class MentionAutocompleteView extends ChangeNotifier {
190202
@override
191203
void dispose() {
192204
store.autocompleteViewManager.unregisterMentionAutocomplete(this);
205+
_sortedUsers = null;
193206
// We cancel in-progress computations by checking [hasListeners] between tasks.
194207
// After [super.dispose] is called, [hasListeners] returns false.
195208
// TODO test that logic (may involve detecting an unhandled Future rejection; how?)
@@ -213,6 +226,7 @@ class MentionAutocompleteView extends ChangeNotifier {
213226
/// Called in particular when we get a [RealmUserEvent].
214227
void refreshStaleUserResults() {
215228
if (_query != null) {
229+
_sortedUsers = null;
216230
_startSearch(_query!);
217231
}
218232
}
@@ -222,6 +236,7 @@ class MentionAutocompleteView extends ChangeNotifier {
222236
/// This will redo the search from scratch for the current query, if any.
223237
void reassemble() {
224238
if (_query != null) {
239+
_sortedUsers = null;
225240
_startSearch(_query!);
226241
}
227242
}
@@ -251,11 +266,95 @@ class MentionAutocompleteView extends ChangeNotifier {
251266
notifyListeners();
252267
}
253268

269+
List<User>? _sortedUsers;
270+
271+
int compareByRelevance({
272+
required User userA,
273+
required User userB,
274+
required int? streamId,
275+
required String? topic,
276+
}) {
277+
// TODO(#234): give preference to "all", "everyone" or "stream".
278+
279+
// TODO(#618): give preference to subscribed users first.
280+
281+
if (streamId != null) {
282+
final conversationPrecedence = store.recentSenders.compareByRecency(
283+
userA,
284+
userB,
285+
streamId: streamId,
286+
topic: topic);
287+
if (conversationPrecedence != 0) {
288+
return conversationPrecedence;
289+
}
290+
}
291+
292+
final dmPrecedence = store.recentDmConversationsView.compareByDms(userA, userB);
293+
if (dmPrecedence != 0) {
294+
return dmPrecedence;
295+
}
296+
297+
if (!userA.isBot && userB.isBot) {
298+
return -1;
299+
} else if (userA.isBot && !userB.isBot) {
300+
return 1;
301+
}
302+
303+
final userAName = store.autocompleteViewManager.autocompleteDataCache
304+
.normalizedNameForUser(userA);
305+
final userBName = store.autocompleteViewManager.autocompleteDataCache
306+
.normalizedNameForUser(userB);
307+
return userAName.compareTo(userBName);
308+
}
309+
310+
List<User> sortByRelevance({
311+
required List<User> users,
312+
required Narrow narrow,
313+
}) {
314+
switch (narrow) {
315+
case StreamNarrow():
316+
users.sort((userA, userB) => compareByRelevance(
317+
userA: userA,
318+
userB: userB,
319+
streamId: narrow.streamId,
320+
topic: null));
321+
case TopicNarrow():
322+
users.sort((userA, userB) => compareByRelevance(
323+
userA: userA,
324+
userB: userB,
325+
streamId: narrow.streamId,
326+
topic: narrow.topic));
327+
case DmNarrow():
328+
users.sort((userA, userB) => compareByRelevance(
329+
userA: userA,
330+
userB: userB,
331+
streamId: null,
332+
topic: null));
333+
case AllMessagesNarrow():
334+
// do nothing in this case for now
335+
}
336+
return users;
337+
}
338+
339+
void _sortUsers() {
340+
final users = store.users.values.toList();
341+
_sortedUsers = sortByRelevance(users: users, narrow: narrow);
342+
}
343+
344+
bool _areUsersModified = false;
345+
void _usersModified() {
346+
_areUsersModified = true;
347+
}
348+
254349
Future<List<MentionAutocompleteResult>?> _computeResults(MentionAutocompleteQuery query) async {
255350
final List<MentionAutocompleteResult> results = [];
256-
final Iterable<User> users = store.users.values;
257351

258-
final iterator = users.iterator;
352+
if (_sortedUsers == null) {
353+
_sortUsers();
354+
}
355+
356+
final iterator = _sortedUsers!.iterator;
357+
store.users.addListener(_usersModified);
259358
bool isDone = false;
260359
while (!isDone) {
261360
// CPU perf: End this task; enqueue a new one for resuming this work
@@ -266,7 +365,12 @@ class MentionAutocompleteView extends ChangeNotifier {
266365
}
267366

268367
for (int i = 0; i < 1000; i++) {
269-
if (!iterator.moveNext()) { // Can throw ConcurrentModificationError
368+
if (_areUsersModified) {
369+
_areUsersModified = false;
370+
_sortedUsers = null;
371+
throw ConcurrentModificationError();
372+
}
373+
if (!iterator.moveNext()) {
270374
isDone = true;
271375
break;
272376
}
@@ -277,7 +381,8 @@ class MentionAutocompleteView extends ChangeNotifier {
277381
}
278382
}
279383
}
280-
return results; // TODO(#228) sort for most relevant first
384+
store.users.removeListener(_usersModified);
385+
return results;
281386
}
282387
}
283388

@@ -337,13 +442,22 @@ class MentionAutocompleteQuery {
337442
}
338443

339444
class AutocompleteDataCache {
445+
final Map<int, String> _normalizedNamesByUser = {};
446+
447+
/// The lowercase `fullName` of [user].
448+
String normalizedNameForUser(User user) {
449+
return _normalizedNamesByUser[user.userId] ??= user.fullName.toLowerCase();
450+
}
451+
340452
final Map<int, List<String>> _nameWordsByUser = {};
341453

342454
List<String> nameWordsForUser(User user) {
343-
return _nameWordsByUser[user.userId] ??= user.fullName.toLowerCase().split(' ');
455+
return _nameWordsByUser[user.userId] ??=
456+
normalizedNameForUser(user).split(' ');
344457
}
345458

346459
void invalidateUser(int userId) {
460+
_normalizedNamesByUser.remove(userId);
347461
_nameWordsByUser.remove(userId);
348462
}
349463
}

lib/model/message_list.dart

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -381,6 +381,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
381381
for (final message in result.messages) {
382382
if (_messageVisible(message)) {
383383
_addMessage(message);
384+
store.recentSenders.handleMessage(message);
384385
}
385386
}
386387
_fetched = true;
@@ -417,6 +418,11 @@ class MessageListView with ChangeNotifier, _MessageSequence {
417418
? result.messages // Avoid unnecessarily copying the list.
418419
: result.messages.where(_messageVisible);
419420

421+
for (final message in fetchedMessages) {
422+
store.recentSenders.handleMessage(message);
423+
}
424+
store.autocompleteViewManager.handleOlderMessages();
425+
420426
_insertAllMessages(0, fetchedMessages);
421427
_haveOldest = result.foundOldest;
422428
} finally {

lib/model/recent_dm_conversations.dart

Lines changed: 40 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,16 +19,30 @@ class RecentDmConversationsView extends ChangeNotifier {
1919
DmNarrow.ofRecentDmConversation(conversation, selfUserId: selfUserId),
2020
conversation.maxMessageId,
2121
)).toList()..sort((a, b) => -a.value.compareTo(b.value));
22+
final map = Map.fromEntries(entries);
23+
final sorted = QueueList.from(entries.map((e) => e.key));
24+
25+
final msgsByUser = <int, int>{};
26+
for (final dmNarrow in sorted) {
27+
for (final userId in dmNarrow.otherRecipientIds) {
28+
// only take the latest message of a user among all the conversations.
29+
if (!msgsByUser.containsKey(userId)) {
30+
msgsByUser[userId] = map[dmNarrow]!;
31+
}
32+
}
33+
}
2234
return RecentDmConversationsView._(
23-
map: Map.fromEntries(entries),
24-
sorted: QueueList.from(entries.map((e) => e.key)),
35+
map: map,
36+
sorted: sorted,
37+
latestMessagesByRecipient: msgsByUser,
2538
selfUserId: selfUserId,
2639
);
2740
}
2841

2942
RecentDmConversationsView._({
3043
required this.map,
3144
required this.sorted,
45+
required this.latestMessagesByRecipient,
3246
required this.selfUserId,
3347
});
3448

@@ -38,8 +52,24 @@ class RecentDmConversationsView extends ChangeNotifier {
3852
/// The [DmNarrow] keys of [map], sorted by latest message descending.
3953
final QueueList<DmNarrow> sorted;
4054

55+
/// Map from user ID to the latest message ID in any conversation with the user.
56+
///
57+
/// Both 1:1 and group DM conversations are considered.
58+
/// The self-user ID is excluded even if there is a self-DM conversation.
59+
///
60+
/// (The identified message was not necessarily sent by the identified user;
61+
/// it might have been sent by anyone in its conversation.)
62+
final Map<int, int> latestMessagesByRecipient;
63+
4164
final int selfUserId;
4265

66+
int compareByDms(User userA, User userB) {
67+
final aLatestMessageId = latestMessagesByRecipient[userA.userId] ?? -1;
68+
final bLatestMessageId = latestMessagesByRecipient[userB.userId] ?? -1;
69+
70+
return bLatestMessageId.compareTo(aLatestMessageId);
71+
}
72+
4373
/// Insert the key at the proper place in [sorted].
4474
///
4575
/// Optimized, taking O(1) time, for the case where that place is the start,
@@ -117,6 +147,14 @@ class RecentDmConversationsView extends ChangeNotifier {
117147
_insertSorted(key, message.id);
118148
}
119149
}
150+
for (final recipient in key.otherRecipientIds) {
151+
latestMessagesByRecipient.update(
152+
recipient,
153+
(latestMessageId) =>
154+
message.id > latestMessageId ? message.id : latestMessageId,
155+
ifAbsent: () => message.id,
156+
);
157+
}
120158
notifyListeners();
121159
}
122160

0 commit comments

Comments
 (0)