Skip to content

Commit 22953d3

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 a7d97d5 commit 22953d3

File tree

5 files changed

+299
-22
lines changed

5 files changed

+299
-22
lines changed

lib/model/autocomplete.dart

+110-3
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,7 @@ class MentionAutocompleteView extends ChangeNotifier {
183183
@override
184184
void dispose() {
185185
store.autocompleteViewManager.unregisterMentionAutocomplete(this);
186+
_sortedUsers = null;
186187
// We cancel in-progress computations by checking [hasListeners] between tasks.
187188
// After [super.dispose] is called, [hasListeners] returns false.
188189
// TODO test that logic (may involve detecting an unhandled Future rejection; how?)
@@ -206,6 +207,7 @@ class MentionAutocompleteView extends ChangeNotifier {
206207
/// Called in particular when we get a [RealmUserEvent].
207208
void refreshStaleUserResults() {
208209
if (_query != null) {
210+
_sortedUsers = null;
209211
_startSearch(_query!);
210212
}
211213
}
@@ -244,11 +246,116 @@ class MentionAutocompleteView extends ChangeNotifier {
244246
notifyListeners();
245247
}
246248

249+
List<User>? _sortedUsers;
250+
251+
int compareByDms(User userA, User userB) {
252+
final aRecencyIndex = store.recentDmConversationsView.getRecencyIndex(userA.userId);
253+
final bRecencyIndex = store.recentDmConversationsView.getRecencyIndex(userB.userId);
254+
255+
if (aRecencyIndex > bRecencyIndex) {
256+
return -1;
257+
} else if (bRecencyIndex > aRecencyIndex) {
258+
return 1;
259+
}
260+
261+
final aIsPartner = store.recentDmConversationsView.isPartner(userA.userId);
262+
final bIsPartner = store.recentDmConversationsView.isPartner(userB.userId);
263+
264+
// This code will never run except in the rare case that one has no
265+
// recent DM message history with a user, but does have some older
266+
// message history.
267+
if (aIsPartner && !bIsPartner) {
268+
return -1;
269+
} else if (bIsPartner && !aIsPartner) {
270+
return 1;
271+
}
272+
273+
if (!userA.isBot && userB.isBot) {
274+
return -1;
275+
} else if (userA.isBot && !userB.isBot) {
276+
return 1;
277+
}
278+
279+
if (userA.fullName.toLowerCase().compareTo(
280+
userB.fullName.toLowerCase()) < 0) {
281+
return -1;
282+
}
283+
return 1;
284+
}
285+
286+
int compareByRelevance({
287+
required User userA,
288+
required User userB,
289+
int Function(User, User)? compareByCurrentConversation,
290+
}) {
291+
// TODO(#234): give preference to "all", "everyone" or "stream".
292+
293+
// TODO(#618): give preference to subscribed users first.
294+
295+
if (compareByCurrentConversation != null) {
296+
final preference = compareByCurrentConversation(userA, userB);
297+
if (preference != 0) {
298+
return preference;
299+
}
300+
}
301+
302+
return compareByDms(userA, userB);
303+
}
304+
305+
List<User> sortByRelevance({
306+
required List<User> users,
307+
required int? currentStreamId,
308+
required String? currentTopic,
309+
}) {
310+
// Sorting for DMs narrow
311+
if (currentStreamId == null) {
312+
users.sort((userA, userB) => compareByRelevance(
313+
userA: userA,
314+
userB: userB));
315+
} else { // Sorting for stream/topic narrow
316+
users.sort((userA, userB) => compareByRelevance(
317+
userA: userA,
318+
userB: userB,
319+
compareByCurrentConversation: (userA, userB) => store.recentSenders.compareByRecency(
320+
userA: userA,
321+
userB: userB,
322+
streamId: currentStreamId,
323+
topic: currentTopic)));
324+
}
325+
return users;
326+
}
327+
328+
void _sortUsers() {
329+
final users = store.users.values.toList();
330+
switch(narrow) {
331+
case StreamNarrow():
332+
_sortedUsers = sortByRelevance(
333+
users: users,
334+
currentStreamId: (narrow as StreamNarrow).streamId,
335+
currentTopic: null);
336+
case TopicNarrow():
337+
_sortedUsers = sortByRelevance(
338+
users: users,
339+
currentStreamId: (narrow as TopicNarrow).streamId,
340+
currentTopic: (narrow as TopicNarrow).topic);
341+
case DmNarrow():
342+
_sortedUsers = sortByRelevance(
343+
users: users,
344+
currentStreamId: null,
345+
currentTopic: null);
346+
case AllMessagesNarrow():
347+
_sortedUsers = []; // TODO: sort (maybe in future)
348+
}
349+
}
350+
247351
Future<List<MentionAutocompleteResult>?> _computeResults(MentionAutocompleteQuery query) async {
248352
final List<MentionAutocompleteResult> results = [];
249-
final Iterable<User> users = store.users.values;
250353

251-
final iterator = users.iterator;
354+
if (_sortedUsers == null) {
355+
_sortUsers();
356+
}
357+
358+
final iterator = _sortedUsers!.iterator;
252359
bool isDone = false;
253360
while (!isDone) {
254361
// CPU perf: End this task; enqueue a new one for resuming this work
@@ -270,7 +377,7 @@ class MentionAutocompleteView extends ChangeNotifier {
270377
}
271378
}
272379
}
273-
return results; // TODO(#228) sort for most relevant first
380+
return results;
274381
}
275382
}
276383

lib/model/message_list.dart

+1
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.processStreamMessage(message);
384385
}
385386
}
386387
_fetched = true;

lib/model/recent_dm_conversations.dart

+37-19
Original file line numberDiff line numberDiff line change
@@ -11,35 +11,53 @@ import 'narrow.dart';
1111
/// This maintains the list of recent DM conversations,
1212
/// plus additional data in order to efficiently maintain the list.
1313
class RecentDmConversationsView extends ChangeNotifier {
14-
factory RecentDmConversationsView({
14+
RecentDmConversationsView({
1515
required List<RecentDmConversation> initial,
16-
required selfUserId,
16+
required this.selfUserId,
1717
}) {
18-
final entries = initial.map((conversation) => MapEntry(
19-
DmNarrow.ofRecentDmConversation(conversation, selfUserId: selfUserId),
20-
conversation.maxMessageId,
21-
)).toList()..sort((a, b) => -a.value.compareTo(b.value));
22-
return RecentDmConversationsView._(
23-
map: Map.fromEntries(entries),
24-
sorted: QueueList.from(entries.map((e) => e.key)),
25-
selfUserId: selfUserId,
26-
);
27-
}
18+
final entries = <MapEntry<DmNarrow, int>>[];
19+
for (final conversation in initial) {
20+
entries.add(MapEntry(
21+
DmNarrow.ofRecentDmConversation(conversation, selfUserId: selfUserId),
22+
conversation.maxMessageId));
2823

29-
RecentDmConversationsView._({
30-
required this.map,
31-
required this.sorted,
32-
required this.selfUserId,
33-
});
24+
for (final userId in conversation.userIds) {
25+
if (userId == selfUserId) {
26+
continue;
27+
}
28+
_addRecencyIndex(userId: userId, maxId: conversation.maxMessageId);
29+
}
30+
}
31+
entries.sort((a, b) => -a.value.compareTo(b.value));
32+
map = Map.fromEntries(entries);
33+
sorted = QueueList.from(entries.map((e) => e.key));
34+
}
3435

3536
/// The latest message ID in each conversation.
36-
final Map<DmNarrow, int> map;
37+
late final Map<DmNarrow, int> map;
3738

3839
/// The [DmNarrow] keys of the map, sorted by latest message descending.
39-
final QueueList<DmNarrow> sorted;
40+
late final QueueList<DmNarrow> sorted;
4041

4142
final int selfUserId;
4243

44+
// The ID of the latest messages exchanged with other users.
45+
final Map<int, int> _dmRecencyData = {};
46+
47+
/// The ID of the latest message exchanged with this user.
48+
///
49+
/// Returns -1 if there has been no DM message exchanged ever.
50+
int getRecencyIndex(final int userId) => _dmRecencyData[userId] ?? -1;
51+
52+
void _addRecencyIndex({required int userId, required int maxId}) {
53+
final oldMaxId = _dmRecencyData[userId] ?? -1;
54+
if (maxId > oldMaxId) {
55+
_dmRecencyData[userId] = maxId;
56+
}
57+
}
58+
59+
bool isPartner(final int userId) => getRecencyIndex(userId) != -1;
60+
4361
/// Insert the key at the proper place in [sorted].
4462
///
4563
/// Optimized, taking O(1) time, for the case where that place is the start,

lib/model/recent_senders.dart

+146
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
2+
import 'dart:math' as math;
3+
4+
import '../api/model/model.dart';
5+
6+
extension Max<T extends num> on Iterable<T> {
7+
/// Finds the maximum number in an [Iterable].
8+
///
9+
/// Returns null if the [Iterable] is empty.
10+
T? max() => isNotEmpty ? reduce(math.max) : null;
11+
}
12+
13+
/// A data structure to keep track of message ids.
14+
class IdTracker {
15+
final Set<int> _ids = {};
16+
17+
int _maxId = -1;
18+
19+
void add(int id) {
20+
_ids.add(id);
21+
22+
if (id > _maxId) {
23+
_maxId = id;
24+
}
25+
}
26+
27+
// TODO: remove
28+
29+
int maxId() {
30+
if (_maxId == -1) {
31+
_maxId = _ids.max() ?? -1;
32+
}
33+
return _maxId;
34+
}
35+
36+
@override
37+
String toString() {
38+
return _ids.toString();
39+
}
40+
}
41+
42+
/// A data structure to keep track of stream and topic messages.
43+
///
44+
/// The owner should call [clear] in order to free resources.
45+
class RecentSenders {
46+
// streamSenders[streamId][senderId] = IdTracker
47+
final Map<int, Map<int, IdTracker>> _streamSenders = {};
48+
49+
// topicSenders[streamId][topic][senderId] = IdTracker
50+
final Map<int, Map<String, Map<int, IdTracker>>> _topicSenders = {};
51+
52+
void clear() {
53+
_streamSenders.clear();
54+
_topicSenders.clear();
55+
}
56+
57+
int maxIdForStreamSender({required int streamId, required int senderId}) {
58+
return _streamSenders[streamId]?[senderId]?.maxId() ?? -1;
59+
}
60+
61+
int maxIdForStreamTopicSender({
62+
required int streamId,
63+
required String topic,
64+
required int senderId,
65+
}) {
66+
topic = topic.toLowerCase();
67+
return _topicSenders[streamId]?[topic]?[senderId]?.maxId() ?? -1;
68+
}
69+
70+
void addStreamMessage({
71+
required int streamId,
72+
required int senderId,
73+
required messageId,
74+
}) {
75+
final senderMap = _streamSenders[streamId] ?? {};
76+
final idTracker = senderMap[senderId] ?? IdTracker();
77+
_streamSenders[streamId] = senderMap;
78+
senderMap[senderId] = idTracker;
79+
idTracker.add(messageId);
80+
}
81+
82+
void addTopicMessage({
83+
required int streamId,
84+
required String topic,
85+
required int senderId,
86+
required messageId,
87+
}) {
88+
topic = topic.toLowerCase();
89+
final topicMap = _topicSenders[streamId] ?? {};
90+
final senderMap = topicMap[topic] ?? {};
91+
final idTracker = senderMap[senderId] ?? IdTracker();
92+
_topicSenders[streamId] = topicMap;
93+
topicMap[topic] = senderMap;
94+
senderMap[senderId] = idTracker;
95+
idTracker.add(messageId);
96+
}
97+
98+
void processStreamMessage(Message message) {
99+
if (message is! StreamMessage) {
100+
return;
101+
}
102+
103+
final streamId = message.streamId;
104+
final topic = message.subject;
105+
final senderId = message.senderId;
106+
final messageId = message.id;
107+
108+
addStreamMessage(
109+
streamId: streamId, senderId: senderId, messageId: messageId);
110+
addTopicMessage(
111+
streamId: streamId,
112+
topic: topic,
113+
senderId: senderId,
114+
messageId: messageId);
115+
}
116+
117+
// TODO: removeTopicMessage
118+
119+
int compareByRecency({
120+
required User userA,
121+
required User userB,
122+
required int streamId,
123+
// TODO(#493): may turn this into a non-nullable string.
124+
required String? topic,
125+
}) {
126+
int aMessageId, bMessageId;
127+
128+
if (topic != null) {
129+
aMessageId = maxIdForStreamTopicSender(
130+
streamId: streamId, topic: topic, senderId: userA.userId);
131+
bMessageId = maxIdForStreamTopicSender(
132+
streamId: streamId, topic: topic, senderId: userB.userId);
133+
134+
if (aMessageId != bMessageId) {
135+
return bMessageId - aMessageId;
136+
}
137+
}
138+
139+
aMessageId =
140+
maxIdForStreamSender(streamId: streamId, senderId: userA.userId);
141+
bMessageId =
142+
maxIdForStreamSender(streamId: streamId, senderId: userB.userId);
143+
144+
return bMessageId - aMessageId;
145+
}
146+
}

0 commit comments

Comments
 (0)