[#227] Refactor ClangdConfigurationManager #229

ghentschke · 2024-01-14T21:39:37Z

Allows vendors to overwrite default behavior

fixes #227

- Allows vendors to overwrite default behavior fixes eclipse-cdt#227

ghentschke · 2024-01-14T21:40:02Z

Still some unit test issues.

ruspl-afed

From #227 (comment) :

So, this should also be a default setup (or at least to be achievable without writing your own plugin) with CDT LSP.

I would say "default setup" request is questionable, but "achievable without writing your own plugin" sounds reasonable. Does this PR satisfy the mentioned need?

ruspl-afed · 2024-01-15T06:35:45Z

.../org.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/editor/ClangdPlugin.java

@@ -48,14 +51,15 @@ public void start(BundleContext context) throws Exception {
 		workspaceTracker.open();
 		workspace = workspaceTracker.getService();
 		compileCommandsMonitor = new CompileCommandsMonitor(workspace).start();
-		cProjectChangeMonitor = new CProjectChangeMonitor().start();
+		cProjectChangeMonitor = Optional.ofNullable(PlatformUI.getWorkbench().getService(CProjectChangeMonitor.class))


component may have methods annotated with
@Activate and @Deactivate, so we can avoid this code

Thanks for the hint. I'll check that.

Please take a look at my last commit

ruspl-afed · 2024-01-15T06:36:35Z

tests/org.eclipse.cdt.lsp.clangd.tests/src/org/eclipse/cdt/lsp/clangd/tests/TestUtils.java

@@ -61,6 +61,24 @@ public static IProject createCProject(String projectName) throws CoreException {
 		return project;
 	}

+	//	static IProject createEmptyCMakeProject(String projectName) {


Do we need this code or not? If not, let's remove it

No, can be removed

ruspl-afed · 2024-01-15T06:39:12Z

...es/org.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/ClangdConfigurationManager.java

+ * Gesa Hentschke (Bachmann electronic GmbH) - initial implementation
+ *******************************************************************************/
+
+package org.eclipse.cdt.lsp.clangd;


Usually the component is a part of implementation, and not a part of [potential] API, it should be placed to internal package

I've to rework this part as I stumped about this https://docs.osgi.org/specification/osgi.cmpn/7.0.0/service.component.html#:~:text=A%20bundle%20containing,components%20being%20activated.

Please take a look at my last commit

ruspl-afed · 2024-01-15T06:44:24Z

...eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/ClangdCProjectDescriptionListener.java

+ * with a service.ranking property > 0 to implement custom behavior
+ * and to replace the {@link ClangdConfigurationManager}
+ */
+public interface ClangdCProjectDescriptionListener {


this interface needs more work, it has 3 methods with tightly-coupled implementation and all of them are open for public invocation. Ideally there should be only one.

Should be fixed with my last commit

- Fix unit tests fixes eclipse-cdt#227

- Support cmake projects as well fixes eclipse-cdt#227

- Add org.eclipse.cdt.cmake.core to target definition fixes eclipse-cdt#227

ghentschke · 2024-01-15T10:25:55Z

From #227 (comment) :

So, this should also be a default setup (or at least to be achievable without writing your own plugin) with CDT LSP.

I would say "default setup" request is questionable, but "achievable without writing your own plugin" sounds reasonable. Does this PR satisfy the mentioned need?

Yes. They have to implement either CProjectChangeMonitor as an OSGi service if they don't want the listener to CProject changes at all (the start() and stop() methods do nothing in that case),
or implement ClangdCProjectDescriptionListener and overwrite enableSetCompilationDatabasePath(IProject project)

I put the default implementations in the org.eclipse.cdt.lsp.clangd package to allow vendors to extend the defualt implementations (can be usefull when only one methods like enableSetCompilationDatabasePath(IProject project) should be overwritten). That's why the interface ClangdCProjectDescriptionListener has 3 (tightly coupled) methods. The ClangdConfigurationFileManager can be seen as an base class for vendors.

ruspl-afed · 2024-01-15T11:16:48Z

Yes, I understand the idea of the fix. My point is that class with @Component annotation usually is not a part of API, however it can extend the API class.

Also, I had an impression that the original request was to have something like preference-controlled strategy rather than API to override. But let's wait for a feedback.

- Fix bundle activator/Service Component Runtime (SCR) problem: The Bundle Activator's start method must not rely upon SCR having activated any of the bundle's components. However, the components can rely upon the Bundle Activator's start method having been called. That is, there is a happens-before relationship between the Bundle Activator's start method being run and the components being activated. fixes eclipse-cdt#227

travkin79 · 2024-01-17T20:43:36Z

...rg.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/ClangdConfigurationFileManager.java

+	 * @param project managed C/C++ project
+	 * @param newCProjectDescription new CProject description
+	 * @param macroResolver helper to resolve macros in the CWD path of the builder
+	 */
 	public void setCompilationDatabasePath(IProject project, ICProjectDescription newCProjectDescription,


I think, since this is no longer part of ClangdCProjectDescriptionListener interface, it's enough to make this method protected.

travkin79 · 2024-01-17T20:43:56Z

...rg.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/ClangdConfigurationFileManager.java

+	 * Enabler for {@link setCompilationDatabasePath}. Can be overriden for customization.
+	 * @param project
+	 * @return true if the database path should be written to .clangd file in the project root.
+	 */
 	public boolean enableSetCompilationDatabasePath(IProject project) {


I think, since this is no longer part of ClangdCProjectDescriptionListener interface, it's enough to make this method protected.

travkin79 · 2024-01-17T20:53:08Z

Also, I had an impression that the original request was to have something like preference-controlled strategy rather than API to override. But let's wait for a feedback.

I'm not sure how such a preference for our case could look like. Disabling the creation of .clangd files per project with a setting would be tedious in our case since it would involve configuring quite a lot of projects. Thus, it's absolutely good enough for our case to sub-class the default implementation, the ClangdConfigurationFileManager class.

I've implemented a working prototype and it works as expected. Thank you @ghentschke.

- make methods protected since they aren't part of the interface anymore. fixes eclipse-cdt#227

[eclipse-cdt#227] Refactor ClangdConfigurationManager

70227d9

- Allows vendors to overwrite default behavior fixes eclipse-cdt#227

ghentschke requested review from jonahgraham and ruspl-afed and removed request for jonahgraham January 14, 2024 21:39

ghentschke mentioned this pull request Jan 14, 2024

Stop creating .clangd file per Eclipse project if the config is defined elsewhere #227

Closed

ruspl-afed reviewed Jan 15, 2024

View reviewed changes

ghentschke added 3 commits January 15, 2024 08:43

[eclipse-cdt#227] Refactor ClangdConfigurationManager

701053b

- Fix unit tests fixes eclipse-cdt#227

[eclipse-cdt#227] Refactor ClangdConfigurationManager

95e8345

- Support cmake projects as well fixes eclipse-cdt#227

[eclipse-cdt#227] Refactor ClangdConfigurationManager

091a8df

- Add org.eclipse.cdt.cmake.core to target definition fixes eclipse-cdt#227

ghentschke requested a review from ruspl-afed January 17, 2024 17:06

travkin79 reviewed Jan 17, 2024

View reviewed changes

[eclipse-cdt#227] Refactor ClangdConfigurationManager

90b9e64

- make methods protected since they aren't part of the interface anymore. fixes eclipse-cdt#227

ghentschke merged commit 2fe1c1d into eclipse-cdt:master Jan 26, 2024
3 checks passed

ghentschke deleted the fix-issue-227 branch February 9, 2024 07:26

ghentschke added this to the 1.1.0 milestone Feb 19, 2024

travkin79 mentioned this pull request Feb 26, 2024

[#242] make API of org.eclipse.cdt.lsp.clangd package public #243

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#227] Refactor ClangdConfigurationManager #229

[#227] Refactor ClangdConfigurationManager #229

ghentschke commented Jan 14, 2024

ghentschke commented Jan 14, 2024

ruspl-afed left a comment

ruspl-afed Jan 15, 2024

ghentschke Jan 15, 2024

ghentschke Jan 17, 2024

ruspl-afed Jan 15, 2024

ghentschke Jan 15, 2024

ghentschke Jan 17, 2024

ruspl-afed Jan 15, 2024

ghentschke Jan 15, 2024

ghentschke Jan 17, 2024

ruspl-afed Jan 15, 2024

ghentschke Jan 17, 2024

ghentschke commented Jan 15, 2024

ruspl-afed commented Jan 15, 2024

travkin79 Jan 17, 2024

travkin79 Jan 17, 2024

travkin79 commented Jan 17, 2024

[#227] Refactor ClangdConfigurationManager #229

[#227] Refactor ClangdConfigurationManager #229

Conversation

ghentschke commented Jan 14, 2024

ghentschke commented Jan 14, 2024

ruspl-afed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghentschke commented Jan 15, 2024

ruspl-afed commented Jan 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travkin79 commented Jan 17, 2024