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

Implement textDocument/prepareRename for better rename experience #70724

Merged
merged 2 commits into from
Nov 9, 2023

Conversation

dibarbet
Copy link
Member

@dibarbet dibarbet commented Nov 8, 2023

Resolves dotnet/vscode-csharp#6643

Why

Two benefits of implementing prepare rename

  1. The UX is better when an item cannot be renamed (see gifs below)
  2. It allows us to play nicer with other extensions that implement rename in different contexts.

Before this change - rename on invalid identifier

rename_fail

After this change - rename on invalid identifier

prepare_rename

@dibarbet dibarbet requested a review from a team as a code owner November 8, 2023 01:54
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 8, 2023

return new DefaultBehaviorPrepareRename
{
DefaultBehavior = true,
Copy link
Member

Choose a reason for hiding this comment

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

curious what 'default' means in this context.

Copy link
Member Author

Choose a reason for hiding this comment

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

Default means default rename behavior. Prepare allows you to specify a special range, but we don't use that.

Copy link
Member

Choose a reason for hiding this comment

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

thanks! what is an example of a 'special range'? (just for my understanding).

Copy link
Member Author

Choose a reason for hiding this comment

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

@davidwengier
Copy link
Contributor

Looked into piggy backing off this to add support to Razor, and FYI this will break rename in VS if the handler lights up there (ie, when LSP is turned on): https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1914930

@dibarbet
Copy link
Member Author

dibarbet commented Nov 9, 2023

@davidwengier it sounds like if I merge this it will break rename in Razor, is that correct? Or will it continue working because Razor doesn't call Roslyn's prepare rename?

@davidwengier
Copy link
Contributor

It's cool, you can merge. We don't advertise prepare rename support, so they won't call us, and we won't call you. In fact, if we do want to support we can workaround the client issue in Razor, by translating your response into a range response, so we're not blocked either way.

@dibarbet dibarbet merged commit 33f7330 into dotnet:main Nov 9, 2023
24 checks passed
@ghost ghost added this to the Next milestone Nov 9, 2023
@dibarbet dibarbet deleted the prepare_rename branch November 9, 2023 19:35
@RikkiGibson RikkiGibson modified the milestones: Next, 17.9 P2 Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement textDocument/prepareRename request to player nicer with other rename providers
5 participants