Enable option to format edited code in SWT examples, snippets and tests

[fedejeanne]

What it does

The purpose of this PR is to set the Save Actions of the Test/Example projects in SWT to:

1. Automatically formatting edited code when saving --> Achieved by setting sp_cleanup.format_source_code and sp_cleanup.format_source_code_changes_only
2. Enable a follow-up PR to reformat the code consistently by doing a (Right click ->) Source -> Format --> Achieved by enabling the formatter_profile and the formatter_settings_version in the JDT UI preferences by adding lots of changes to the JDT Core preferences (Note: those changes are automatically added when one enables the specific project configuration and sets the "Eclipse [built-in]" as the formatter (see https://github.com/eclipse-platform/eclipse.platform.swt/pull/1238#issuecomment-2122329410)

What it doesn't do

• It doesn't configure any other clean-ups that might be run via (Right Click ->) Source -> Clean Up

[fedejeanne]

I believe it is not a good idea. We had discussions in the past. Mixing old/new code styles wouldn't be helpful.

How about running the formatter once for every project and activate this in the future? I mean so the code styles look all "new"?

[iloveeclipse]

I believe it would be OK for SWT test/example projects except SWT main code itself, which is very "special" and where re-formatting would cause only troubles.

[github-actions[bot]]

Test Results

   418 files  ±0     418 suites  ±0   8m 10s :stopwatch: +52s  4 121 tests ±0   4 113 :white_check_mark: ±0   8 :zzz: ±0  0 :x: ±0  16 313 runs  ±0  16 221 :white_check_mark: ±0  92 :zzz: ±0  0 :x: ±0 

Results for commit ef79ed62. ± Comparison against base commit 218d4abe.

:recycle: This comment has been updated with latest results.

[fedejeanne]

What kind of problems could be caused by reformatting the code? You mean like actually breaking it (failed tests/new bugs)?

[iloveeclipse]

No, the git blame will be unusable, and that is the main issue, a blocker.

Beside this, yes, no one will be able to review tons of reformatted production code, so an occasional formatter bug may introduce some bug too. Less likely, but possible.

[fedejeanne]

@iloveeclipse I made some changes and adapted the commit text, the description of this PR and its title. Only the tests and examples have the preferences (all projects). Would you please review again?

[HeikoKlare]

I have two questions/remarks on this:

1. Why should we enable auto-formatting for projects that do not have formatter setting specified? If I see correctly, the SWT proejcts don't have any project-specific formatter settings. This will result in changes being auto-formatted with whatever the developer has specified for their workspace or installation. In my opinion, if you enable auto-formatting, the project should also define what its proper formatting rules are (like done in the other platform projects).
2. Why are the additions of the jdt.ui cleanup preferences necessary?

[fedejeanne]

1. Why should we enable auto-formatting for projects that do not have formatter setting specified? If I see correctly, the SWT proejcts don't have any project-specific formatter settings. This will result in changes being auto-formatted with whatever the developer has specified for their workspace or installation. In my opinion, if you enable auto-formatting, the project should also define what its proper formatting rules are (like done in the other platform projects).

Good point. I suggest using the "Eclipse [built-in]" profile. Agreed @HeikoKlare / @HannesWell ?

2. Why are the additions of the jdt.ui cleanup preferences necessary?

@HeikoKlare which ones do you mean and in which file(s)?

[HeikoKlare]

Good point. I suggest using the "Eclipse [built-in]" profile. Agreed @HeikoKlare / @HannesWell ?

That's the one we use in the Platform repos, so yes, that's probably reasonable. But it would be good to check whether the projects to which you apply it are in any way conforming to those settings.

@HeikoKlare which ones do you mean and in which file(s)?

I meant every file with further changes to cleanup preferences than only the formatter thing, but it now actually just shows that I misunderstood this PR. I expected the changes to be applied to auto-save actions, but it's actually about cleanup. Then I ask a different question: why do we need to configure clean-up actions at all? I don't think they are maintained in any way and I do not see a real value for having them configured for these project. Executing them will just lead to a bunch a changes to the project (even without the formatter configuration), but applying the cleanup actions is a manual process anyway. In case we want to have some automatic enforcement of specific rules, I see a better a way in getting the auto-save actions up to day in a way as done in the platform repo:

1. Check which options we want to have automatically ensured by auto-save actions.
2. Execute cleanup actions for these single options to have the code conform to them and check whether any problematic changes are made by a single cleanup action.
3. Enable the option(s) for the auto-save actions.

In general, I see a mismatch between the project that should be affected by this PR (according to the commit message) and the actually modified projects.

[fedejeanne]

Good point. I suggest using the "Eclipse [built-in]" profile. Agreed @HeikoKlare / @HannesWell ?

That's the one we use in the Platform repos, so yes, that's probably reasonable. But it would be good to check whether the projects to which you apply it are in any way conforming to those settings.

👍

@HeikoKlare which ones do you mean and in which file(s)?

I meant every file with further changes to cleanup preferences than only the formatter thing, but it now actually just shows that I misunderstood this PR. I expected the changes to be applied to auto-save actions, but it's actually about cleanup.

It is actually about the save actions. I originally changed the 2 preferences (sp_cleanup.format_source_code and sp_cleanup.format_source_code_changes_only) under Java Editor > Save Actions:

Then I ask a different question: why do we need to configure clean-up actions at all? I don't think they are maintained in any way and I do not see a real value for having them configured for these project. Executing them will just lead to a bunch a changes to the project (even without the formatter configuration), but applying the cleanup actions is a manual process anyway.

The name of the configuration looks confusing because of the prefix sp_cleanup but I'm actually only interested in the automatic save action in this PR and not in the clean-ups that can be run via (Right click ->) Source -> Clean Up. The purpose of this PR is to:

1. Automatically formatting edited code when saving
2. Enable a follow-up PR to reformat the code consistently by doing a (Right click ->) Source -> Format (see https://github.com/eclipse-platform/eclipse.platform.swt/pull/1238#issuecomment-2117619024 and https://github.com/eclipse-platform/eclipse.platform.swt/pull/1238#issuecomment-2117624498).

I changed the description of this PR to clarify these points.

In case we want to have some automatic enforcement of specific rules, I see a better a way in getting the auto-save actions up to day in a way as done in the platform repo:

1. Check which options we want to have automatically ensured by auto-save actions.

2. Execute cleanup actions for these single options to have the code conform to them and check whether any problematic changes are made by a single cleanup action.

3. Enable the option(s) for the auto-save actions.

I'm not interested in any other ~clean-up~ save actions in this PR, just formatting the edited code automatically when saving.

In general, I see a mismatch between the project that should be affected by this PR (according to the commit message) and the actually modified projects.

I changed the commit text

[HeikoKlare]

It is actually about the save actions. I originally changed the 2 preferences (sp_cleanup.format_source_code and sp_cleanup.format_source_code_changes_only) under Java Editor > Save Actions:

Thanks for clarification. I mixed up the properties names. Having the commit message adapted according is good.

I'm not interested in any other ~clean-up~ save actions in this PR, just formatting the edited code automatically when saving.

My comment was based on the assumption that this affects clean-up operations rather than save actions. Still, the PR is not limited to enabling auto-formatting edited code. It also enables a bunch of other save actions in some of the projects, like org.eclipse.swt.examples.watchdog.

[fedejeanne]

My comment was based on the assumption that this affects clean-up operations rather than save actions. Still, the PR is not limited to enabling auto-formatting edited code. It also enables a bunch of other save actions in some of the projects, like org.eclipse.swt.examples.watchdog.

I changed the newly added JDT UI preferences so they only set the necessary configurations, i.e. they went from this:

... to this:

[fedejeanne]

@iloveeclipse have I addressed your comments?

[iloveeclipse]

@iloveeclipse have I addressed your comments?

Yes, thanks.

Where are the new org.eclipse.jdt.core.prefs formatter settings coming from? I haven't reviewed every file, I assume they are all copy/pasted from some single source.

[fedejeanne]

Where are the new org.eclipse.jdt.core.prefs formatter settings coming from? I haven't reviewed every file, I assume they are all copy/pasted from some single source.

They were automatically created when I enabled the project-specific settings for the formatter and selected the "Eclipse [built-in]" profile, i.e. they do not come from a single source but "from the UI (preferences dialog)"

[HannesWell]

Having unified code formatting and a code that has it applied is something that I think is good in general.

Since this PR adds many new lines of preferences, have you considered to link the jdt preference files from a common location e.g. from the examples or tests folder? (like it is done for the native fragments? This would save many lines and would ensure that they are consistent in the future. On the long run, we hopefully have something like https://github.com/eclipse-platform/eclipse.platform/issues/89

[HeikoKlare]

Since this PR adds many new lines of preferences, have you considered to link the jdt preference files from a common location e.g. from the examples or tests folder? (like it is done for the native fragments? This would save many lines and would ensure that they are consistent in the future.

I really like that idea. @Bananeweizen explained in a platform PR that they copy preferences files once in a while from a central place to have them consistent across all projects (https://github.com/eclipse-platform/eclipse.platform/pull/1282#issuecomment-2039138299). Linking them instead sounds like an even better solution. You just have to be aware that whenever you change something in the according preferences, all projects (using that same linked file) will be affected, but I think that's acceptable as they will not be changed often and should then be reviewed anyway.

On the long run, we hopefully have something like eclipse-platform/eclipse.platform#89

Thank you for that pointer. I was not aware of that idea. It sounds really useful.

[fedejeanne]

Since this PR adds many new lines of preferences, have you considered to link the jdt preference files from a common location e.g. from the examples or tests folder? (like it is done for the native fragments?

@HannesWell I like the idea but it has a big drawback: only the whole .settings folder may be linked (linking single files don't work). This would mean not only that formatting the code is enabled but that all configurations are the same. The purpose of this PR is to only enable the formatting of edited lines without altering other configurations (see https://github.com/eclipse-platform/eclipse.platform.swt/pull/1238#issuecomment-2122654447)

I propose we leave that for the future since we would need to agree on what are some sinful shared settings for all examples and tests.

[HeikoKlare]

I like the idea but it has a big drawback: only the whole .settings folder may be linked (linking single files don't work).

You can also link files, like done for .classpath files in the SWT fragments, e.g.: https://github.com/eclipse-platform/eclipse.platform.swt/blob/08473c2c0677272b670646b2aafa7b0772a04e7c/binaries/org.eclipse.swt.win32.win32.x86_64/.project#L35-L39

And even for the .api_filters within the .settings folder (just like it would be for the preferences file): https://github.com/eclipse-platform/eclipse.platform.swt/blob/08473c2c0677272b670646b2aafa7b0772a04e7c/binaries/org.eclipse.swt.win32.win32.x86_64/.project#L40-L44

[fedejeanne]

Thank you for the hint @HeikoKlare , I was doing it wrong (I was creating links from the Windows Explorer), that's why it didn't work.

I replaced the newly created configurations with links and stored the contents under examples\.settings_global.

Some points to consider:

• The global settings are not (yet) used by all projects, only by some example projects. Unifying the settings is still not a goal of this PR
• The name of the folder could be changed to say .settings, I just felt that adding the _global suffix would suit it better.
• There are 2 commits in this PR. Once you give the green light, I will squash them.

[HannesWell]

Some points to consider:

• The global settings are not (yet) used by all projects, only by some example projects. Unifying the settings is still not a goal of this PR

Could this then be done in advance (in another PR if you prefer)? This now adds even more lines than before. I haven't looked in the details yet, but is there a reason why different example projects should have the different JDT settings?

• The name of the folder could be changed to say .settings, I just felt that adding the _global suffix would suit it better.

That would is fine for me. Or maybe _common since it's not shared by all?

[HannesWell]

Some points to consider:

• The global settings are not (yet) used by all projects, only by some example projects. Unifying the settings is still not a goal of this PR

Could this then be done in advance (in another PR if you prefer)? This now adds even more lines than before. I haven't looked in the details yet, but is there a reason why different example projects should have the different JDT settings?

I had a short look into this and unless you want to provide a PR for that, I could do it this evening. At least unifying the jdt.core preferences should be doable in advance.

[HannesWell]

Some points to consider:

• The global settings are not (yet) used by all projects, only by some example projects. Unifying the settings is still not a goal of this PR

Could this then be done in advance (in another PR if you prefer)? This now adds even more lines than before. I haven't looked in the details yet, but is there a reason why different example projects should have the different JDT settings?

I had a short look into this and unless you want to provide a PR for that, I could do it this evening. At least unifying the jdt.core preferences should be doable in advance.

Please see https://github.com/eclipse-platform/eclipse.platform.swt/pull/1253.

[fedejeanne]

Please see #1253.

@HannesWell thank you! I just reviewed it, it looks very good :-) . I will then leave this PR on hold (as a draft) until #1253 is merged and rebase my changes in order to avoid duplication/inconsistencies.

[akurtakov]

There are a number of conflicts here. Do you plan to finish this one or it should be abandoned?

[fedejeanne]

There are a number of conflicts here. Do you plan to finish this one or it should be abandoned?

I'll take a look at it next week (I'm in Mainz for the OCX this week) and I will fix it/abandon it accordingly. I can always create a new PR if I come back to it in the future.