Provide refactoring capabilities specific to maven properties

[dstango]

Coming here from https://github.com/eclipse/wildwebdeveloper/issues/1140#issuecomment-1475353878 as suggested by @angelozerr:

I'd propose to provide extra support for maven properties for the pom.xml-editor (shortcuts are suggestions based on shortcuts with similar functions in the java editor):

• Renaming Maven-properties by a simple keyboard shortcut:
  • Ctrl-2 + R (local rename): rename all usages of the property under cursor in the local file
  • Alt-Shift-R (rename all references): rename all usages of the property under cursor in the local file, parent-pom(s), sub-pom(s), and imported poms.
• Extract property (Alt-Shift-L): make the current selection a property, with the option to replace all other occurrences of the selected value in the file.
• Inline property (Alt-Shift-I): replace the current property under cursor with the actual value, with the option to replace all usages or only the current usage.

Such functions would make it a breeze to clean up properties in a pom file :-).

[vrubezhny]

@dstango Interesting proposal. Not sure if every proposed keyboard shortcut is available for re-defining (for instance, Ctrl-R for sure doesn't work for me in any combinations with any other keys, while most of the others invoke the content assist (probably can be re-defined)). In my case the only Alt-Shift-R is supposed to generate a Rename request so we can start here.

A contribution would be welcomed.

[mickaelistria]

The shortcuts are not really in the scope of Lemminx-Maven anyway; what would be good to add to lemminx-maven would be the codeActions for the suggested refactorings.

[laeubi]

See also:

• https://github.com/eclipse/lemminx-maven/issues/257

[vrubezhny]

Currently it's not possible to create a code action without having an error/warning/info diagnostics created for the same text range in a document - The https://github.com/eclipse/lemminx/issues/1516 issue is created in order to allow creation of code actions in case of diagnostics is absent.

[angelozerr]

Currently it's not possible to create a code action without having an error/warning/info diagnostics created for the same text range in a document - The https://github.com/eclipse/lemminx/issues/1516 issue is created in order to allow creation of code actions in case of diagnostics is absent.

Right! Any PR are welcome!

[vrubezhny]

Closing as fixed with https://github.com/eclipse/lemminx-maven/pull/404/commits/9a1dab8aa58aa98b97a39d9d57e920c97ae3803e, https://github.com/eclipse/lemminx-maven/pull/407/commits/7213b110fd6042547601021ff572b4041dd1ca14 and https://github.com/eclipse/lemminx-maven/pull/413/commits/0d299d8830f06d37e86b0d6e23f1b2524c38a235

[laeubi]

@vrubezhny would you mind giving a short summary how this is now supposed to work (maybe with a screenshot)?

[vrubezhny]

@laeubi Unfortunately, not usable yet due to https://github.com/eclipse/lsp4e/issues/699, https://github.com/eclipse/lemminx/pull/1522 and some changes required in Lemminx-Maven.

I'll try sharing the screenshots as soon as it will be possible.

[vrubezhny]

@laeubi Here is the brief "summary" of how refactoring will work after fixing the issues:

Screencast_06_21_2023_04:11:56_AM.webm

[laeubi]

That looks really cool! Just a few remarks/questions:

1. name can't be null in the error message might better be "name can't be empty", just for non-programmer users :-)
2. is there somewhere a description what is the difference between "refactoring" and "code actions"
3. Would it be possible to select if the property should be placed in the parent or somewhere else in the hierarchy?

[mickaelistria]

is there somewhere a description what is the difference between "refactoring" and "code actions"

The definition of a code action in LSP more or less tells that. Basically, code actions are quick-fixes/quick-edits in Eclipse IDE. Some of those quick-edits are big enough to be considered "refactorings". But technically, there is nothing that differentiate a small change (such as addition of a word in 1 doc) from a refactoring affecting multiple files. codeActions support both identically.

[mickaelistria]

Would it be possible to select if the property should be placed in the parent or somewhere else in the hierarchy?

We can't properly start dialog from LSP codeActions (this is a textual interaction protocol). I think the best way to render it with plain LSP interactions would be to have several codeActions, 1 for each possible host.

[laeubi]

Basically, code actions are quick-fixes/quick-edits in Eclipse IDE

Would be great to have them with STRG + 1 shortcut then?

I think the best way to render it with plain LSP interactions would be to have several codeActions, 1 for each possible host.

That sounds good.

[mickaelistria]

Would be great to have them with STRG + 1 shortcut then?

They're already there.

[vrubezhny]

1. name can't be null in the error message might better be "name can't be empty", just for non-programmer users :-)

@laeubi That's not a something that comes from Lemminx-Maven. It's common for all LSP4E clients :smile: - https://github.com/eclipse/lsp4e/pull/701

[vrubezhny]

I think the best way to render it with plain LSP interactions would be to have several codeActions, 1 for each possible host.

I agree on that it could be an acceptable solution... Just one little problem here at the moment, as the labels become more large (like, Extract to a property in "xxx/yyy/pom.xml" or Extract to a property in "ggg:aaa:vvv" instead of just Extract to a property) - see: https://github.com/eclipse/lsp4e/issues/699

[vrubezhny]

Yet another moment is selecting a name when extracting a value into a property... if a maven project or one of its parents:

• already has a <properties/> section and some values in it, and
• some property exists with a value that is identical to the value to be extracted

Then we would probably suggest also using that exiting property instead of creating a new one with new generated name., Change to "${already.existing.property} . Currently, if you're extracting the similar values (like, same text in a parent elements with the same name) - a new Maven property will be created for each operation. WDYT?

[laeubi]

Yes I think this can be useful, you should also consider the case that one has already using the same version multiple times, so what a user most likely want is replace all the same versions with one property.

Regard the naming I would probably use something like:

• Extract as new property in current module
• Extract as new property in parent ggg:aaa:vvv
• Replace with existing "${already.existing.property}" property

[vrubezhny]

replace all the same versions with one property

Just note that It's not about exactly "versions" - the code action currently suggests all entrances of <element-with-the-same-name>the same value</element-with-the-same-name> in a document to be extracted, so in terms of POM it should work for group IDs, artifact IDs and other elements as well.

The only exclusions probably should be added here (a TODO) is the group/artifact/version of the POM itself (as far as I understand, the use of Maven properties is not allowed here).

[laeubi]

In version only some properties are allowed: https://maven.apache.org/maven-ci-friendly.html

[laeubi]

And yes, its not only versions, but I think it makes sense to restrict the search to all the same text within the same tag.

[vrubezhny]

Here is how the "Extract" code actions look at the moment:

Screencast_06_22_2023_11:56:36_AM.webm

Code Actions context menu looks buggy, but still can be used if opened by mouse-clicking for the second time (one click to close it, yet another one to open it again).

Code Actions, despite the fact that they are collected one-by-one into a List, get completely re-ordered for some reason - I don't know what controls the action ordering here and if I can change it.

[dstango]

Thanks for taking care of the issue!!! Looks pretty cool in the screencast. Will there be the "regular keyboard shortcuts" like Alt-Shift-L (or F) and Alt-Shift-I available, too?

[vrubezhny]

Unfortunately neither the Language Server (Lemminx) nor its extension (Lemminx-Maven in this case) does not take care of keyboard shortcuts. A Client should take care of keyboard shortcuts, in case of Maven project editing in Eclipse M2E-Core Editor Lemminx , IMHO, is the best candidate for such an issue to be reported to, see: https://github.com/eclipse-m2e/m2e-core/issues.

[dstango]

Unfortunately neither the Language Server (Lemminx) nor its extension (Lemminx-Maven in this case) does not take care of keyboard shortcuts. A Client should take care of keyboard shortcuts, in case of Maven project editing in Eclipse M2E-Core Editor Lemminx , IMHO, is the best candidate for such an issue to be reported to, see: https://github.com/eclipse-m2e/m2e-core/issues.

Somehow it's funny, but it also produces a stack overflow in my head ;-):

• I posted my original issue here: https://github.com/eclipse-m2e/m2e-core/issues/1315
• and was then sent to go wildwebdeveloper: https://github.com/eclipse-wildwebdeveloper/wildwebdeveloper/issues/1140
• and was then send here to lemmingx-maven.
• and now you send me back...

[vrubezhny]

Yes, it looks like... Sorry about that... But how could keyboard shortcuts help you without having the commands been supported by the language server itself? Now we're able to proceed with a solution for the LS-Clients to support these keyboard shortcuts, so we're at least one step closer to the exit from this loop.

[dstango]

@vrubezhny I understand and can provide an issue on m2e.

Before I do that I have two questions:

• does the provided implementation include all three aspects (extract, inline, rename) mentioned in the first message?
• as far as I understand it refactoring commands are sometimes available as separate commands (and are configurable by the keys setting), or are sometimes only available via Ctrl-1. How about these code actions? For some Ctrl-1 actions I would love to have the ability to configure am individual shortcut, but can't find them in the keys settings (or with Ctrl-3). Would it make sense to address such a thing in a (maybe even broader) general issue, not only targeted on these new features?

Thanks for your perspective on this :-)

[vrubezhny]

@dstango Actually, I have to say sorry for asking you to target the issue directly to m2e-core. Probably I was wrong.

It looks like there is a number of commands and handlers defined in LSP4E, for example, for selection, formatting, call and type hierarchy as well as the rename command - Alt-Shift-R - it doesn't work yet in POM Editor for me, but I hope this will be fixed when updated to Lemminx v.0.26.1) ... And probably it's logical to place the handlers there also for inline/extract commands here as well.

The issue you've opened in WWD https://github.com/eclipse-wildwebdeveloper/wildwebdeveloper/issues/1140 for not working Alt-Shift-Up/Alt-Shift-Down - I think it can be closed as fixed, and I believe the fix was provided somehow in LSP4E (could you please give it a try with the latest m2e-core editor lemminx and LSP4E installed?).

The commands/handlers/keyboard shortcuts for this functionality are defined in LSP4E, so I think inline/extact is also to be supported in LSP4E - Lemminx LS and Lemminx-maven extension provide all the needed support now.

[vrubezhny]

Or may be I'm still wrong...

Lemminx-Maven extension doesn't provide a command like "Inline" or "Extract" - all it does is handling of textDocument/codeActions request returning the set of code action available for the specified text position - so when the position points to a Maven property use entry it can return a set of "inline" Code Actions and if the specified position points to a text value it can return a set of "Extract" Code Actions.

In order to show these Quick Fixes the LSP4E should support Ctrl+1 keyboard shortcut, but probably it doesn't or the current realization requires having an error/warning marker to exist for the position (which isn't compatible with "Inline"/"Extract" code actions as they do not require any error Diagnostic) - this probably is an issue to be reported to LSP4E.

To support "Inline/Extract" we should probably support textDocument.inlineCompletion request, if I'm correct - https://github.com/microsoft/language-server-protocol/blob/gh-pages/_specifications/lsp/3.18/language/inlineCompletion.md

This should be supported in both - LSP4E and Language Server (Lemminx and Lemminx-Maven extension in case of POM editing), then it's be possible to create (in LSP4E) the according command handlers and define the keyboard shortcuts.

@angelozerr, @mickaelistria What do you think?

[mickaelistria]

Ctrl+1 does work in LSP4E, it shows marker resolutions (which are codelens for a given diagnostic) and quick-edits (which are code-lenses without given diagnostic)

[vrubezhny]

Ctrl+1 does work in LSP4E, it shows marker resolutions (which are codelens for a given diagnostic) and quick-edits (which are code-lenses without given diagnostic)

In case of "Inline" / "Extract" Code Actions it looks like a bug, as these are Code Actions (not Code Lenses) that do not require any diagnostic

[mickaelistria]

Sorry, I wrote codelens, but those are code actions. But I double-checked and it could indeed be that only quick-fixes are shown and not regular code actions (which are available on right-click > Code Actions). Please open an issue to LSP4E about that.

[vrubezhny]

Sorry, I wrote codelens, but those are code actions. But I double-checked and it could indeed be that only quick-fixes are shown and not regular code actions (which are available on right-click > Code Actions). Please open an issue to LSP4E about that.

https://github.com/eclipse/lsp4e/issues/734 is created in LSP4E

[vrubezhny]

@mickaelistria @angelozerr Not sure though if textDocument.inlineCompletion is the correct LSP API to be supported for "Inline"/"Extract" [Maven Property] actions...

However, probably this can be a right API to be implemented as we can even select what kind of completion we'd like to receive (we can choose is it for "Inline" or "Extract") using the InlineCompletionContext.triggerKind ..

Also his probably can be useful for fixing https://github.com/eclipse/lemminx-maven/issues/237

WDYT?

[vrubezhny]

https://github.com/eclipse/lsp4e/issues/734 is created in LSP4E

I've closed the LSP4E issue for Ctrl + 1 not working - I've double checked and it appears to be working when the latest set of snapshots is installed for WWD + M2E-Core +Lemminx + Lemminx-Maven.