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

Evaluate AnnotatedTextEdit and ChangeAnnotation#needsConfirmation #1155

Closed
BoykoAlex opened this issue Dec 1, 2023 · 11 comments
Closed

Evaluate AnnotatedTextEdit and ChangeAnnotation#needsConfirmation #1155

BoykoAlex opened this issue Dec 1, 2023 · 11 comments
Assignees
Labels
for: vscode something that is specific for VSCode theme: refactoring type: enhancement

Comments

@BoykoAlex
Copy link
Contributor

VScode supports annotated edits which can a common ChangeAnnotation that has needsConfirmation flag. Once the flag is set VSCode shows nice compare and merge UI where user can preview changes and accept/reject them.

This feature needs to be evaluated whether we can take advantage of it as its purpose seem to partially accept changes. Spring Boot version upgrade usually assumes all changes are accepted otherwise code may not compile but there are exceptions from this of course.

@BoykoAlex
Copy link
Contributor Author

@martinlippert i have put in first trial for this: 1539beb
When recipes are applied via Upgrade Spring Boot Version... or Refactor Spring Boot Project... UI user is prompted to select Apply or Preview changes. Code actions and quick fixes don't have this yet but perhaps it is for the better...
The needsConfirmation flag is say "yes" to a change. If the change has this flag on it is not check-marked in the Refactor Preview. Selecting all changes in the Refactor View is annoying... I wish we could just bring up the Refactor View (i.e. showPreview flag) rather than have needsConfirmation.

@martinlippert
Copy link
Member

I tried the initial implementation (latest available pre-release), some initial thoughts:

  • I find it somewhat strange to select the Apply or Preview flag when executing the command from the palette (while walking through the quick picks). A more VSCode-natural way would be - at least I think - to present this choice as a popup when the changes have been computed. It would be similar to the initializr integration, where there is a popup showing up once the project got created whether to open the project in a new window or in the existing workspace.
  • I agree to your observation about the refactoring preview. It would be nice if the changes would be pre-selected, so that clicking on the individual changes results in a diff view showing up with the actual code changes. At the moment, when you execute the upgrade, you see all those changes (presented in a slightly weird way) in the refactoring preview, then you would like to see the details, you click on a change, and you see exactly nothing in the diff editor, because the change you clicked on is not checked yet.

I think we can try to implement the first part and probably raise an issue against VSCode for the second part to see what the team from VSCode says. Maybe there is a way to turn on this pre-selection.

I also find it a bit weird that there is no "select all" option in the refactoring preview.

@martinlippert
Copy link
Member

One more thought: Moving the Apply or Preview choice into the popup that the language server would initiate once the changes are computed would also allow this to appear nicely when executing these things from the quick fixes... :-)

@BoykoAlex
Copy link
Contributor Author

BoykoAlex commented Jan 2, 2024

@martinlippert Hmm... VSCode offers to apply refactoring right away or show the preview first via the UI before the refactoring
Screenshot 2024-01-02 at 13 17 25

Shift-Enter would show preview. This is what i was trying to follow given the UI restrictions we have.

Or you'd still like to try the popup?

@martinlippert
Copy link
Member

Or you'd still like to try the popup?

I would like to give it a try. At least for those "larger" changes (like version upgrades, running recipes on the project). So this would split the quick fixes into two categories:

  • the ones that execute the upgrade recipes or other recipes on the whole project (show the popup)
  • the "small" quick fixes that operate on a specific location (e.g. remove public from bean method) or a single file (do NOT show popup)

@BoykoAlex
Copy link
Contributor Author

I have pushed the support for the above with e34b39f

@martinlippert
Copy link
Member

I tried this in the latest snapshot builds and like it. I am still not happy that the refactoring preview doesn't pre-select all the changes, so that the user has to check everything manually. Maybe we can ask the VSCode team about this?

I also saw this popup showing up when executing the upgrade via the quick fix action, but without having any effect. The changes got applied independent of the popup showing up.

@BoykoAlex
Copy link
Contributor Author

BoykoAlex commented Jan 4, 2024

Re: No preview. Works fine from quick fixes for me... the preview is shown if the "Preview" button clicked... Was that in Eclipse?

There are multiple issues created against VSCode for "Refactor Preview". Usability issues for select all and similar features as well as just against the current behaviour. I'll look them up and link here.

The problem is that needsConfirmation and previewChanges are different things. We want previewChanges: "Refactor Preview" shown all changes selected. However, we only have needsConfirmation in the LSP which is show preview and changes not selected as they are expected to be confirmed - nothing is confirmed at the start.

"Refactor Preview" related VSCode issues:
microsoft/vscode#144893
microsoft/vscode#200991 (from me)
microsoft/vscode#156886

@BoykoAlex
Copy link
Contributor Author

@martinlippert I think we've done what we could with it within 4.21.1. Moving to the next release to hopefully enhance the experience.

@martinlippert
Copy link
Member

Let's split this issue and put additional activities into a new issue, so that we can close this one here with the changes that will already make it into 4.21.1

@BoykoAlex
Copy link
Contributor Author

Closing. The rest is in #1176

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: vscode something that is specific for VSCode theme: refactoring type: enhancement
Projects
None yet
Development

No branches or pull requests

2 participants