-
Notifications
You must be signed in to change notification settings - Fork 436
Topic link autocomplete #2137
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
base: main
Are you sure you want to change the base?
Topic link autocomplete #2137
Changes from 2 commits
37bebaf
39606eb
d565fd2
9a645a0
1e0e93e
7a86896
c407d85
5ddd4d9
a611455
d9a3b48
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 | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -312,13 +312,30 @@ class AutocompleteViewManager { | |||||||||||||||||
| void handleChannelDeleteEvent(ChannelDeleteEvent event) { | ||||||||||||||||||
| for (final channelId in event.channelIds) { | ||||||||||||||||||
| autocompleteDataCache.invalidateChannel(channelId); | ||||||||||||||||||
| autocompleteDataCache.invalidateChannelTopic(channelId); | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| void handleChannelUpdateEvent(ChannelUpdateEvent event) { | ||||||||||||||||||
| autocompleteDataCache.invalidateChannel(event.streamId); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| void handleUpdateMessageEvent(UpdateMessageEvent event, {required PerAccountStore store}) { | ||||||||||||||||||
| if (event.moveData == null) return; | ||||||||||||||||||
| final UpdateMessageMoveData( | ||||||||||||||||||
| :origStreamId, :origTopic, :newStreamId, :newTopic, :propagateMode, | ||||||||||||||||||
| ) = event.moveData!; | ||||||||||||||||||
|
|
||||||||||||||||||
| switch(propagateMode) { | ||||||||||||||||||
| case PropagateMode.changeOne: | ||||||||||||||||||
| case PropagateMode.changeLater: | ||||||||||||||||||
| return; | ||||||||||||||||||
| case PropagateMode.changeAll: | ||||||||||||||||||
| autocompleteDataCache.invalidateChannelTopic(origStreamId, | ||||||||||||||||||
| topic: origTopic.displayName ?? store.realmEmptyTopicDisplayName); | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// Called when the app is reassembled during debugging, e.g. for hot reload. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// Calls [AutocompleteView.reassemble] for all that are registered. | ||||||||||||||||||
|
|
@@ -1094,7 +1111,21 @@ class AutocompleteDataCache { | |||||||||||||||||
|
|
||||||||||||||||||
| List<String> normalizedNameWordsForChannel(ZulipStream channel) { | ||||||||||||||||||
| return _normalizedNameWordsByChannel[channel.streamId] | ||||||||||||||||||
| ?? normalizedNameForChannel(channel).split(' '); | ||||||||||||||||||
| ??= normalizedNameForChannel(channel).split(' '); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| final Map<int, Map<String, String>> _normalizedNamesByChannelTopic = {}; | ||||||||||||||||||
|
|
||||||||||||||||||
| String normalizedNameForChannelTopic(int channelId, String topic) { | ||||||||||||||||||
| return (_normalizedNamesByChannelTopic[channelId] ??= {})[topic] | ||||||||||||||||||
| ??= AutocompleteQuery.lowercaseAndStripDiacritics(topic); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| final Map<int, Map<String, List<String>>> _normalizedNameWordsByChannelTopic = {}; | ||||||||||||||||||
|
|
||||||||||||||||||
| List<String> normalizedNameWordsForChannelTopic(int channelId, String topic) { | ||||||||||||||||||
| return (_normalizedNameWordsByChannelTopic[channelId] ?? {})[topic] | ||||||||||||||||||
| ??= normalizedNameForChannelTopic(channelId, topic).split(' '); | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+1271
to
1274
Collaborator
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.
Suggested change
Right? |
||||||||||||||||||
|
|
||||||||||||||||||
| void invalidateUser(int userId) { | ||||||||||||||||||
|
|
@@ -1112,6 +1143,16 @@ class AutocompleteDataCache { | |||||||||||||||||
| _normalizedNamesByChannel.remove(channelId); | ||||||||||||||||||
| _normalizedNameWordsByChannel.remove(channelId); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| void invalidateChannelTopic(int channelId, {String? topic}) { | ||||||||||||||||||
| if (topic == null) { | ||||||||||||||||||
| _normalizedNamesByChannelTopic.remove(channelId); | ||||||||||||||||||
| _normalizedNameWordsByChannelTopic.remove(channelId); | ||||||||||||||||||
| } else { | ||||||||||||||||||
| _normalizedNamesByChannelTopic[channelId]?.remove(topic); | ||||||||||||||||||
| _normalizedNameWordsByChannelTopic[channelId]?.remove(topic); | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// A result the user chose, or might choose, from an autocomplete interaction. | ||||||||||||||||||
|
|
@@ -1581,3 +1622,201 @@ class ChannelLinkAutocompleteResult extends ComposeAutocompleteResult { | |||||||||||||||||
| // mentioned before) and also present in the description. | ||||||||||||||||||
| final int rank; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// An [AutocompleteView] for a #channel>topic autocomplete interaction, | ||||||||||||||||||
| /// an example of a [ComposeAutocompleteView]. | ||||||||||||||||||
| class TopicLinkAutocompleteView extends AutocompleteView<TopicLinkAutocompleteQuery, TopicLinkAutocompleteResult> { | ||||||||||||||||||
| TopicLinkAutocompleteView._({ | ||||||||||||||||||
| required super.store, | ||||||||||||||||||
| required super.query, | ||||||||||||||||||
| required this.channelId, | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
| factory TopicLinkAutocompleteView.init({ | ||||||||||||||||||
| required PerAccountStore store, | ||||||||||||||||||
| required TopicLinkAutocompleteQuery query, | ||||||||||||||||||
| }) { | ||||||||||||||||||
| return TopicLinkAutocompleteView._(store: store, query: query, channelId: query.channelId) | ||||||||||||||||||
| .._fetch(); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| final int channelId; | ||||||||||||||||||
|
|
||||||||||||||||||
| Iterable<TopicName> _topics = []; | ||||||||||||||||||
|
|
||||||||||||||||||
| /// Fetches topics of the selected channel, if needed. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// When the results are fetched, this restarts the search to refresh UI | ||||||||||||||||||
| /// showing the newly fetched topics. | ||||||||||||||||||
| Future<void> _fetch() async { | ||||||||||||||||||
| // TODO: handle fetch failure | ||||||||||||||||||
| // TODO(#2154): do not fetch topics for "only general chat" channel | ||||||||||||||||||
| _topics = (await store.topics.getChannelTopics(channelId)).map((e) => e.name); | ||||||||||||||||||
| return _startSearch(); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| @override | ||||||||||||||||||
| Future<List<TopicLinkAutocompleteResult>?> computeResults() async { | ||||||||||||||||||
| final unsorted = <TopicLinkAutocompleteResult>[]; | ||||||||||||||||||
|
|
||||||||||||||||||
| final channelResult = query.testChannel(channelId); | ||||||||||||||||||
| if (channelResult != null) unsorted.add(channelResult); | ||||||||||||||||||
|
|
||||||||||||||||||
| if (await filterCandidates(filter: _testTopic, | ||||||||||||||||||
| candidates: _topics, results: unsorted)) { | ||||||||||||||||||
| return null; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| final queryTopicResult = query.testQueryTopic(TopicName(query.raw), | ||||||||||||||||||
| matchedTopics: unsorted.whereType<TopicLinkAutocompleteTopicResult>() | ||||||||||||||||||
| .map((r) => r.topic)); | ||||||||||||||||||
| if (queryTopicResult != null) unsorted.add(queryTopicResult); | ||||||||||||||||||
|
Comment on lines
+1670
to
+1673
Collaborator
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. It should be possible to check whether a topic exists in constant time; let's try to avoid a linear scan through the filtered topics ( |
||||||||||||||||||
|
|
||||||||||||||||||
| return bucketSort(unsorted, (r) => r.rank, | ||||||||||||||||||
| numBuckets: TopicLinkAutocompleteQuery._numResultRanks); | ||||||||||||||||||
|
Comment on lines
+1675
to
+1676
Collaborator
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. This bucket-sort step (here and in other autocompletes) is really handling two things: For example, in @-mention autocomplete, we rank user-group results below user results even when the user-group result would be a better match for the query. Here, the ranking of How about hard-coding the rank of +/// A result for just a channel link, after all, not any topic.
+///
+/// Offered at the top of the list when the topic query is empty.
class TopicLinkAutocompleteChannelResult extends TopicLinkAutocompleteResult {
- TopicLinkAutocompleteChannelResult({required this.channelId, required this.rank});
+ TopicLinkAutocompleteChannelResult({required this.channelId});
@override
final int channelId;
+ /// This should always come first in the list of options.
@override
- final int rank;
+ int get rank => 0;
}Then follow the resulting analyzer errors, on paths that should make sense intuitively, which I think means: if (query.raw.isEmpty) {
unsorted.add(TopicLinkAutocompleteChannelResult(channelId: channelId));
}and so on; also update dartdocs appropriately, e.g.:
/// A measure of a topic result's quality in the context of the query,
- /// from 0 (best) to one less than [_numResultRanks].
+ /// from 1 (best) to one less than [_numResultRanks].
///
- /// See also [_rankChannelResult].
+ /// (Rank 0 is reserved for [TopicLinkAutocompleteChannelResult].)
static int _rankTopicResult({
- /// The number of possible values returned by [_rankResult].
+ /// The number of possible values returned by [_rankTopicResult],
+ /// plus one for the [TopicLinkAutocompleteChannelResult].
static const _numResultRanks = 5; |
||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| TopicLinkAutocompleteTopicResult? _testTopic(TopicLinkAutocompleteQuery query, TopicName topic) { | ||||||||||||||||||
| return query.testTopic(topic, store); | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// A #channel>topic autocomplete query, used by [TopicLinkAutocompleteView]. | ||||||||||||||||||
| class TopicLinkAutocompleteQuery extends ComposeAutocompleteQuery { | ||||||||||||||||||
| TopicLinkAutocompleteQuery(super.raw, {required this.channelId}); | ||||||||||||||||||
|
|
||||||||||||||||||
| final int channelId; | ||||||||||||||||||
|
|
||||||||||||||||||
| @override | ||||||||||||||||||
| ComposeAutocompleteView initViewModel({ | ||||||||||||||||||
| required PerAccountStore store, | ||||||||||||||||||
| required ZulipLocalizations localizations, | ||||||||||||||||||
| required Narrow narrow, | ||||||||||||||||||
| }) { | ||||||||||||||||||
| return TopicLinkAutocompleteView.init(store: store, query: this); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| TopicLinkAutocompleteChannelResult? testChannel(int channelId) { | ||||||||||||||||||
| assert(this.channelId == channelId); | ||||||||||||||||||
| if (raw.isNotEmpty) return null; | ||||||||||||||||||
| return TopicLinkAutocompleteChannelResult( | ||||||||||||||||||
| channelId: channelId, rank: TopicLinkAutocompleteQuery._rankChannelResult); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| TopicLinkAutocompleteTopicResult? testTopic(TopicName topic, PerAccountStore store) { | ||||||||||||||||||
| final cache = store.autocompleteViewManager.autocompleteDataCache; | ||||||||||||||||||
| final userFacingName = topic.displayName ?? store.realmEmptyTopicDisplayName; | ||||||||||||||||||
| final matchQuality = _matchName( | ||||||||||||||||||
| normalizedName: cache.normalizedNameForChannelTopic(channelId, userFacingName), | ||||||||||||||||||
| normalizedNameWords: cache.normalizedNameWordsForChannelTopic(channelId, userFacingName)); | ||||||||||||||||||
| if (matchQuality == null) return null; | ||||||||||||||||||
| return TopicLinkAutocompleteTopicResult( | ||||||||||||||||||
| channelId: channelId, topic: topic, | ||||||||||||||||||
| rank: _rankTopicResult(matchQuality: matchQuality)); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| TopicLinkAutocompleteTopicResult? testQueryTopic(TopicName queryTopic, { | ||||||||||||||||||
| required Iterable<TopicName> matchedTopics, | ||||||||||||||||||
| }) { | ||||||||||||||||||
| assert(raw == queryTopic.apiName); | ||||||||||||||||||
| if (raw.isEmpty) return null; | ||||||||||||||||||
| final queryTopicExists = matchedTopics.any((t) => t.isSameAs(queryTopic)); | ||||||||||||||||||
| if (queryTopicExists) return null; | ||||||||||||||||||
| return TopicLinkAutocompleteTopicResult( | ||||||||||||||||||
| channelId: channelId, topic: queryTopic, isNew: true, | ||||||||||||||||||
| rank: TopicLinkAutocompleteQuery._rankTopicResult(isNewTopic: true)); | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+1718
to
+1728
Collaborator
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. Like the I would also probably make a new subclass of |
||||||||||||||||||
|
|
||||||||||||||||||
| /// A measure of the channel result's quality in the context of the query, | ||||||||||||||||||
| /// from 0 (best) to one less than [_numResultRanks]. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// See also [_rankTopicResult]. | ||||||||||||||||||
| static const _rankChannelResult = 0; | ||||||||||||||||||
|
|
||||||||||||||||||
| /// A measure of a topic result's quality in the context of the query, | ||||||||||||||||||
| /// from 0 (best) to one less than [_numResultRanks]. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// See also [_rankChannelResult]. | ||||||||||||||||||
| static int _rankTopicResult({ | ||||||||||||||||||
| NameMatchQuality? matchQuality, | ||||||||||||||||||
| bool isNewTopic = false, | ||||||||||||||||||
| }) { | ||||||||||||||||||
| assert((matchQuality != null) ^ isNewTopic); | ||||||||||||||||||
| if (isNewTopic) return 1; | ||||||||||||||||||
| return switch(matchQuality!) { | ||||||||||||||||||
| NameMatchQuality.exact => 2, | ||||||||||||||||||
| NameMatchQuality.totalPrefix => 3, | ||||||||||||||||||
| NameMatchQuality.wordPrefixes => 4, | ||||||||||||||||||
| }; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// The number of possible values returned by [_rankResult]. | ||||||||||||||||||
| static const _numResultRanks = 5; | ||||||||||||||||||
|
|
||||||||||||||||||
| @override | ||||||||||||||||||
| String toString() { | ||||||||||||||||||
| return '${objectRuntimeType(this, 'TopicLinkAutocompleteQuery')}(raw: $raw, channelId: $channelId)'; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| @override | ||||||||||||||||||
| bool operator ==(Object other) { | ||||||||||||||||||
| if (other is! TopicLinkAutocompleteQuery) return false; | ||||||||||||||||||
| return other.raw == raw && other.channelId == channelId; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| @override | ||||||||||||||||||
| int get hashCode => Object.hash('TopicLinkAutocompleteQuery', raw, channelId); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// An autocomplete result for a #channel>topic autocomplete interaction. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// This is abstract because there are two kind of results that can both be | ||||||||||||||||||
| /// offered in the same #channel>topic autocomplete interaction: one for the | ||||||||||||||||||
| /// channel and the other for its topics. | ||||||||||||||||||
| sealed class TopicLinkAutocompleteResult extends ComposeAutocompleteResult { | ||||||||||||||||||
| int get channelId; | ||||||||||||||||||
|
|
||||||||||||||||||
| /// A measure of the result's quality in the context of the query. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// Used internally by [TopicLinkAutocompleteView] for ranking the results. | ||||||||||||||||||
| // Behavior we have that web doesn't and might like to follow: | ||||||||||||||||||
| // - A "word-prefixes" match quality on topic names: | ||||||||||||||||||
| // see [NameMatchQuality.wordPrefixes], which we rank on. | ||||||||||||||||||
| // | ||||||||||||||||||
| // Behavior web has that seems undesired, which we don't plan to follow: | ||||||||||||||||||
| // - A "word-boundary" match quality on topic names: | ||||||||||||||||||
| // special rank when the whole query appears contiguously | ||||||||||||||||||
| // right after a word-boundary character. | ||||||||||||||||||
| // Our [NameMatchQuality.wordPrefixes] seems smarter. | ||||||||||||||||||
| // - Ranking some case-sensitive matches differently from case-insensitive | ||||||||||||||||||
| // matches. Users will expect a lowercase query to be adequate. | ||||||||||||||||||
| int get rank; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| class TopicLinkAutocompleteChannelResult extends TopicLinkAutocompleteResult { | ||||||||||||||||||
| TopicLinkAutocompleteChannelResult({required this.channelId, required this.rank}); | ||||||||||||||||||
|
|
||||||||||||||||||
| @override | ||||||||||||||||||
| final int channelId; | ||||||||||||||||||
|
|
||||||||||||||||||
| @override | ||||||||||||||||||
| final int rank; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| class TopicLinkAutocompleteTopicResult extends TopicLinkAutocompleteResult { | ||||||||||||||||||
| TopicLinkAutocompleteTopicResult({ | ||||||||||||||||||
| required this.channelId, | ||||||||||||||||||
| required this.topic, | ||||||||||||||||||
| this.isNew = false, | ||||||||||||||||||
| required this.rank, | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
| @override | ||||||||||||||||||
| final int channelId; | ||||||||||||||||||
|
|
||||||||||||||||||
| final TopicName topic; | ||||||||||||||||||
| final bool isNew; | ||||||||||||||||||
|
|
||||||||||||||||||
| @override | ||||||||||||||||||
| final int rank; | ||||||||||||||||||
| } | ||||||||||||||||||
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.
Indeed, thanks for catching this!