Skip to content
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

Merged
merged 4 commits into from
Apr 27, 2020

Conversation

ChrisPapp
Copy link
Contributor

@ChrisPapp ChrisPapp commented Apr 21, 2020

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...

this._cts = new EditorStateCancellationTokenSource(this.editor, CodeEditorStateFlag.Position | CodeEditorStateFlag.Value);

... but the input field thinks we didn't cancel because the cursor is still within range.

let onCursorChanged = () => {
const editorPosition = this._editor.getPosition();
if (!editorPosition || !Range.containsPosition(where, editorPosition)) {
this.cancelInput(true);
}
};

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.

@@ -190,6 +193,8 @@ class RenameController implements IEditorContribution {
return undefined;
}

this._cts = new EditorStateCancellationTokenSource(this.editor, CodeEditorStateFlag.Position | CodeEditorStateFlag.Value);
Copy link
Member

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

Copy link
Contributor Author

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;
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@ChrisPapp ChrisPapp requested a review from jrieken April 27, 2020 10:30
@jrieken jrieken added this to the April 2020 milestone Apr 27, 2020
@jrieken
Copy link
Member

jrieken commented Apr 27, 2020

Thanks

@jrieken jrieken merged commit 5487172 into microsoft:master Apr 27, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jun 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] [Regression] "Rename failed to execute" when renaming a variable in TypeScript
2 participants