Refactor Preview for WorkspaceEdit with needsConfirmation flag
#953
Conversation
There was a problem hiding this comment.
Hi Alex,
the PR makes sense to me. I looks good to me but I do not know the Refactoring API at all.
My only comments are just some minor improvements on the code structure. If @martinlippert or @mickaelistria don't have further comments I would be happy if we merge it
| }); | ||
|
|
||
| if (wsEdit.getChangeAnnotations() != null && wsEdit.getChangeAnnotations().values().stream().anyMatch(ca -> ca.getNeedsConfirmation())) { | ||
| Refactoring refactoring = new Refactoring() { |
There was a problem hiding this comment.
Could we move lines 938-973 into runRefactorWizardOperation passing the change to that method instead of the wizard as now?
| } | ||
|
|
||
| }; | ||
| if (Display.getCurrent() == null) { |
There was a problem hiding this comment.
for consistency within this method, could we invert the condition on Display.getCurrent() in the new or the old code so that they use the same condition and the the if/else is in the same order?
There was a problem hiding this comment.
I'll create runOnUiThread(Runnable) in the UI class and use it here in both places
There was a problem hiding this comment.
I had a quick look and it seems good overally,
I'm disappointed that there isn't a utility for that already in Platform, something like new ChangeRefactoringWizard(change), nor nothing alike (as far as I search it). So in absence of such a more straightforward API, this implementation seems to use the correct bits.
I support the overall comments about factorizing more stuff into the runRefactorWizardOperation.
| if (Display.getCurrent() == null) { | ||
| UI.getDisplay().asyncExec(() -> runRefactorWizardOperation(wizard, label)); | ||
| } else { | ||
| runRefactorWizardOperation(wizard, label); | ||
| } |
There was a problem hiding this comment.
I think it would be better to put the UI thread check directly in runRefactorWizardOperation.
|
By the way, I'd like to add that if such a wizard taking a Change as input is useful to you in other places, you could consider creating such a wizard as an API directly in Platform, that would be welcome. We could keep this code in LSP4E until Platform provides a more generic API for it and then we adopt it. |
|
@rubenporras @mickaelistria Thank you both very much for the prompt review :-)
|
|
thank you |
WorkspaceEdithas a map of change annotations where any change annotation is associated with edits from the workspace edit. If any of change annotations hasneedsConfirmationflag on then VSCode would bring up the refactor preview with annotated changes unchecked.This PR proposes similar (but not the same, there are limitations) behaviour into Eclipse.
If there is at lease one change annotation that requires confirmation then refactor wizard is opened with the Preview page opened. All changes would be checked marked as I couldn't find how to control this piece. Another limitation is Eclipse text edits are in reverse order (they are applied in reverse order for a reason) but the preview doesn't allow to set the tree sorter on the preview tree.
@rubenporras @mickaelistria @martinlippert feedback, ideas are welcomed :-)