IEP 1517: Handling case for conversion of existing json file to eim_idf.json #1218
Conversation
alirana01
requested review from
kolipakakondal and
sigmaaa
and removed request for
kolipakakondal
WalkthroughThis update introduces a major refactor of the Espressif IDF Eclipse plugin, replacing the legacy toolset management with a new EIM (Espressif Installation Manager)-based configuration. It removes numerous legacy classes, wizards, handlers, and UI resources related to tool installation and management, and introduces new classes for EIM-based configuration, JSON parsing, change watching, and UI integration. Environment variable handling is simplified, and new startup and notification flows are implemented. Test timeouts and configurations are also updated. Changes
Sequence Diagram(s)sequenceDiagram
participant EclipseIDE
participant EspressifToolStartup
participant ToolInitializer
participant EimIdfConfiguratinParser
participant EimJsonWatchService
participant ESPIDFManagerEditor
EclipseIDE->>EspressifToolStartup: On early startup
EspressifToolStartup->>ToolInitializer: Check for old config & EIM installation
ToolInitializer-->>EspressifToolStartup: Returns config state
EspressifToolStartup->>EimIdfConfiguratinParser: Load EIM JSON
EimIdfConfiguratinParser-->>EspressifToolStartup: Returns EimJson
EspressifToolStartup->>ESPIDFManagerEditor: Open editor if needed
EspressifToolStartup->>EimJsonWatchService: Register UI change handler
EimJsonWatchService-->>EspressifToolStartup: Notify on JSON file changes
EspressifToolStartup->>EclipseIDE: Show notifications/dialogs as needed
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 21
♻️ Duplicate comments (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java (1)
185-185: Significantly increased test timeout for SDK Configuration tabSame concern as the previous method - the timeout was increased from 5 seconds to 15 minutes.
🧹 Nitpick comments (27)
tests/pom.xml (2)
33-33: Extract memory settings into a Maven property: Hardcoding-Xms4096m -Xmx8192min<argLine>can make future adjustments cumbersome. Define a property (e.g.,<test.memory.args>) and reference it:<properties> + <test.memory.args>-Xms4096m -Xmx8192m</test.memory.args> </properties> ... <argLine>${test.memory.args} -DtestRun=true</argLine>
32-35: Document thetestRunflag in documentation: Please update README or test guidelines to explain the purpose of-DtestRun=trueand how test-specific behavior is toggled..github/workflows/ci.yml (1)
29-29: Remove tabs and trailing spaces
Line 29 contains a tab character before the action name, and line 43 has trailing whitespace, both of which can cause YAML syntax errors. Please convert tabs to spaces and trim trailing spaces.Also applies to: 43-43
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 29-29: syntax error: found character '\t' that cannot start any token
(syntax)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/messages.properties (2)
1-7: Message keys naming convention inconsistency with refactoringThe message keys still reference "InstallToolsHandler" even though according to the PR summary and AI-generated summary, this class has been removed in favor of the new EIM-based approach. Consider renaming these keys to align with the new class names (e.g., SetupToolsInIde) for consistency.
-InstallToolsHandler_CopyingOpenOCDRules=Copying OpenOCD Rules -InstallToolsHandler_OpenOCDRulesCopyPaths=Copying File: %s to destination: %s -InstallToolsHandler_OpenOCDRulesCopyWarning=Warning -InstallToolsHandler_OpenOCDRulesCopyWarningMessage=The rules file is already present in the /etc/udev/rules.d/ directory, Do you want to replace the file with the file from OpenOCD directory -InstallToolsHandler_OpenOCDRulesNotCopied=Rules Not Copied to system -InstallToolsHandler_OpenOCDRulesCopyError=Unable to copy rules for OpenOCD to system directory, try running the eclipse with sudo command -InstallToolsHandler_OpenOCDRulesCopied=Rules Copied to system +SetupToolsInIde_CopyingOpenOCDRules=Copying OpenOCD Rules +SetupToolsInIde_OpenOCDRulesCopyPaths=Copying File: %s to destination: %s +SetupToolsInIde_OpenOCDRulesCopyWarning=Warning +SetupToolsInIde_OpenOCDRulesCopyWarningMessage=The rules file is already present in the /etc/udev/rules.d/ directory, Do you want to replace the file with the file from OpenOCD directory +SetupToolsInIde_OpenOCDRulesNotCopied=Rules Not Copied to system +SetupToolsInIde_OpenOCDRulesCopyError=Unable to copy rules for OpenOCD to system directory, try running the eclipse with sudo command +SetupToolsInIde_OpenOCDRulesCopied=Rules Copied to system
4-4: Improve readability of warning messageThe warning message is a bit lengthy and lacks proper punctuation, which might affect readability in a dialog box.
-InstallToolsHandler_OpenOCDRulesCopyWarningMessage=The rules file is already present in the /etc/udev/rules.d/ directory, Do you want to replace the file with the file from OpenOCD directory +InstallToolsHandler_OpenOCDRulesCopyWarningMessage=The rules file is already present in the /etc/udev/rules.d/ directory. Do you want to replace it with the file from OpenOCD directory?bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties (2)
22-23: Formatting issue in message textThe message at line 23 has a trailing space and an extra newline character at the end of the string, which might lead to unwanted spacing in the UI.
-ToolsInitializationEimMissingMsgBoxMessage=ESP-IDF is not found on your system. To use the IDE, install ESP-IDF using <a href="{0}">EIM - GUI Installer</a>. \n\nOnce installed, the IDE will automatically detect ESP-IDF. You can verify and activate it from the ESP-IDF Manager, accessible via the menu: Espressif > ESP-IDF Manager.\n\n +ToolsInitializationEimMissingMsgBoxMessage=ESP-IDF is not found on your system. To use the IDE, install ESP-IDF using <a href="{0}">EIM - GUI Installer</a>. \n\nOnce installed, the IDE will automatically detect ESP-IDF. You can verify and activate it from the ESP-IDF Manager, accessible via the menu: Espressif > ESP-IDF Manager.
29-32: Inconsistent spacing between message propertiesThere's an empty line between some message properties, which is inconsistent with the rest of the file.
IDFBuildConfiguration_PreCheck_DifferentIdfPath=The project was built using the ESP-IDF located at the {0} path.\nThe currently active ESP-IDF path in the IDE is {1}.\nPlease clean the project using "ESP-IDF:Project Full Clean" menu option to use the active ESP-IDF configuration. IDFToolChainsMissingErrorMsg=Toolchains are missing. Please use ESP-IDF Manager for configuring - NoActiveEspIdfInWorkspaceMsgTitle=ESP-IDF Setup NoActiveEspIdfInWorkspaceMsg=ESP-IDF is required to use Espressif IDE. Would you like to configure it now? -bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java (3)
8-18: Improve collection handling and add documentationThe class structure is good, but the collection handling could be improved and documentation should be added.
- Add class-level JavaDoc explaining this class's purpose (handling EIM configuration)
- Add defensive copying for the list getter/setter to prevent external modification:
public class EimJson { + /** + * Data model for the eim_idf.json configuration file that stores ESP-IDF and + * Espressif Installation Manager (EIM) settings. + */ @Expose private String eimPath; @Expose private String gitPath; @Expose private String idfSelectedId; @Expose - private List<IdfInstalled> idfInstalled = Collections.emptyList(); + private List<IdfInstalled> idfInstalled = Collections.emptyList();
39-47: Add defensive copying to protect collectionThe current implementation returns the internal list directly, which could lead to unexpected modifications.
public List<IdfInstalled> getIdfInstalled() { - return idfInstalled; + return Collections.unmodifiableList(idfInstalled); } public void setIdfInstalled(List<IdfInstalled> idfInstalled) { - this.idfInstalled = idfInstalled; + this.idfInstalled = idfInstalled != null ? + new ArrayList<>(idfInstalled) : Collections.emptyList(); }
1-60: Consider adding utility methods for object managementThis class would benefit from additional utility methods for better object-oriented design.
Consider adding:
- A
toString()method for debuggingequals()andhashCode()implementations- A builder pattern or fluent setters for easier construction
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java (1)
1-4: Incorrect copyright yearThe copyright year is set to 2025, which is in the future.
/******************************************************************************* - * Copyright 2025 Espressif Systems (Shanghai) PTE LTD. All rights reserved. + * Copyright 2023 Espressif Systems (Shanghai) PTE LTD. All rights reserved. * Use is subject to license terms. *******************************************************************************/bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.java (1)
1-3: Update copyright yearThe copyright year is set to 2025, which is in the future. It should be updated to the current year (2024).
-/******************************************************************************* - * Copyright 2025 Espressif Systems (Shanghai) PTE LTD. All rights reserved. - * Use is subject to license terms. +/******************************************************************************* + * Copyright 2024 Espressif Systems (Shanghai) PTE LTD. All rights reserved. + * Use is subject to license terms.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.java (1)
12-12: Fix spelling in class nameThere's a spelling error in the class name - "Configuratin" should be "Configuration".
-public class EimIdfConfiguratinParser +public class EimIdfConfigurationParserbundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.java (2)
3-6: Remove unused import
java.util.ArrayListis no longer referenced.
Cleaning unused imports avoids unnecessary recompilations and keeps the code tidy.
46-57: Graceful handling when configuration parsing fails
IfEimIdfConfiguratinParser#getEimJson(true)throws, the editor still opens with an emptyEimJson, but the user is not informed and an error is only logged.
Consider notifying the user (e.g. a warning dialog or Eclipse error log entry) so they understand why no IDF versions are listed.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/EimEditorInput.java (1)
25-31: Delegate adapter queries to the platform
Returningnullprevents downstream consumers from adapting this input. Use the AdapterManager:- public <T> T getAdapter(Class<T> adapter) - { - // TODO Auto-generated method stub - return null; - } + public <T> T getAdapter(Class<T> adapter) + { + return org.eclipse.core.runtime.Platform.getAdapterManager().getAdapter(this, adapter); + }bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/EspressifGeneralStartup.java (1)
82-92: Consider removing unnecessary suppressions.The
@SuppressWarnings("static-access")annotations may not be needed iflaunchBarListener.setIgnoreTargetChange(boolean)is an instance method rather than a static method.- @SuppressWarnings("static-access") private void disableLaunchBarEvents() { launchBarListener.setIgnoreTargetChange(true); } - @SuppressWarnings("static-access") private void enableLaunchBarEvents() { launchBarListener.setIgnoreTargetChange(false); }bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java (1)
64-83: Add specific error message for conversion failure.The error message "Error in conversion" is generic. Consider providing a more specific error message that indicates what went wrong during the conversion process.
- return new Status(IStatus.ERROR, IDFCorePlugin.getId(), -1, "Error in conversion", null); //$NON-NLS-1$ + return new Status(IStatus.ERROR, IDFCorePlugin.getId(), -1, + "Old configuration file not found or could not be converted", null); //$NON-NLS-1$bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.java (1)
69-75: Prevent duplicate listeners
CopyOnWriteArrayListallows duplicates, so callers registering twice will receive events twice. Add a containment check to keep semantics predictable:- eimJsonChangeListeners.add(listener); + if (!eimJsonChangeListeners.contains(listener)) + { + eimJsonChangeListeners.add(listener); + }bundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/JsonConfigServer.java (1)
157-164: Environment PATH merging may clobber case-sensitive keys on Windows
You overwrite whichever casing (PATH/Path) you discover first, but keep both if they already co-exist. Down-stream you removePathinProcessBuilderFactory.run(). Prefer normalising to one key here to avoid subtle path leaks.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (3)
148-151: Typos in status messages (“pyhton”, “web-socket”)Minor, but these appear in the UI/console:
- monitor.setTaskName("Installing pyhton dependency web-socket"); - log("Installing pyhton dependency web-socket", IStatus.INFO); + monitor.setTaskName("Installing python dependency websocket-client"); + log("Installing python dependency websocket-client", IStatus.INFO);
96-105: Variable name misspelled (arguemnts)Although consistent, the typo reduces readability and may propagate.
- List<String> arguemnts = new ArrayList<>(); + List<String> arguments = new ArrayList<>(); … - arguemnts = ToolsUtility.getExportScriptCommand(…); + arguments = ToolsUtility.getExportScriptCommand(…);
498-503: Duplicate env-vars in activation output may throwIllegalStateException
Collectors.toMapfails on duplicate keys. Some activation scripts echo the same variable twice (e.g., when sourcing multiple profiles).Consider using
Collectors.toMap(..., (v1, v2) -> v2)to retain the last value.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (1)
330-337: UnusedgitPathparameter inToolsUtility.getIdfVersionCall sites pass
eimJson.getGitPath()but the method ignores it, indicating either dead code or an unfinished feature.Drop the parameter or integrate its use (e.g., prepend Git to
PATH).bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (1)
27-33:gitPathparameter is unused
getIdfVersion(IdfInstalled idfInstalled, String gitPath)never referencesgitPath, yet every caller supplies it. This is misleading and clutters the API.-public static String getIdfVersion(IdfInstalled idfInstalled, String gitPath) +public static String getIdfVersion(IdfInstalled idfInstalled)Adjust callers accordingly, or actually employ
gitPathwhen building the environment.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java (2)
25-25: Inconsistent naming: remove underscore in key.
The keyIDFGuideLinkLabel_Textuses an underscore, deviating from the camel-case convention seen in other message keys. Rename toIDFGuideLinkLabelTextfor consistency.
27-27: Inconsistent naming: remove underscore in key.
The keyIDFToolsHandler_ToolsManagerConsolecombines two concerns with an underscore. For clarity and to match the existing style, rename toIDFToolsHandlerToolsManagerConsole.
🛑 Comments failed to post (21)
.github/workflows/ci_windows.yml (1)
8-14:
⚠️ Potential issueFix YAML indentation for branch filters
The branch arrays underpushandpull_requestare currently indented incorrectly (5 spaces instead of 6), which will result in a YAML parsing error. Please apply the following diff:@@ lines 8-14 - push: - branches: - - master - - release/** + push: + branches: + - master + - release/** - pull_request: - branches: - - master - - release/** + pull_request: + branches: + - master + - release/**📝 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.push: branches: - master - release/** pull_request: branches: - master - release/**🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 9-9: wrong indentation: expected 6 but found 5
(indentation)
[warning] 13-13: wrong indentation: expected 6 but found 5
(indentation)
.github/workflows/ci.yml (1)
8-14:
⚠️ Potential issueFix YAML indentation for branch filters
Thebrancheslists underpushandpull_requestneed consistent 6-space indentation for the dash entries to avoid syntax errors:@@ lines 8-14 - push: - branches: - - master - - release/** + push: + branches: + - master + - release/** - pull_request: - branches: - - master - - release/** + pull_request: + branches: + - master + - release/**📝 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.push: branches: - master - release/** pull_request: branches: - master - release/**🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 9-9: wrong indentation: expected 6 but found 5
(indentation)
[warning] 13-13: wrong indentation: expected 6 but found 5
(indentation)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java (1)
178-178: 💡 Verification agent
🧩 Analysis chain
Significantly increased test timeout for SDK Configuration tab
The timeout was increased from 5 seconds to 15 minutes, which seems exceptionally long for a UI test operation.
While this change aligns with the PR objective of handling older workspaces and ensuring compatibility, such a long timeout could mask performance issues or result in very long test execution times. Consider:
- Is this extreme timeout increase necessary, or could a more moderate value work?
- Is there a way to improve the initialization process to reduce this wait time?
- Could you add logging during this wait period to help diagnose what's taking so long?
🏁 Script executed:
#!/bin/bash # Check for any timeout-related configuration in test utility classes fd TestWidgetWaitUtility.java --exec cat {} | grep -A 10 -B 10 waitForSDKConfigurationTabLength of output: 557
🏁 Script executed:
#!/bin/bash # Locate all invocations of waitForSDKConfigurationTab to see past defaults rg -n "waitForSDKConfigurationTab" . # Enumerate all waitFor* methods in TestWidgetWaitUtility for comparison fd TestWidgetWaitUtility.java --exec grep -n "public static void waitFor" {}Length of output: 1202
🏁 Script executed:
#!/bin/bash # Check typical timeout values for waitForDialogToAppear in tests rg -n "waitForDialogToAppear" tests/com.espressif.idf.ui.test # Check typical usage of waitForTreeItem to see if timeouts are passed rg -n "waitForTreeItem" tests/com.espressif.idf.ui.test # Inspect other waitUntil usages to gauge common default timeouts rg -n "waitUntil" tests/com.espressif.idf.ui.testLength of output: 7141
Re-evaluate 15-minute timeout for SDK Configuration tab
The test now callswaitForSDKConfigurationTab(bot, 900000)(15 min), whereas other UI waits use 5 000–10 000 ms. Even though some long-running operations (e.g. env setup) use very large timeouts, a 15-minute UI wait can mask regressions and bloat test runs.• Could we reduce this to a more moderate value (e.g. 60 000 ms)?
• Is it possible to optimize the SDK Configuration view’s initialization to speed up tab activation?
• Add interim logging or progress markers inside theDefaultConditionto surface where the delay occurs.Example patch:
- TestWidgetWaitUtility.waitForSDKConfigurationTab(bot, 900000); + TestWidgetWaitUtility.waitForSDKConfigurationTab(bot, 60000); // lowered to 60 s📝 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.TestWidgetWaitUtility.waitForSDKConfigurationTab(bot, 60000); // lowered to 60 sbundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.java (1)
7-7: 💡 Verification agent
🧩 Analysis chain
Verify path construction for non-standard home directories
The POSIX path construction uses the user's home directory, but be careful with edge cases like non-standard home directory paths.
🏁 Script executed:
#!/bin/bash # Check if the code handles spaces or special characters in home directory paths echo "Testing home directory path handling for: ${HOME}" if [[ "${HOME}" == *" "* || "${HOME}" == *"'"* || "${HOME}" == *"\""* ]]; then echo "WARNING: Home directory contains spaces or quotes which might cause issues in path construction" fiLength of output: 215
Construct EIM_POSIX_DIR with the Path API to handle all home‐directory edge cases
String concatenation of
System.getProperty("user.home")with"/.espressif/tools/"can break on non‐standard paths (spaces, trailing separators, OS-specific separators, etc.). Instead, use Java NIO’sPaths.get(...):• File: bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.java:7
• Replace:- String EIM_POSIX_DIR = System.getProperty("user.home") - .concat("/.espressif/tools/"); //$NON-NLS-1$//$NON-NLS-2$ + import java.nio.file.Path; + import java.nio.file.Paths; + + Path eimPosixPath = Paths.get( + System.getProperty("user.home"), + ".espressif", + "tools" + ); + String EIM_POSIX_DIR = eimPosixPath.toString(); //$NON-NLS-1$ //$NON-NLS-2$This ensures correct separators, quoting and encoding for any valid home directory path.
Committable suggestion skipped: line range outside the PR's diff.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java (2)
27-30: 🛠️ Refactor suggestion
Add null check for preferences in constructor
The constructor should validate the preferences parameter to prevent NullPointerExceptions later.
public EimJsonUiChangeHandler(Preferences preferences) { + if (preferences == null) { + throw new IllegalArgumentException("Preferences cannot be null"); + } this.preferences = preferences; }📝 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.public EimJsonUiChangeHandler(Preferences preferences) { if (preferences == null) { throw new IllegalArgumentException("Preferences cannot be null"); } this.preferences = preferences; }
32-42: 🛠️ Refactor suggestion
Add defensive programming to onJsonFileChanged
The method should check if the file parameter is null and handle potential UI exceptions.
@Override public void onJsonFileChanged(Path file) { + if (file == null) { + return; + } + Display.getDefault().asyncExec(() -> { - MessageDialog.openWarning(Display.getDefault().getActiveShell(), Messages.EimJsonChangedMsgTitle, - Messages.EimJsonChangedMsgDetail); + try { + MessageDialog.openWarning(Display.getDefault().getActiveShell(), Messages.EimJsonChangedMsgTitle, + Messages.EimJsonChangedMsgDetail); + } catch (Exception e) { + // Log the exception but don't let it propagate + // Consider adding proper logging here + } }); EimJsonStateChecker checker = new EimJsonStateChecker(preferences); checker.updateLastSeenTimestamp(); }bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/SetupToolsJobListener.java (2)
55-59:
⚠️ Potential issueNull-check before invoking rollback
setupToolsInIdemay legitimately benull(see default ctor), which would trigger an NPE on failed jobs.
Add a guard:- // Rollback all the changes - setupToolsInIde.rollback(); + // Roll-back if we have a valid job instance + if (setupToolsInIde != null) + { + setupToolsInIde.rollback(); + }
39-41: 🛠️ Refactor suggestion
⚠️ Potential issuePotential NPE from default constructor
setupToolsInIdeis never initialised when the no-arg constructor is used.
If any caller still uses this constructor (e.g. via DI or reflective instantiation) thedone()method will dereference a null value and crash Eclipse.- public SetupToolsJobListener() - { - } + /** + * Default constructor kept only for framework usage.<br> + * ⚠️ Avoid using this, prefer the parameterised constructor so that + * {@link #setupToolsInIde} is never {@code null}. + */ + public SetupToolsJobListener() + { + this(null, null); + }Alternatively, remove the default constructor entirely and update call-sites.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.java (1)
61-63: 💡 Verification agent
🧩 Analysis chain
firstStartupflag never set in the editor input
Later logic inESPIDFManagerEditorrelies on this flag to trigger initial setup, but it remainsfalse. Pass the flag explicitly:-IDE.openEditor(activeww.getActivePage(), new EimEditorInput(eimJson), ESPIDFManagerEditor.EDITOR_ID, true); +EimEditorInput input = new EimEditorInput(eimJson); +input.setFirstStartup(/* determine first-startup here */ true); +IDE.openEditor(activeww.getActivePage(), input, ESPIDFManagerEditor.EDITOR_ID, true);
🏁 Script executed:
#!/bin/bash # Find the EimEditorInput class declaration rg "class EimEditorInput" -n # Find any declaration or usage of firstStartup rg "firstStartup" -n # Find whether a setter exists for firstStartup rg "setFirstStartup" -n # Inspect any usage of firstStartup within ESPIDFManagerEditor rg -C3 "firstStartup" -nLength of output: 3121
🏁 Script executed:
#!/bin/bash # Show context around EimEditorInput instantiation in ManageEspIdfVersionsHandler rg -n -C5 "new EimEditorInput" bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.javaLength of output: 400
🏁 Script executed:
#!/bin/bash # Locate ESPIDFManagerEditor and inspect its use of firstStartup rg -l "class ESPIDFManagerEditor" -n bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager | \ xargs grep -n -C3 "isFirstStartup" rg -l "class ESPIDFManagerEditor" -n bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager | \ xargs grep -n -C3 "firstStartup"Length of output: 589
🏁 Script executed:
#!/bin/bash # Show context around setFirstStartup in EspressifToolStartup rg -n -C5 "input.setFirstStartup" bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java # Also show how EimEditorInput is used there rg -n -C5 "new EimEditorInput" bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.javaLength of output: 993
🏁 Script executed:
#!/bin/bash FILE="bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.java" echo "----- Lines 1-100 -----" sed -n '1,100p' "$FILE" echo "----- Lines 101-200 -----" sed -n '101,200p' "$FILE"Length of output: 2761
Set the
firstStartupflag when opening the ESP-IDF Manager editorWithout explicitly setting
firstStartup, the editor’s initial‐setup logic (isFirstStartup()) never runs when invoked from ManageEspIdfVersionsHandler. Add a setter before callingIDE.openEditor(...):• File: bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.java
• Around line 61:- IDE.openEditor(activeww.getActivePage(), - new EimEditorInput(eimJson), - ESPIDFManagerEditor.EDITOR_ID, - true); + // Prepare editor input and mark as first startup so initial setup runs + EimEditorInput input = new EimEditorInput(eimJson); + input.setFirstStartup(true); + IDE.openEditor(activeww.getActivePage(), + input, + ESPIDFManagerEditor.EDITOR_ID, + true);This ensures
ESPIDFManagerEditorwill invoke itssetupInitialEspIdf()path.📝 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.// Prepare editor input and mark as first startup so initial setup runs EimEditorInput input = new EimEditorInput(eimJson); input.setFirstStartup(true); IDE.openEditor(activeww.getActivePage(), input, ESPIDFManagerEditor.EDITOR_ID, true); }bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/EimEditorInput.java (2)
44-48:
⚠️ Potential issueReturn meaningful text for editor tabs & tool-tips
Returningnullcauses Eclipse to show a blank label and may lead to NPEs in some views.- public String getName() - { - return null; - } + public String getName() + { + return FILE_NAME; + } ... - public String getToolTipText() - { - return null; - } + public String getToolTipText() + { + return "ESP-IDF EIM configuration"; + }Also applies to: 56-60
13-14: 🛠️ Refactor suggestion
Make
FILE_NAMEa real constant
FILE_NAMEshould bestatic finalto reflect its constant nature and avoid per-instance duplication.- private String FILE_NAME = "idf_eim.json"; //$NON-NLS-1$ + private static final String FILE_NAME = "idf_eim.json"; //$NON-NLS-1$📝 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.private static final String FILE_NAME = "idf_eim.json"; //$NON-NLS-1$bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.java (3)
52-60: 🛠️ Refactor suggestion
getInstance()can returnnullwhen construction fails
Ifnew EimJsonWatchService()throws,INSTANCEremainsnull, and subsequent callers will NPE. Consider falling back to a no-op implementation or re-throwing a checked exception so callers can handle the failure explicitly.
30-44: 🛠️ Refactor suggestion
Avoid launching the watcher thread from inside the constructor
Callingstart()inside the constructor causes the partially-constructed object (this) to escape, which can lead to subtle concurrency bugs (the thread may observe un-initialised state if more fields are added later). A safer pattern is to expose a staticstart()/init()factory or let callers start the thread explicitly after construction.- setDaemon(true); - start(); + setDaemon(true);…and start it in
getInstance()or an explicitstartService()method.📝 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.private EimJsonWatchService() throws IOException { String directoryPathString = Platform.getOS().equals(Platform.OS_WIN32) ? EimConstants.EIM_WIN_DIR : EimConstants.EIM_POSIX_DIR; watchDirectoryPath = Paths.get(directoryPathString); watchService = FileSystems.getDefault().newWatchService(); watchDirectoryPath.register(watchService, StandardWatchEventKinds.ENTRY_CREATE, StandardWatchEventKinds.ENTRY_DELETE, StandardWatchEventKinds.ENTRY_MODIFY); Logger.log("Watcher added to the directory: " + directoryPathString); //$NON-NLS-1$ setName("EimJsonWatchService"); //$NON-NLS-1$ setDaemon(true); }
32-38:
⚠️ Potential issueEnsure the watched directory exists before registering it
Paths.get(directoryPathString)will throwNoSuchFileExceptionatregister()time if the path does not exist. Create the directory (including parents) on first run to avoid a hard failure on clean machines.- watchDirectoryPath = Paths.get(directoryPathString); + watchDirectoryPath = Paths.get(directoryPathString); + if (Files.notExists(watchDirectoryPath)) + { + Files.createDirectories(watchDirectoryPath); + }📝 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 directoryPathString = Platform.getOS().equals(Platform.OS_WIN32) ? EimConstants.EIM_WIN_DIR : EimConstants.EIM_POSIX_DIR; watchDirectoryPath = Paths.get(directoryPathString); if (Files.notExists(watchDirectoryPath)) { Files.createDirectories(watchDirectoryPath); } watchService = FileSystems.getDefault().newWatchService(); watchDirectoryPath.register(watchService, StandardWatchEventKinds.ENTRY_CREATE, StandardWatchEventKinds.ENTRY_DELETE, StandardWatchEventKinds.ENTRY_MODIFY);bundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/JsonConfigServer.java (2)
96-100:
⚠️ Potential issuePossible NPE when
idfExportPathends upnullor empty
prepEnvMap()may putnullvalues intoenv, butProcessBuilder.environment()rejectsnullvalues and will throwNullPointerExceptionwhenputAll()executes inProcessBuilderFactory.run(...).Add a guard to guarantee non-null values:
- systemEnv.put(pathVar, idfExportPath); + if (!StringUtil.isEmpty(idfExportPath)) + { + systemEnv.put(pathVar, idfExportPath); + }Committable suggestion skipped: line range outside the PR's diff.
108-115:
⚠️ Potential issue
pythonPathcan benull→ ProcessBuilder failure
IDFUtil.getIDFPythonEnvPath()returnsnullwhen the executable cannot be located. Addingnullto theargumentslist will triggerProcessBuilder’sNullPointerException.- String pythonPath = IDFUtil.getIDFPythonEnvPath(); + String pythonPath = IDFUtil.getIDFPythonEnvPath(); + if (StringUtil.isEmpty(pythonPath)) + { + pythonPath = "python"; // last-chance fallback + }📝 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 pythonPath = IDFUtil.getIDFPythonEnvPath(); if (StringUtil.isEmpty(pythonPath)) { pythonPath = "python"; // last-chance fallback } arguments.add(pythonPath); arguments.add(idfPythonScriptFile.getAbsolutePath()); arguments.add("-B"); //$NON-NLS-1$ arguments.add(IDFUtil.getBuildDir(project)); arguments.add("-DSDKCONFIG=".concat(file.getName())); //$NON-NLS-1$ arguments.add(IDFConstants.CONF_SERVER_CMD); Logger.log(arguments.toString());bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java (1)
94-99:
⚠️ Potential issueIncorrect
MessageDialogconstructor parameters – code will not compile
MessageDialogexpects(Shell, String, Image, String, int, String[] , int). The current call passes twoints before theString[], violating the signature and causing a compilation error.- MessageDialog messageDialog = new MessageDialog(display.getActiveShell(), - Messages.OldConfigFoundMsgBoxTitle, null, Messages.OldConfigFoundMsgBoxMsg, 0, 0, - new String[] { Messages.ToolsInitializationDifferentPathMessageBoxOptionYes, - Messages.ToolsInitializationDifferentPathMessageBoxOptionNo }); + MessageDialog messageDialog = new MessageDialog(display.getActiveShell(), + Messages.OldConfigFoundMsgBoxTitle, null, Messages.OldConfigFoundMsgBoxMsg, + MessageDialog.WARNING, + new String[] { Messages.ToolsInitializationDifferentPathMessageBoxOptionYes, + Messages.ToolsInitializationDifferentPathMessageBoxOptionNo }, + 0);📝 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.MessageDialog messageDialog = new MessageDialog(display.getActiveShell(), Messages.OldConfigFoundMsgBoxTitle, null, Messages.OldConfigFoundMsgBoxMsg, MessageDialog.WARNING, new String[] { Messages.ToolsInitializationDifferentPathMessageBoxOptionYes, Messages.ToolsInitializationDifferentPathMessageBoxOptionNo }, 0); response[0] = messageDialog.open();bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (2)
548-560:
⚠️ Potential issue*Hard-coding ‘;’ as path separator breaks nix installations
addGitToEnvironmentprepends the Git directory using";"which is only valid on Windows.
UseFile.pathSeparatorinstead to remain platform-agnostic.- path1 = gitDir.concat(";").concat(path1); + path1 = gitDir + File.pathSeparator + path1; … - path2 = gitDir.concat(";").concat(path2); + path2 = gitDir + File.pathSeparator + path2;📝 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 gitDir = gitPath.removeLastSegments(1).toOSString(); String path1 = envMap.get("PATH"); //$NON-NLS-1$ String path2 = envMap.get("Path"); //$NON-NLS-1$ if (!StringUtil.isEmpty(path1) && !path1.contains(gitDir)) // Git not found on the PATH environment { - path1 = gitDir.concat(";").concat(path1); //$NON-NLS-1$ + path1 = gitDir + File.pathSeparator + path1; //$NON-NLS-1$ envMap.put("PATH", path1); //$NON-NLS-1$ } else if (!StringUtil.isEmpty(path2) && !path2.contains(gitDir)) // Git not found on the Path environment { - path2 = gitDir.concat(";").concat(path2); //$NON-NLS-1$ + path2 = gitDir + File.pathSeparator + path2; //$NON-NLS-1$ envMap.put("Path", path2); //$NON-NLS-1$ }
100-109:
⚠️ Potential issueActivation script is executed without the augmented environment map
You build an
envmap (adding Git to thePATH) but then discard it and passSystem.getenv()torunInBackground.
On hosts where Git is not already on the systemPATH, the activation script will fail.- IStatus status = processBuilderFactory.runInBackground(arguemnts, Path.ROOT, System.getenv()); + IStatus status = processBuilderFactory.runInBackground(arguemnts, Path.ROOT, env);bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (2)
392-397: 🛠️ Refactor suggestion
Repeated
idf.py --versioncalls in cell renderer hurt UI responsiveness
getIdfVersion()spawns an external process for every table refresh, which can freeze the UI on large IDF lists.Cache the version inside
IdfInstalled(populate once during JSON parse) or memoise in aMap<Id,String>.
530-546:
⚠️ Potential issueColumn-index / comparator mismatch causes incorrect sorting
setComparatorForColsassigns indices:
0 → Version, 1 → Name, 2 → Location.
compare()currently maps
0 →getName(), 1 →getPath(), 2 → active state – none of which align.Users will see unexpected sort order.
case 0: // Version - rc = p1.getName().compareTo(p2.getName()); + rc = ToolsUtility.getIdfVersion(p1, eimJson.getGitPath()) + .compareTo(ToolsUtility.getIdfVersion(p2, eimJson.getGitPath())); break; case 1: // Name - rc = p1.getPath().compareTo(p2.getPath()); + rc = p1.getName().compareTo(p2.getName()); break; case 2: // Location - Boolean p1State = …; - Boolean p2State = …; - rc = p1State.compareTo(p2State); + rc = p1.getPath().compareTo(p2.getPath()); break;Or remap the
colIndexarguments passed tosetComparatorForCols.
Please adjust whichever is simpler to maintain.📝 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.IdfInstalled p1 = (IdfInstalled) e1; IdfInstalled p2 = (IdfInstalled) e2; int rc = 0; switch (propertyIndex) { case 0: // Version rc = ToolsUtility.getIdfVersion(p1, eimJson.getGitPath()) .compareTo(ToolsUtility.getIdfVersion(p2, eimJson.getGitPath())); break; case 1: // Name rc = p1.getName().compareTo(p2.getName()); break; case 2: // Location rc = p1.getPath().compareTo(p2.getPath()); break; default:
| preferences = org.eclipse.core.runtime.preferences.InstanceScope.INSTANCE.getNode(UIPlugin.PLUGIN_ID); | ||
| toolInitializer = new ToolInitializer(preferences); | ||
|
|
||
| if (toolInitializer.isEspIdfSet() && toolInitializer.isOldEspIdfConfigPresent() |
There was a problem hiding this comment.
Two things here:
Do we need the isEspIdfSet() condition when we are already checking toolInitializer.isOldEspIdfConfigPresent()? I'm thinking — if the old configuration is present, doesn't that already imply that the IDF is set?
Since we're calling this in earlyStartup(), it means it will be invoked as soon as the IDE is launched. If EIM is not available, the conversion will fail anyway because EIM itself isn't available. So, are we checking whether EIM is installed before invoking the conversion?
@AndriiFilippov Please consider this case while testing E2E.
| Logger.log("Tools Conversion Process Message: "); | ||
| Logger.log(status.getMessage()); |
There was a problem hiding this comment.
Introduce a new method Logger.log(String message, Exception e) to log these kind of messages if it's not available in the class Logger.
| @@ -32,10 +32,10 @@ NoActiveEspIdfInWorkspaceMsgTitle=ESP-IDF Setup | |||
| NoActiveEspIdfInWorkspaceMsg=ESP-IDF is required to use Espressif IDE. Would you like to configure it now? | |||
|
|
|||
| OldConfigFoundMsgBoxTitle=Old Configuration Found | |||
| OldConfigFoundMsgBoxMsg=An old ESP-IDF configuration has been detected.\nIf you want to use these you need to export these configuration and pass them on to EIM for updated configurations.\nWould you like to export these settings?\n\nPress 'Yes' to choose a location to save. | |||
| OldConfigFoundMsgBoxMsg=Please export the current workspace configuration to the EIM configuration for proper environment setup. Do you want to export configurations? | |||
There was a problem hiding this comment.
Can we convey below points in the message?
- Explain why the prompt is appearing (legacy setup detected).
- Clarify what will happen (conversion to EIM format).
- State how it benefits the user (proper management and IDE will be ready to use with the existing setup).
Something like below or whatever you think that will convey above points -
Espressif IDE now uses EIM to manage ESP-IDF Installations. A legacy ESP-IDF configuration was detected in your workspace. Would you like to convert it to the EIM-compatible format to prepare the IDE for use?
| OldConfigExportCompleteSuccessMsg=The configuration has been successfully Imported | ||
| OldConfigExportCompleteFailMsgTitle=Import Failed | ||
| OldConfigExportCompleteFailMsg=An error occurred while importing old configuration. |
There was a problem hiding this comment.
I think we are "converting" from the user perspective
Description
Handling case for conversion of existing json file to eim_idf.json
Fixes # (IEP-1517)
Type of change
Please delete options that are not relevant.
How has this been tested?
Please try to use an older workspace with this build to test it. The condition for older workspace is that it should have the old json file that was create by installation manager before eim integration.
Test Configuration:
Checklist
Summary by CodeRabbit
New Features
Improvements
Removals
Bug Fixes
Documentation
Chores