autocomplete: Introduce channel/topic #-mention #884
Conversation
The name Autocomplete is too generic and it collides with flutter's material Autocomplete widget name. So this uses the more specific name 'ComposeContentAutocomplete'.
The mechanism of query and results is generic to the idea of autocomplete in general, it's not specific to autocomplete on @-mentions vs. topics vs. anything else.
Most of the logic in `ComposeAutocomplete` is not specific to the content input it self rather it is general logic that applies to any autocomplete field.
Most of the logic in `ComposeAutocomplete` is not specific to the content input it self rather it is general logic that applies to any autocomplete field.
| Iterable<Object> getSortedItemsToTest(MentionAutocompleteQuery query) { | ||
| switch (query) { | ||
| case UserMentionAutocompleteQuery(): | ||
| return sortedUsers; | ||
| case ChannelMentionAutocompleteQuery(:var raw): |
There was a problem hiding this comment.
Instead of putting conditional logic within MentionAutocompleteView to handle both @-mentions and channels/topics, let's leave that class focused on @-mentions and make a new class for channels/topics.
I guess the existing "mention" naming made that design ambiguous, because I do say "#-mention" for mentioning a channel or topic. But the intent of MentionAutocompleteView and MentionAutocompleteQuery is that they're about @-mentions.
Probably a good prep commit is to rename all the MentionAutocomplete… classes to say AtMentionAutocomplete… instead, to make that clearer. ("User mention" isn't quite right, because the same autocomplete interaction can produce a mention of a group or of a wildcard like @channel.)
Similarly the new classes can say HashMentionAutocomplete… — "channel mention" isn't quite right because these also include mentioning a topic.
There was a problem hiding this comment.
Thanks @gnprice! I'd like to get you're suggestion in that case as we'd have two view models for the same autocomplete field, which is not implemented in the current design.
So do you think it'd be OK to update AutocompleteField so it has a list of AutocompleteViewModel and then, based on the query select and use one of them to search for results? Or would you take another approach?
There was a problem hiding this comment.
The autocomplete view model's lifetime corresponds to the interaction the user has to attempt an autocomplete. There should be only one of those going on at a time — we won't want to try to show @-mention and #-mention results at the same time, for example — so there should only need to be one autocomplete view model for the field at a time.
The AutocompleteIntent will specify which kind of autocomplete is desired. So that can be used to construct the right type of AutocompleteView. When there isn't (yet) an AutocompleteIntent, we don't make an AutocompleteView.
| case UserMentionAutocompleteQuery(): | ||
| item as User; |
There was a problem hiding this comment.
The need for this cast here (and the corresponding fact that CandidateT is just Object when the truth is that the candidate type is determined by the query type and is always either User or ZulipStream) is also a sign that this isn't the right arrangement of the types.
| noMatches: noMatches); | ||
| } | ||
|
|
||
| ({List<T> matches, List<T> rest}) _triage<T> ({ |
There was a problem hiding this comment.
Because this function (and its helper _triageRaw) are designed to closely mimic particular pieces of web's logic, they should get comments linking to the corresponding web code.
That helps a reviewer compare to the web code to see what differences there might be. It also helps in the future — particularly if that link is a permalink, which it should be — for a reader to compare against what's changed in web since then, and see if changes should be copied over to here.
Fixes: #124