-
Notifications
You must be signed in to change notification settings - Fork 306
autocomplete: Sort user-mention autocomplete results #608
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
Changes from all commits
967ebca
0621c0e
2b825ab
09d3351
4c93f17
35b3928
2996ce5
cc715e6
5448993
2b5711b
c710a5c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -125,26 +125,37 @@ class AutocompleteViewManager { | |
assert(removed); | ||
} | ||
|
||
void handleRealmUserAddEvent(RealmUserAddEvent event) { | ||
/// Recomputes the autocomplete results for users. | ||
/// | ||
/// Calls [MentionAutocompleteView.refreshStaleUserResults] for all that are registered. | ||
void _refreshStaleUserResults() { | ||
for (final view in _mentionAutocompleteViews) { | ||
view.refreshStaleUserResults(); | ||
} | ||
} | ||
|
||
void handleRealmUserAddEvent(RealmUserAddEvent event) { | ||
_refreshStaleUserResults(); | ||
} | ||
|
||
void handleRealmUserRemoveEvent(RealmUserRemoveEvent event) { | ||
for (final view in _mentionAutocompleteViews) { | ||
view.refreshStaleUserResults(); | ||
} | ||
_refreshStaleUserResults(); | ||
autocompleteDataCache.invalidateUser(event.userId); | ||
} | ||
|
||
void handleRealmUserUpdateEvent(RealmUserUpdateEvent event) { | ||
for (final view in _mentionAutocompleteViews) { | ||
view.refreshStaleUserResults(); | ||
} | ||
_refreshStaleUserResults(); | ||
autocompleteDataCache.invalidateUser(event.userId); | ||
} | ||
|
||
void handleMessageEvent(MessageEvent event) { | ||
_refreshStaleUserResults(); | ||
} | ||
|
||
void handleOlderMessages() { | ||
_refreshStaleUserResults(); | ||
} | ||
|
||
/// Called when the app is reassembled during debugging, e.g. for hot reload. | ||
/// | ||
/// Calls [MentionAutocompleteView.reassemble] for all that are registered. | ||
|
@@ -190,6 +201,7 @@ class MentionAutocompleteView extends ChangeNotifier { | |
@override | ||
void dispose() { | ||
store.autocompleteViewManager.unregisterMentionAutocomplete(this); | ||
_sortedUsers = null; | ||
// We cancel in-progress computations by checking [hasListeners] between tasks. | ||
// After [super.dispose] is called, [hasListeners] returns false. | ||
// TODO test that logic (may involve detecting an unhandled Future rejection; how?) | ||
|
@@ -213,6 +225,7 @@ class MentionAutocompleteView extends ChangeNotifier { | |
/// Called in particular when we get a [RealmUserEvent]. | ||
void refreshStaleUserResults() { | ||
if (_query != null) { | ||
_sortedUsers = null; | ||
_startSearch(_query!); | ||
} | ||
} | ||
|
@@ -222,6 +235,7 @@ class MentionAutocompleteView extends ChangeNotifier { | |
/// This will redo the search from scratch for the current query, if any. | ||
void reassemble() { | ||
if (_query != null) { | ||
_sortedUsers = null; | ||
_startSearch(_query!); | ||
} | ||
} | ||
|
@@ -251,22 +265,106 @@ class MentionAutocompleteView extends ChangeNotifier { | |
notifyListeners(); | ||
} | ||
|
||
List<User>? _sortedUsers; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following up on #608 (review) :
A further improvement to the sequence of commits would be to pull out the changes that set up and maintain this One reason that's helpful is that it makes it easier to reorder the commits that add comparisons, for example in order to merge some that are ready while we're still working on others. Another is that the rest of these mechanics, outside the particular comparison itself, have some pretty subtle aspects. That makes it useful to have them in an isolated commit of their own, the better to read and understand them. |
||
|
||
int compareByRelevance({ | ||
required User userA, | ||
required User userB, | ||
required int? streamId, | ||
required String? topic, | ||
}) { | ||
// TODO(#234): give preference to "all", "everyone" or "stream". | ||
|
||
// TODO(#618): give preference to subscribed users first. | ||
|
||
if (streamId != null) { | ||
final conversationPrecedence = store.recentSenders.compareByRecency( | ||
userA, | ||
userB, | ||
streamId: streamId, | ||
topic: topic); | ||
if (conversationPrecedence != 0) { | ||
return conversationPrecedence; | ||
} | ||
} | ||
final dmPrecedence = store.recentDmConversationsView.compareByDms(userA, userB); | ||
if (dmPrecedence != 0) { | ||
return dmPrecedence; | ||
} | ||
|
||
if (!userA.isBot && userB.isBot) { | ||
return -1; | ||
} else if (userA.isBot && !userB.isBot) { | ||
return 1; | ||
} | ||
|
||
final userAName = store.autocompleteViewManager.autocompleteDataCache | ||
.normalizedNameForUser(userA); | ||
final userBName = store.autocompleteViewManager.autocompleteDataCache | ||
.normalizedNameForUser(userB); | ||
return userAName.compareTo(userBName); | ||
} | ||
|
||
List<User> sortByRelevance({ | ||
required List<User> users, | ||
required Narrow narrow, | ||
}) { | ||
switch (narrow) { | ||
case StreamNarrow(): | ||
users.sort((userA, userB) => compareByRelevance( | ||
userA: userA, | ||
userB: userB, | ||
streamId: narrow.streamId, | ||
topic: null)); | ||
case TopicNarrow(): | ||
users.sort((userA, userB) => compareByRelevance( | ||
userA: userA, | ||
userB: userB, | ||
streamId: narrow.streamId, | ||
topic: narrow.topic)); | ||
case DmNarrow(): | ||
users.sort((userA, userB) => compareByRelevance( | ||
userA: userA, | ||
userB: userB, | ||
streamId: null, | ||
topic: null)); | ||
case AllMessagesNarrow(): | ||
// do nothing in this case for now | ||
} | ||
return users; | ||
} | ||
|
||
void _sortUsers() { | ||
final users = store.users.values.toList(); | ||
_sortedUsers = sortByRelevance(users: users, narrow: narrow); | ||
} | ||
|
||
Future<List<MentionAutocompleteResult>?> _computeResults(MentionAutocompleteQuery query) async { | ||
final List<MentionAutocompleteResult> results = []; | ||
final Iterable<User> users = store.users.values; | ||
|
||
final iterator = users.iterator; | ||
if (_sortedUsers == null) { | ||
_sortUsers(); | ||
} | ||
sm-sayedi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
final sortedUsers = _sortedUsers!; | ||
final iterator = sortedUsers.iterator; | ||
bool isDone = false; | ||
while (!isDone) { | ||
// CPU perf: End this task; enqueue a new one for resuming this work | ||
await Future(() {}); | ||
|
||
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(); | ||
} | ||
|
||
if (query != _query || !hasListeners) { // false if [dispose] has been called. | ||
return null; | ||
} | ||
|
||
for (int i = 0; i < 1000; i++) { | ||
if (!iterator.moveNext()) { // Can throw ConcurrentModificationError | ||
if (!iterator.moveNext()) { | ||
isDone = true; | ||
break; | ||
} | ||
|
@@ -277,7 +375,7 @@ class MentionAutocompleteView extends ChangeNotifier { | |
} | ||
} | ||
} | ||
return results; // TODO(#228) sort for most relevant first | ||
return results; | ||
} | ||
} | ||
|
||
|
@@ -337,13 +435,21 @@ class MentionAutocompleteQuery { | |
} | ||
|
||
class AutocompleteDataCache { | ||
final Map<int, String> _normalizedNamesByUser = {}; | ||
|
||
/// The lowercase `fullName` of [user]. | ||
String normalizedNameForUser(User user) { | ||
return _normalizedNamesByUser[user.userId] ??= user.fullName.toLowerCase(); | ||
} | ||
|
||
final Map<int, List<String>> _nameWordsByUser = {}; | ||
|
||
List<String> nameWordsForUser(User user) { | ||
return _nameWordsByUser[user.userId] ??= user.fullName.toLowerCase().split(' '); | ||
return _nameWordsByUser[user.userId] ??= normalizedNameForUser(user).split(' '); | ||
} | ||
|
||
void invalidateUser(int userId) { | ||
_normalizedNamesByUser.remove(userId); | ||
_nameWordsByUser.remove(userId); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -381,6 +381,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { | |
for (final message in result.messages) { | ||
if (_messageVisible(message)) { | ||
_addMessage(message); | ||
store.recentSenders.handleMessage(message); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The ideal version of this would go on top of a central message store, #648, so that This PR doesn't need to block on that one; for now, it's fine to add these calls in a more ad-hoc way like this. The current version of this PR doesn't call this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure whether to add it inside the if block or outside!! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question. What does the dartdoc on that Based on that, what would be the consequence of having that condition control whether to make this call? And what would be the consequence of ignoring that condition when making this call? |
||
} | ||
} | ||
_fetched = true; | ||
|
@@ -417,6 +418,11 @@ class MessageListView with ChangeNotifier, _MessageSequence { | |
? result.messages // Avoid unnecessarily copying the list. | ||
: result.messages.where(_messageVisible); | ||
|
||
for (final message in fetchedMessages) { | ||
store.recentSenders.handleMessage(message); | ||
} | ||
store.autocompleteViewManager.handleOlderMessages(); | ||
|
||
_insertAllMessages(0, fetchedMessages); | ||
_haveOldest = result.foundOldest; | ||
} finally { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if it is necessary to pass the
event
parameter as it is not used for now. I just followed the code forhandleRealmUserAddEvent(RealmUserAddEvent event)
method.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, following that pattern is good. This is also the pattern you'll find at the call site in
PerAccountStore.handleEvent
.