[sm-sayedi]

Fixes: #124

A follow-up to #1902.

Design discussion: #mobile-design > topic link autocomplete

Screenshots

Empty query (light)	Empty query (dark)
Empty query (shortcut syntax)	New topic
Long names	Option tapped

Screencast

Topic.link.autocomplete.mov

[alya]

Hmm, I think the multi-line channel name doesn't look great (topic is not too bad). On web, we abbreviate both to one line -- maybe we should do the same here?

[sm-sayedi]

Thank you @alya for the suggestion. Made it one liner.

Before	After

[chrisbobbe]

Thanks! Sorry again that I've let this sit. I noticed the symptom of #226 when testing this manually, and considered it a blocker—the topic items don't pop up immediately when you tap a channel, which they definitely should. Anyway, I've sent flutter/flutter#183705 upstream for that, and hopefully it'll land soon, giving us new API that we'll use to fix the bug. 🙂

Comments below.

[chrisbobbe]

Indeed, thanks for catching this!

[chrisbobbe]

Suggested change

	List<String> normalizedNameWordsForChannelTopic(int channelId, String topic) {
	return (_normalizedNameWordsByChannelTopic[channelId] ?? {})[topic]
	??= normalizedNameForChannelTopic(channelId, topic).split(' ');
	}
	List<String> normalizedNameWordsForChannelTopic(int channelId, String topic) {
	return (_normalizedNameWordsByChannelTopic[channelId] ??= {})[topic]
	??= normalizedNameForChannelTopic(channelId, topic).split(' ');
	}

Right?

[sm-sayedi]

Indeed, thanks.

[chrisbobbe]

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 (unsorted) on every keystroke just for that. How about calling store.topics.latestMessageInTopic, where .latestMessageInTopic is a new int?-returning method you add to Topics, which checks _latestMessageIdsByChannelTopic.

[chrisbobbe]

This bucket-sort step (here and in other autocompletes) is really handling two things:
a) Query relevance i.e. match quality
b) Everything we want to take precedence over query relevance

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 TopicLinkAutocompleteChannelResult is clearly (b): its position relative to other list items has nothing to do with how well it matches the query. In fact we're not really "matching" this result to the query at all—we're just implementing the product choice that it should be present, at the top, when the query is empty, and absent otherwise.

How about hard-coding the rank of TopicLinkAutocompleteChannelResult to 0? (Also adding some dartdoc so its meaning is clearer):

+/// 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: TopicLinkAutocompleteQuery.testChannel disappears, and its caller is replaced with something like

    if (query.raw.isEmpty) {
      unsorted.add(TopicLinkAutocompleteChannelResult(channelId: channelId));
    }

and so on; also update dartdocs appropriately, e.g.:

• Dartdoc on TopicLinkAutocompleteResult.rank moves to subclass TopicLinkAutocompleteTopicResult.rank, because it's only about topics
• _rankChannelResult disappears
• _rankTopicResult's dartdoc is updated, 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({

• and _numResultRanks:

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

[sm-sayedi]

Interesting points. Brought the changes except for TopicLinkAutocompleteQuery.testChannel still in place. I think it and the other method (testQueryTopic, now testNewTopic) make it easier for the test code to check the rankings. Please let me know if it needs changing! 🙂

[chrisbobbe]

Like the testChannel case, I think this work can be inlined in TopicLinkAutocompleteView.computeResults, for similar reasons. (Also see my previous comment about checking for topic existence.)

I would also probably make a new subclass of TopicLinkAutocompleteResult just for the new-topic item, with rank hard-coded to 1 (similarly to the channel result's hardcoding to 0), making sure to update TopicLinkAutocompleteResult's dartdoc which refers to two subclasses.

[chrisbobbe]

autocomplete: Identify when the user intends a topic link autocomplete

Would you separate the "fallback link" edge-case handling into its own commit? There's a lot going on in this commit, and I'd like to focus on the common case first.

[chrisbobbe]

            // Replace "#**…** >" with "#**…>" to trigger the topic autocomplete
            // interaction for the channel.

This seems helpful, but it also seems like its own "coherent idea" that should go in its own commit, for ease of review.

Also: I wouldn't expect side effects (setting value) in the autocompleteIntent method. I think of it as basically a getter, except by convention we write getters as methods when they involve expensive computation, basically as a hint to potential callers that they should think about performance.

Can we change value at a different layer? From Flutter docs, I think a custom TextInputFormatter looks like the right thing for this. It would need to do its own scan backward through the text, but that can be skipped except when there's a ">" to the left of the cursor.

Then this autocompleteIntent pseudo-getter would stay focused on the job of detecting an intent, making it much easier to reason about.

[chrisbobbe]

Can this happen?

[sm-sayedi]

Yeah, in cases like ChannelDeleteEvent.

[chrisbobbe]

nit: indentation

[chrisbobbe]

nit: put parens on same line

[sm-sayedi]

Thanks @chrisbobbe for the review, and sorry for the late revision. Pushed new changes, PTAL.

[sm-sayedi]

(rebased atop main, resolving conflicts.)

[chrisbobbe]

Coming back to this, I see I had a partial review that I forgot to send—apologies, here it is.

[chrisbobbe]

Hmm, I thought I already said this but I'm not finding it for some reason:

Let's skip the complexity of nesting topics under channel IDs in these caches. We still want to memoize the work of producing these normalized forms. But the channel ID is irrelevant to that work: we can just have Maps keyed by topic display name, right:

• _normalizedTopicDisplayNames
• _normalizedTopicDisplayNameWords

And we can skip cache invalidation too; there's no stateful input to the computation of "some topic" and ["some", "topic"] from "söme ToPiC".

[chrisbobbe]

nit: double space before { at end of line

[chrisbobbe]

How about "smoke" instead of "misc" (as in "smoke test")

[chrisbobbe]

The "misc" test also checks that results are loaded, but I guess this test is different because it also checks the intermediate state after TopicLinkAutocompleteView.init and before the results are loaded. With the amount of code in each test, I'd prefer to just drop one of these tests; or could alternatively deduplicate with helper functions.

(edit: but see a later comment where I suggest a change that would make the intermediate state less interesting, by not showing the "channel result" or "new topic result")

[sm-sayedi]

Dropped the first one.

[chrisbobbe]

nit: The two #s in the "fallback" form confused me at first; then I realized they had different meanings. How about spelling out more of the abbreviation, like [#…](#narrow…)>

[chrisbobbe]

This existing comment looks wrong; would you fix it? It should say "and any text following that" (or "after that"), right? (Similarly in _topicLinkIntentRegex where you copy-pasted the error.)

[chrisbobbe]

">" is valid in a channel name, isn't it?, at least according to the comment here.

There's tension, though, because of this character's special role in topic-link autocomplete. Rather than add > to this list and explain why it's there, though (and you haven't explained it in this revision actually 🙂), can we instead cause topic-link autocomplete to "win" by switching the order of checking for matches of _topicLinkIntentRegex and _channelLinkIntentRegex? So that we check _topicLinkIntentRegex first.

[sm-sayedi]

We could remove > from nameCharExclusions and reorder checking for matches, but it would mismatch some other syntaxes. For example, ‍‍‍‍‍‍[#…](#narrow…)>topic, which is a fallback topic link intent with topic as the query string, would be detected as a channel link intent with narrow…)>topic as the query string.

How about renaming nameCharExclusions to something that describes the purpose more accurately; how about queryCharExclusions?

[chrisbobbe]

I see; the point is that we want to show the (probably single) autocomplete option when you backspace through the end of the topic-link syntax, and in particular we want do that even when the topic-link syntax is extremely long.

From some profiling data, it looks like this is unlikely to cause perf problems, at least—#mobile-team > profilePrint test on autocomplete with different maxlookback @ 💬—but I still think it might be best to hard-code a value, to save the dev overhead of maintaining this complicated expression. Happy to wait for Greg's thoughts in integration review though.

[chrisbobbe]

Can we avoid adding this dependency on external state (channels) and handle the channel lookup at a different layer than this string-processing step?

I'm thinking:

• TopicLinkAutocompleteQuery stores the channel name as String?, with null for the shorthand #>topic case (which I learned about just today from reading this PR!)
• TopicLinkAutocompleteView receives the narrow from the widget code and stores that, just like ChannelLinkAutocompleteView and other AutocompleteViews do.
• TopicLinkAutocompleteView looks up the channel from the channel name on the query or from the narrow

[sm-sayedi]

The current revision has this refactoring, but revisiting this, I am somewhat skeptical about it. 🙂

• I think it adds more complexity to the code, specifically to the TopicLinkAutocompleteView class, where we should check channelId != null in several places.
• I think allowing channelId to be null inside the TopicLinkAutocompleteView class makes it a little hard to reason about, or at least raises the question about "how could it be null?", for which the main answer is that the channel doesn't exist! So if the channel doesn't exist, surely we don't want to provide the autocomplete for its topics. So wouldn't it be better to avoid triggering an autocomplete intent (thus creating a TopicLinkAutocompleteView object) in the first place?
• There are already some dependencies on external states (eg, store.maxChannelNameLength and store.maxTopicLength) inside the autocompleteIntent method. I am not suggesting that this is a legit reason to add more too, but I assume it would be a good trade-off for more code clarity.

[chrisbobbe]

This _startSearch() will always come after a _startSearch() that's doomed to find zero results because it happens before we get the topics list. I mean the one in the super constructor:

  /// Construct a view-model for an autocomplete interaction,
  /// and begin the search for the initial query.
  AutocompleteView({required this.store, required this._query}) {
    _startSearch();
    store.autocompleteViewManager.registerAutocomplete(this);
  }

Can we do just one _startSearch() at the beginning instead of two? How about instead making the await store.topics.getChannelTopics call at the top of computeResults? It's true that computeResults is a hot path (called on each keystroke of the query) but getChannelTopics is intentionally designed to not fetch redundantly for the same channel.

I guess this means we'd delay showing the "channel result" or "new topic result" until the topics are fetched. That seems fine to me, and in fact more correct for the latter (because we can't know if the topic is "new" until we know the existing topics).

[sm-sayedi]

Yeah, we could do that, but I wonder if it would make the user wait for the topics to be fetched, even if they only wanted to generate a channel link. Started a new discussion on CZO.

[sm-sayedi]

As decided in the discussion linked above, brought the new changes.

[sm-sayedi]

Thanks @chrisbobbe for the review. Pushed new changes, PTAL. Also added a few replies in the thread above: #2137 (comment) and #2137 (comment).