Refactor clangd preferences #196

ghentschke · 2023-09-08T15:07:34Z

separated editor from clangd preferences (needed for #178)

ghentschke · 2023-09-11T12:14:02Z

Okay, IMO the refactoring is done. When this LSP4E issue has been merged, I'll apply the FormatOnSave implementation.

I know, it has become a larger PR :-/
But I hope @ruspl-afed you have time to take a look.

jonahgraham · 2023-09-11T13:14:01Z

Okay, IMO the refactoring is done. When this eclipse-lsp4e/lsp4e#783 has been merged, I'll apply the FormatOnSave implementation.

Does this mean that CDT LSP will depend on a new version of LSP4E being released with that PR?

ghentschke · 2023-09-11T14:08:02Z

Does this mean that CDT LSP will depend on a new version of LSP4E being released with that PR?

Yes. Thats's why I commented out FormatOnSave. But when I think about it, it would be better to remove the format-on-save stuff from this refactoring PR.

ghentschke · 2023-09-11T14:32:33Z

Hm, after merging the master into the PR the licence check failed.

jonahgraham · 2023-09-11T14:36:12Z

Hm, after merging the master into the PR the licence check failed.

Looks like temp error - I have restarted the check

jonahgraham · 2023-09-11T14:36:52Z

BTW the error was:

Failed to execute goal org.eclipse.dash:license-tool-plugin:1.0.3-SNAPSHOT:license-check (default-cli) on project org.eclipse.cdt.lsp.root: Execution default-cli of goal org.eclipse.dash:license-tool-plugin:1.0.3-SNAPSHOT:license-check failed: Plugin org.eclipse.dash:license-tool-plugin:1.0.3-SNAPSHOT or one of its dependencies could not be resolved: Failed to collect dependencies at org.eclipse.dash:license-tool-plugin:jar:1.0.3-20230911.055036-111 -> org.eclipse.dash:org.eclipse.dash.licenses:jar:1.0.3-SNAPSHOT: Failed to read artifact descriptor for org.eclipse.dash:org.eclipse.dash.licenses:jar:1.0.3-SNAPSHOT: The following artifacts could not be resolved: org.eclipse.dash:org.eclipse.dash.licenses:pom:1.0.3-20230911.055036-111 (absent): Could not transfer artifact org.eclipse.dash:org.eclipse.dash.licenses:pom:1.0.3-20230911.055036-111 from/to dash-licenses-snapshots (https://repo.eclipse.org/content/repositories/dash-licenses-snapshots/): status code: 403, reason phrase: Forbidden (403) -> [Help 1]

jonahgraham · 2023-09-11T14:37:40Z

Hm, after merging the master into the PR the licence check failed.

Looks like temp error - I have restarted the check

Worked now.

ghentschke · 2023-09-13T16:25:56Z

In my latest commit I added the format-on-save feature again, because the LSP4E PR #783 has been merged.

@jonahgraham Does this bring us into trouble, because the cdt-lsp release depends then on the LSP4E snapshot?

jonahgraham

In my latest commit I added the format-on-save feature again, because the LSP4E PR #783 has been merged.

@jonahgraham Does this bring us into trouble, because the cdt-lsp release depends then on the LSP4E snapshot?

Yes :-(

A few items aren't working as expected (but I haven't looked at preferences closely in a while, so some of these may not be new):

The "Enable project-specific settings" checkbox in the UI is not initialized correctly in the project properties. As you can see in this screenshot the settings say to format, but opening the page it is unchecked:

The "Configure Workspace Settings" in the project properties goes to the correct preference page, but the tree on the left is incorrect only showing the root node

jonahgraham · 2023-09-13T18:11:32Z

bundles/org.eclipse.cdt.lsp/src/org/eclipse/cdt/lsp/editor/format/FormatOnSave.java

+import org.osgi.service.component.annotations.Reference;
+
+@Component(property = { "serverDefinitionId:String=org.eclipse.cdt.lsp.server" })
+public class FormatOnSave implements IFormatRegionsProvider {


With this new class reference we should the MANIFEST entry should be updated to a version that will include it - i.e.:

org.eclipse.lsp4e;bundle-version="0.17.2",

I made a LSP4E PR to bump the version to 0.17.3. Then we can increase it here to 0.17.3 as well

Unfortunately that means we can't release CDT LSP because it depends on an unreleased LSP4E. We can:

Not merge this change and do release of 1.0.0 as is

Remove the format on save changes from this PR, so that we are dependent on 0.17.1 of LSP4E

Request and wait for LSP4E to release and then add the new version of LSP4E to our p2 site so it can be installed by users

WDYT?

Option 4 is we do a soft/milestone "release" which can depend on snapshot LSP4E

Remove the format on save changes from this PR, so that we are dependent on 0.17.1 of LSP4E

I would prefer this one. I'll put the format-on-save stuff in a new PR.

bundles/org.eclipse.cdt.lsp/src/org/eclipse/cdt/lsp/editor/EditorConfigurationPage.java

There is no OSGi service implementation needed in LSP4E. The service should be provided by external bundles.

- separated editor from clangd preferences (needed for eclipse-cdt#178)

since the classes are used in project properties and workspace preferences.

- make lsp.editor classes extendable for clangd

it should be checked in performOk only

to prevent duplicated code

can be (re)added when LSP4E PR eclipse-lsp4e/lsp4e#783 has been merged

because eclipse-lsp4e/lsp4e#783 has been merged

The restart clangd dialog should be opened when: - The clangd options have been modified or - The user modifies the 'Enable project-specific settings' and the clangd options on project level differ from workspace or - 'Restore Defaults' in project scope should trigger dialog only if the project options differ from workspace.

to 0.17.3

since it depends on not yet released LSP4E dependency.

ghentschke · 2023-09-15T13:30:25Z

@jonahgraham Please check the last two commits. I removed the format-on-save stuff and dependency on LSP4E 0.17.3

jonahgraham

LGTM - once merged I will start release process

jonahgraham · 2023-09-15T14:55:37Z

LGTM - once merged I will start release process

PS I will create an endgame issue like eclipse-cdt/cdt#548, and I'll document all the steps I take to try to make sure that others are able to do it in the future.

ghentschke · 2023-09-15T16:41:45Z

@jonahgraham Thank you!

ghentschke changed the title ~~Refactor clangd prefernces~~ Refactor clangd preferences Sep 8, 2023

ghentschke requested a review from ruspl-afed September 8, 2023 15:14

ghentschke force-pushed the implement-IFormatOnSave branch from 7d5723f to 8f87122 Compare September 8, 2023 15:30

ghentschke requested a review from jonahgraham September 13, 2023 05:55

jonahgraham reviewed Sep 13, 2023

View reviewed changes

bundles/org.eclipse.cdt.lsp/src/org/eclipse/cdt/lsp/editor/EditorConfigurationPage.java Outdated Show resolved Hide resolved

ghentschke and others added 9 commits September 14, 2023 06:03

Remove obsolete default implementation file

6d9796d

There is no OSGi service implementation needed in LSP4E. The service should be provided by external bundles.

Refactor clangd preferences

fc83495

- separated editor from clangd preferences (needed for eclipse-cdt#178)

rename page classes

1691957

since the classes are used in project properties and workspace preferences.

Refactor clangd prefs

09fe771

- make lsp.editor classes extendable for clangd

do not check restart in performDefaults

4c65637

it should be checked in performOk only

Use super implementation

470a31e

to prevent duplicated code

Remove 'Save Action' form pref and properties page

52270d6

can be (re)added when LSP4E PR eclipse-lsp4e/lsp4e#783 has been merged

Add 'Save Action' form pref and properties page

3a977a2

because eclipse-lsp4e/lsp4e#783 has been merged

Fix preference bugs

c01c941

ghentschke force-pushed the implement-IFormatOnSave branch from 57d214b to c01c941 Compare September 14, 2023 04:25

ghentschke and others added 4 commits September 14, 2023 11:05

Update LSP4E minimum bundle-version

e658a5f

to 0.17.3

Remove format-on-save for first release

21d1b77

since it depends on not yet released LSP4E dependency.

Remove save actions name

7d67b7f

ghentschke requested a review from jonahgraham September 15, 2023 13:31

jonahgraham approved these changes Sep 15, 2023

View reviewed changes

ghentschke merged commit aaf3f80 into eclipse-cdt:master Sep 15, 2023

jonahgraham added this to the 1.0.0 milestone Sep 18, 2023

ghentschke deleted the implement-IFormatOnSave branch September 19, 2023 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor clangd preferences #196

Refactor clangd preferences #196

ghentschke commented Sep 8, 2023 •

edited

Loading

ghentschke commented Sep 11, 2023

jonahgraham commented Sep 11, 2023

ghentschke commented Sep 11, 2023 •

edited

Loading

ghentschke commented Sep 11, 2023

jonahgraham commented Sep 11, 2023

jonahgraham commented Sep 11, 2023

jonahgraham commented Sep 11, 2023

ghentschke commented Sep 13, 2023 •

edited

Loading

jonahgraham left a comment

jonahgraham Sep 13, 2023

ghentschke Sep 14, 2023

jonahgraham Sep 14, 2023

jonahgraham Sep 14, 2023

ghentschke Sep 15, 2023

ghentschke commented Sep 15, 2023

jonahgraham left a comment

jonahgraham commented Sep 15, 2023

ghentschke commented Sep 15, 2023

Refactor clangd preferences #196

Refactor clangd preferences #196

Conversation

ghentschke commented Sep 8, 2023 • edited Loading

ghentschke commented Sep 11, 2023

jonahgraham commented Sep 11, 2023

ghentschke commented Sep 11, 2023 • edited Loading

ghentschke commented Sep 11, 2023

jonahgraham commented Sep 11, 2023

jonahgraham commented Sep 11, 2023

jonahgraham commented Sep 11, 2023

ghentschke commented Sep 13, 2023 • edited Loading

jonahgraham left a comment

Choose a reason for hiding this comment

jonahgraham Sep 13, 2023

Choose a reason for hiding this comment

ghentschke Sep 14, 2023

Choose a reason for hiding this comment

jonahgraham Sep 14, 2023

Choose a reason for hiding this comment

jonahgraham Sep 14, 2023

Choose a reason for hiding this comment

ghentschke Sep 15, 2023

Choose a reason for hiding this comment

ghentschke commented Sep 15, 2023

jonahgraham left a comment

Choose a reason for hiding this comment

jonahgraham commented Sep 15, 2023

ghentschke commented Sep 15, 2023

ghentschke commented Sep 8, 2023 •

edited

Loading

ghentschke commented Sep 11, 2023 •

edited

Loading

ghentschke commented Sep 13, 2023 •

edited

Loading