-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
Prevent unexpected rename cancellation #95739
Conversation
@@ -190,6 +193,8 @@ class RenameController implements IEditorContribution { | |||
return undefined; | |||
} | |||
|
|||
this._cts = new EditorStateCancellationTokenSource(this.editor, CodeEditorStateFlag.Position | CodeEditorStateFlag.Value); |
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.
How about allowing to define a range in which position changes are OK? You could pass the rename-range into the ctor and test if the position is still within the range when selection or position changes happen
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 updated it. Since the token is used before the range is calculated, I also added a setter method. Could synchronization issues could arise in the future?
@@ -173,14 +173,15 @@ class RenameController implements IEditorContribution { | |||
let selection = this.editor.getSelection(); | |||
let selectionStart = 0; | |||
let selectionEnd = loc.text.length; | |||
(this._cts as EditorStateCancellationTokenSource).range = loc.range; |
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.
This is not nice. The range
property should not be mutable as that counters the idea of capturing a state to compare it against a future state. How about creating a new token? E.g. the token from line 146 is used to resolve the rename locations, then here dispose it and create new token with a (readonly) range
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.
Done!
Thanks |
This PR fixes #92507
The problem was that the rename command was getting cancelled. When moving the cursor's position (but still staying within the rename target's range) the cancellation token thinks it should cancel because the cursor moved...
vscode/src/vs/editor/contrib/rename/rename.ts
Line 146 in d769191
... but the input field thinks we didn't cancel because the cursor is still within range.
vscode/src/vs/editor/contrib/rename/renameInputField.ts
Lines 188 to 193 in d769191
Therefore we can either update the input field's logic to be in line with the cancellation token rules, or we can temporarily stop using the cancellation token.
I implemented the latter.