IEP-1334: EIM Integration #1116
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (5)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (1)
205-237: Clean up commented-out code related to the Remove All functionalityThe code block for the
removeAllButtonhas been commented out. If this functionality is no longer needed, consider removing the code entirely to maintain code cleanliness. If it will be used later, add a comment explaining its future use.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.java (1)
1-10: Use a final class instead of an interface for constantsUsing an interface to hold constants is an anti-pattern in Java. It's better to use a final class with a private constructor to hold constants. This approach prevents unintended implementation and makes the intent clearer.
Refactor
EimConstantsto a utility class:-public interface EimConstants +public final class EimConstants { - String EIM_WIN_PATH = "C:\\Espressif\\tools\\esp_ide.json"; //$NON-NLS-1$ + public static final String EIM_WIN_PATH = "C:\\Espressif\\tools\\esp_ide.json"; //$NON-NLS-1$ - String EIM_URL = "https://github.com/espressif/idf-im-cli"; //$NON-NLS-1$ + public static final String EIM_URL = "https://github.com/espressif/idf-im-cli"; //$NON-NLS-1$ - String EIM_POSIX_PATH = "~/.espressif/tools/eim_idf.json"; //$NON-NLS-1$ + public static final String EIM_POSIX_PATH = "~/.espressif/tools/eim_idf.json"; //$NON-NLS-1$ + private EimConstants() { + // Prevent instantiation + } }bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java (1)
7-14: Add class-level documentation and field validation.The class would benefit from:
- JavaDoc explaining the purpose of this configuration class
- Documentation for each field explaining their significance
- Null validation in setters
Example implementation:
+/** + * Represents the EIM (ESP-IDF Installation Manager) configuration. + * This class is used for JSON serialization/deserialization of EIM settings. + */ public class EimJson { @Expose + /** Path to the Git executable */ private String gitPath; @Expose + /** Currently selected IDF installation ID */ private String idfSelectedId; @Expose + /** List of installed IDF versions */ private List<IdfInstalled> idfInstalled;bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IdfInstalled.java (1)
5-18: Add class documentation and field validation.The class needs:
- JavaDoc documentation
- Field validation
- toString() implementation for debugging
+/** + * Represents an installed ESP-IDF instance with its configuration. + * This class contains paths and settings for a specific IDF installation. + */ public class IdfInstalled { @Expose + /** Script to activate this IDF installation */ private String activationScript; @Expose + /** Unique identifier for this installation */ private String id; @Expose + /** Path to the IDF tools directory */ private String idfToolsPath; @Expose + /** Display name of this installation */ private String name; @Expose + /** Root path of this IDF installation */ private String path; @Expose + /** Python interpreter path for this installation */ private String python;bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties (1)
22-23: Consider adding documentation links.To help users better understand the EIM installation process, consider adding a link to the official documentation.
Add documentation link to the message:
-ToolsInitializationEimMissingMsgBoxMessage=ESP-IDF is not found on your system. Please use EIM (Espressif Installation Manager) to install the ESP-IDF on your system to use the IDE. When prompted, click Yes to open the EIM download page in your browser. +ToolsInitializationEimMissingMsgBoxMessage=ESP-IDF is not found on your system. Please use EIM (Espressif Installation Manager) to install the ESP-IDF on your system to use the IDE. When prompted, click Yes to open the EIM download page in your browser. For more information, visit: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/get-started/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IdfInstalled.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java(4 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/ESPIDFManagerEditor.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/EimEditorInput.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java(14 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build_macos
🔇 Additional comments (3)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java (1)
290-290:⚠️ Potential issueCorrect the method name 'userNotficationToInstallEim'
The method
userNotficationToInstallEim()has a typo in its name. 'Notfication' should be 'Notification'.Apply this diff to fix the method name:
-private void userNotficationToInstallEim() +private void userNotificationToInstallEim()Run the following script to find and correct all occurrences of the misspelled method name:
✅ Verification successful
Rename method to fix typo: 'userNotficationToInstallEim' → 'userNotificationToInstallEim'
The method name has a typo that needs to be corrected in two locations in
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java:
- Method declaration
- Method invocation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find occurrences of 'userNotficationToInstallEim' in the codebase. rg 'userNotficationToInstallEim' -t javaLength of output: 282
Script:
#!/bin/bash # Check for any similar typos in method names or strings rg -i 'notfication' -t javaLength of output: 269
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IdfInstalled.java (1)
35-38: Ensure ID uniqueness.The ID setter should validate the format to ensure uniqueness across installations.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java (1)
36-38: LGTM!The new message constants are properly integrated with the NLS system and follow the existing pattern in the file.
| catch (IOException e) | ||
| { | ||
| Logger.log(e); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in the catch block
When catching IOException while obtaining eimJson, consider informing the user or taking corrective action rather than just logging it. This will improve error visibility and user experience.
| String path = StringUtil.EMPTY; | ||
| if (Platform.getOS().equals(Platform.OS_WIN32)) | ||
| { | ||
| JSONObject jsonObj = (JSONObject) parser.parse(new FileReader(idf_json_file)); | ||
| String gitExecutablePath = (String) jsonObj.get(GIT_PATH); | ||
| String idfToolsPath = (String) jsonObj.get(IDF_TOOLS_PATH_KEY); | ||
| String idfVersionId = (String) jsonObj.get(IDF_VERSIONS_ID); | ||
| JSONObject list = (JSONObject) jsonObj.get(IDF_INSTALLED_LIST_KEY); | ||
| if (list != null) | ||
| { | ||
| // selected esp-idf version information | ||
| JSONObject selectedIDFInfo = (JSONObject) list.get(idfVersionId); | ||
| String idfPath = (String) selectedIDFInfo.get(IDF_PATH); | ||
| String pythonExecutablePath = (String) selectedIDFInfo.get(PYTHON_PATH); | ||
| IDFToolSet newToolSet = new IDFToolSet(); | ||
| newToolSet.setIdfLocation(idfPath); | ||
| newToolSet.setSystemGitExecutablePath(gitExecutablePath); | ||
| newToolSet.setSystemPythonExecutablePath(pythonExecutablePath); | ||
| newToolSet.setActive(true); | ||
| Preferences prefs = InstanceScope.INSTANCE.getNode(IDFCorePlugin.PLUGIN_ID); | ||
| prefs.put(IDFCorePreferenceConstants.IDF_TOOLS_PATH, idfToolsPath); | ||
| try | ||
| { | ||
| prefs.flush(); | ||
| } | ||
| catch (BackingStoreException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| ToolsActivationJob toolsActivationJob = new ToolsActivationJob(newToolSet, pythonExecutablePath, gitExecutablePath); | ||
| ToolsActivationJobListener toolsActivationJobListener = new ToolsActivationJobListener(ESPIDFMainTablePage.getInstance()); | ||
| toolsActivationJob.addJobChangeListener(toolsActivationJobListener); | ||
| toolsActivationJob.schedule(); | ||
| } | ||
|
|
||
| // save state | ||
| Preferences prefs = getPreferences(); | ||
| prefs.putBoolean(IS_INSTALLER_CONFIG_SET, true); | ||
| try | ||
| { | ||
| prefs.flush(); | ||
| } | ||
| catch (BackingStoreException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| IDFUtil.updateEspressifPrefPageOpenocdPath(); | ||
|
|
||
| path = EimConstants.EIM_WIN_PATH; | ||
| } | ||
| catch ( | ||
| IOException | ||
| | ParseException e) | ||
| else | ||
| { | ||
| Logger.log(e); | ||
| path = EimConstants.EIM_POSIX_PATH; | ||
| } | ||
|
|
||
| File file = new File(path); | ||
| return file.exists(); | ||
| } |
There was a problem hiding this comment.
Expand '~' in POSIX paths to the user's home directory
In the method isEimIdfJsonPresent(), the POSIX path EimConstants.EIM_POSIX_PATH uses '~' to represent the user's home directory. Java's File class does not automatically expand '~', which will result in the file not being found. Use System.getProperty("user.home") to construct the correct path.
Apply this diff to fix the path resolution:
String path = StringUtil.EMPTY;
if (Platform.getOS().equals(Platform.OS_WIN32))
{
path = EimConstants.EIM_WIN_PATH;
}
else
{
- path = EimConstants.EIM_POSIX_PATH;
+ String userHome = System.getProperty("user.home");
+ path = userHome + EimConstants.EIM_POSIX_PATH.substring(1); // Remove the '~' character
}
File file = new File(path);
return file.exists();📝 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 path = StringUtil.EMPTY; | |
| if (Platform.getOS().equals(Platform.OS_WIN32)) | |
| { | |
| JSONObject jsonObj = (JSONObject) parser.parse(new FileReader(idf_json_file)); | |
| String gitExecutablePath = (String) jsonObj.get(GIT_PATH); | |
| String idfToolsPath = (String) jsonObj.get(IDF_TOOLS_PATH_KEY); | |
| String idfVersionId = (String) jsonObj.get(IDF_VERSIONS_ID); | |
| JSONObject list = (JSONObject) jsonObj.get(IDF_INSTALLED_LIST_KEY); | |
| if (list != null) | |
| { | |
| // selected esp-idf version information | |
| JSONObject selectedIDFInfo = (JSONObject) list.get(idfVersionId); | |
| String idfPath = (String) selectedIDFInfo.get(IDF_PATH); | |
| String pythonExecutablePath = (String) selectedIDFInfo.get(PYTHON_PATH); | |
| IDFToolSet newToolSet = new IDFToolSet(); | |
| newToolSet.setIdfLocation(idfPath); | |
| newToolSet.setSystemGitExecutablePath(gitExecutablePath); | |
| newToolSet.setSystemPythonExecutablePath(pythonExecutablePath); | |
| newToolSet.setActive(true); | |
| Preferences prefs = InstanceScope.INSTANCE.getNode(IDFCorePlugin.PLUGIN_ID); | |
| prefs.put(IDFCorePreferenceConstants.IDF_TOOLS_PATH, idfToolsPath); | |
| try | |
| { | |
| prefs.flush(); | |
| } | |
| catch (BackingStoreException e) | |
| { | |
| Logger.log(e); | |
| } | |
| ToolsActivationJob toolsActivationJob = new ToolsActivationJob(newToolSet, pythonExecutablePath, gitExecutablePath); | |
| ToolsActivationJobListener toolsActivationJobListener = new ToolsActivationJobListener(ESPIDFMainTablePage.getInstance()); | |
| toolsActivationJob.addJobChangeListener(toolsActivationJobListener); | |
| toolsActivationJob.schedule(); | |
| } | |
| // save state | |
| Preferences prefs = getPreferences(); | |
| prefs.putBoolean(IS_INSTALLER_CONFIG_SET, true); | |
| try | |
| { | |
| prefs.flush(); | |
| } | |
| catch (BackingStoreException e) | |
| { | |
| Logger.log(e); | |
| } | |
| IDFUtil.updateEspressifPrefPageOpenocdPath(); | |
| path = EimConstants.EIM_WIN_PATH; | |
| } | |
| catch ( | |
| IOException | |
| | ParseException e) | |
| else | |
| { | |
| Logger.log(e); | |
| path = EimConstants.EIM_POSIX_PATH; | |
| } | |
| File file = new File(path); | |
| return file.exists(); | |
| } | |
| String path = StringUtil.EMPTY; | |
| if (Platform.getOS().equals(Platform.OS_WIN32)) | |
| { | |
| path = EimConstants.EIM_WIN_PATH; | |
| } | |
| else | |
| { | |
| String userHome = System.getProperty("user.home"); | |
| path = userHome + EimConstants.EIM_POSIX_PATH.substring(1); // Remove the '~' character | |
| } | |
| File file = new File(path); | |
| return file.exists(); | |
| } |
| { | ||
| // TODO: Installer config flag is not set so we need to start IDF Manager and let user select the IDF for workspace | ||
| Display.getDefault().syncExec(()-> { | ||
| Shell shell = Display.getDefault().getActiveShell(); | ||
| MessageBox messageBox = new MessageBox(shell, SWT.ICON_WARNING| SWT.YES | SWT.NO); | ||
| messageBox.setText("No Active ESP-IDF in workspace"); | ||
| messageBox.setMessage("No active ESP-IDF found in current workspace. Do you want to select the ESP-IDF for current workspace?"); | ||
|
|
||
| int response = messageBox.open(); | ||
| if (response == SWT.NO) | ||
| if (response == SWT.YES) | ||
| { | ||
| IDFEnvironmentVariables idfEnvironmentVariables = new IDFEnvironmentVariables(); | ||
| updateEspIdfJsonFile(idf_json_file, | ||
| idfEnvironmentVariables.getEnvValue(IDFEnvironmentVariables.IDF_PATH)); | ||
| Preferences prefs = getPreferences(); | ||
| prefs.putBoolean(IS_INSTALLER_CONFIG_SET, true); | ||
| IWorkbenchWindow activeww = EclipseHandler.getActiveWorkbenchWindow(); | ||
| IDFUtil.closeWelcomePage(activeww); | ||
| try | ||
| { | ||
| prefs.flush(); | ||
| IDE.openEditor(activeww.getActivePage(), new EimEditorInput(eimJson), ESPIDFManagerEditor.EDITOR_ID); | ||
| } | ||
| catch (BackingStoreException e) | ||
| catch (PartInitException e) |
There was a problem hiding this comment.
Ensure eimJson is not null before using it
In the block where you open the editor with eimJson, ensure that eimJson is not null to prevent a potential NullPointerException.
Apply this diff to add a null check:
if (!isInstallerConfigSet())
{
// ...
if (response == SWT.YES)
{
+ if (eimJson != null)
+ {
IWorkbenchWindow activeww = EclipseHandler.getActiveWorkbenchWindow();
IDE.openEditor(activeww.getActivePage(), new EimEditorInput(eimJson), ESPIDFManagerEditor.EDITOR_ID);
+ }
+ else
+ {
+ // Handle the case where eimJson is null
+ MessageBox errorMessageBox = new MessageBox(shell, SWT.ICON_ERROR | SWT.OK);
+ errorMessageBox.setText("Error");
+ errorMessageBox.setMessage("EIM configuration data is unavailable.");
+ errorMessageBox.open();
+ }
}
}Committable suggestion skipped: line range outside the PR's diff.
| import com.espressif.idf.core.tools.EimIdfConfiguratinParser; | ||
| import com.espressif.idf.core.tools.vo.EimJson; |
There was a problem hiding this comment.
💡 Codebase verification
Coordinated rename required across core and UI bundles
The typo exists in multiple locations and requires coordinated changes:
- Core bundle:
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.java(class definition)
- UI bundle:
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java(import and field)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java(import and field)
Rename the class from EimIdfConfiguratinParser to EimIdfConfigurationParser and update all references.
🔗 Analysis chain
Correct the spelling of 'Configuratin' to 'Configuration'
There is a misspelling in the class name EimIdfConfiguratinParser and the variable eimIdfConfiguratinParser. Please correct 'Configuratin' to 'Configuration' to maintain consistency and avoid confusion.
Apply this diff to fix the typo:
-import com.espressif.idf.core.tools.EimIdfConfiguratinParser;
+import com.espressif.idf.core.tools.EimIdfConfigurationParser;
...
-private EimIdfConfiguratinParser eimIdfConfiguratinParser;
+private EimIdfConfigurationParser eimIdfConfigurationParser;Run the following script to find and correct all occurrences of the typo in the codebase:
Also applies to: 87-88
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find and list all occurrences of 'Configuratin' in filenames and code.
# Find files with 'Configuratin' in their names
fd -e java -p 'Configuratin'
# Search code for 'Configuratin'
rg 'Configuratin' -t java
Length of output: 1737
| public static ESPIDFMainTablePage getInstance(EimJson eimJson) | ||
| { | ||
| if (espidfMainTablePage == null) | ||
| { | ||
| espidfMainTablePage = new ESPIDFMainTablePage(); | ||
| } | ||
|
|
||
| ESPIDFMainTablePage.eimJson = eimJson; |
There was a problem hiding this comment.
Avoid using a static eimJson variable to prevent threading issues
The static variable eimJson is being modified in the getInstance(EimJson eimJson) method. This could lead to threading issues if multiple threads access or modify eimJson concurrently. Consider making eimJson an instance variable or ensuring thread safety.
| public class EimIdfConfiguratinParser | ||
| { |
There was a problem hiding this comment.
Fix class name typo.
The class name contains a typo: "Configuratin" should be "Configuration".
-public class EimIdfConfiguratinParser
+public class EimIdfConfigurationParser📝 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 class EimIdfConfiguratinParser | |
| { | |
| public class EimIdfConfigurationParser | |
| { |
| private void load() throws IOException | ||
| { | ||
| try (FileReader fileReader = new FileReader( | ||
| Platform.getOS().equals(Platform.OS_WIN32) ? EimConstants.EIM_WIN_PATH : EimConstants.EIM_POSIX_PATH)) | ||
| { | ||
| eimJson = gson.fromJson(fileReader, EimJson.class); | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve error handling in load() method.
The load method should handle JSON parsing errors and file not found scenarios more gracefully.
private void load() throws IOException
{
+ File configFile = new File(
+ Platform.getOS().equals(Platform.OS_WIN32) ?
+ EimConstants.EIM_WIN_PATH :
+ EimConstants.EIM_POSIX_PATH
+ );
+
+ if (!configFile.exists()) {
+ throw new FileNotFoundException("EIM configuration file not found: " + configFile.getPath());
+ }
+
try (FileReader fileReader = new FileReader(
- Platform.getOS().equals(Platform.OS_WIN32) ? EimConstants.EIM_WIN_PATH : EimConstants.EIM_POSIX_PATH))
+ configFile))
{
eimJson = gson.fromJson(fileReader, EimJson.class);
+ if (eimJson == null) {
+ throw new IOException("Failed to parse EIM configuration file");
+ }
+ } catch (JsonSyntaxException e) {
+ throw new IOException("Invalid JSON format in EIM configuration file", e);
}
}📝 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 void load() throws IOException | |
| { | |
| try (FileReader fileReader = new FileReader( | |
| Platform.getOS().equals(Platform.OS_WIN32) ? EimConstants.EIM_WIN_PATH : EimConstants.EIM_POSIX_PATH)) | |
| { | |
| eimJson = gson.fromJson(fileReader, EimJson.class); | |
| } | |
| } | |
| private void load() throws IOException | |
| { | |
| File configFile = new File( | |
| Platform.getOS().equals(Platform.OS_WIN32) ? | |
| EimConstants.EIM_WIN_PATH : | |
| EimConstants.EIM_POSIX_PATH | |
| ); | |
| if (!configFile.exists()) { | |
| throw new FileNotFoundException("EIM configuration file not found: " + configFile.getPath()); | |
| } | |
| try (FileReader fileReader = new FileReader(configFile)) | |
| { | |
| eimJson = gson.fromJson(fileReader, EimJson.class); | |
| if (eimJson == null) { | |
| throw new IOException("Failed to parse EIM configuration file"); | |
| } | |
| } catch (JsonSyntaxException e) { | |
| throw new IOException("Invalid JSON format in EIM configuration file", e); | |
| } | |
| } |
| public void setActivationScript(String activationScript) | ||
| { | ||
| this.activationScript = activationScript; | ||
| } |
There was a problem hiding this comment.
Validate path parameters.
Path-related setters should validate their input to ensure they contain valid paths.
public void setPath(String path)
{
+ if (path == null || path.trim().isEmpty()) {
+ throw new IllegalArgumentException("Path cannot be null or empty");
+ }
this.path = path;
}
public void setActivationScript(String activationScript)
{
+ if (activationScript == null || activationScript.trim().isEmpty()) {
+ throw new IllegalArgumentException("Activation script path cannot be null or empty");
+ }
this.activationScript = activationScript;
}Also applies to: 65-68
| ESPIDFMainTablePage espidfMainTablePage = ESPIDFMainTablePage | ||
| .getInstance(((EimEditorInput) getEditorInput()).getEimJson()); |
There was a problem hiding this comment.
Add error handling for the editor input cast and EimJson retrieval.
The current implementation assumes the editor input will always be of type EimEditorInput. This could lead to runtime exceptions if a different input type is provided.
Add proper error handling:
- ESPIDFMainTablePage espidfMainTablePage = ESPIDFMainTablePage
- .getInstance(((EimEditorInput) getEditorInput()).getEimJson());
+ IEditorInput input = getEditorInput();
+ if (!(input instanceof EimEditorInput)) {
+ throw new IllegalStateException("Expected EimEditorInput but got: " + input.getClass().getName());
+ }
+ EimJson eimJson = ((EimEditorInput) input).getEimJson();
+ if (eimJson == null) {
+ throw new IllegalStateException("EimJson configuration is null");
+ }
+ ESPIDFMainTablePage espidfMainTablePage = ESPIDFMainTablePage.getInstance(eimJson);📝 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.
| ESPIDFMainTablePage espidfMainTablePage = ESPIDFMainTablePage | |
| .getInstance(((EimEditorInput) getEditorInput()).getEimJson()); | |
| IEditorInput input = getEditorInput(); | |
| if (!(input instanceof EimEditorInput)) { | |
| throw new IllegalStateException("Expected EimEditorInput but got: " + input.getClass().getName()); | |
| } | |
| EimJson eimJson = ((EimEditorInput) input).getEimJson(); | |
| if (eimJson == null) { | |
| throw new IllegalStateException("EimJson configuration is null"); | |
| } | |
| ESPIDFMainTablePage espidfMainTablePage = ESPIDFMainTablePage.getInstance(eimJson); |
| IncreasePartitionSizeMessage=Less than 30% of application partition size is free({0} of {1} bytes), would you like to increase it? Please click <a href={2}>here</a> to check more details. | ||
| ToolsInitializationDifferentPathMessageBoxTitle=Different IDF path found in the config file | ||
| ToolsInitializationEimMissingMsgBoxTitle=ESP-IDF Missing | ||
| ToolsInitializationEimMissingMsgBoxMessage=ESP-IDF is not found on your system. Please user EIM to install the ESP-IDF on your system to use the IDE. Click Yes to go to the download page and install EIM. |
There was a problem hiding this comment.
Fix typo and improve message clarity.
The message contains a typo and could be more specific about the user action required.
Fix the typo and improve the message:
-ToolsInitializationEimMissingMsgBoxMessage=ESP-IDF is not found on your system. Please user EIM to install the ESP-IDF on your system to use the IDE. Click Yes to go to the download page and install EIM.
+ToolsInitializationEimMissingMsgBoxMessage=ESP-IDF is not found on your system. Please use EIM (Espressif Installation Manager) to install the ESP-IDF on your system to use the IDE. When prompted, click Yes to open the EIM download page in your browser.📝 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.
| ToolsInitializationEimMissingMsgBoxMessage=ESP-IDF is not found on your system. Please user EIM to install the ESP-IDF on your system to use the IDE. Click Yes to go to the download page and install EIM. | |
| ToolsInitializationEimMissingMsgBoxMessage=ESP-IDF is not found on your system. Please use EIM (Espressif Installation Manager) to install the ESP-IDF on your system to use the IDE. When prompted, click Yes to open the EIM download page in your browser. |
There was a problem hiding this comment.
Actionable comments posted: 16
🔭 Outside diff range comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (1)
Line range hint
517-534: Fix incorrect column index mapping in compare method.The switch statement's case indices don't match the column order defined in
setupColumns(). This will result in incorrect sorting behavior.Apply this fix:
switch (propertyIndex) { case 0: - rc = p1.getName().compareTo(p2.getName()); + rc = Boolean.compare( + ToolsUtility.isIdfInstalledActive(p1), + ToolsUtility.isIdfInstalledActive(p2)); break; case 1: - rc = p1.getPath().compareTo(p2.getPath()); + rc = getIdfVersion(p1).compareTo(getIdfVersion(p2)); break; case 2: - Boolean p1State = ToolsUtility.isIdfInstalledActive(p1); - Boolean p2State = ToolsUtility.isIdfInstalledActive(p2); - rc = p1State.compareTo(p2State); + rc = p1.getName().compareTo(p2.getName()); + break; +case 3: + rc = p1.getPath().compareTo(p2.getPath()); break; default: break;
🧹 Nitpick comments (8)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (1)
386-400: Consider extracting the setup tools initialization to a separate method.The button event handler contains complex initialization logic that could be moved to a dedicated method for better maintainability.
Extract the setup tools initialization:
+private void initializeSetupTools(IdfInstalled idfInstalled, Button button) { + IDFConsole idfConsole = new IDFConsole(); + MessageConsoleStream standardConsoleStream = + idfConsole.getConsoleStream(Messages.IDFToolsHandler_ToolsManagerConsole, null, false, true); + MessageConsoleStream errorConsoleStream = + idfConsole.getConsoleStream(Messages.IDFToolsHandler_ToolsManagerConsole, null, true, true); + SetupToolsInIde setupToolsInIde = + new SetupToolsInIde(idfInstalled, eimJson, errorConsoleStream, standardConsoleStream); + SetupToolsJobListener toolsActivationJobListener = + new SetupToolsJobListener(ESPIDFMainTablePage.this, setupToolsInIde); + setupToolsInIde.addJobChangeListener(toolsActivationJobListener); + setupToolsInIde.schedule(); + button.setEnabled(false); +} setActiveButton.addListener(SWT.Selection, e -> { Button btn = (Button) e.widget; - IDFConsole idfConsole = new IDFConsole(); - MessageConsoleStream standardConsoleStream = ... - MessageConsoleStream errorConsoleStream = ... - SetupToolsInIde setupToolsInIde = ... - SetupToolsJobListener toolsActivationJobListener = ... - setupToolsInIde.addJobChangeListener(toolsActivationJobListener); - setupToolsInIde.schedule(); - btn.setEnabled(false); + initializeSetupTools((IdfInstalled) btn.getData(IDF_TOOL_SET_BTN_KEY), btn); });bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (5)
38-55: Use try-with-resources to ensure the FileInputStream is properly closedThe
FileInputStreamshould be enclosed in a try-with-resources statement to automatically handle the resource closing, even if an exception occurs. This improves resource management and prevents potential resource leaks.Apply this refactor:
-public static String getFileChecksum(MessageDigest digest, File file) throws IOException -{ - FileInputStream fis = new FileInputStream(file); - byte[] byteArray = new byte[1024]; - int bytesCount = 0; - while ((bytesCount = fis.read(byteArray)) != -1) - { - digest.update(byteArray, 0, bytesCount); - } - ; - fis.close(); - byte[] bytes = digest.digest(); - StringBuilder sb = new StringBuilder(); - for (int i = 0; i < bytes.length; i++) - { - sb.append(Integer.toString((bytes[i] & 0xff) + 0x100, 16).substring(1)); - } - return sb.toString(); -} +public static String getFileChecksum(MessageDigest digest, File file) throws IOException +{ + try (FileInputStream fis = new FileInputStream(file)) + { + byte[] byteArray = new byte[1024]; + int bytesCount = 0; + while ((bytesCount = fis.read(byteArray)) != -1) + { + digest.update(byteArray, 0, bytesCount); + } + } + byte[] bytes = digest.digest(); + StringBuilder sb = new StringBuilder(); + for (int i = 0; i < bytes.length; i++) + { + sb.append(Integer.toString((bytes[i] & 0xff) + 0x100, 16).substring(1)); + } + return sb.toString(); +}
47-47: Remove unnecessary semicolonThere is an extraneous semicolon after the while loop that is not required and can be removed to clean up the code.
Apply this change:
- ;
58-96: Remove unused parameter 'gitPath' ingetIdfVersionmethodThe parameter
gitPathis not utilized within thegetIdfVersionmethod. Removing it will simplify the method signature and avoid confusion.Apply this change:
-public static String getIdfVersion(IdfInstalled idfInstalled, String gitPath) +public static String getIdfVersion(IdfInstalled idfInstalled)Ensure to update any calls to this method accordingly in the codebase.
65-96: Check the exit value of the process to ensure successful executionAfter executing the activation script, it's good practice to verify if the process exited successfully. Checking the exit code can help handle errors gracefully and provide better debugging information.
You can enhance the code as follows:
process.waitFor(); +int exitCode = process.exitValue(); +if (exitCode != 0) { + Logger.log("Activation script exited with code: " + exitCode); + // Handle the error as needed +}
122-124: Optimize environment variable retrievalCreating a new
IDFEnvironmentVariablesinstance each time may not be efficient. Consider reusing an existing instance or implementing a singleton pattern if appropriate.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (1)
343-364: Optimize process waiting by usingwaitForwith a timeoutThe current implementation uses a custom loop with
Thread.sleep()to wait for the process to complete, which may not be efficient.Consider using
process.waitFor(long timeout, TimeUnit unit)to wait for the process with a specified timeout. This simplifies the code and enhances reliability.Apply this diff to refactor the code:
try { Process process = processRunner.run(arguments, org.eclipse.core.runtime.Path.ROOT, env); BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream())); String line; while ((line = reader.readLine()) != null) { output.append(line).append(System.lineSeparator()); } - int waitCount = 10; - while (process.isAlive() && waitCount > 0) - { - try - { - Thread.sleep(300); - } - catch (InterruptedException e) - { - Logger.log(e); - } - waitCount--; - } - if (waitCount == 0) + boolean finished = process.waitFor(3000, TimeUnit.MILLISECONDS); + if (!finished) { log("Process possibly stuck", IStatus.ERROR); //$NON-NLS-1$ return Status.CANCEL_STATUS; }bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java (1)
62-63: Consider using dependency injectionThe direct instantiation of
EimIdfConfiguratinParsercreates tight coupling between UI and core components. Consider using dependency injection to improve modularity and testability.
- Define an interface in the core bundle:
public interface IEimConfigurationParser { EimJson getEimJson(boolean refresh) throws IOException; }
- Use OSGi services or a dependency injection framework to inject the implementation:
@Inject private IEimConfigurationParser configurationParser;Also applies to: 101-104
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bundles/com.espressif.idf.ui/icons/tools/delete.pngis excluded by!**/*.png
📒 Files selected for processing (14)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFEnvironmentVariables.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/Messages.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/messages.properties(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java(2 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java(3 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFNewToolsWizard.java(2 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.java(2 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java(2 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/SetupToolsJobListener.java(4 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java(13 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build_macos
🔇 Additional comments (14)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java (1)
121-121: LGTM! Message declarations follow established patterns.The new message declarations for
EspIdfManagerNameColandIDFToolsHandler_ToolsManagerConsoleare consistent with the existing naming conventions and properly support the EIM integration UI components.Also applies to: 133-133
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties (3)
117-117: LGTM! Clear and concise column header.The message value "Name" for
EspIdfManagerNameColis appropriately simple and follows UI text best practices.
133-134: LGTM! Clear user instruction.The message for
IDFDownloadHandler_DownloadPageMsgprovides clear guidance to users about their options.
135-135: LGTM! Consistent console title.The message for
IDFToolsHandler_ToolsManagerConsolefollows product naming conventions and clearly identifies the console purpose.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (2)
68-68:⚠️ Potential issueConsider making
eimJsonan instance variable to prevent threading issues.The static
eimJsonvariable could lead to race conditions if multiple instances of the plugin are running concurrently.Convert the static variable to an instance variable:
-private static EimJson eimJson; +private EimJson eimJson;Likely invalid or redundant comment.
437-441:⚠️ Potential issueImplement the reload functionality.
The reload button's event handler contains commented-out code that needs to be implemented.
Let's verify if the required classes exist in the codebase:
✅ Verification successful
Uncomment the reload functionality implementation
The required classes and methods exist in the codebase. The commented code should be uncommented as it contains the correct implementation using:
ToolsActivationJobfrom com.espressif.idf.ui.toolsIDFUtil.getPythonExecutable()andIDFUtil.getGitExecutablePathFromSystem()from com.espressif.idf.core.util🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ToolsActivationJob class ast-grep --pattern 'class ToolsActivationJob { $$$ }' # Search for IDFUtil class methods ast-grep --pattern 'class IDFUtil { $$$ getPythonExecutable() { $$$ } $$$ getGitExecutablePathFromSystem() { $$$ } $$$ }'Length of output: 203
Script:
#!/bin/bash # Search for ToolsActivationJob and related classes rg -l "ToolsActivationJob|ToolsActivationJobListener" --type java # Search for IDFUtil class and its methods rg "class.*IDFUtil|getPythonExecutable|getGitExecutablePathFromSystem" -A 5 --type java # Search for similar job patterns in case names changed rg "class.*Job.*extends.*Job.*activation" -A 5 --type javaLength of output: 10993
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.java (1)
15-15: LGTM!The integration of
EimEditorInputand the updated configuration parser enhances the configuration management approach. The implementation is clean and aligns with best practices.Also applies to: 41-41
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (1)
99-117: ValidateactivationScriptPathto prevent security risksEnsure that
activationScriptPathis properly validated to prevent command injection or execution of unintended scripts.Please confirm that
activationScriptPathcomes from a trusted source or implement validation checks.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/Messages.java (1)
1-27: Code structure and internationalization setup look goodThe
Messagesclass is properly configured for internationalization usingNLS.bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFEnvironmentVariables.java (1)
55-56: LGTM! Environment variable constant follows conventions.The new constant
ESP_IDF_EIM_IDfollows the established naming pattern and is properly documented.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.java (1)
9-9: Ensure POSIX path resolves the user's home directoryThe constant
EIM_POSIX_PATHuses'~'to represent the user's home directory. This will not be automatically expanded when used withFile.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java (3)
37-38: Fix typo in class name and fieldThe class name and field have a typo: 'Configuratin' should be 'Configuration'.
This issue was previously identified and requires coordinated changes across core and UI bundles. Please refer to the previous review comment for the complete fix.
Also applies to: 62-63
142-156: Fix path handling for POSIX systemsThe POSIX path handling doesn't expand the '~' character to the user's home directory.
This issue was previously identified. Please refer to the previous review comment for the complete fix.
106-114: 🛠️ Refactor suggestionImprove error handling for EIM JSON parsing
The error handling for
IOExceptionis insufficient. The error should be propagated to the user since this is a critical initialization step.Apply this diff to improve error handling:
try { eimJson = eimIdfConfiguratinParser.getEimJson(true); } catch (IOException e) { Logger.log(e); + Display.getDefault().asyncExec(() -> { + Shell shell = Display.getDefault().getActiveShell(); + if (shell == null) { + shell = new Shell(Display.getDefault()); + } + MessageBox errorBox = new MessageBox(shell, SWT.ICON_ERROR | SWT.OK); + errorBox.setText("EIM Configuration Error"); + errorBox.setMessage("Failed to load EIM configuration: " + e.getMessage()); + errorBox.open(); + if (shell.isDisposed()) { + shell.dispose(); + } + }); }Likely invalid or redundant comment.
| public static String getIDFPythonEnvPath(String idfPyEnvPath) | ||
| { | ||
| idfPyEnvPath = idfPyEnvPath.strip(); | ||
| if (!StringUtil.isEmpty(idfPyEnvPath)) | ||
| { | ||
|
|
||
| if (Platform.getOS().equals(Platform.OS_WIN32)) | ||
| { | ||
| idfPyEnvPath = idfPyEnvPath + "/" + "Scripts"; //$NON-NLS-1$ //$NON-NLS-2$ | ||
| } | ||
| else | ||
| { | ||
| idfPyEnvPath = idfPyEnvPath + "/" + "bin"; //$NON-NLS-1$ //$NON-NLS-2$ | ||
| } | ||
| java.nio.file.Path commandPath = findCommand(IDFConstants.PYTHON_CMD, idfPyEnvPath); | ||
| if (commandPath != null) | ||
| { | ||
| return commandPath.toFile().getAbsolutePath(); | ||
| } | ||
| } | ||
| return findCommandFromBuildEnvPath(IDFConstants.PYTHON_CMD); | ||
|
|
||
| } |
There was a problem hiding this comment.
Add null check and improve error handling.
The new method has potential issues that need to be addressed:
- Missing null check could lead to NullPointerException
- Silent failure when command path is not found
- Platform-dependent path separator usage
Apply this diff to fix the issues:
public static String getIDFPythonEnvPath(String idfPyEnvPath)
{
+ if (idfPyEnvPath == null) {
+ Logger.log("idfPyEnvPath is null, falling back to build environment PATH");
+ return findCommandFromBuildEnvPath(IDFConstants.PYTHON_CMD);
+ }
idfPyEnvPath = idfPyEnvPath.strip();
if (!StringUtil.isEmpty(idfPyEnvPath))
{
if (Platform.getOS().equals(Platform.OS_WIN32))
{
- idfPyEnvPath = idfPyEnvPath + "/" + "Scripts"; //$NON-NLS-1$ //$NON-NLS-2$
+ idfPyEnvPath = idfPyEnvPath + File.separator + "Scripts"; //$NON-NLS-1$
}
else
{
- idfPyEnvPath = idfPyEnvPath + "/" + "bin"; //$NON-NLS-1$ //$NON-NLS-2$
+ idfPyEnvPath = idfPyEnvPath + File.separator + "bin"; //$NON-NLS-1$
}
java.nio.file.Path commandPath = findCommand(IDFConstants.PYTHON_CMD, idfPyEnvPath);
if (commandPath != null)
{
return commandPath.toFile().getAbsolutePath();
+ } else {
+ Logger.log("Python command not found in " + idfPyEnvPath + ", falling back to build environment PATH");
}
}
return findCommandFromBuildEnvPath(IDFConstants.PYTHON_CMD);
}Committable suggestion skipped: line range outside the PR's diff.
🛠️ Refactor suggestion
Reduce code duplication with the existing getIDFPythonEnvPath() method.
The new method duplicates most of the logic from the existing method. Consider refactoring to share the common code.
Apply this diff to reduce duplication:
+private static String getIDFPythonEnvPathInternal(String idfPyEnvPath) {
+ if (idfPyEnvPath == null) {
+ Logger.log("idfPyEnvPath is null, falling back to build environment PATH");
+ return findCommandFromBuildEnvPath(IDFConstants.PYTHON_CMD);
+ }
+ idfPyEnvPath = idfPyEnvPath.strip();
+ if (!StringUtil.isEmpty(idfPyEnvPath))
+ {
+ if (Platform.getOS().equals(Platform.OS_WIN32))
+ {
+ idfPyEnvPath = idfPyEnvPath + File.separator + "Scripts"; //$NON-NLS-1$
+ }
+ else
+ {
+ idfPyEnvPath = idfPyEnvPath + File.separator + "bin"; //$NON-NLS-1$
+ }
+ java.nio.file.Path commandPath = findCommand(IDFConstants.PYTHON_CMD, idfPyEnvPath);
+ if (commandPath != null)
+ {
+ return commandPath.toFile().getAbsolutePath();
+ }
+ Logger.log("Python command not found in " + idfPyEnvPath + ", falling back to build environment PATH");
+ }
+ return findCommandFromBuildEnvPath(IDFConstants.PYTHON_CMD);
+}
public static String getIDFPythonEnvPath()
{
- String idfPyEnvPath = new IDFEnvironmentVariables().getEnvValue(IDFEnvironmentVariables.IDF_PYTHON_ENV_PATH);
- idfPyEnvPath = idfPyEnvPath.strip();
- if (!StringUtil.isEmpty(idfPyEnvPath))
- {
- if (Platform.getOS().equals(Platform.OS_WIN32))
- {
- idfPyEnvPath = idfPyEnvPath + "/" + "Scripts"; //$NON-NLS-1$ //$NON-NLS-2$
- }
- else
- {
- idfPyEnvPath = idfPyEnvPath + "/" + "bin"; //$NON-NLS-1$ //$NON-NLS-2$
- }
- java.nio.file.Path commandPath = findCommand(IDFConstants.PYTHON_CMD, idfPyEnvPath);
- if (commandPath != null)
- {
- return commandPath.toFile().getAbsolutePath();
- }
- }
- return findCommandFromBuildEnvPath(IDFConstants.PYTHON_CMD);
+ return getIDFPythonEnvPathInternal(
+ new IDFEnvironmentVariables().getEnvValue(IDFEnvironmentVariables.IDF_PYTHON_ENV_PATH)
+ );
}
public static String getIDFPythonEnvPath(String idfPyEnvPath)
{
- idfPyEnvPath = idfPyEnvPath.strip();
- if (!StringUtil.isEmpty(idfPyEnvPath))
- {
- if (Platform.getOS().equals(Platform.OS_WIN32))
- {
- idfPyEnvPath = idfPyEnvPath + "/" + "Scripts"; //$NON-NLS-1$ //$NON-NLS-2$
- }
- else
- {
- idfPyEnvPath = idfPyEnvPath + "/" + "bin"; //$NON-NLS-1$ //$NON-NLS-2$
- }
- java.nio.file.Path commandPath = findCommand(IDFConstants.PYTHON_CMD, idfPyEnvPath);
- if (commandPath != null)
- {
- return commandPath.toFile().getAbsolutePath();
- }
- }
- return findCommandFromBuildEnvPath(IDFConstants.PYTHON_CMD);
+ return getIDFPythonEnvPathInternal(idfPyEnvPath);
}📝 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 static String getIDFPythonEnvPath(String idfPyEnvPath) | |
| { | |
| idfPyEnvPath = idfPyEnvPath.strip(); | |
| if (!StringUtil.isEmpty(idfPyEnvPath)) | |
| { | |
| if (Platform.getOS().equals(Platform.OS_WIN32)) | |
| { | |
| idfPyEnvPath = idfPyEnvPath + "/" + "Scripts"; //$NON-NLS-1$ //$NON-NLS-2$ | |
| } | |
| else | |
| { | |
| idfPyEnvPath = idfPyEnvPath + "/" + "bin"; //$NON-NLS-1$ //$NON-NLS-2$ | |
| } | |
| java.nio.file.Path commandPath = findCommand(IDFConstants.PYTHON_CMD, idfPyEnvPath); | |
| if (commandPath != null) | |
| { | |
| return commandPath.toFile().getAbsolutePath(); | |
| } | |
| } | |
| return findCommandFromBuildEnvPath(IDFConstants.PYTHON_CMD); | |
| } | |
| private static String getIDFPythonEnvPathInternal(String idfPyEnvPath) { | |
| if (idfPyEnvPath == null) { | |
| Logger.log("idfPyEnvPath is null, falling back to build environment PATH"); | |
| return findCommandFromBuildEnvPath(IDFConstants.PYTHON_CMD); | |
| } | |
| idfPyEnvPath = idfPyEnvPath.strip(); | |
| if (!StringUtil.isEmpty(idfPyEnvPath)) | |
| { | |
| if (Platform.getOS().equals(Platform.OS_WIN32)) | |
| { | |
| idfPyEnvPath = idfPyEnvPath + File.separator + "Scripts"; //$NON-NLS-1$ | |
| } | |
| else | |
| { | |
| idfPyEnvPath = idfPyEnvPath + File.separator + "bin"; //$NON-NLS-1$ | |
| } | |
| java.nio.file.Path commandPath = findCommand(IDFConstants.PYTHON_CMD, idfPyEnvPath); | |
| if (commandPath != null) | |
| { | |
| return commandPath.toFile().getAbsolutePath(); | |
| } | |
| Logger.log("Python command not found in " + idfPyEnvPath + ", falling back to build environment PATH"); | |
| } | |
| return findCommandFromBuildEnvPath(IDFConstants.PYTHON_CMD); | |
| } | |
| public static String getIDFPythonEnvPath() | |
| { | |
| return getIDFPythonEnvPathInternal( | |
| new IDFEnvironmentVariables().getEnvValue(IDFEnvironmentVariables.IDF_PYTHON_ENV_PATH) | |
| ); | |
| } | |
| public static String getIDFPythonEnvPath(String idfPyEnvPath) | |
| { | |
| return getIDFPythonEnvPathInternal(idfPyEnvPath); | |
| } |
| // Button removeButton = new Button(buttonComposite, SWT.PUSH | SWT.FLAT); | ||
| // removeButton.pack(); | ||
| // removeButton.setData(IDF_TOOL_SET_BTN_KEY, idfInstalled); | ||
| // removeButton.setImage(UIPlugin.getImage(REMOVE_ICON)); | ||
| // removeButton.setToolTipText(Messages.EspIdfManagerDeleteBtnToolTip); | ||
| // removeButton.addListener(SWT.Selection, e -> { | ||
| // Button btn = (Button) e.widget; | ||
| // IdfInstalled selectedToolSet = (IdfInstalled) btn.getData(IDF_TOOL_SET_BTN_KEY); | ||
| // performDeleteOperation(selectedToolSet); | ||
| // refreshEditorUI(); | ||
| // }); | ||
| // removeButton.setSize(cellBounds.width, buttonHeight); | ||
| // removeButton.redraw(); |
There was a problem hiding this comment.
💡 Codebase verification
Fix type mismatch before implementing remove functionality
The remove button implementation is commented out, but simply uncommenting won't work. There's a type mismatch:
- The button stores data of type
IdfInstalled - The
performDeleteOperationmethod expectsIDFToolSet
🔗 Analysis chain
Implement the remove functionality.
The remove button's creation and event handling code is commented out, which means the UI lacks the ability to remove IDF installations.
Let's verify if we can safely uncomment this code:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for REMOVE_ICON constant
rg "REMOVE_ICON.*=.*" -A 1
# Search for performDeleteOperation method implementation
ast-grep --pattern 'performDeleteOperation(IdfInstalled $_) {
$$$
}'
Length of output: 101
Script:
#!/bin/bash
# Search for any icon-related constants and remove/delete operations
rg -i "remove.*icon|delete.*icon" -g "*.java"
# Search for any method containing delete or remove in the name
ast-grep --pattern 'void $_delete$_($_) {
$$$
}'
# Search for any method containing delete or remove with different signature
ast-grep --pattern 'boolean $_delete$_($_) {
$$$
}'
# Look for Messages class and tooltip
rg "EspIdfManagerDeleteBtnToolTip" -g "*.java"
Length of output: 662
Script:
#!/bin/bash
# Search for performDeleteOperation with any signature
rg "performDeleteOperation" -g "*.java" -A 5
# Search for any method containing perform and delete
rg "perform.*delete|delete.*perform" -g "*.java" -A 5
# Get more context around the button implementation
rg -B 10 -A 10 "removeButton.setImage" -g "*.java"
Length of output: 4845
|
|
||
| import com.espressif.idf.core.logging.Logger; | ||
| import com.espressif.idf.core.tools.IToolsInstallationWizardConstants; | ||
| import com.espressif.idf.core.tools.EimIdfConfiguratinParser; |
There was a problem hiding this comment.
Correct the typo in EimIdfConfiguratinParser.
The class name EimIdfConfiguratinParser appears to have a typographical error. It should be EimIdfConfigurationParser. This typo affects the import statement and its usage in the code.
Apply this diff to correct the class name:
- import com.espressif.idf.core.tools.EimIdfConfiguratinParser;
+ import com.espressif.idf.core.tools.EimIdfConfigurationParser;- EimIdfConfiguratinParser eimIdfConfiguratinParser = new EimIdfConfiguratinParser();
+ EimIdfConfigurationParser eimIdfConfigurationParser = new EimIdfConfigurationParser();- IDE.openEditor(activeww.getActivePage(), new EimEditorInput(eimIdfConfiguratinParser.getEimJson(true)), ESPIDFManagerEditor.EDITOR_ID);
+ IDE.openEditor(activeww.getActivePage(), new EimEditorInput(eimIdfConfigurationParser.getEimJson(true)), ESPIDFManagerEditor.EDITOR_ID);Also applies to: 39-41
| public static boolean isIdfInstalledActive(IdfInstalled idfInstalled) | ||
| { | ||
| if (StringUtil.isEmpty(path)) | ||
| { | ||
| Map<String, String> env = IDFUtil.getSystemEnv(); | ||
| if (env.containsKey(IDFEnvironmentVariables.PATH)) | ||
| path = env.get(IDFEnvironmentVariables.PATH); | ||
| else | ||
| path = env.get("Path"); //$NON-NLS-1$ | ||
| } | ||
|
|
||
| SystemExecutableFinder systemExecutableFinder = new SystemExecutableFinder(new ToolsSystemWrapper(path)); | ||
| return systemExecutableFinder.find(toolName); | ||
| IDFEnvironmentVariables idfEnvironmentVariables = new IDFEnvironmentVariables(); | ||
| String espIdfIdEim = idfEnvironmentVariables.getEnvValue(IDFEnvironmentVariables.ESP_IDF_EIM_ID); | ||
| return idfInstalled.getId().equals(espIdfIdEim); |
There was a problem hiding this comment.
Prevent potential NullPointerException in isIdfInstalledActive method
If idfInstalled.getId() could be null, calling .equals() on it may lead to a NullPointerException. Use Objects.equals() to safely compare the IDs.
Apply this fix:
-import com.espressif.idf.core.IDFEnvironmentVariables;
+import com.espressif.idf.core.IDFEnvironmentVariables;
+import java.util.Objects;
...
-return idfInstalled.getId().equals(espIdfIdEim);
+return Objects.equals(idfInstalled.getId(), espIdfIdEim);📝 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 static boolean isIdfInstalledActive(IdfInstalled idfInstalled) | |
| { | |
| if (StringUtil.isEmpty(path)) | |
| { | |
| Map<String, String> env = IDFUtil.getSystemEnv(); | |
| if (env.containsKey(IDFEnvironmentVariables.PATH)) | |
| path = env.get(IDFEnvironmentVariables.PATH); | |
| else | |
| path = env.get("Path"); //$NON-NLS-1$ | |
| } | |
| SystemExecutableFinder systemExecutableFinder = new SystemExecutableFinder(new ToolsSystemWrapper(path)); | |
| return systemExecutableFinder.find(toolName); | |
| IDFEnvironmentVariables idfEnvironmentVariables = new IDFEnvironmentVariables(); | |
| String espIdfIdEim = idfEnvironmentVariables.getEnvValue(IDFEnvironmentVariables.ESP_IDF_EIM_ID); | |
| return idfInstalled.getId().equals(espIdfIdEim); | |
| import com.espressif.idf.core.IDFEnvironmentVariables; | |
| import java.util.Objects; | |
| public static boolean isIdfInstalledActive(IdfInstalled idfInstalled) | |
| { | |
| IDFEnvironmentVariables idfEnvironmentVariables = new IDFEnvironmentVariables(); | |
| String espIdfIdEim = idfEnvironmentVariables.getEnvValue(IDFEnvironmentVariables.ESP_IDF_EIM_ID); | |
| return Objects.equals(idfInstalled.getId(), espIdfIdEim); |
| if (line.startsWith("ESP_IDF_VERSION=")) //$NON-NLS-1$ | ||
| { | ||
| espIdfVersion = line.split("=")[1]; //$NON-NLS-1$ | ||
| break; | ||
| } |
There was a problem hiding this comment.
Avoid potential ArrayIndexOutOfBoundsException when splitting the version string
When splitting the line to extract espIdfVersion, ensure that the line contains an = character to prevent an ArrayIndexOutOfBoundsException.
Apply this fix:
if (line.startsWith("ESP_IDF_VERSION=")) //$NON-NLS-1$
{
+ if (line.contains("=")) {
espIdfVersion = line.split("=")[1]; //$NON-NLS-1$
break;
+ }
}📝 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.
| if (line.startsWith("ESP_IDF_VERSION=")) //$NON-NLS-1$ | |
| { | |
| espIdfVersion = line.split("=")[1]; //$NON-NLS-1$ | |
| break; | |
| } | |
| if (line.startsWith("ESP_IDF_VERSION=")) //$NON-NLS-1$ | |
| { | |
| if (line.contains("=")) { | |
| espIdfVersion = line.split("=")[1]; //$NON-NLS-1$ | |
| break; | |
| } | |
| } |
| ToolsActivationJob toolsActivationJob = new ToolsActivationJob(idfToolSets.get(0), pythonPath, gitPath); | ||
| ToolsActivationJobListener toolsActivationJobListener = new ToolsActivationJobListener(espidfMainTablePage); | ||
| toolsActivationJob.addJobChangeListener(toolsActivationJobListener); | ||
| toolsActivationJob.schedule(); | ||
| // SetupToolsJobListener toolsActivationJobListener = new SetupToolsJobListener(espidfMainTablePage4); | ||
| // toolsActivationJob.addJobChangeListener(toolsActivationJobListener); | ||
| // toolsActivationJob.schedule(); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Critical: Tools activation must not be disabled
The commented-out code prevents essential IDE setup:
- Environment variables required for ESP-IDF development won't be configured
- Toolchains and build targets won't be initialized
- OpenOCD debugging capabilities on Linux won't be properly set up
- WebSocket client installation will be skipped
🔗 Analysis chain
Verify the impact of disabling tools activation.
The tools activation process has been disabled by commenting out the listener setup. This could affect the IDE's ability to use the tools after installation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to ToolsActivationJob to understand the impact
rg "ToolsActivationJob" -A 5
Length of output: 3960
Script:
#!/bin/bash
# Get the implementation of ToolsActivationJob
cat bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsActivationJob.java
Length of output: 7300
|
|
||
| public interface EimConstants | ||
| { | ||
| String EIM_WIN_PATH = "C:\\Espressif\\tools\\esp_ide.json"; //$NON-NLS-1$ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making Windows path configurable.
The hardcoded Windows path C:\\Espressif\\tools\\esp_ide.json might not be suitable for all installations. Consider:
- Making it configurable through preferences
- Using a relative path to the user's home directory
| 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 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make error message platform-agnostic.
The error message specifically mentions the sudo command, which is Linux/Unix specific. Consider making it platform-agnostic, such as "try running Eclipse with administrator privileges".
-InstallToolsHandler_OpenOCDRulesCopyError=Unable to copy rules for OpenOCD to system directory, try running the eclipse with sudo command
+InstallToolsHandler_OpenOCDRulesCopyError=Unable to copy rules for OpenOCD to system directory. Please try running Eclipse with administrator privileges📝 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.
| InstallToolsHandler_OpenOCDRulesCopyError=Unable to copy rules for OpenOCD to system directory, try running the eclipse with sudo command | |
| InstallToolsHandler_OpenOCDRulesCopyError=Unable to copy rules for OpenOCD to system directory. Please try running Eclipse with administrator privileges |
| private void userNotficationToInstallEim() | ||
| { | ||
| Display.getDefault().asyncExec(()-> { | ||
| Shell shell = new Shell(Display.getDefault()); | ||
| MessageBox messageBox = new MessageBox(shell, SWT.ICON_WARNING | SWT.YES | SWT.NO); | ||
|
|
||
| messageBox.setText(Messages.ToolsInitializationEimMissingMsgBoxTitle); | ||
| messageBox.setMessage(Messages.ToolsInitializationEimMissingMsgBoxMessage); | ||
| int response = messageBox.open(); | ||
| if (response == SWT.YES) | ||
| { | ||
| Program.launch(EimConstants.EIM_URL); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Improve shell management and URL handling
Several issues in the notification method:
- Shell is created but never disposed
- Direct URL launch could be a security concern
Apply this diff to improve the implementation:
private void userNotficationToInstallEim()
{
Display.getDefault().asyncExec(()-> {
- Shell shell = new Shell(Display.getDefault());
+ Shell shell = Display.getDefault().getActiveShell();
+ if (shell == null) {
+ shell = new Shell(Display.getDefault());
+ }
MessageBox messageBox = new MessageBox(shell, SWT.ICON_WARNING | SWT.YES | SWT.NO);
messageBox.setText(Messages.ToolsInitializationEimMissingMsgBoxTitle);
messageBox.setMessage(Messages.ToolsInitializationEimMissingMsgBoxMessage);
int response = messageBox.open();
if (response == SWT.YES)
{
- Program.launch(EimConstants.EIM_URL);
+ // Validate URL before launching
+ String url = EimConstants.EIM_URL;
+ if (url != null && (url.startsWith("https://") || url.startsWith("http://"))) {
+ Program.launch(url);
+ } else {
+ Logger.log("Invalid URL format: " + url);
+ MessageBox errorBox = new MessageBox(shell, SWT.ICON_ERROR | SWT.OK);
+ errorBox.setText("Error");
+ errorBox.setMessage("Invalid URL format");
+ errorBox.open();
+ }
}
+ if (shell.isDisposed()) {
+ shell.dispose();
+ }
});
}Committable suggestion skipped: line range outside the PR's diff.
| Display.getDefault().syncExec(()-> { | ||
| Shell shell = Display.getDefault().getActiveShell(); | ||
| MessageBox messageBox = new MessageBox(shell, SWT.ICON_WARNING| SWT.YES | SWT.NO); | ||
| messageBox.setText("No Active ESP-IDF in workspace"); | ||
| messageBox.setMessage("No active ESP-IDF found in current workspace. Do you want to select the ESP-IDF for current workspace?"); | ||
|
|
||
| int response = messageBox.open(); | ||
| if (response == SWT.NO) | ||
| if (response == SWT.YES) | ||
| { | ||
| IDFEnvironmentVariables idfEnvironmentVariables = new IDFEnvironmentVariables(); | ||
| updateEspIdfJsonFile(idf_json_file, | ||
| idfEnvironmentVariables.getEnvValue(IDFEnvironmentVariables.IDF_PATH)); | ||
| Preferences prefs = getPreferences(); | ||
| prefs.putBoolean(IS_INSTALLER_CONFIG_SET, true); | ||
| IWorkbenchWindow activeww = EclipseHandler.getActiveWorkbenchWindow(); | ||
| IDFUtil.closeWelcomePage(activeww); | ||
| try | ||
| { | ||
| prefs.flush(); | ||
| IDE.openEditor(activeww.getActivePage(), new EimEditorInput(eimJson), ESPIDFManagerEditor.EDITOR_ID); | ||
| } | ||
| catch (BackingStoreException e) | ||
| catch (PartInitException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure proper shell disposal and handle edge cases
The code has several issues:
- The shell obtained from
Display.getDefault().getActiveShell()might be null if no window is active. - The code runs in
syncExecwhich could cause UI freezes. Consider usingasyncExecfor better responsiveness.
Apply this diff to improve the implementation:
- Display.getDefault().syncExec(()-> {
- Shell shell = Display.getDefault().getActiveShell();
+ Display.getDefault().asyncExec(()-> {
+ Shell shell = Display.getDefault().getActiveShell();
+ if (shell == null) {
+ shell = new Shell(Display.getDefault());
+ }
MessageBox messageBox = new MessageBox(shell, SWT.ICON_WARNING| SWT.YES | SWT.NO);
messageBox.setText("No Active ESP-IDF in workspace");
messageBox.setMessage("No active ESP-IDF found in current workspace. Do you want to select the ESP-IDF for current workspace?");
int response = messageBox.open();
if (response == SWT.YES)
{
IWorkbenchWindow activeww = EclipseHandler.getActiveWorkbenchWindow();
+ if (activeww == null) {
+ Logger.log("No active workbench window found");
+ return;
+ }
IDFUtil.closeWelcomePage(activeww);
try
{
IDE.openEditor(activeww.getActivePage(), new EimEditorInput(eimJson), ESPIDFManagerEditor.EDITOR_ID);
}
catch (PartInitException e)
{
Logger.log(e);
+ MessageBox errorBox = new MessageBox(shell, SWT.ICON_ERROR | SWT.OK);
+ errorBox.setText("Error");
+ errorBox.setMessage("Failed to open ESP-IDF Manager: " + e.getMessage());
+ errorBox.open();
}
}
+ if (shell.isDisposed()) {
+ shell.dispose();
+ }
});Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/InstallToolsHandler.java (1)
Integration tests needed for critical EIM functionality
The verification confirms that EIM integration touches several critical paths (tools initialization, project setup, installation) but lacks test coverage. The original architectural advice remains valid and important:
- Add integration tests covering the complete EIM installation workflow
- Include test cases for EIM configuration parsing
- Add tests for EIM-related project setup scenarios
🔗 Analysis chain
Line range hint
40-109: Consider adding integration tests for the EIM workflow.Since this is part of the EIM integration and modifies the installation tools workflow:
- Consider adding integration tests that verify the complete EIM installation flow
- Document the new EIM-related configuration options in the plugin's documentation
Let's check for existing test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for test files related to EIM integration fd -e java -e xml test.*EimLength of output: 31
Script:
#!/bin/bash # Look for test files related to installation tools and EIM rg -l "class.*Test" | rg -i "install|tools" rg -l "EimConstants" --type javaLength of output: 599
♻️ Duplicate comments (7)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (3)
66-66:⚠️ Potential issueConvert static
eimJsonto instance variable to prevent threading issues.The static
eimJsonvariable can lead to race conditions if multiple plugin windows are opened simultaneously.Convert to instance variable:
-private static EimJson eimJson; +private EimJson eimJson;
132-139:⚠️ Potential issueImprove error handling for configuration parsing.
The catch block only logs the error without proper user feedback or recovery mechanism.
Add error handling and user feedback:
try { eimJson = eimIdfConfiguratinParser.getEimJson(true); } catch (IOException e) { Logger.log(e); + MessageDialog.openError( + Display.getDefault().getActiveShell(), + "Configuration Error", + "Failed to load IDF configuration: " + e.getMessage()); + return; }
405-423:⚠️ Potential issueImplement remove functionality for IDF installations.
The remove button implementation is missing, which prevents users from removing unwanted IDF installations.
Let's verify if we can safely implement the remove functionality:
#!/bin/bash # Search for any existing remove/delete implementation rg "remove.*idf|delete.*idf" -g "*.java" # Search for any UI messages related to remove/delete rg "EspIdfManagerDeleteBtn|EspIdfManagerRemoveBtn" -g "*.java"bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java (1)
158-171: 🛠️ Refactor suggestionAdd method documentation and verify URL safety
The method needs documentation and URL validation.
+ /** + * Displays a notification dialog prompting the user to install EIM when the + * configuration is missing. If the user agrees, opens the EIM download page + * in the default browser. + */ private void userNotficationToInstallEim() // Verify URL safety + if (!EimConstants.EIM_URL.startsWith("https://")) { + Logger.log("Unsafe URL protocol detected: " + EimConstants.EIM_URL); + return; + }Shell management and disposal issues.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (2)
253-253:⚠️ Potential issueFix syntax error due to extraneous parenthesis
At line 253, there is an extra closing parenthesis causing a syntax error in the
logmethod call.Apply this diff to fix the syntax error:
- log("Executing " + arguments.toString(), IStatus.OK); //$NON-NLS-1$) + log("Executing " + arguments.toString(), IStatus.OK); //$NON-NLS-1$
171-226:⚠️ Potential issueHandle permissions when copying OpenOCD rules on Linux
The method
copyOpenOcdRules()attempts to copy OpenOCD rules to/etc/udev/rules.d/60-openocd.rules, which requires root permissions. The current implementation does not handle permission issues that may arise if the application is not running with sufficient privileges. This could lead to anIOException.Consider adding proper error handling for permission issues and provide guidance to the user on how to proceed if the copy fails due to insufficient permissions. For example, prompt the user to run the application with elevated privileges or provide instructions on how to copy the rules manually.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/SetupToolsJobListener.java (1)
39-41:⚠️ Potential issuePrevent potential
NullPointerExceptiondue to uninitializedsetupToolsInIdeIn the
donemethod,setupToolsInIdemay benullif the default constructorSetupToolsJobListener()is used without initializingsetupToolsInIde. This could result in aNullPointerExceptionwhensetupToolsInIde.rollback()is called.To resolve this issue, consider adding a null check before calling
setupToolsInIde.rollback(), or remove the default constructor if it's unnecessary.Apply this diff to add a null check:
if (event.getResult().getSeverity() != IStatus.OK) { + if (setupToolsInIde != null) + { // Rollback all the changes setupToolsInIde.rollback(); + } + else + { + Logger.log("setupToolsInIde is null; rollback cannot be performed."); + } }Alternatively, if the default constructor is not needed, you can remove it:
- public SetupToolsJobListener() - { - }Also applies to: 55-59
🧹 Nitpick comments (6)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/NewProjectHandlerUtil.java (1)
Line range hint
28-54: Consider enhancing error handling and user experience.The current implementation could benefit from the following improvements:
- Consider combining the validation checks to provide a comprehensive list of issues rather than stopping at the first failure
- Add logging for debugging purposes
- Consider using a builder pattern for validation checks
Example refactor:
public static boolean installToolsCheck() { + Logger.debug("Checking IDF project prerequisites..."); + List<String> issues = new ArrayList<>(); + String idfPath = IDFUtil.getIDFPath(); if (StringUtil.isEmpty(idfPath)) { - showMessage(Messages.NewProjectHandler_CouldntFindIdfPath); - return false; + issues.add(Messages.NewProjectHandler_CouldntFindIdfPath); } IEnvironmentVariable pathEnv = new IDFEnvironmentVariables().getEnv(IDFEnvironmentVariables.PATH); String path = Optional.ofNullable(pathEnv).map(o -> o.getValue()).orElse(null); if (StringUtil.isEmpty(path)) { - showMessage(Messages.NewProjectHandler_CouldntFindPath); - return false; + issues.add(Messages.NewProjectHandler_CouldntFindPath); } boolean isToolsInstalled = scopedPreferenceStore.getBoolean(EimConstants.INSTALL_TOOLS_FLAG, false); if (!isToolsInstalled) { - showMessage(Messages.NewProjectHandler_CouldntFindTools); - return false; + issues.add(Messages.NewProjectHandler_CouldntFindTools); } - return true; + + if (!issues.isEmpty()) { + Logger.debug("Prerequisites check failed: " + String.join(", ", issues)); + showMessage(String.join("\n", issues)); + return false; + } + + Logger.debug("All prerequisites satisfied"); + return true; }bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (1)
37-37: Fix typo in class nameEimIdfConfiguratinParser.The class name contains a typo: "Configuratin" should be "Configuration".
This requires updating the class name in the core package as well.
Also applies to: 67-67, 76-76
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java (2)
101-104: Move parser initialization to constructorInitialize
eimIdfConfiguratinParserin the constructor to ensure it's always available and to improve code organization.public class InitializeToolsStartup implements IStartup { + public InitializeToolsStartup() { + eimIdfConfiguratinParser = new EimIdfConfiguratinParser(); + }
233-236: Add method documentationDocument the purpose of this check and explain the significance of the default value.
+ /** + * Checks if ESP-IDF tools are installed and configured for the current workspace. + * This setting is stored in the workspace preferences and is set to true after + * successful tool installation through EIM. + * + * @return true if ESP-IDF is set up, false otherwise (default) + */ private boolean isEspIdfSet()bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (2)
148-149: Correct the typo in "python" in log messagesThere is a typo in the word "python" in the task name and log message.
Apply this diff to correct the spelling:
- monitor.setTaskName("Installing pyhton dependency web-socket"); //$NON-NLS-1$ - log("Installing pyhton dependency web-socket", IStatus.INFO); //$NON-NLS-1$ + monitor.setTaskName("Installing python dependency websocket-client"); //$NON-NLS-1$ + log("Installing python dependency websocket-client", IStatus.INFO); //$NON-NLS-1$
356-373: Refactor the process waiting mechanismThe current implementation uses a loop with sleep to wait for the process to finish, which may not be reliable if the process takes longer than expected. If the process exceeds the wait time, it may incorrectly assume the process is stuck.
Consider using
process.waitFor(timeout, TimeUnit.SECONDS)to wait for the process completion with a specified timeout. This method is more reliable and makes the code cleaner.Apply this diff to refactor the waiting mechanism:
- int waitCount = 10; ... - while (process.isAlive() && waitCount > 0) - { - try - { - Thread.sleep(300); - } - catch (InterruptedException e) - { - Logger.log(e); - } - waitCount--; - } - - if (waitCount == 0) + boolean finished = process.waitFor(5, TimeUnit.SECONDS); + if (!finished) { log("Process possibly stuck", IStatus.ERROR); //$NON-NLS-1$ return Status.CANCEL_STATUS; }Remember to import
java.util.concurrent.TimeUnitat the beginning of the file:+ import java.util.concurrent.TimeUnit;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java(4 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/NewProjectHandlerUtil.java(2 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/SetupToolsJobListener.java(4 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java(13 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/InstallToolsHandler.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_macos
🔇 Additional comments (6)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/NewProjectHandlerUtil.java (2)
19-19: LGTM! Good practice to centralize constants.The addition of
EimConstantsimport aligns with the goal of centralizing configuration constants.
38-38: Verify all references to the installation flag are updated.This change from
InstallToolsHandler.INSTALL_TOOLS_FLAGtoEimConstants.INSTALL_TOOLS_FLAGis a breaking change.Let's verify all references are updated:
Additionally, please ensure this change is documented in:
- Migration guide
- Release notes
- API documentation
✅ Verification successful
Breaking change migration is complete
All references to the installation flag have been updated to use
EimConstants.INSTALL_TOOLS_FLAGconsistently, with no remaining references to the old constant.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old constant rg "InstallToolsHandler\.INSTALL_TOOLS_FLAG" # Search for usage of the new constant to ensure consistency rg "EimConstants\.INSTALL_TOOLS_FLAG"Length of output: 928
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/InstallToolsHandler.java (1)
40-40: LGTM! Import follows proper layering.The addition of EimConstants import from the core package maintains proper architectural layering.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java (3)
37-38: Fix typo in class and variable names: 'Configuratin' → 'Configuration'The misspelling exists in both the import and field declaration.
Also applies to: 62-63
142-156: Add method documentation and improve path handlingThe method needs documentation to explain its purpose and behavior.
+ /** + * Checks if the EIM JSON configuration file exists at the OS-specific location. + * For Windows, checks at {@link EimConstants#EIM_WIN_PATH}. + * For POSIX systems, checks at {@link EimConstants#EIM_POSIX_PATH}. + * + * @return true if the configuration file exists, false otherwise + */ private boolean isEimIdfJsonPresent()Path handling issue with '~' in POSIX paths.
106-113: 🛠️ Refactor suggestionEnhance error handling for EIM JSON parsing
The current implementation only logs the IOException. Consider informing the user about the configuration parsing failure.
try { eimJson = eimIdfConfiguratinParser.getEimJson(true); } catch (IOException e) { Logger.log(e); + Display.getDefault().asyncExec(() -> { + Shell shell = Display.getDefault().getActiveShell(); + if (shell == null) { + shell = new Shell(Display.getDefault()); + } + MessageBox errorBox = new MessageBox(shell, SWT.ICON_ERROR | SWT.OK); + errorBox.setText("Configuration Error"); + errorBox.setMessage("Failed to read EIM configuration: " + e.getMessage()); + errorBox.open(); + }); }Likely invalid or redundant comment.
| private String getIdfVersion(IdfInstalled idfInstalled) | ||
| { | ||
| String version = ToolsUtility.getIdfVersion(idfInstalled, eimJson.getGitPath()); | ||
| return version; | ||
| } |
There was a problem hiding this comment.
Add error handling to getIdfVersion method.
The method doesn't handle potential null values or exceptions that might occur when accessing eimJson.getGitPath().
Add proper error handling:
private String getIdfVersion(IdfInstalled idfInstalled) {
+ if (idfInstalled == null || eimJson == null) {
+ return "Unknown";
+ }
+ try {
String version = ToolsUtility.getIdfVersion(idfInstalled, eimJson.getGitPath());
- return version;
+ return version != null ? version : "Unknown";
+ } catch (Exception e) {
+ Logger.log(e);
+ return "Error";
+ }
}📝 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 String getIdfVersion(IdfInstalled idfInstalled) | |
| { | |
| String version = ToolsUtility.getIdfVersion(idfInstalled, eimJson.getGitPath()); | |
| return version; | |
| } | |
| private String getIdfVersion(IdfInstalled idfInstalled) | |
| { | |
| if (idfInstalled == null || eimJson == null) { | |
| return "Unknown"; | |
| } | |
| try { | |
| String version = ToolsUtility.getIdfVersion(idfInstalled, eimJson.getGitPath()); | |
| return version != null ? version : "Unknown"; | |
| } catch (Exception e) { | |
| Logger.log(e); | |
| return "Error"; | |
| } | |
| } |
| }; | ||
| Preferences scopedPreferenceStore = InstanceScope.INSTANCE.getNode(UIPlugin.PLUGIN_ID); | ||
| scopedPreferenceStore.putBoolean(INSTALL_TOOLS_FLAG, true); | ||
| scopedPreferenceStore.putBoolean(EimConstants.INSTALL_TOOLS_FLAG, true); |
There was a problem hiding this comment.
💡 Codebase verification
Migration to EimConstants.INSTALL_TOOLS_FLAG is incomplete
The constant is currently defined in multiple locations:
ToolsActivationJob.javastill has its own declaration and uses it directly- Both
EimConstants.javaandIToolsInstallationWizardConstants.javadefine the same constant
The constant should be defined only in EimConstants.java and referenced from there in all other files.
🔗 Analysis chain
Verify all references to INSTALL_TOOLS_FLAG are updated.
The migration to EimConstants.INSTALL_TOOLS_FLAG looks good, but since this is part of the EIM integration:
Let's verify all references are consistently updated:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct references to INSTALL_TOOLS_FLAG
# that haven't been migrated to use EimConstants
# Look for string literals of INSTALL_TOOLS_FLAG
rg -l "INSTALL_TOOLS_FLAG" --type java
# Look for constant declarations of INSTALL_TOOLS_FLAG
ast-grep --pattern 'static final String INSTALL_TOOLS_FLAG = $_'
Length of output: 719
Script:
#!/bin/bash
# Check the constant definition and usages
# Show EimConstants definition
echo "=== EimConstants.java ==="
rg -A 1 "INSTALL_TOOLS_FLAG" bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.java
# Show any potential conflicting definitions
echo -e "\n=== IToolsInstallationWizardConstants.java ==="
rg -A 1 "INSTALL_TOOLS_FLAG" bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsInstallationWizardConstants.java
# Show usage in other files
echo -e "\n=== Usage in other files ==="
for f in $(rg -l "INSTALL_TOOLS_FLAG" --type java); do
if [[ "$f" != *"EimConstants.java"* && "$f" != *"IToolsInstallationWizardConstants.java"* ]]; then
echo -e "\n--- $f ---"
rg -A 1 "INSTALL_TOOLS_FLAG" "$f"
fi
done
Length of output: 5227
| messageBox.setText("No Active ESP-IDF in workspace"); | ||
| messageBox.setMessage("No active ESP-IDF found in current workspace. Do you want to select the ESP-IDF for current workspace?"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Externalize dialog messages
Move the hardcoded strings to the Messages class for better maintainability and localization support.
-messageBox.setText("No Active ESP-IDF in workspace");
-messageBox.setMessage("No active ESP-IDF found in current workspace. Do you want to select the ESP-IDF for current workspace?");
+messageBox.setText(Messages.NoActiveEspIdfTitle);
+messageBox.setMessage(Messages.NoActiveEspIdfMessage);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties (1)
5-5: Maintain consistent naming convention across UI elements.The new column name
EspIdfManagerNameColfollows a different pattern compared to other column names. While others useManagerLocationCol,ManagerVersionCol, this usesManagerNameCol. Consider aligning with existing pattern.-EspIdfManagerNameCol=Name +EspIdfManagerNameCol=ESP-IDF Namebundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (3)
26-26: Follow utility class best practices.As this is a utility class containing only static methods:
- Mark the class as
finalto prevent inheritance- Add a private constructor to prevent instantiation
Apply this diff:
-public class ToolsUtility +public final class ToolsUtility { + private ToolsUtility() { + throw new AssertionError("Utility class - do not instantiate"); + }
27-28: Remove unused parameter.The
gitPathparameter is not used in the method implementation.Remove the unused parameter:
-public static String getIdfVersion(IdfInstalled idfInstalled, String gitPath) +public static String getIdfVersion(IdfInstalled idfInstalled)
68-86: Enhance OS handling and use constants.The method could be improved by:
- Adding explicit handling for unsupported OS
- Using constants for command strings
- Validating the input path
+private static final String POWERSHELL_EXE = "powershell.exe"; +private static final String EXECUTION_POLICY = "-ExecutionPolicy"; +private static final String BYPASS = "Bypass"; +private static final String FILE_PARAM = "-File"; +private static final String ECHO_PARAM = "-e"; +private static final String SH = "sh"; public static List<String> getExportScriptCommand(String activationScriptPath) { + if (activationScriptPath == null || activationScriptPath.trim().isEmpty()) { + throw new IllegalArgumentException("Activation script path cannot be null or empty"); + } List<String> command = new ArrayList<>(); if (Platform.getOS().equals(Platform.OS_WIN32)) { - command.add("powershell.exe"); - command.add("-ExecutionPolicy"); - command.add("Bypass"); - command.add("-File"); + command.add(POWERSHELL_EXE); + command.add(EXECUTION_POLICY); + command.add(BYPASS); + command.add(FILE_PARAM); command.add(activationScriptPath); - command.add("-e"); + command.add(ECHO_PARAM); } else { - command.add("sh"); + command.add(SH); command.add(activationScriptPath); - command.add("-e"); + command.add(ECHO_PARAM); } return command; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsInstallationWizardConstants.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsJsonKeys.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/JsonKey.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolSetConfigurationManager.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsJsonParser.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsPlatformMapping.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsSystemWrapper.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java(2 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IDFToolSet.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/ToolsVO.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionDetailsVO.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionsVO.java(0 hunks)bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/core/IDFMonitor.java(0 hunks)bundles/com.espressif.idf.ui/OSGI-INF/l10n/bundle.properties(0 hunks)bundles/com.espressif.idf.ui/plugin.xml(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/UpdateEspIdfHandler.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/GitRepositoryBuilder.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFDownloadPage.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFNewToolsWizard.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/Messages.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/messages.properties(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/messages_zh.properties(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/GitWizardRepProgressMonitor.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/LogMessagesThread.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsActivationJob.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationJob.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/InstallToolsHandler.java(0 hunks)
💤 Files with no reviewable changes (28)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/messages.properties
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsJsonKeys.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/JsonKey.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/GitRepositoryBuilder.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsSystemWrapper.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionsVO.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/Messages.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsInstallationWizardConstants.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationJob.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFNewToolsWizard.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/LogMessagesThread.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsPlatformMapping.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/InstallToolsHandler.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionDetailsVO.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IDFToolSet.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/ToolsVO.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFDownloadPage.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolSetConfigurationManager.java
- bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/core/IDFMonitor.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsJsonParser.java
- bundles/com.espressif.idf.ui/OSGI-INF/l10n/bundle.properties
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsActivationJob.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/GitWizardRepProgressMonitor.java
- bundles/com.espressif.idf.ui/plugin.xml
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/messages_zh.properties
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/UpdateEspIdfHandler.java
🚧 Files skipped from review as they are similar to previous changes (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java
🔇 Additional comments (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (2)
50-54: Prevent potential ArrayIndexOutOfBoundsException.When splitting the line to extract
espIdfVersion, ensure that the line contains an=character.
89-93: Prevent potentialNullPointerExceptioninisIdfInstalledActivemethod.If
idfInstalled.getId()could benull, calling.equals()on it may lead to aNullPointerException.
| IDFDownloadHandler_ESPIDFConfiguration=ESP-IDF Configuration | ||
| IDFDownloadHandler_DownloadPage_Title=Download or use ESP-IDF | ||
| IDFDownloadHandler_DownloadPageMsg=Please choose ESP-IDF version to download, or use an existing ESP-IDF copy | ||
| IDFToolsHandler_ToolsManagerConsole=Espressif IDF Tools Console |
There was a problem hiding this comment.
💡 Codebase verification
Missing Chinese translation for "IDFToolsHandler_ToolsManagerConsole"
The string "IDFToolsHandler_ToolsManagerConsole" is missing in the Chinese localization file messages_zh.properties. This needs to be added to maintain consistent user experience for Chinese users.
- Add the translation for "IDFToolsHandler_ToolsManagerConsole" in
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages_zh.properties
🔗 Analysis chain
Verify translations for new UI strings.
As this is a breaking change that adds new UI strings, ensure these changes are propagated to all localized message files (if any exist) to maintain consistent user experience across different languages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other localized versions of messages.properties
echo "Searching for localized message files..."
fd "messages_.*\.properties" "bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/"
Length of output: 265
Script:
#!/bin/bash
# Check if the new string exists in the Chinese translation file
echo "Content of Chinese translation file:"
cat "bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages_zh.properties"
Length of output: 6146
| } | ||
|
|
||
| tararchiveinputstream.close(); | ||
| process.waitFor(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout to process execution.
Add a timeout to process.waitFor() to prevent hanging in case the script execution takes too long.
-process.waitFor();
+if (!process.waitFor(30, TimeUnit.SECONDS)) {
+ process.destroyForcibly();
+ Logger.log("Process timed out while executing: " + activationScript);
+ return null;
+}Committable suggestion skipped: line range outside the PR's diff.
| String activationScript = idfInstalled.getActivationScript(); | ||
| String espIdfVersion = null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate input parameters.
Add validation for the activationScript to prevent potential NPEs or invalid paths.
Add parameter validation:
String activationScript = idfInstalled.getActivationScript();
+if (activationScript == null || activationScript.trim().isEmpty()) {
+ Logger.log("Activation script path is null or empty");
+ return null;
+}
+File scriptFile = new File(activationScript);
+if (!scriptFile.exists() || !scriptFile.canExecute()) {
+ Logger.log("Activation script does not exist or is not executable: " + activationScript);
+ return null;
+}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java (3)
37-38:⚠️ Potential issueFix typo in class name 'EimIdfConfiguratinParser'.
The class name contains a typo: 'Configuratin' should be 'Configuration'.
141-155:⚠️ Potential issueFix home directory path resolution.
The POSIX path using '~' won't be expanded by Java's File class.
157-170:⚠️ Potential issueImprove shell management and URL validation.
The shell creation and URL handling need improvements:
- Shell is created but never disposed
- Direct URL launch without validation
🧹 Nitpick comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java (1)
116-116: Remove TODO comment and implement proper documentation.The TODO comment should be replaced with proper documentation explaining the workflow.
Apply this diff to improve documentation:
- // TODO: Installer config flag is not set so we need to start IDF Manager and let user select the IDF for workspace + // When no ESP-IDF is configured for the workspace, prompt the user to select one using the IDF Manager. + // This ensures proper tool configuration before development can begin.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties
🔇 Additional comments (5)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java (2)
37-38: LGTM! Message constants follow Eclipse NLS patterns.The new message constants for EIM installation notifications are properly defined following Eclipse's internationalization patterns.
42-43: LGTM! Message constants follow Eclipse NLS patterns.The new message constants for ESP-IDF workspace notifications are properly defined following Eclipse's internationalization patterns.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java (3)
232-235: LGTM! Preference access is properly implemented.The method correctly checks the installation flag from Eclipse preferences.
93-113:⚠️ Potential issueImprove error handling and null checks.
The EIM JSON initialization block has several issues:
- No error message is shown to the user when IOException occurs
eimJsonmight be null when used laterApply this diff to improve error handling:
try { eimJson = eimIdfConfiguratinParser.getEimJson(true); } catch (IOException e) { Logger.log(e); + Display.getDefault().asyncExec(() -> { + Shell shell = Display.getDefault().getActiveShell(); + MessageBox errorBox = new MessageBox(shell, SWT.ICON_ERROR | SWT.OK); + errorBox.setText("Error"); + errorBox.setMessage("Failed to read EIM configuration: " + e.getMessage()); + errorBox.open(); + }); }Likely invalid or redundant comment.
117-138: 🛠️ Refactor suggestionImprove UI responsiveness and error handling.
The synchronous UI update can cause freezes, and error cases aren't properly handled.
Apply this diff to improve the implementation:
- Display.getDefault().syncExec(()-> { + Display.getDefault().asyncExec(()-> { Shell shell = Display.getDefault().getActiveShell(); + if (shell == null) { + shell = new Shell(Display.getDefault()); + } MessageBox messageBox = new MessageBox(shell, SWT.ICON_WARNING| SWT.YES | SWT.NO); messageBox.setText(Messages.NoActiveEspIdfInWorkspaceMsgTitle); messageBox.setMessage(Messages.NoActiveEspIdfInWorkspaceMsg); int response = messageBox.open(); if (response == SWT.YES) { IWorkbenchWindow activeww = EclipseHandler.getActiveWorkbenchWindow(); + if (activeww == null) { + Logger.log("No active workbench window found"); + return; + } IDFUtil.closeWelcomePage(activeww); try { + if (eimJson == null) { + MessageBox errorBox = new MessageBox(shell, SWT.ICON_ERROR | SWT.OK); + errorBox.setText("Error"); + errorBox.setMessage("EIM configuration is not available"); + errorBox.open(); + return; + } IDE.openEditor(activeww.getActivePage(), new EimEditorInput(eimJson), ESPIDFManagerEditor.EDITOR_ID); } catch (PartInitException e) { Logger.log(e); + MessageBox errorBox = new MessageBox(shell, SWT.ICON_ERROR | SWT.OK); + errorBox.setText("Error"); + errorBox.setMessage("Failed to open ESP-IDF Manager: " + e.getMessage()); + errorBox.open(); } } + if (!shell.isDisposed()) { + shell.dispose(); + } });Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java (1)
Update Chinese translation file to match new UI messages
The verification shows that the breaking changes in UI messages are well-contained, with only one required action:
- The Chinese translation file (
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages_zh.properties) needs to be updated to remove old messages and add translations for the new UI messages.🔗 Analysis chain
Line range hint
1-37: Document the breaking changes in UI messages.This file shows significant changes to UI messages as part of the EIM integration. Since this is marked as a breaking change in the PR objectives, please ensure:
- The documentation is updated to reflect the new UI messages and removed ones
- Any dependent modules using the removed message constants are updated
- Translation files for all supported languages are synchronized
Let's verify the impact:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential usage of removed message constants # Search for usage of removed constants in the codebase echo "Searching for usage of removed constants..." rg "EspIdfManagerStateCol|EspIdfManagerDeleteBtnToolTip|EspIdfManagerAddToolsBtn|EspIdfManagerRemoveAllBtn|EspIdfManagerDeleteBtn|IDFDownloadHandler_ESPIDFConfiguration|IDFDownloadHandler_DownloadPage_Title|IDFDownloadHandler_DownloadPageMsg"Length of output: 901
Script:
#!/bin/bash # Check for documentation and related files echo "=== Checking documentation files ===" fd -e md -e txt -e html . bundles/com.espressif.idf.ui/ echo -e "\n=== Checking all message properties files ===" fd messages.properties bundles/com.espressif.idf.ui/ echo -e "\n=== Checking Java files that might use these messages ===" fd -e java . bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ echo -e "\n=== Checking for old vs new message keys in properties files ===" for f in $(fd messages.properties bundles/com.espressif.idf.ui/); do echo "=== File: $f ===" cat "$f" doneLength of output: 30578
♻️ Duplicate comments (4)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (2)
58-58: 🛠️ Refactor suggestionAdd timeout to process execution.
The current implementation waits indefinitely for the process to complete.
89-93:⚠️ Potential issuePrevent potential NPE and add input validation.
The current implementation might throw NPE when comparing IDs.
Apply this diff:
public static boolean isIdfInstalledActive(IdfInstalled idfInstalled) { + if (idfInstalled == null) { + return false; + } IDFEnvironmentVariables idfEnvironmentVariables = new IDFEnvironmentVariables(); String espIdfIdEim = idfEnvironmentVariables.getEnvValue(IDFEnvironmentVariables.ESP_IDF_EIM_ID); - return idfInstalled.getId().equals(espIdfIdEim); + return Objects.equals(idfInstalled.getId(), espIdfIdEim); }bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.java (1)
11-11:⚠️ Potential issueCorrect the typo in
EimIdfConfiguratinParser.The class name contains a typographical error:
EimIdfConfiguratinParsershould beEimIdfConfigurationParser.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (1)
253-253:⚠️ Potential issueFix syntax error in log statement.
Remove the extraneous closing parenthesis.
- log("Executing " + arguments.toString(), IStatus.OK); //$NON-NLS-1$) + log("Executing " + arguments.toString(), IStatus.OK); //$NON-NLS-1$
🧹 Nitpick comments (5)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (1)
68-86: Add parameter validation and improve command construction.The method should validate the input parameter and handle path quoting for spaces.
Apply this diff:
public static List<String> getExportScriptCommand(String activationScriptPath) { + if (activationScriptPath == null || activationScriptPath.trim().isEmpty()) { + throw new IllegalArgumentException("Activation script path cannot be null or empty"); + } + List<String> command = new ArrayList<>(); if (Platform.getOS().equals(Platform.OS_WIN32)) { command.add("powershell.exe"); //$NON-NLS-1$ command.add("-ExecutionPolicy"); //$NON-NLS-1$ command.add("Bypass"); //$NON-NLS-1$ command.add("-File"); //$NON-NLS-1$ - command.add(activationScriptPath); + command.add("\"" + activationScriptPath.replace("\"", "\"\"") + "\""); command.add("-e"); //$NON-NLS-1$ } else { command.add("sh"); //$NON-NLS-1$ - command.add(activationScriptPath); + command.add("\"" + activationScriptPath.replace("\"", "\\\"") + "\""); command.add("-e"); //$NON-NLS-1$ } return command; }bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (1)
377-381: Refactor duplicated job creation logic.The job creation logic is duplicated between the activate and reload buttons. Consider extracting it to a helper method.
+private void scheduleSetupJob(IdfInstalled idfInstalled) { + SetupToolsInIde setupToolsInIde = new SetupToolsInIde( + idfInstalled, + eimJson, + getConsoleStream(true), + getConsoleStream(false) + ); + SetupToolsJobListener toolsActivationJobListener = new SetupToolsJobListener( + this, + setupToolsInIde + ); + setupToolsInIde.addJobChangeListener(toolsActivationJobListener); + setupToolsInIde.schedule(); +} // Then use it in both places: -SetupToolsInIde setupToolsInIde = new SetupToolsInIde(idfInstalled, eimJson, getConsoleStream(true), getConsoleStream(false)); -SetupToolsJobListener toolsActivationJobListener = new SetupToolsJobListener( - ESPIDFMainTablePage.this, setupToolsInIde); -setupToolsInIde.addJobChangeListener(toolsActivationJobListener); -setupToolsInIde.schedule(); +scheduleSetupJob(idfInstalled);Also applies to: 419-423
.github/workflows/ci.yml (1)
13-28: Optimize workflow configuration.Consider the following improvements to enhance workflow efficiency and maintainability:
- Use job dependencies to ensure proper execution order
- Implement caching for Python and Maven dependencies
- Share common configuration using composite actions
Example implementation:
jobs: build_linux: runs-on: [self-hosted, eclipse, BrnoUBU0004] + needs: [] # Add dependencies if needed steps: - uses: actions/checkout@v2 - name: Set up Python uses: actions/setup-python@v2 with: python-version: '3.10' + # Add caching + - uses: actions/cache@v3 + with: + path: ~/.cache/pip + key: ${{ runner.os }}-pip-${{ hashFiles('**/requirements.txt') }} + restore-keys: | + ${{ runner.os }}-pip- - name: Install ESP-IDF via eim - uses: Hahihula/esp-idf-install-action@v1 + # Use composite action for shared configuration + uses: ./.github/actions/install-esp-idf with: esp_idf_version: 'v5.3'bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (1)
459-477: Consider using a builder pattern for environment setup.The
setupEnvVarsInEclipse()method handles multiple environment variable operations. Consider refactoring to use a builder pattern for better maintainability and testability.Example refactor:
public class EnvironmentBuilder { private final Map<String, String> envVars = new HashMap<>(); public EnvironmentBuilder withPath(String path) { envVars.put(IDFEnvironmentVariables.PATH, path); return this; } public EnvironmentBuilder withComponentManager() { envVars.put(IDFEnvironmentVariables.IDF_COMPONENT_MANAGER, "1"); return this; } public void apply(IDFEnvironmentVariables env) { envVars.forEach(env::addEnvVariable); } }bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java (1)
116-116: Remove TODO comment and implement the feature.The TODO comment indicates missing functionality for IDF Manager initialization.
Would you like me to help implement this feature or create an issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bundles/com.espressif.idf.ui/icons/tools/delete.pngis excluded by!**/*.png
📒 Files selected for processing (51)
.github/workflows/ci.yml(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFEnvironmentVariables.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsInstallationWizardConstants.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsJsonKeys.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/JsonKey.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/Messages.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolSetConfigurationManager.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsJsonParser.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsPlatformMapping.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsSystemWrapper.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/messages.properties(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java(2 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IDFToolSet.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IdfInstalled.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/ToolsVO.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionDetailsVO.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionsVO.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java(1 hunks)bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/core/IDFMonitor.java(0 hunks)bundles/com.espressif.idf.ui/OSGI-INF/l10n/bundle.properties(0 hunks)bundles/com.espressif.idf.ui/plugin.xml(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java(4 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/NewProjectHandlerUtil.java(2 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/UpdateEspIdfHandler.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/GitRepositoryBuilder.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFDownloadPage.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFNewToolsWizard.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/Messages.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/messages.properties(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/messages_zh.properties(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/GitWizardRepProgressMonitor.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/LogMessagesThread.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.java(2 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/SetupToolsJobListener.java(4 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsActivationJob.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationJob.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/ESPIDFManagerEditor.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/EimEditorInput.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java(13 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/InstallToolsHandler.java(0 hunks)tests/com.espressif.idf.ui.test/configs/default-test-linux.properties(1 hunks)
💤 Files with no reviewable changes (28)
- bundles/com.espressif.idf.ui/OSGI-INF/l10n/bundle.properties
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/messages.properties
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionsVO.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsInstallationWizardConstants.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/UpdateEspIdfHandler.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationJob.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsJsonKeys.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/LogMessagesThread.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsSystemWrapper.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/Messages.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/JsonKey.java
- bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/core/IDFMonitor.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolSetConfigurationManager.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IDFToolSet.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/GitRepositoryBuilder.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/InstallToolsHandler.java
- bundles/com.espressif.idf.ui/plugin.xml
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionDetailsVO.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/ToolsVO.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/GitWizardRepProgressMonitor.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFNewToolsWizard.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFDownloadPage.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsActivationJob.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsPlatformMapping.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsJsonParser.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/messages_zh.properties
🚧 Files skipped from review as they are similar to previous changes (14)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFEnvironmentVariables.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/NewProjectHandlerUtil.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/ESPIDFManagerEditor.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/messages.properties
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/Messages.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/EimEditorInput.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IdfInstalled.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_macos
🔇 Additional comments (20)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (3)
7-17: LGTM! Clean imports and well-documented class structure.The imports are appropriate for the new functionality, and the class documentation is complete with copyright and author information.
50-54:⚠️ Potential issueAdd null check and proper error handling for version parsing.
The current implementation might throw ArrayIndexOutOfBoundsException if the line doesn't contain '='.
Apply this diff:
if (line.startsWith("ESP_IDF_VERSION=")) //$NON-NLS-1$ { - espIdfVersion = line.split("=")[1]; //$NON-NLS-1$ - break; + String[] parts = line.split("=", 2); //$NON-NLS-1$ + if (parts.length == 2) { + espIdfVersion = parts[1].trim(); + break; + } }Likely invalid or redundant comment.
27-30: 🛠️ Refactor suggestionRemove unused parameter and add input validation.
The
gitPathparameter is not used in the method. Additionally, the activation script path should be validated.Apply this diff:
-public static String getIdfVersion(IdfInstalled idfInstalled, String gitPath) +public static String getIdfVersion(IdfInstalled idfInstalled) { String activationScript = idfInstalled.getActivationScript(); + if (activationScript == null || activationScript.trim().isEmpty()) { + Logger.log("Activation script path is null or empty"); + return null; + } + File scriptFile = new File(activationScript); + if (!scriptFile.exists() || !scriptFile.canExecute()) { + Logger.log("Activation script does not exist or is not executable: " + activationScript); + return null; + }Likely invalid or redundant comment.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.java (1)
39-41: Verify the impact of this breaking change.This change represents a significant architectural shift from file-based to JSON-based configuration. Ensure that:
- All dependent components are updated to handle the new configuration format
- Migration path exists for existing configurations
- Documentation is updated to reflect these changes
Run the following script to identify potential impacts:
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (4)
66-66:⚠️ Potential issueConsider making
eimJsonan instance variable to prevent thread safety issues.The static
eimJsonfield could lead to race conditions if multiple instances of the UI are opened simultaneously. Consider making it an instance variable instead.-private static EimJson eimJson; +private EimJson eimJson;Likely invalid or redundant comment.
351-355:⚠️ Potential issueAdd null checks in getIdfVersion method.
The method could throw NPE if either idfInstalled or eimJson is null.
private String getIdfVersion(IdfInstalled idfInstalled) { + if (idfInstalled == null || eimJson == null) { + return "Unknown"; + } + try { String version = ToolsUtility.getIdfVersion(idfInstalled, eimJson.getGitPath()); - return version; + return version != null ? version : "Unknown"; + } catch (Exception e) { + Logger.log(e); + return "Error"; + } }Likely invalid or redundant comment.
79-87: 🛠️ Refactor suggestionRefactor singleton pattern to avoid static field modification.
The current implementation modifies a static field in getInstance, which could lead to race conditions. Consider passing the EimJson instance to the constructor instead.
-public static ESPIDFMainTablePage getInstance(EimJson eimJson) +public static ESPIDFMainTablePage getInstance() { if (espidfMainTablePage == null) { - espidfMainTablePage = new ESPIDFMainTablePage(); + espidfMainTablePage = new ESPIDFMainTablePage(eimJson); } - ESPIDFMainTablePage.eimJson = eimJson; return espidfMainTablePage; }Likely invalid or redundant comment.
132-141:⚠️ Potential issueImprove error handling in refreshEditorUI.
The current implementation has two issues:
- The catch block only logs the error without proper user feedback
- Potential NPE in line 141 if eimJson is null after exception
try { eimJson = eimIdfConfiguratinParser.getEimJson(true); } catch (IOException e) { Logger.log(e); + Display.getDefault().asyncExec(() -> { + MessageDialog.openError( + Display.getDefault().getActiveShell(), + "Configuration Error", + "Failed to load IDF configuration: " + e.getMessage()); + }); + return; } +if (eimJson == null) { + return; +} idfInstalledList = eimJson.getIdfInstalled();Likely invalid or redundant comment.
.github/workflows/ci.yml (2)
23-23: LGTM: Python version update.The Python version is appropriately set to 3.10, which is compatible with ESP-IDF v5.3.
13-13: Verify self-hosted runner configuration.The job has been renamed to
build_linuxand now runs on a specific self-hosted runnerBrnoUBU0004. Please ensure:
- The runner is properly maintained and available
- The required labels
self-hosted,eclipse, andBrnoUBU0004are configured- The runner has sufficient resources for the build process
Also applies to: 15-15
✅ Verification successful
Self-hosted runner configuration follows established patterns
The runner configuration is consistent with the repository's CI infrastructure:
- Linux builds use dedicated self-hosted runners (BrnoUBU0004)
- Similar pattern exists for Windows builds (BrnoWIN0007)
- Runner labels follow the standard format used across workflows
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify runner configuration and availability # Check if the runner is online and properly configured gh api -X GET repos/:owner/:repo/actions/runners \ --jq '.runners[] | select(.name=="BrnoUBU0004") | {name, status, busy, labels: [.labels[].name]}'Length of output: 390
Script:
#!/bin/bash # Search for the runner name across all workflow files echo "=== Searching for runner usage ===" rg "BrnoUBU0004" .github/workflows/ -l echo -e "\n=== Checking workflow file history ===" git log -p --since="3 months ago" .github/workflows/ci.yml | grep -A 5 -B 5 "build_linux\|BrnoUBU0004" echo -e "\n=== Looking for other workflow files ===" fd . .github/workflows/ -e yml -e yamlLength of output: 760
Script:
#!/bin/bash # Check runner configurations in CI workflow files echo "=== CI workflow runner configurations ===" for file in ci.yml ci_beta.yml ci_release.yml ci_windows.yml; do echo -e "\n=== $file ===" rg "runs-on:" -A 2 .github/workflows/$file done echo -e "\n=== Checking recent changes to CI workflows ===" for file in ci.yml ci_beta.yml ci_release.yml ci_windows.yml; do echo -e "\n=== $file changes ===" git log -n 1 --pretty=format:"%h - %an, %ar : %s" .github/workflows/$file doneLength of output: 1642
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/SetupToolsJobListener.java (2)
80-83: LGTM! Good defensive programming.The null check for
espidfMainTablePagebefore refreshing the UI is a good practice.
55-59:⚠️ Potential issueAdd null check before calling rollback.
The
rollback()call onsetupToolsInIdeneeds a null check to preventNullPointerException.if (event.getResult().getSeverity() != IStatus.OK) { + if (setupToolsInIde != null) { // Rollback all the changes setupToolsInIde.rollback(); + } else { + Logger.log("setupToolsInIde is null; rollback cannot be performed."); + } }Likely invalid or redundant comment.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (2)
62-69: LGTM! Well-structured constructor.The constructor properly initializes all required fields and provides a descriptive job name.
169-227:⚠️ Potential issueImprove OpenOCD rules installation on Linux.
The
copyOpenOcdRules()method has several issues:
- Copying to
/etc/udev/rules.d/requires root privileges- UI operations are performed in a background job
- No validation of the rules file content before copying
Consider these improvements:
- Check for write permissions before attempting to copy
- Move UI operations outside the job
- Validate rules file content
- Provide alternative installation methods (e.g., user-level rules directory)
private void copyOpenOcdRules() { if (Platform.getOS().equals(Platform.OS_LINUX) && !IDFUtil.getOpenOCDLocation().equalsIgnoreCase(StringUtil.EMPTY)) { log(Messages.InstallToolsHandler_CopyingOpenOCDRules, IStatus.OK); StringBuilder pathToRules = new StringBuilder(); pathToRules.append(IDFUtil.getOpenOCDLocation()); pathToRules.append("/../share/openocd/contrib/60-openocd.rules"); File rulesFile = new File(pathToRules.toString()); if (rulesFile.exists()) { java.nio.file.Path source = Paths.get(pathToRules.toString()); java.nio.file.Path target = Paths.get("/etc/udev/rules.d/60-openocd.rules"); + // Check write permissions + if (!Files.isWritable(target.getParent())) { + log("Insufficient permissions to write to " + target.toString(), IStatus.ERROR); + log("Please run the IDE with sudo or copy rules manually", IStatus.ERROR); + return; + } + // Validate rules file + try { + String content = new String(Files.readAllBytes(source)); + if (!content.contains("SUBSYSTEM==\"usb\"")) { + log("Invalid OpenOCD rules file", IStatus.ERROR); + return; + } + } catch (IOException e) { + log("Failed to validate rules file", e); + return; + }Likely invalid or redundant comment.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java (1)
23-23: Verify the corresponding entries in messages.properties.The new message keys need corresponding entries in the properties file.
Run this script to verify the entries exist:
Also applies to: 26-26
✅ Verification successful
Message keys are properly defined in messages.properties
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the new message keys have entries in messages.properties # Search for the new keys in messages.properties echo "Checking messages.properties for new keys..." rg -A 1 "EspIdfManagerNameCol|IDFToolsHandler_ToolsManagerConsole" bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.propertiesLength of output: 384
tests/com.espressif.idf.ui.test/configs/default-test-linux.properties (1)
10-10: Verify ESP-IDF version compatibility.The path has been updated to use ESP-IDF v5.3. Ensure that this version is compatible with all test scenarios and CI/CD pipelines.
✅ Verification successful
ESP-IDF v5.3 is compatible with the test environment
The codebase explicitly supports ESP-IDF v5.0 and higher. No version-specific test cases or incompatible dependencies were found that would prevent using v5.3.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check ESP-IDF version compatibility in test files and configurations. # Search for version-specific ESP-IDF references echo "Searching for ESP-IDF version references in test files..." rg -g '*.java' -g '*.properties' 'esp-idf-v[0-9]' || echo "No explicit version references found" # Check test configurations echo -e "\nChecking test configurations..." fd -e properties -e xml 'test' -x cat {} \; | grep -i 'esp.*idf.*path'Length of output: 1145
Script:
#!/bin/bash # Search for version-specific test cases and build configurations # Check for version-specific test cases echo "Checking for version-specific test cases..." rg -g '*.java' -g '*.xml' -g '*.properties' -A 3 -B 3 '(esp-idf|ESP-IDF).*(version|v[0-9]|V[0-9])' # Check for version requirements in build files echo -e "\nChecking build configurations..." fd -e xml pom.xml -x cat {} \; | grep -A 5 -B 5 'esp.*idf' # Check for version-related configuration files echo -e "\nChecking for version configuration files..." fd -g '*version*.{xml,properties,json}'Length of output: 24276
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java (4)
36-38: Fix typo in class and variable names.The class name
EimIdfConfiguratinParsercontains a typo ('Configuratin' should be 'Configuration'). This affects both the import statement and the field declaration.Also applies to: 61-62
143-154: Fix path resolution for POSIX systems.The POSIX path containing '~' won't be automatically expanded by Java's File class.
157-170: Improve shell management and URL handling.The current implementation has several issues:
- Shell is created but never disposed
- Direct URL launch could be a security concern
105-112: 🛠️ Refactor suggestionEnhance error handling for EIM JSON parsing.
The catch block only logs the error without any user notification or recovery mechanism. Consider informing the user about the parsing failure.
catch (IOException e) { Logger.log(e); + Display.getDefault().asyncExec(() -> { + Shell shell = Display.getDefault().getActiveShell(); + if (shell == null) { + shell = new Shell(Display.getDefault()); + } + MessageBox errorBox = new MessageBox(shell, SWT.ICON_ERROR | SWT.OK); + errorBox.setText(Messages.ErrorDialogTitle); + errorBox.setMessage(Messages.EimJsonParsingError); + errorBox.open(); + }); }Likely invalid or redundant comment.
| EimIdfConfiguratinParser eimIdfConfiguratinParser = new EimIdfConfiguratinParser(); | ||
|
|
||
| IDE.openEditor(activeww.getActivePage(), new EimEditorInput(eimIdfConfiguratinParser.getEimJson(true)), ESPIDFManagerEditor.EDITOR_ID); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and user feedback for configuration parsing.
The current implementation lacks proper error handling and user feedback for configuration parsing failures. Consider the following improvements:
- Add specific error handling for
getEimJson(true) - Validate the parsed configuration
- Provide user feedback on parsing failures
Apply this diff to improve error handling:
EimIdfConfiguratinParser eimIdfConfiguratinParser = new EimIdfConfiguratinParser();
-
-IDE.openEditor(activeww.getActivePage(), new EimEditorInput(eimIdfConfiguratinParser.getEimJson(true)), ESPIDFManagerEditor.EDITOR_ID);
+var eimJson = eimIdfConfiguratinParser.getEimJson(true);
+if (eimJson == null) {
+ Logger.log("Failed to parse EIM configuration");
+ MessageDialog.openError(
+ activeww.getShell(),
+ "Configuration Error",
+ "Failed to parse ESP-IDF configuration. Please check the configuration file."
+ );
+ return;
+}
+
+IDE.openEditor(
+ activeww.getActivePage(),
+ new EimEditorInput(eimJson),
+ ESPIDFManagerEditor.EDITOR_ID
+);📝 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.
| EimIdfConfiguratinParser eimIdfConfiguratinParser = new EimIdfConfiguratinParser(); | |
| IDE.openEditor(activeww.getActivePage(), new EimEditorInput(eimIdfConfiguratinParser.getEimJson(true)), ESPIDFManagerEditor.EDITOR_ID); | |
| EimIdfConfiguratinParser eimIdfConfiguratinParser = new EimIdfConfiguratinParser(); | |
| var eimJson = eimIdfConfiguratinParser.getEimJson(true); | |
| if (eimJson == null) { | |
| Logger.log("Failed to parse EIM configuration"); | |
| MessageDialog.openError( | |
| activeww.getShell(), | |
| "Configuration Error", | |
| "Failed to parse ESP-IDF configuration. Please check the configuration file." | |
| ); | |
| return; | |
| } | |
| IDE.openEditor( | |
| activeww.getActivePage(), | |
| new EimEditorInput(eimJson), | |
| ESPIDFManagerEditor.EDITOR_ID | |
| ); |
| IdfInstalled p1 = (IdfInstalled) e1; | ||
| IdfInstalled p2 = (IdfInstalled) e2; | ||
| int rc = 0; | ||
| switch (propertyIndex) | ||
| { | ||
| case 0: | ||
| rc = p1.getIdfVersion().compareTo(p2.getIdfVersion()); | ||
| rc = p1.getName().compareTo(p2.getName()); | ||
| break; | ||
| case 1: | ||
| rc = p1.getIdfLocation().compareTo(p2.getIdfLocation()); | ||
| rc = p1.getPath().compareTo(p2.getPath()); | ||
| break; | ||
| case 2: | ||
| Boolean p1State = p1.isActive(); | ||
| Boolean p2State = p2.isActive(); | ||
| Boolean p1State = ToolsUtility.isIdfInstalledActive(p1); | ||
| Boolean p2State = ToolsUtility.isIdfInstalledActive(p2); | ||
| rc = p1State.compareTo(p2State); |
There was a problem hiding this comment.
Fix column sorting logic and add null checks.
The current implementation has several issues:
- Column indices don't match the UI order (activate, version, name, location)
- No null checks in comparisons
- Version comparison is missing
-IdfInstalled p1 = (IdfInstalled) e1;
-IdfInstalled p2 = (IdfInstalled) e2;
+if (!(e1 instanceof IdfInstalled) || !(e2 instanceof IdfInstalled)) {
+ return 0;
+}
+IdfInstalled p1 = (IdfInstalled) e1;
+IdfInstalled p2 = (IdfInstalled) e2;
int rc = 0;
switch (propertyIndex) {
case 0:
- rc = p1.getName().compareTo(p2.getName());
+ rc = Boolean.compare(
+ ToolsUtility.isIdfInstalledActive(p1),
+ ToolsUtility.isIdfInstalledActive(p2));
break;
case 1:
- rc = p1.getPath().compareTo(p2.getPath());
+ String v1 = getIdfVersion(p1);
+ String v2 = getIdfVersion(p2);
+ rc = v1.compareTo(v2);
break;
case 2:
- Boolean p1State = ToolsUtility.isIdfInstalledActive(p1);
- Boolean p2State = ToolsUtility.isIdfInstalledActive(p2);
- rc = p1State.compareTo(p2State);
+ rc = compareNullSafe(p1.getName(), p2.getName());
+ break;
+case 3:
+ rc = compareNullSafe(p1.getPath(), p2.getPath());
break;
}Add this helper method:
private int compareNullSafe(String s1, String s2) {
if (s1 == null) return s2 == null ? 0 : -1;
return s2 == null ? 1 : s1.compareTo(s2);
}
.github/workflows/ci.yml
Outdated
| - name: Install ESP-IDF via eim | ||
| uses: Hahihula/esp-idf-install-action@v1 | ||
| with: | ||
| esp_idf_version: 'v5.3' |
There was a problem hiding this comment.
💡 Codebase verification
The action Hahihula/esp-idf-install-action@v1 is not available (404 Not Found). This poses a significant security risk as:
- The repository could have been deleted or made private
- A malicious actor could register this name and inject harmful code
- Using a major version tag (
v1) would automatically accept any new v1.x version
Recommendations:
- Use the official ESP-IDF installation methods from espressif/esp-idf
- Create a custom workflow step using official tools
- If using any third-party actions, always pin to specific commit hashes instead of version tags
🔗 Analysis chain
Review the custom ESP-IDF installation action.
The workflow uses a custom action Hahihula/esp-idf-install-action@v1 which might pose security risks:
- The action is from a personal repository rather than an official Espressif source
- It's pinned to a major version tag (
v1) which could allow breaking changes
Consider:
- Using the official Espressif action if available
- Pinning to a specific version hash instead of a major version tag
- Adding SHA256 verification for the ESP-IDF download
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the custom action's repository and version
# Check the action's repository
echo "Checking action repository..."
gh api repos/Hahihula/esp-idf-install-action \
--jq '{
created_at,
updated_at,
default_branch,
archived,
disabled,
security: .security_and_analysis
}'
# Check available versions/tags
echo "Checking available versions..."
gh api repos/Hahihula/esp-idf-install-action/tags \
--jq '.[].name'
Length of output: 674
Script:
#!/bin/bash
# Check for official Espressif actions
echo "Checking Espressif organization..."
gh api orgs/espressif/repos \
--jq '.[] | select(.topics | contains(["github-actions"])) | {name, html_url, description}'
echo "Searching for ESP-IDF related repositories..."
gh api search/repositories \
--raw-field 'q=org:espressif idf install action' \
--jq '.items[] | {name, html_url, description}'
Length of output: 624
Script:
#!/bin/bash
# Search GitHub Marketplace for ESP-IDF related actions
echo "Searching GitHub Marketplace for ESP-IDF actions..."
gh api search/code \
--raw-field 'q=path:.github/workflows+org:espressif+esp-idf' \
--jq '.items[] | {repository.full_name, path, html_url}'
# Direct check of potential official repositories
echo "Checking specific Espressif repositories..."
for repo in "esp-idf" "idf-installer" "esp-idf-actions"; do
echo "Checking $repo..."
gh api repos/espressif/$repo --jq '{name, description, html_url}' || true
done
Length of output: 1562
| public SetupToolsJobListener() | ||
| { | ||
| } |
There was a problem hiding this comment.
Remove unused default constructor.
The default constructor leaves setupToolsInIde uninitialized, which could lead to a NullPointerException when rollback() is called in the done method. Since the class requires both espidfMainTablePage and setupToolsInIde to function correctly, the default constructor should be removed.
-public SetupToolsJobListener()
-{
-}| private IStatus handleWebSocketClientInstall() | ||
| { | ||
| String websocketClient = "websocket-client"; //$NON-NLS-1$ | ||
| // pip install websocket-client | ||
| List<String> arguments = new ArrayList<String>(); | ||
| final String pythonEnvPath = IDFUtil.getIDFPythonEnvPath(envVarsFromActivationScriptMap.get(IDFEnvironmentVariables.IDF_PYTHON_ENV_PATH)); | ||
| if (pythonEnvPath == null || !new File(pythonEnvPath).exists()) | ||
| { | ||
| log(String.format("%s executable not found. Unable to run `%s -m pip install websocket-client`", //$NON-NLS-1$ | ||
| IDFConstants.PYTHON_CMD, IDFConstants.PYTHON_CMD), IStatus.ERROR); | ||
| return IDFCorePlugin.errorStatus( | ||
| String.format("%s executable not found. Unable to run `%s -m pip install websocket-client`", //$NON-NLS-1$ | ||
| IDFConstants.PYTHON_CMD, IDFConstants.PYTHON_CMD), | ||
| null); | ||
| } | ||
| arguments.add(pythonEnvPath); | ||
| arguments.add("-m"); //$NON-NLS-1$ | ||
| arguments.add("pip"); //$NON-NLS-1$ | ||
| arguments.add("list"); //$NON-NLS-1$ | ||
|
|
||
| ProcessBuilderFactory processRunner = new ProcessBuilderFactory(); | ||
|
|
||
| try | ||
| { | ||
| log("Executing " + arguments.toString(), IStatus.OK); //$NON-NLS-1$) | ||
|
|
||
| Map<String, String> environment = new HashMap<>(System.getenv()); | ||
| prepEnvMap(environment); | ||
| Logger.log(environment.toString()); | ||
|
|
||
| IStatus status = processRunner.runInBackground(arguments, org.eclipse.core.runtime.Path.ROOT, environment); | ||
| if (status == null) | ||
| { | ||
| Logger.log(IDFCorePlugin.getPlugin(), | ||
| IDFCorePlugin.errorStatus("Unable to get the process status.", null)); //$NON-NLS-1$ | ||
| log("Unable to get the process status", IStatus.ERROR); //$NON-NLS-1$ | ||
| return IDFCorePlugin.errorStatus("Unable to get the process status.", null); //$NON-NLS-1$ | ||
| } | ||
|
|
||
| String cmdOutput = status.getMessage(); | ||
| if (cmdOutput.contains(websocketClient)) | ||
| { | ||
| return IDFCorePlugin.okStatus("websocket-client already installed", null); //$NON-NLS-1$ | ||
| } | ||
|
|
||
| // websocket client not installed so installing it now. | ||
| arguments.remove(arguments.size() - 1); | ||
| arguments.add("install"); //$NON-NLS-1$ | ||
| arguments.add(websocketClient); | ||
|
|
||
| status = processRunner.runInBackground(arguments, org.eclipse.core.runtime.Path.ROOT, environment); | ||
| if (status == null) | ||
| { | ||
| Logger.log(IDFCorePlugin.getPlugin(), | ||
| IDFCorePlugin.errorStatus("Unable to get the process status.", null)); //$NON-NLS-1$ | ||
| log("Unable to get the process status", IStatus.ERROR); //$NON-NLS-1$ | ||
| return IDFCorePlugin.errorStatus("Unable to get the process status.", null); //$NON-NLS-1$ | ||
| } | ||
|
|
||
| log(status.getMessage(), IStatus.OK); | ||
|
|
||
| return status; | ||
| } | ||
| catch (Exception e1) | ||
| { | ||
| Logger.log(IDFCorePlugin.getPlugin(), e1); | ||
| if (errorConsoleStream != null) | ||
| { | ||
| errorConsoleStream.println(e1.getLocalizedMessage()); | ||
| } | ||
| return IDFCorePlugin.errorStatus(e1.getLocalizedMessage(), e1); // $NON-NLS-1$; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Python package installation error handling.
The handleWebSocketClientInstall() method has several issues:
- Hardcoded package name and version
- No timeout for pip operations
- No verification of successful installation
Consider adding:
- Package version specification
- Installation verification
- Timeout mechanism
- Requirements.txt approach for dependency management
private IStatus handleWebSocketClientInstall() {
- String websocketClient = "websocket-client";
+ String websocketClient = "websocket-client>=1.0.0"; // Specify minimum version
+ int timeout = 60; // seconds
// ... existing code ...
status = processRunner.runInBackground(arguments, org.eclipse.core.runtime.Path.ROOT, environment);
+ // Add timeout
+ long startTime = System.currentTimeMillis();
+ while (process.isAlive()) {
+ if ((System.currentTimeMillis() - startTime) > timeout * 1000) {
+ process.destroy();
+ return IDFCorePlugin.errorStatus("Package installation timed out", null);
+ }
+ Thread.sleep(100);
+ }
+ // Verify installation
+ if (status.isOK()) {
+ return verifyPackageInstallation(websocketClient, environment);
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci.yml
15-15: label "eclipse" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
15-15: label "BrnoUBU0004" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
21-21: the runner of "actions/setup-python@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_macos
.github/workflows/ci.yml
Outdated
| - name: Install ESP-IDF via eim | ||
| uses: espressif/esp-idf-install-action@v1 | ||
| with: | ||
| esp_idf_version: 'v5.3' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pin action to specific commit hash for security.
While using the official Espressif action is good, version tags can be moved. For better security:
- Pin to a specific commit hash:
- uses: espressif/esp-idf-install-action@v1
+ uses: espressif/esp-idf-install-action@a2c9c4adf5648d7c935d0c9d0d48f2c2c6e2e98b- Verify the action's integrity:
#!/bin/bash
# Description: Verify action's commit hash and repository
echo "Verifying action commit..."
gh api repos/espressif/esp-idf-install-action/commits/v1 --jq '{sha, commit.verification.verified}'[skip_cloning]
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Python | ||
| uses: actions/setup-python@v2 | ||
| with: | ||
| python-version: '3.10' | ||
| python-version: '3.10' | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update actions/setup-python to the latest version.
The current setup uses an outdated version of the action. Update to v4 for the latest security improvements and features:
- - uses: actions/checkout@v4
+ - uses: actions/checkout@v4
- name: Set up Python
- uses: actions/setup-python@v2
+ uses: actions/setup-python@v4
with:
python-version: '3.10'📝 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.
| - uses: actions/checkout@v4 | |
| - name: Set up Python | |
| uses: actions/setup-python@v2 | |
| with: | |
| python-version: '3.10' | |
| python-version: '3.10' | |
| - uses: actions/checkout@v4 | |
| - name: Set up Python | |
| uses: actions/setup-python@v4 | |
| with: | |
| python-version: '3.10' | |
🧰 Tools
🪛 actionlint (1.7.4)
21-21: the runner of "actions/setup-python@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.java
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci.yml
15-15: label "eclipse" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
15-15: label "BrnoUBU0004" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
21-21: the runner of "actions/setup-python@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.35.1)
.github/workflows/ci.yml
[error] 25-25: syntax error: found character '\t' that cannot start any token
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_macos
🔇 Additional comments (5)
.github/workflows/ci.yml (5)
13-13: LGTM! Clear and descriptive job name.The rename from
buildtobuild_linuxbetter reflects the job's purpose.
15-15:⚠️ Critical security risk: Unprotected self-hosted runner in public repository🧰 Tools
🪛 actionlint (1.7.4)
15-15: label "eclipse" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
15-15: label "BrnoUBU0004" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
21-23: Updateactions/setup-pythonto the latest version.🧰 Tools
🪛 actionlint (1.7.4)
21-21: the runner of "actions/setup-python@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
Line range hint
31-146: LGTM! Well-structured configuration.The remaining configuration follows best practices with proper version pinning and clear step organization.
🧰 Tools
🪛 actionlint (1.7.4)
15-15: label "eclipse" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
15-15: label "BrnoUBU0004" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
21-21: the runner of "actions/setup-python@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.35.1)
[error] 25-25: syntax error: found character '\t' that cannot start any token
(syntax)
26-28: 🛠️ Refactor suggestionPin ESP-IDF installation action to specific commit hash.
Using version tags (
v1) for third-party actions poses a security risk. Pin to a specific commit hash for better security.
| python-version: '3.10' | ||
| python-version: '3.10' | ||
|
|
||
| - name: Install ESP-IDF via eim |
There was a problem hiding this comment.
Fix YAML syntax error: Remove tab character.
The step name contains a tab character which can cause YAML parsing issues.
- - name: Install ESP-IDF via eim
+ - name: Install ESP-IDF via eim📝 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.
| - name: Install ESP-IDF via eim | |
| - name: Install ESP-IDF via eim |
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 25-25: syntax error: found character '\t' that cannot start any token
(syntax)
| - self-hosted | ||
| - eclipse | ||
| - BrnoUBU0004 | ||
| runs-on: [self-hosted, eclipse, BrnoUBU0004] |
There was a problem hiding this comment.
💡 Codebase verification
Custom runner labels need documentation
The custom labels eclipse and BrnoUBU0004 are indeed undocumented. They are only used in ci.yml for a self-hosted runner but their purpose and requirements are not explained anywhere in the repository. The original suggestions remain valid:
- Add these labels to
.github/actionlint.yamlto validate workflow syntax - Document their purpose in workflow comments to explain:
- What the
eclipselabel represents (e.g., required Eclipse development environment) - What
BrnoUBU0004represents (specific Ubuntu machine in Brno)
- What the
🔗 Analysis chain
Document custom runner labels.
The custom labels eclipse and BrnoUBU0004 should be documented:
- Add these labels to
.github/actionlint.yaml - Document their purpose in workflow comments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if labels are documented
echo "Checking for actionlint.yaml..."
fd -g "actionlint.yaml" .github/
echo "Checking for label documentation in workflow..."
grep -B 2 -A 2 "eclipse\|BrnoUBU0004" .github/workflows/ci.yml
Length of output: 352
Script:
#!/bin/bash
# Search for any documentation or usage of these labels
echo "Searching for label documentation in workflow files..."
rg -i "eclipse|BrnoUBU0004" .
echo "Checking for README or documentation files..."
fd -e md . | xargs rg -i "eclipse|BrnoUBU0004"
Length of output: 68250
Script:
#!/bin/bash
# Look for other workflow files and any documentation about runners
echo "Checking for other workflow files..."
fd -e yml -e yaml . .github/workflows/
echo "Checking for any documentation about runners..."
rg -i "runner|self-hosted|BrnoUBU" .github/
Length of output: 1314
🧰 Tools
🪛 actionlint (1.7.4)
15-15: label "eclipse" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
15-15: label "BrnoUBU0004" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
Initial integration for the eim starting from the start to handle the empty workspace. Tools activation is still to be done along with code cleanup. Just the basic display is working which will also be enhanced.
* cleanup of unused classes and code
|
|
|
|
|
Tested under:
install ESP-IDF using EIM Installer -> Open IDE -> select installed ESP-IDF in ESP-IDF Manager and install tools -> close IDE -> delete ESP-IDF -> open IDE -> ESP-IDF Manager still pointing to deleted ESP-IDF : RESTART - doesn't help Is there any chance to remove deleted ESP-IDF from eim_idf.json file or at least make it inactive ? |
|
Windows 11 Custom build folder build does not work, getting error:
console: and IDE has started using the external Python from the Python311 folder. |
|
If the internet connection is poor, there is no adequate timeout(10-15 min now) or retry mechanism in place to handle such situations effectively. and if the retry attempt fails, what steps should the user take to manually reinstall the submodules, considering the process begins during project creation? |
Next commits will do some refactoring and then implement the logic to handle these changes
So this is very basic first level of file watcher service that watches the file and then also takes care that the file is wasnt changes since last startup. Initial changes only take that the file last modified is verified and just any change in file simply will give you a message box telling that something changed. More advanced and robust checks in upcoming commits
Description
EIM Integration
Fixes # (IEP-1334)
Type of change
Please delete options that are not relevant.
How has this been tested?
All flows relevant to onboarding and ide tools installation must be selected.
Try setting up ide without EIM installed. User should get a message to download and install the EIM and then start the IDE.
Try starting IDE after EIM is installed. The user must be notified that no active ESP-IDF configuration is found and must come towards the ESP-IDF Manager which will now only have option to activate or refresh an installation from the given list that comes from the EIM
Try starting IDE in a workspace with the older version and this should notify the user to export the configuration that must be passed on by the user to EIM for proper handling and creating the eim stable configs.
Test Configuration:
Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation