IEP-1427 GH #1130: Espressif-IDE LSP Editor is using sdkconfig.h from the last built project for ALL other projects #1155
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request updates how the LSP service receives its compile commands configuration. In the core build configuration, the method call is changed to directly use a build command variable, and a corresponding new method is added in the LSP service to handle the update. Additionally, new UI components are introduced: a startup extension is registered in the plugin configuration, and listener classes are implemented to respond to file open events by updating the LSP configuration. Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/ListenerStartup.java (2)
1-4: Update copyright year.The copyright year is set to 2025, which is incorrect as it's beyond the current year.
-* Copyright 2025 Espressif Systems (Shanghai) PTE LTD. All rights reserved. +* Copyright 2024 Espressif Systems (Shanghai) PTE LTD. All rights reserved.
19-37: Consider adding error handling and cleanup.While the implementation is correct, consider these improvements:
- Add error handling for UI operations.
- Implement cleanup/disposal of the listener when the plugin is stopped.
Here's a suggested implementation with error handling:
@Override public void earlyStartup() { - IWorkbench workbench = PlatformUI.getWorkbench(); + try { + IWorkbench workbench = PlatformUI.getWorkbench(); - // Execute on the UI thread - workbench.getDisplay().asyncExec(() -> { - IWorkbenchWindow window = workbench.getActiveWorkbenchWindow(); - if (window != null) - { - IWorkbenchPage page = window.getActivePage(); - if (page != null) - { - partListener = new FileOpenListener(); - page.addPartListener(partListener); - } - } - }); + // Execute on the UI thread + workbench.getDisplay().asyncExec(() -> { + try { + IWorkbenchWindow window = workbench.getActiveWorkbenchWindow(); + if (window != null) + { + IWorkbenchPage page = window.getActivePage(); + if (page != null) + { + partListener = new FileOpenListener(); + page.addPartListener(partListener); + } + } + } catch (Exception e) { + // Log error using Eclipse's logging mechanism + StatusManager.getManager().handle( + new Status(IStatus.ERROR, "com.espressif.idf.ui", + "Error initializing FileOpenListener", e)); + } + }); + } catch (Exception e) { + // Log error using Eclipse's logging mechanism + StatusManager.getManager().handle( + new Status(IStatus.ERROR, "com.espressif.idf.ui", + "Error in earlyStartup", e)); + } }And add a cleanup method:
public void dispose() { if (partListener != null) { try { IWorkbench workbench = PlatformUI.getWorkbench(); IWorkbenchWindow window = workbench.getActiveWorkbenchWindow(); if (window != null) { IWorkbenchPage page = window.getActivePage(); if (page != null) { page.removePartListener(partListener); } } } catch (Exception e) { // Log error using Eclipse's logging mechanism StatusManager.getManager().handle( new Status(IStatus.ERROR, "com.espressif.idf.ui", "Error disposing FileOpenListener", e)); } partListener = null; } }bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/LspService.java (2)
71-88: LGTM! Consider adding null checks.The method correctly updates the compile commands directory while preserving other additional options. However, consider adding null checks for the preferences.get() call.
Apply this diff to add null checks:
- String existingOptions = preferences.get(identifier, StringUtil.EMPTY); + String existingOptions = preferences.get(identifier, null); + if (existingOptions == null) { + existingOptions = StringUtil.EMPTY; + }
82-85: Use a more precise regex pattern.The current regex pattern
.+is greedy and might match more than intended. Consider using a non-greedy pattern or a more specific one that stops at the next option.Apply this diff to use a more precise pattern:
- ? existingOptions.replaceAll(compileCommandsDirString + ".+", //$NON-NLS-1$ + ? existingOptions.replaceAll(compileCommandsDirString + "[^ ]+", //$NON-NLS-1$bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/FileOpenListener.java (3)
1-4: Update the copyright year.The copyright year is set to 2025, but it should reflect when the file was created (likely 2024 or earlier).
-* Copyright 2025 Espressif Systems (Shanghai) PTE LTD. All rights reserved. +* Copyright 2024 Espressif Systems (Shanghai) PTE LTD. All rights reserved.
24-28: Consider adding documentation for empty methods.While it's acceptable to have empty implementations for unused interface methods, adding documentation explaining why they're empty would improve maintainability.
Example:
@Override public void partOpened(IWorkbenchPartReference partRef) { + // Not used: We only need to handle part activation events }Also applies to: 58-87
34-39: Add file type validation.The method should check if the file is relevant for LSP processing (e.g., C/C++ files) before updating the LSP configuration to avoid unnecessary updates.
if (part instanceof IEditorPart ieditorpart && ieditorpart.getEditorInput() instanceof FileEditorInput fileInput) { IFile file = fileInput.getFile(); + String fileExtension = file.getFileExtension(); + if (fileExtension == null || (!fileExtension.equals("c") && !fileExtension.equals("cpp") && !fileExtension.equals("h") && !fileExtension.equals("hpp"))) { + return; + } IProject project = file.getProject();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/LspService.java(2 hunks)bundles/com.espressif.idf.ui/plugin.xml(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/ListenerStartup.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/FileOpenListener.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_macos
🔇 Additional comments (3)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/ListenerStartup.java (1)
15-18: LGTM! Clean class structure.The class correctly implements the IStartup interface and maintains a reference to the FileOpenListener.
bundles/com.espressif.idf.ui/plugin.xml (1)
570-572: LGTM! Startup extension is correctly configured.The startup extension is properly added with the correct class reference.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/FileOpenListener.java (1)
21-22: LGTM!The class name is descriptive and appropriately implements the
IPartListener2interface for handling Eclipse UI part lifecycle events.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java
Outdated
Show resolved
Hide resolved
| String buildDir = StringUtil.EMPTY; | ||
| try | ||
| { | ||
| buildDir = IDFUtil.getBuildDir(project); | ||
| } | ||
| catch (CoreException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| LspService lspService = new LspService(); | ||
| lspService.updateCompileCommandsDir(buildDir); | ||
| lspService.restartLspServers(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and resource management.
Several improvements can be made to the implementation:
- The
buildDirvariable should be null-checked before use - Consider reusing the
LspServiceinstance instead of creating a new one for each activation - Add logging for successful updates and potential LSP server restart failures
Here's the suggested implementation:
- String buildDir = StringUtil.EMPTY;
+ String buildDir = null;
try
{
buildDir = IDFUtil.getBuildDir(project);
+ if (buildDir == null || buildDir.isEmpty()) {
+ Logger.log("Failed to get build directory for project: " + project.getName());
+ return;
+ }
+
+ LspService lspService = LspService.getInstance(); // Assuming singleton pattern
+ lspService.updateCompileCommandsDir(buildDir);
+ lspService.restartLspServers();
+ Logger.log("Successfully updated LSP configuration for project: " + project.getName());
}
catch (CoreException e)
{
Logger.log(e);
+ } catch (Exception e) {
+ Logger.log("Failed to restart LSP servers: " + e.getMessage());
}
- LspService lspService = new LspService();
- lspService.updateCompileCommandsDir(buildDir);
- lspService.restartLspServers();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| String buildDir = StringUtil.EMPTY; | |
| try | |
| { | |
| buildDir = IDFUtil.getBuildDir(project); | |
| } | |
| catch (CoreException e) | |
| { | |
| Logger.log(e); | |
| } | |
| LspService lspService = new LspService(); | |
| lspService.updateCompileCommandsDir(buildDir); | |
| lspService.restartLspServers(); | |
| } | |
| String buildDir = null; | |
| try | |
| { | |
| buildDir = IDFUtil.getBuildDir(project); | |
| if (buildDir == null || buildDir.isEmpty()) { | |
| Logger.log("Failed to get build directory for project: " + project.getName()); | |
| return; | |
| } | |
| LspService lspService = LspService.getInstance(); // Assuming singleton pattern | |
| lspService.updateCompileCommandsDir(buildDir); | |
| lspService.restartLspServers(); | |
| Logger.log("Successfully updated LSP configuration for project: " + project.getName()); | |
| } | |
| catch (CoreException e) | |
| { | |
| Logger.log(e); | |
| } catch (Exception e) { | |
| Logger.log("Failed to restart LSP servers: " + e.getMessage()); | |
| } |
|
@sigmaaa hi ! Tested under: the LSP editor now uses the sdkconfig.h file of particular project not from the last built project for all projects as it was before ✅ But the same path is empty under "Preferences - clangd" : |
|
@sigmaaa hi ! Tested under: Mac OS ✅ |
|
Hi @sigmaaa Can you resolve conflicts and merge this PR? |
Description
Added a part listener that updates Clang's additional options and restarts the LSP server when files from different projects are opened.
Improved handling of the --compile-commands-dir option. Previously, building the project replaced all additional options with a single --compile-commands-dir entry; now, only this option is updated, preserving any other options.
Fixes # (IEP-1427)
Type of change
Please delete options that are not relevant.
How has this been tested?
The good use case is mentioned in this ticket: #1130
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit