Also read JDT UI project prefs to configure cleanups

[mickaelistria]

No description provided.

[rgrunber]

Wouldn't it have been easier to create a mapping like :

Map.of(
CleanUpConstants.STRINGCONCAT_TO_TEXTBLOCK, "stringConcatToTextBlock",
CleanUpConstants.VARIABLE_DECLARATIONS_USE_FINAL, "addFinalModifier",
...
...
)

and then read the jdt.ui preference node for each supported+enabled cleanup and send them over in the java.cleanup.actionsOnSave after going through the mapping (client-side) ? Or did you not like the idea of having to independently track the IDs JDT-LS assigns ?

This will probably work as well, but it's just code that JDT-LS shouldn't have to know about when there's already a mechanism to configure the cleanups.

[mickaelistria]

Wouldn't it have been easier to create a mapping like :

Yes. However I was unsure here: if some 3rd party contributors may provide some cleanups, then it's easier to make it also possible for those to declare some secondary ids. Or even if we add cleanups into JDT-LS directly, it still seems easier to maintain secondary ids directly in the class, so that it doesn't have to be added in a separate and easily forgotten place.

send them over in the java.cleanup.actionsOnSave after going through the mapping (client-side) ?

A secondary goal is also to add support for project-specific formatting in JDT-LS directly, making it possible to process eclipse .settings/ file when existent, even in VSCode.

it's just code that JDT-LS shouldn't have to know about when there's already a mechanism to configure the cleanups.

The current mechanism doesn't allow project-specific configuration.

So IMO, it's better to have the mapping in JDT-LS directly and make JDT-LS capable of understanding some settings of JDT-UI that may be configured in the project. But if you think that approach is problematic, I can change it.

[rgrunber]

A secondary goal is also to add support for project-specific formatting in JDT-LS directly, making it possible to process eclipse .settings/ file when existent, even in VSCode.

I don't think there's interest in .settings/ files on the VS Code side. JDT-LS supports .settings in Eclipse & the Invisible project-scoped cases but if a user is on VS Code, they'll configure with a settings.json file, and that file can specify location for Java compiler / formatter settings. Changes like #2589 also show a push to move away from the Eclipse formatter setting style, rather than supporting it as-is. The long format of the JDT settings files makes it really difficult to manually edit. Of course Eclipse has UI around all those options but currently the Java extension doesn't so manual editing is an important consideration.

The current mechanism doesn't allow project-specific configuration.

I think there's a case where a change was introduced into JDT Core that had settings per compilation unit, and in JDT-LS we set options like that in some instances.

eclipse.jdt.ls/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CodeActionHandler.java

Line 129 in 4769cd9

unit.setOptions(customOptions);

. So basically, this sort of exists. See #1157 & https://bugs.eclipse.org/bugs/show_bug.cgi?id=569336 for the full story. The custom settings come from the client, and they're retrieved using workspace/configuration, which takes a URI parameter.

@testforstephen do you know why something like sending a didChangeConfiguration after every didOpen to have file-level settings wasn't suggested ? It doesn't seem all that different from the current approach for code actions (except the server queries back the client for the actual settings).

[mickaelistria]

OK, I agree eclipse prefs files are hard to manually edit and may not be as comfortable as editorconfig or other settings files (although I'm not much convinced json is better without advanced editor capability to guide user in creating the right settings). This particular topic has been long discussed in Eclipse Platform, but nothing was attempted to improve preferences formatting so far, and I doubt it will be addressed soon.
So let's forget the secondary goal of per-project configuration in JDT-LS relying on the .settings/ content then; and stick to the main goal: portability from JDT-UI editor to JDT-LS.
So the question remains: if a project has some .settings/org.eclipse.jdt.ui.prefs, does it make sense in general for JDT-LS to try to honor it as much as possible? Depending and the answer here, we can either move all the mapping to client-side, or leave it as it, or add a configuration feature flag (opt-in or opt-out)...?
It's your call here.

[rgrunber]

Let's just leave the change for now and we can merge for 1.24.0. The approach is a bit cleaner and the entrypoints, getJdtUiProjectPreferences & getCleanupsFromJDTUIPreferences should make it clear it's a noop for all other clients.

The concern I have is that we're basically sending data between eclipseide-jdtls and JDT-LS by using the Eclipse platform API (preference nodes) as opposed to the LSP connection for something the LSP connection could handle. Of course we've added plenty of skipFoo extended client capabilities (for eclipseide-jdtls) but much of that was to let the LS know an entire feature would be handled by the client. In this case we're still relying on the LS to perform a feature, but establishing a new (non-spec) way of configuring it. I think you just need to be careful that preference nodes will support the scopes you care about.

[mickaelistria]

The concern I have is that we're basically sending data between eclipseide-jdtls and JDT-LS by using the Eclipse platform API (preference nodes) as opposed to the LSP connection for something the LSP connection could handle. Of course we've added plenty of skipFoo extended client capabilities (for eclipseide-jdtls) but much of that was to let the LS know an entire feature would be handled by the client. In this case we're still relying on the LS to perform a feature, but establishing a new (non-spec) way of configuring it.

I see it more as a project configuration file, kind of similar to pom.xml or build.gradle or .classpath or other .prefs file (like m2e's or jdt.core's ones) that describe what to do with the project and allow to modify the behavior of the LS without really being part of the LS spec either. In that context,

I think you just need to be careful that preference nodes will support the scopes you care about.

I don't fully understand it. Can you please explicit what it means for the current change? ie is there something I should modify?

[rgrunber]

I think you just need to be careful that preference nodes will support the scopes you care about.

I don't fully understand it. Can you please explicit what it means for the current change? ie is there something I should modify?

No changes needed here.

[rgrunber]

No interest in enabling this & qualifyStaticMembers ?

[mickaelistria]

Those cleanups don't map to a single JDT-UI cleanup id. JDT-UI has a single cleanup id for "style" which delegates to other configurations. The grain is different in JDT-LS to easily map those cleanups to a JDT-UI id

[rgrunber]

Overall looks good. Just a question regarding why you didn't add anything for the 2 cleanups that qualify, and a quick fix for an existing issue we can address here.

[testforstephen]

do you know why something like sending a didChangeConfiguration after every didOpen to have file-level settings wasn't suggested ? It doesn't seem all that different from the current approach for code actions (except the server queries back the client for the actual settings).

@rgrunber Since VS Code supports file-level indentation, which has a significant impact on formatting, we make an additional request to synchronize it with the language server. However, didChangeConfiguration is not supported at the file level. VS Code only supports settings at the user, workspace, and root folder levels.