-
Notifications
You must be signed in to change notification settings - Fork 281
[SuperEditor] Don't activate an action tag when placing the caret at an existing tag pattern. (Resolves #2392) #2771
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?
Conversation
…an existing tag pattern. (Resolves #2392)
range: SpanRange(tagAroundPosition.indexedTag.startOffset, tagAroundPosition.indexedTag.endOffset), | ||
) | ||
.isNotEmpty; | ||
if (changeList.none((event) => event is DocumentEdit) && !hasComposingTagAttribution) { |
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.
Is there a reason we wait so long to do this check? Can't we do this check at the very beginning before all this other stuff?
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 don't think we can move it to the beginning, because we need to let the code that clears the attributions when the user presses ESC run.
return; | ||
} | ||
|
||
final hasComposingTagAttribution = textNode!.text |
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.
Are we not already checking for an existing composing tag around the user's selection?
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.
We check if there is a text that matches a tag pattern, but not if it is already attributed.
expect(tagNotificationCount, 7); | ||
}); | ||
|
||
testWidgetsOnAllPlatforms("does not start composing when placing the caret at an existing tag pattern", |
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'm not sure this is what the original ticket was about, was it? Isn't the issue about the surrounding text, not the user taping to place the caret?
Your example here says "origin/main" - but even if the user was typing, do we expect the "/" to trigger an action tag with "origin" immediately upstream? Maybe we want that for action tags, but I know we don't for "@" mentions or "#" tags....
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'm not sure this is what the original ticket was about, was it? Isn't the issue about the surrounding text, not the user taping to place the caret?
No, that was other ticket. This ticket is about initializing the editor with an existing document that contains text that matches the tag pattern. Tapping at such text shouldn't start a tag composition.
[SuperEditor] Don't activate an action tag when placing the caret at an existing tag pattern. (Resolves #2392)
Steps to reproduce
Actual result
The editor starts composing an action tag.
Expected result
The editor shouldn't start composing an action tag.
The issue is that the tag reaction only checks if there is a tag around the caret. It does not check if the user is actually modifying the document.
This PR modifies the action tag reaction to ensure that there is at least an edit in the change list before trying to start composing a tag.
While testing, I found another behavior that I'm not sure is intended. When moving the caret within an existing tag, the tag composition is updated:
Screen.Recording.2025-08-16.at.16.46.32.mov
I kept this behavior and added a test for that. If this isn't intended, then I can remove this behavior and update the test.