further refinements for quick fixes around mapping annotations

[martinlippert]

Follow-up refinements from #1107:

In case I have something like:

@RequestMapping(path = "/something")

I get quick fixes presented:

• Replace with @GetMapping
• Replace with @PostMapping

etc. (looks all good to me).

In case I have something like:

@RequestMapping(path = "/something", method = RequestMethod.POST)

I get three quick fixes that look a bit strange in combination to each other:

• Replace @RequestMapping with specific @GetMapping, @PostMapping etc in file
• Replace @RequestMapping with specific @GetMapping, @PostMapping etc in project
• Replace with @PostMapping

This doesn't look consistent to me. I would expect that the quick fix for the first two elements in this list (for "in file" and "in project") would also take into account that this is about @PostMapping, and not random mappings. The quick fixes should look like:

• Replace with @PostMapping
• Replace with @PostMapping in file
• Replace with @PostMapping in project

[BoykoAlex]

The recipe that performs the refactoring won't only convert mappings into @PostMapping but would pick something appropriate based on specified request method and then convert to @PostMapping or @GetMapping etc. where the choice is clear. Places where the replacement isn't clear and multiple options are presented won't be touched by the OR recipe hence this bit of incosistentcy for file and project scope fixes..

[martinlippert]

Hmmm... I think I understand. While the quick fix for the line with the warning is precise (we know it converts the @RequestMapping on this line to a @PostMapping, executing the recipe on the file might result in various different changes, depending on each line.

Maybe we need to change the description then to make this more obvious to the user. The last part at the end in file suggests this behavior, but is hard to recognize. Maybe this should be called

• Replace all @RequestMapping in file <name of the file> with @GetMapping, @PostMapping etc
• Replace all @RequestMapping in project <name of the project> with @GetMapping, @PostMapping, etc

And have the individual one listed as the first item in the quick fix list. What do you think?

[BoykoAlex]

All sound good with likely one exception - the order of quick fixes in eclipse might be out of our control or not easy to adjust. Maybe I'll reword the method scope fix label such that Eclipse sorting is acceptable ;-)

[BoykoAlex]

@martinlippert I experimented with wording the labels for quick fixes such that they are sorted acceptably but gave up due to various inconsistencies...
Eclipse attempts to sort quick fixes alphabetically because there is no "relevance" number available for marker resolutions. There is a "relevance" number but it all the same for marker resolutions unless the resolution implements IJavacompletionProposal (See the comparator class org.eclipse.jdt.ui.text.java.CompletionProposalComparator). Furthermore, there is no "relevance" or "sorting key" for CodeAction in LSP. The only piece of data i can utilize for sorting is:

	/**
	 * Marks this as a preferred action. Preferred actions are used by the
	 * `auto fix` command and can be targeted by keybindings.
	 *
	 * A quick fix should be marked preferred if it properly addresses the
	 * underlying error. A refactoring should be marked preferred if it is the
	 * most reasonable choice of actions to take.
	 *
	 * @since 3.15.0
	 */
	isPreferred?: boolean;

Once the CodeAction (quickfix) has isPreferred set I could attempt to bump it to the top of the list on the client. In order to move the "preferred" fix to the top of the list i have to do a bit of work in LSP4E, namely to implement IJavaCompletionProposal interface in LSP4E CodeActionMarkerResolution. Only the getRelevance() method returns something the rest of the methods coming from IJavaCompletionProposal are throwing UnsupportedOperationException but looks like the only thing that is needed is just the relevance number so it all seems to work.
If this seems acceptable compromise I could fire a PR for LSP4E to see what they think.

[martinlippert]

The core of LSP4E should not have a dependency on JDT, so implementing IJavaCompletinProposal in core LSP4E classes doesn't work. All the JDT-specific parts of LSP4E are implemented in a separate bundle org.eclipse.lsp4e.jdt, so if we want to implement IJavaCompletionProposal, that would need to be inside of that bundle, but that sounds difficult due to the fact that the CodeActionMarkerResolution is in the LSP4E core. 🤔 Maybe worth to file an issue for this and discuss with the other LSP4E contributors, maybe someone has another idea?

[BoykoAlex]

Oops... yes, indeed lsp4e shouldn't be dependent on JDT... Seems like we'd need some interface in eclipse platform with a method int getRelevance() to make this work... Probably, best ask lsp4e folk indeed.

[BoykoAlex]

@martinlippert Handled sorting of quickfixes on our side (tooling.boot.ls plugin): d1e9ed5
Some new optics that came in with this commit:

Spring Boot type of error rather than generic Language Servers:

Spring icon next to quickfix. Felt like a good "spot on" thing to emphasize that this is a Spring fix. (Of course we could either switch it off or find more appropriate icon)