Repository navigation
Autocomplete: Reopen @-mention typeahead when editing inside ** - #2166
MritunjayTiwari14 wants to merge 2 commits into
Conversation
|
@chrisbobbe Ready for review. |
There was a problem hiding this comment.
Thanks @MritunjayTiwari14! I think this is close, a few notes bellow.
There was a problem hiding this comment.
I think this should be bumped to be + 3. The longest prefix after @ is now _** (3 chars, for a silent mention like @_**foo). With a small maxChannelNameLength, the lookback limit can prevent the scanner from reaching the @ and then the intent would be missed.
For example:
@_**ab with maxChannelName: 1, lookback is 1+2+2=5 but the pattern is 6 chars.
There was a problem hiding this comment.
Yes, it does clearly make sense. This should be increased to 3 because of _**. Thanks for noticing!
| + r'[^' + fullNameAndEmailCharExclusions + r']*' | ||
| + r'(?:' | ||
| // Reject on whitespace and "**" right after "@" or "@_". Emails can't start | ||
| // with both, and full_name can't either (it's run through Python's `.strip()`). |
There was a problem hiding this comment.
(nit):
| // with both, and full_name can't either (it's run through Python's `.strip()`). | |
| // with either, and full_name can't either (it's run through Python's `.strip()`). |
| doTest('Ask @**Chris"Bobbe^', null); | ||
|
|
||
| // "**" are not allowed inside the query. | ||
| doTest('Ask @**Chris**Bobbe^', null); |
There was a problem hiding this comment.
I'd add more edge cases here, maybe:
// Three asterisks: "@***" = prefix "@**" + query "*".
doTest('Ask ~@***^', mention('*'));
doTest('Ask ~@_***^', silentMention('*'));
doTest('~@_**ab^', silentMention('ab'), maxChannelName: 1);
doTest('~@**abc^', mention('abc'), maxChannelName: 1);
doTest('~@**ab^', mention('ab'), maxChannelName: 1);424de02 to
6b77919
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes the compose-box autocomplete behavior for @-mentions when a user edits inside the @**Full Name** syntax (e.g., after selecting a mention and then backspacing into the bolded name), so the typeahead reopens using the intended query text.
Changes:
- Extend @-mention intent parsing to recognize
@**…-prefixed mention text and exclude the leading**from the query. - Adjust autocomplete lookback bounds to account for the longer optional prefix (
@_**). - Expand and refine unit tests for @-mention and channel-link lookback/query parsing, including UTF-16 code-unit edge cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
lib/model/autocomplete.dart |
Updates mention-intent regex and lookback length to support reopening autocomplete while editing inside @**... mention syntax. |
test/model/autocomplete_test.dart |
Adds comprehensive tests for @**... edit/backspace scenarios and updates lookback-boundary tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| doTest('Ask @****^', null); | ||
| doTest('Ask @_****^', null); | ||
|
|
||
| // " ' " (Aspotrophe) and "-" are allowed inside the query. |
There was a problem hiding this comment.
Spelling: "Aspotrophe" should be "Apostrophe" in this comment.
| // " ' " (Aspotrophe) and "-" are allowed inside the query. | |
| // " ' " (Apostrophe) and "-" are allowed inside the query. |
6b77919 to
81eda1a
Compare
|
@chrisbobbe Waiting for your review on this... |
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks, and I'm sorry I've let this sit.
Comments below, including one that unlocks some simplification, I think: if we can continue prohibiting stars ("*") in the query, then I think we can neatly express three mutually exclusive branches in the regex:
- no stars before or after the query
- two stars before the query and no stars after
- two stars before the query and one star after (for the case where you've backspaced through one star after finishing an autocomplete)
Would you give that a try?
There was a problem hiding this comment.
The reasoning in #2166 (comment) for bumping the 2 to a 3 doesn't quite hold, I think. The max length of a channel name isn't relevant when you're trying to do a silent @-mention, because in that case you're searching for users, not channels.
I think the complexity of trying to maintain an exactly correct number here isn't worth it, and we should just do the simple thing of hard-coding it to 30 or 60 or something. This came up in another context recently: #2137 (comment)
There was a problem hiding this comment.
Well, I disagree, maxlookback should be bumped from 2 to 3. I totally agree, channel name isn't relevant. But how the channel name is affected when the value is bumped from 2 to 3 is what I can't understand in your comment.
Those two are totally independent things, when we introduced silent mention. The min length would inflate from being **, i.e., 2, to _**, i.e., 3?
| @@ -73,7 +73,7 @@ extension ComposeContentAutocomplete on ComposeContentController { | |||
| if (charAtPos == '@') { | |||
| final match = _mentionIntentRegex.matchAsPrefix(textUntilCursor, pos); | |||
| if (match == null) continue; | |||
| query = MentionAutocompleteQuery(match[2]!, silent: match[1]! == '_'); | |||
| query = MentionAutocompleteQuery(match[2] ?? match[3]!, silent: match[1]! == '_'); | |||
There was a problem hiding this comment.
If I understand correctly, I think this could be simplified by using duplicate named capture groups? I see an existing TODO mentioning that feature:
// TODO(upstream): maybe use duplicate-named capture groups for better readability?
// https://github.com/dart-lang/sdk/issues/61337and that Dart PR was merged recently! 🎉 Can we try that?
| doTest('Ask ~@_***^', silentMention('*')); | ||
| doTest('~@_**ab^', silentMention('ab'), maxChannelName: 1); | ||
| doTest('~@**abc^', mention('abc'), maxChannelName: 1); | ||
| doTest('~@**ab^', mention('ab'), maxChannelName: 1); |
There was a problem hiding this comment.
Is there a reason to allow "*" in the query at all? We have a comment suggesting that no users will ever match if that character (or a handful of others) is in the query:
// Characters that would defeat searches in full_name and emails, since
// they're prohibited in both forms. These are all the characters prohibited
// in full_name except "@", which appears in emails. (For the form of
// full_name, find uses of UserProfile.NAME_INVALID_CHARS in zulip/zulip.)
const fullNameAndEmailCharExclusions = r'\*`\\>"\p{Other}';Fixes zulip#1967. RegEx logic is now divide into 3 mutually exclusive case where 1st case deals with query not starting with "**", while the 2nd and 3rd case handles query starting with "**" and, 1. Not containing any "*" afterwards. 2. Ending with an "*".
…dcoding
Recently, dart added support for duplicate-named group.
Using this would improve readability.
Following this, we should use duplicate-named group instead of
access the same query in different cases using hardcoded indices
using former provides better readability.
This was implemented by typecasting match to RegExpMatch. This seems
safe since match was obtained by a regex operation and was required as only
RegExpMatch supports `namedGroup('name')` method to access capture group named 'name'.
notes:
1. Implemention of named capturing group was not done for capturing group
which were accessible by single definite index, since there is only one
capture group.
more details about namedGroup:
https://api.flutter.dev/flutter/dart-core/RegExpMatch/namedGroup.html
dart upstream issue which was resolved:
dart-lang/sdk#61337
81eda1a to
50240ab
Compare
|
Thanks, @chrisbobbe, for the review. I have left a comment on your feedback. As well as addressing all of your feedback. PTAL. Changes:
|
Fixes #1967.
Changes
before.webm
after.webm