Release/v4.0.0 (Review for rebase) #1308
Conversation
WalkthroughEIM (Espressif IDF Manager) integration added: new EIM constants, parser, loader, watcher, UI wiring and IDE startup hooks. Legacy tools-installation and toolset management classes removed. Environment handling shifted to use System.getenv() broadly. CI workflows, version metadata, NVS editor, terminal, tests, and extensive Chinese docs updated. Changes
Sequence Diagram(s)%%{init: {"themeVariables":{"actorBorder":"#2b6cb0","actorBg":"#e6f0fb","sequenceNumber":"#6b7280"}}}%%
sequenceDiagram
participant IDE as Espressif IDE
participant Startup as EspressifToolStartup
participant Watcher as EimJsonWatchService
participant Parser as EimIdfConfiguratinParser
participant EIM as Espressif IDF Manager (EIM)
participant Setup as SetupToolsInIde
IDE->>Startup: earlyStartup()
Startup->>Parser: loadEimJson()?
alt eim_idf.json exists
Parser-->>Startup: EimJson
Startup->>Watcher: add listener (EimJsonUiChangeHandler)
Startup->>IDE: optionally open EIM editor
else missing
Startup->>EIM: download & launch (EimLoader)
EIM-->>Parser: write eim_idf.json
Parser-->>Watcher: file change observed
end
Watcher->>EimJsonUiChangeHandler: onJsonFileChanged(path, paused)
EimJsonUiChangeHandler->>Parser: loadEimJson()
EimJsonUiChangeHandler->>Setup: setupToolsInIde(EimJson)
Setup->>IDE: update environment, toolchains, preferences
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 39
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
.github/workflows/ci_release.yml (3)
31-41: Keystore handling: wrong secret reference, macOS base64 flag, and secret exposure via -X.
- “{{secrets.JARSIGNER_KEYSTORE}}” won’t interpolate in a shell; also no such env is provided.
- macOS base64 needs “-D”.
- Avoid “-X” (debug) printing sensitive props.
Apply:
- - name: Decode keystore and build with Maven + - name: Decode keystore and build with Maven env: JARSIGNER_KEYSTORE_B64: ${{ secrets.JARSIGNER_REL_KEYSTORE_B64 }} JARSIGNER_STOREPASS: ${{ secrets.JARSIGNER_REL_STOREPASS }} JARSIGNER_ALIAS: ${{ secrets.JARSIGNER_REL_ALIAS }} run: | - KEYSTORE_FILE="${PWD}/{{secrets.JARSIGNER_KEYSTORE}}" - echo "${KEYSTORE_FILE}" - printf "%s" "${JARSIGNER_KEYSTORE_B64}" | base64 -d > "${KEYSTORE_FILE}" - mvn -e -X clean install -Djarsigner.keystore="${KEYSTORE_FILE}" -Djarsigner.alias="${JARSIGNER_ALIAS}" -Djarsigner.storepass="${JARSIGNER_STOREPASS}" -DskipTests=true + KEYSTORE_FILE="$RUNNER_TEMP/rel_keystore.jks" + printf "%s" "${JARSIGNER_KEYSTORE_B64}" | base64 -D > "${KEYSTORE_FILE}" + mvn clean install \ + -Djarsigner.keystore="${KEYSTORE_FILE}" \ + -Djarsigner.alias="${JARSIGNER_ALIAS}" \ + -Djarsigner.storepass="${JARSIGNER_STOREPASS}" \ + -DskipTests=true rm -v "${KEYSTORE_FILE}"
48-49: macOS base64 long option unsupported.Replace “--decode” with “-D”.
- echo $MACOS_CERTIFICATE | base64 --decode > certificate.p12 + echo "$MACOS_CERTIFICATE" | base64 -D > certificate.p12
285-314: Zipping with “*” drops dotfiles; unzip path assumption brittle.Zip the directory to include dotfiles and keep consistent extraction.
- zip -r "com.espressif.idf.update-v$VERSION.zip" artifacts/update/* + zip -rq "com.espressif.idf.update-v$VERSION.zip" artifacts/update @@ - unzip -q "com.espressif.idf.update-v$VERSION.zip" -d "$TMP_UPDATE_DIR" + unzip -q "com.espressif.idf.update-v$VERSION.zip" -d "$TMP_UPDATE_DIR" @@ - aws s3 cp --acl=public-read --recursive "$TMP_UPDATE_DIR/artifacts/update/" "s3://${{ secrets.DL_BUCKET }}/dl/idf-eclipse-plugin/updates/$UPLOAD_PATH" - aws s3 cp --acl=public-read --recursive "$TMP_UPDATE_DIR/artifacts/update/" "s3://${{ secrets.DL_BUCKET }}/dl/idf-eclipse-plugin/updates/v$VERSION" + aws s3 cp --acl=public-read --recursive "$TMP_UPDATE_DIR/artifacts/update/" "s3://${{ secrets.DL_BUCKET }}/dl/idf-eclipse-plugin/updates/$UPLOAD_PATH" + aws s3 cp --acl=public-read --recursive "$TMP_UPDATE_DIR/artifacts/update/" "s3://${{ secrets.DL_BUCKET }}/dl/idf-eclipse-plugin/updates/v$VERSION"Note: Paths remain valid while ensuring hidden metadata is preserved.
releng/com.espressif.idf.configuration/pom.xml (1)
111-120: Upgrade SpotBugs Maven Plugin for JDK 21 compatibility (releng/com.espressif.idf.configuration/pom.xml:111-120)4.7.0.0 does not support Java 21 class files (major version 65) and will throw "Unsupported class file major version 65" — bump to >=4.8.x (preferably the latest 4.9.4.1) and re-run CI.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/size/IDFSizeDataManager.java (1)
248-258: Handle “v”-prefixed versions to avoid Version.parse() failures.ESP-IDF commonly uses tags like “v5.1”. Version.parse() will throw for the leading “v”. Normalize before parsing.
Apply this diff:
- Version currentVersion = Version.parse(currentIDFVersion); + String normalized = currentIDFVersion != null ? currentIDFVersion.trim() : ""; + if (normalized.startsWith("v") || normalized.startsWith("V")) { + normalized = normalized.substring(1); + } + Version currentVersion = Version.parse(normalized);bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/size/IDFSizeAnalysisEditor.java (1)
100-107: Fix version check: 5.1 should be accepted (off‑by‑one).Current condition requires minor > 1, which excludes 5.1. Should be >= 1.
Apply this diff:
- else if (major == 5 && minor > 1) + else if (major == 5 && minor >= 1)Optionally, reuse a single helper (e.g., IDFSizeDataManager.isVersionAtLeast) to avoid duplicated version logic across UI and data paths.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/DfuCommandsUtil.java (1)
142-156: Avoid logging full environment and use File.pathSeparator when merging PATH/Path
- DfuCommandsUtil (bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/DfuCommandsUtil.java, ~lines 142–156): remove Logger.log(environment.toString()) and replace with a minimal, non-sensitive signal (e.g., log only PATH length or a short status).
- Replace Collectors.joining(";") with Collectors.joining(File.pathSeparator) for cross-platform correctness.
- IDFEnvironmentVariables.getSystemEnvMap() (bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFEnvironmentVariables.java) does NOT normalize/merge PATH/Path (it copies System.getenv() then overlays CDT variables), so keep the explicit merge in DfuCommandsUtil but implement it as above.
Apply this diff:
- Logger.log(environment.toString()); + // Avoid logging full env; keep minimal signal + String pathPreview = environment.getOrDefault("PATH", ""); + Logger.log("DFU env prepared; PATH length=" + pathPreview.length()); @@ - String mergedPath = Stream.of(environment.get("PATH"), environment.get("Path")).filter(Objects::nonNull) //$NON-NLS-1$ //$NON-NLS-2$ - .collect(Collectors.joining(";")); //$NON-NLS-1$ + String mergedPath = Stream.of(environment.get("PATH"), environment.get("Path")) // $NON-NLS-1$ //$NON-NLS-2$ + .filter(Objects::nonNull) + .collect(Collectors.joining(File.pathSeparator));bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java (1)
199-215: Project refresh job is never scheduled.refreshJob.join() is a no‑op without schedule(); the project may not refresh.
runEspIdfSbomCommand(); try { + refreshJob.schedule(); refreshJob.join(); }Also applies to: 227-235
bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/core/IDFMonitor.java (1)
184-184: Same: avoid logging entire environment.- Logger.log(environment.toString()); + // Avoid logging entire environment; redact if needed.bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (1)
441-449: Guard against null PYTHON pathIf
IDFUtil.getIDFPythonEnvPath()returns null, the argument becomes-DPYTHON=null, breaking configure.- command.add("-DPYTHON=" + IDFUtil.getIDFPythonEnvPath()); //$NON-NLS-1$ + String py = IDFUtil.getIDFPythonEnvPath(); + if (StringUtil.isEmpty(py)) { + throw new CmakeBuildException("Python executable not found. Ensure IDF_PYTHON_ENV_PATH or PATH is set."); //$NON-NLS-1$ + } + command.add("-DPYTHON=" + py); //$NON-NLS-1$
🧹 Nitpick comments (84)
.github/workflows/ci_windows.yml (1)
12-14: Expect duplicate builds on release branches (push + pull_request). Consider deduping.Merges into release/* will trigger both push and pull_request runs for the same commit. If that’s undesirable, pick one of these:
Option A — run release/* only on PRs:
on: push: branches: - - master - - release/** + - master pull_request: branches: - master - release/**Option B — keep both triggers but add a concurrency group to collapse reruns:
concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: trueOperational note (self‑hosted runners): if you accept PRs from forks, gate this job to trusted PRs only to protect self‑hosted Windows runners:
jobs: build_windows: if: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.fork == false }}bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/messages.properties (1)
4-7: Fix grammar/capitalization and avoid suggesting "run Eclipse with sudo"
File: bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/messages.properties (lines 4–7)
Use clearer wording and recommend polkit/pkexec or manual sudo; gate Linux-specific prompts by OS.-InstallToolsHandler_OpenOCDRulesCopyWarningMessage=The rules file is already present in the /etc/udev/rules.d/ directory, Do you want to replace the file with the file from OpenOCD directory -InstallToolsHandler_OpenOCDRulesNotCopied=Rules Not Copied to system -InstallToolsHandler_OpenOCDRulesCopyError=Unable to copy rules for OpenOCD to system directory, try running the eclipse with sudo command -InstallToolsHandler_OpenOCDRulesCopied=Rules Copied to system +InstallToolsHandler_OpenOCDRulesCopyWarningMessage=A rules file already exists in /etc/udev/rules.d. Replace it with the one from the OpenOCD directory? +InstallToolsHandler_OpenOCDRulesNotCopied=OpenOCD udev rules were not copied to the system. +InstallToolsHandler_OpenOCDRulesCopyError=Unable to copy OpenOCD udev rules to the system directory. Elevated privileges are required. Use a privileged prompt (polkit/pkexec) or copy the file manually with sudo. +InstallToolsHandler_OpenOCDRulesCopied=OpenOCD udev rules were copied to the system..github/workflows/ci.yml (1)
29-33: Tabs in YAML and pinning.
- YAML has tabs on this block; replace with spaces.
- Consider pinning the action to a commit SHA for supply-chain hardening.
- - name: Install ESP-IDF via eim - uses: espressif/install-esp-idf-action@v1 + - name: Install ESP-IDF via eim + uses: espressif/install-esp-idf-action@v1 with: version: 'v5.4'bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/AbstractToolsHandler.java (5)
144-151: Ensure child process env includes IDF_PATH after switching to System.getenv().Relying solely on System.getenv() may drop workspace-scoped vars. idf_tools.py and downstream scripts often expect IDF_PATH. Add it explicitly to the child env.
Please confirm EIM fully replaces prior IDE-injected vars (e.g., PIP_EXTRA_INDEX_URL/IDF_GITHUB_ASSETS) for these flows.
- Map<String, String> environment = new HashMap<>(System.getenv()); + Map<String, String> environment = new HashMap<>(System.getenv()); Logger.log(environment.toString()); environment.put("PYTHONUNBUFFERED", "1"); //$NON-NLS-1$ //$NON-NLS-2$ + if (!StringUtil.isEmpty(idfPath)) { + environment.put("IDF_PATH", idfPath); + }
264-275: Use platform path separator and normalize PATH key.Hard-coded “;” breaks on Unix/macOS. Also, choose a single PATH key to update.
- String path1 = environment.get("PATH"); //$NON-NLS-1$ - String path2 = environment.get("Path"); //$NON-NLS-1$ - if (!StringUtil.isEmpty(path1) && !path1.contains(gitDir)) // Git not found on the PATH environment - { - path1 = gitDir.concat(";").concat(path1); //$NON-NLS-1$ - environment.put("PATH", path1); //$NON-NLS-1$ - } - else if (!StringUtil.isEmpty(path2) && !path2.contains(gitDir)) // Git not found on the Path environment - { - path2 = gitDir.concat(";").concat(path2); //$NON-NLS-1$ - environment.put("Path", path2); //$NON-NLS-1$ - } + final String sep = java.io.File.pathSeparator; + final String pathKey = environment.containsKey("PATH") ? "PATH" + : (environment.containsKey("Path") ? "Path" : "PATH"); + final String existing = environment.get(pathKey); + if (StringUtil.isEmpty(existing)) { + environment.put(pathKey, gitDir); + } else if (!existing.contains(gitDir)) { + environment.put(pathKey, gitDir + sep + existing); + }
183-187: Route stderr to the error console stream.Keeps stdout/stderr separated and improves troubleshooting.
- errorThread = new InputStreamConsoleThread(errorStream, console); + errorThread = new InputStreamConsoleThread(errorStream, errorConsoleStream);
201-201: Return a more informative error status.Current message “Error” lacks context.
- return new Status(IStatus.ERROR, IDFCorePlugin.PLUGIN_ID, "Error"); //$NON-NLS-1$ + return new Status(IStatus.ERROR, IDFCorePlugin.PLUGIN_ID, + "Process exited with code " + exitValue); //$NON-NLS-1$
48-48: Fix typo: pythonExecutablenPath → pythonExecutablePath.Minor readability nit; avoids propagating the typo.
- protected String pythonExecutablenPath; + protected String pythonExecutablePath; ... - pythonExecutablenPath = getPythonExecutablePath(); + pythonExecutablePath = getPythonExecutablePath(); ... - pythonExecutablenPath, pythonVersions, idfPath, gitExecutablePath); + pythonExecutablePath, pythonVersions, idfPath, gitExecutablePath); ... - pythonExecutablenPath = dir.getPythonExecutable(); + pythonExecutablePath = dir.getPythonExecutable(); ... - if (StringUtil.isEmpty(pythonExecutablenPath) || StringUtil.isEmpty(gitExecutablePath) + if (StringUtil.isEmpty(pythonExecutablePath) || StringUtil.isEmpty(gitExecutablePath) || StringUtil.isEmpty(idfPath)) ... - pythonExecutablenPath = IDFUtil.getIDFPythonEnvPath(); - return pythonExecutablenPath; + pythonExecutablePath = IDFUtil.getIDFPythonEnvPath(); + return pythonExecutablePath; ... - arguments.add(0, pythonExecutablenPath); + arguments.add(0, pythonExecutablePath);Also applies to: 78-78, 81-83, 87-87, 94-96, 119-123, 137-139
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java (2)
35-38: Select the correct Console before waiting to avoid false positives.If multiple consoles exist, the text might be in a different console.
Consider clearing/selecting the active console prior to waiting:
SWTBotView consoleView = bot.viewById("org.eclipse.ui.console.ConsoleView"); consoleView.show(); consoleView.setFocus(); + // Optional: ensure we're reading the active console buffer + bot.toolbarButtonWithTooltip("Clear Console").click();
39-40: Use the editor handle instead of generic cTabItem lookups.Reduces flakiness and ties actions to the exact editor instance.
Apply this diff:
- bot.cTabItem("ESP-IDF Manager").activate(); - bot.cTabItem("ESP-IDF Manager").close(); + espIdfManagerView.activate(); + espIdfManagerView.close();releng/com.espressif.idf.product/idf.product (2)
94-99: Switch Eclipse releases repo to HTTPS.Even though disabled, prefer HTTPS for consistency and future enablement.
Apply:
- <repository location="http://download.eclipse.org/releases/latest/" name="" enabled="false" /> + <repository location="https://download.eclipse.org/releases/latest/" name="" enabled="false" />
45-49: Drop or update the Solaris VM entry.Legacy Solaris/JSE‑11 stanza is unused (
include="false") and outdated. Remove to avoid confusion.- <solaris include="false">org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-11</solaris>docs/_static/docs_version.js (1)
1-1: Use const for an immutable global.Minor JS hygiene.
-var DOCUMENTATION_VERSIONS = { +const DOCUMENTATION_VERSIONS = {docs/en/installation.rst (9)
4-8: “Two main steps” conflicts with a third bullet — fix phrasing.Make migration explicitly optional, or say “steps below.”
-Configuring and installing the Espressif-IDE involves two main steps: +Configuring and installing the Espressif-IDE involves the steps below (plus an optional migration if upgrading from an older workspace): @@ -3. :ref:`Migrating old workspace to new <esp-idf-tools-migration>` +3. (Optional) :ref:`Migrate an old workspace to the new format <esp-idf-tools-migration>`
19-23: Tighten wording around the EIM requirement.Clarify in one sentence and fix line breaks.
-Please note that the Espressif-IDE requires that -you install ESP-IDF via Espressif Installation Manager (EIM) to manage the ESP-IDF versions and tools. -For detailed documentation on using the Espressif Installation Manager (EIM), -please refer to the official guide `here <https://docs.espressif.com/projects/idf-im-ui/en/latest/>`_. +Espressif‑IDE requires installing ESP‑IDF via the Espressif Installation Manager (EIM) to manage ESP‑IDF versions and tools. For details, see the `EIM guide <https://docs.espressif.com/projects/idf-im-ui/en/latest/>`_.
39-44: Grammar tweak in EIM launch/migration sentence.-The Espressif-IDE allows you to download and launch the EIM directly from the IDE, making it easier to manage your ESP-IDF versions and tools. If you are coming trying to open the workspace from and older version of Espressif-IDE you will be prompted to convert the older configuration to use with EIM. +You can download and launch EIM directly from the IDE to manage ESP‑IDF versions and tools. If you open a workspace from an older Espressif‑IDE, you’ll be prompted to convert its configuration for use with EIM.
51-56: Clarify progress wording and update timing.- You will see the progress of download and installation in the console. Once the EIM is launched, you can use it to install the ESP-IDF and related tools. -3. Once you close the EIM your ESP-IDF Manager editor will be updated with the latest information about the ESP-IDF versions and tools available. + You will see download/installation progress in the Console view. After EIM launches, use it to install ESP‑IDF and related tools. +3. After you close EIM, the ESP‑IDF Manager editor updates with the latest ESP‑IDF versions and tools.
60-61: Remove stray colon; minor punctuation.-After the EIM is closed the ESP-IDF Manager editor will be updated with the latest information about the ESP-IDF versions and tools available.: +After you close EIM, the ESP‑IDF Manager editor updates with the latest ESP‑IDF versions and tools available.
66-69: Light grammar fixes.-To activate any specific version, simply click on the radio button next to it in the ``Active`` column. -The refresh button in the last column for the active ESP-IDF version can be used to reload any changes in the ESP-IDF directory you made. +To activate a version, click the radio button in the ``Active`` column. +Use the refresh button (last column of the active row) to reload any changes you made to the ESP‑IDF directory.
71-72: Clarify auto-detection vs manual refresh.- Any manual changes made to the ESP-IDF directory will not be reflected in the IDE until you refresh the active ESP-IDF version using the refresh button but if you made any changes to the installation of ESP-IDF using the EIM, those changes will be detected and a popup message will be shown to you to update the ESP-IDF version in the IDE. + Manual changes to the ESP‑IDF directory require clicking refresh to be reflected in the IDE. Changes applied via EIM are auto‑detected; you’ll see a prompt to update the active ESP‑IDF in the IDE.
85-89: Migration step 1: add “ensure directory exists” and minor grammar.-1. Copy the downloaded EIM executable to `.espressif/eim_gui/` in user home directory for Windows and Linux. - For example, on Windows, it would be `C:\Users\<username>\.espressif\eim_gui\eim.exe`. - On Linux , it would be `~/.espressif/eim_gui/eim`. +1. Copy the downloaded EIM executable to `.espressif/eim_gui/` in your home directory (create the folder if it does not exist). + For example, on Windows: `C:\Users\<username>\.espressif\eim_gui\eim.exe`. + On Linux: `~/.espressif/eim_gui/eim`.
90-98: Migration steps: grammar and clarity.-2. Open the Espressif-IDE if already running restart it. -3. After the IDE detects the EIM executable and a valid older workspace, it will prompt you to convert the old workspace to the new format.: +2. If Espressif‑IDE is already running, restart it. +3. After the IDE detects the EIM executable and a valid older workspace, it prompts you to convert the workspace to the new format: @@ -5. Now in Espressif-IDE, go to ``Espressif > ESP-IDF Manager``. - The ESP-IDF Manager editor will open, and you can proceed to select activate against the version you want. +5. In Espressif‑IDE, go to ``Espressif > ESP-IDF Manager``. + The ESP‑IDF Manager editor opens; activate the version you want.tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
702-702: Make console find timeout configurable (align with other waits).Hardcoding 30000 is inconsistent with other waits that use DefaultPropertyFetcher. Parameterize it so CI can tune without code changes.
Apply this diff:
- TestWidgetWaitUtility.waitUntilViewContains(bot, findText, consoleView, 30000); + TestWidgetWaitUtility.waitUntilViewContains( + bot, + findText, + consoleView, + DefaultPropertyFetcher.getLongPropertyValue("console.find.timeout", 30000) + );Can you confirm the longest observed time for this wait on slower runners so we can pick a sensible default?
tests/pom.xml (1)
33-33: Confirm how -DtestRun=true is consumed and consider passing it as a system property instead of via argLine.If this flag gates test-only behavior, prefer explicit systemPropertyVariables to avoid argLine being overridden by other plugins (e.g., JaCoCo) and to keep heap flags separate.
- Which modules read testRun?
- If you want, I can propose a snippet to move it under tycho-surefire-plugin .
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/help/ProductInformationHandler.java (1)
134-135: System.getenv usage: verify consistency with IDF/CDT overlay.Switching to System.getenv() may skip Eclipse/CDT-added env (if any). Ensure the Python discovery still works when IDF vars are only injected into the IDE environment.
If needed, pass new IDFEnvironmentVariables().getSystemEnvMap() here for parity with showEspIdfVersion().
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/ESPToolChainManager.java (1)
318-320: Confirm env choice for idf_tools.py export.Using System.getenv() may omit IDE-layer vars needed by idf_tools.py. If export depends on IDF_PYTHON_ENV_PATH from the IDE, consider IDFEnvironmentVariables().getSystemEnvMap().
Can you run this on a clean workspace where IDF vars are only set via the plugin to ensure export still succeeds?
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/EspToolCommands.java (1)
29-31: Redundant environment copy; rely on default or add explicit overlay.ProcessBuilder already inherits the parent env. Calling environment().putAll(System.getenv()) is a no-op and adds overhead. If you intended to inject IDF/EIM vars, use IDFEnvironmentVariables().getSystemEnvMap().
Apply this diff:
- processBuilder.environment().putAll(System.getenv()); + // Inherit default env; add explicit overrides only if neededRepeat the same change in eraseFlash() and writeFlash().
Do these commands require any extra PATH entries from EIM? If so, overlay them explicitly here.
Also applies to: 38-40, 47-49
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java (4)
286-286: Prefer env with CDT overlays (consistency with IDFMonitor).Using only System.getenv() may miss CDT/user‑defined vars. Align with IDFEnvironmentVariables().getSystemEnvMap() as the baseline.
- Map<String, String> environment = new HashMap<>(System.getenv()); + Map<String, String> environment = new com.espressif.idf.core.IDFEnvironmentVariables().getSystemEnvMap();Add import if desired:
import com.espressif.idf.core.IDFEnvironmentVariables;
438-438: Same env handling here.Apply the same change to include CDT/user env.
- Map<String, String> environment = new HashMap<>(System.getenv()); + Map<String, String> environment = new com.espressif.idf.core.IDFEnvironmentVariables().getSystemEnvMap();
453-453: Same env handling (pip list).Use the overlay env map here too for consistency.
- Map<String, String> environment = new HashMap<>(System.getenv()); + Map<String, String> environment = new com.espressif.idf.core.IDFEnvironmentVariables().getSystemEnvMap();
417-422: Validate parent directory writability when file doesn’t exist.checkIfFileIsNotWritable() returns false for non‑existent files; ensure parent dir is writable.
bundles/com.espressif.idf.core/META-INF/MANIFEST.MF (1)
19-21: Duplicate Require‑Bundle entry (org.eclipse.jgit).Not from this PR, but worth tidying.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
23-26: Guard against null plugin to avoid NPE at startup/shutdown edges.Low risk, but trivial to harden.
- public static void log(String message, Exception e) - { - log(IDFCorePlugin.getPlugin(), message, e); - } + public static void log(String message, Exception e) { + Plugin plugin = IDFCorePlugin.getPlugin(); + if (plugin != null) { + log(plugin, message, e); + } + }bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonChangeListener.java (1)
14-17: Interface is clear — consider @FunctionalInterface.Allows lambdas, enforces single‑method contract.
bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/core/IDFMonitor.java (2)
159-167: Simplify env composition.getSystemEnvMap() already starts from System.getenv() and overlays CDT vars; no need to seed with System.getenv() first.
- Map<String, String> environment = new HashMap<>(System.getenv()); - environment.putAll(idfEnvMap); + Map<String, String> environment = new HashMap<>(idfEnvMap);
80-81: Guard against null ELF path.getElfFilePath(project) may return null → NPE.
- args.add(getElfFilePath(project).toString()); + IPath elf = getElfFilePath(project); + if (elf != null) { + args.add(elf.toString()); + } else { + throw new CoreException(IDFCorePlugin.errorStatus("ELF file not found", null)); + }bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFEnvironmentVariables.java (1)
137-149: Persist original PATH for downstream logic.Optionally store SYSTEM_PATH once when composing env; helps callers unify PATH/Path safely.
- Map<String, String> envMap = new HashMap<String, String>(System.getenv()); + Map<String, String> envMap = new HashMap<>(System.getenv()); + String sysPath = envMap.containsKey("PATH") ? envMap.get("PATH") : envMap.get("Path"); + if (sysPath != null) { + envMap.put(SYSTEM_PATH, sysPath); + }bundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/JsonConfigServer.java (1)
134-139: IDF_CCACHE_ENABLE valueUse "0"/"1" to match other places expecting numeric flags.
- env.put("IDF_CCACHE_ENABLE", "false"); //$NON-NLS-1$ //$NON-NLS-2$ + env.put("IDF_CCACHE_ENABLE", "0"); //$NON-NLS-1$ //$NON-NLS-2$bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties (5)
24-24: Fix filename typo: “esp_idf.json.json” → “esp_idf.json”.Prevents confusing/incorrect guidance.
Apply this diff:
-ToolsInitializationDifferentPathMessageBoxMessage=A different ESP-IDF path was found in the esp_idf.json.json configuration file. Do you want to install the tools in the new path or the old path? Please click on the appropriate button.\nNew Path: {0}\nOld Path: {1} +ToolsInitializationDifferentPathMessageBoxMessage=A different ESP-IDF path was found in the esp_idf.json configuration file. Do you want to install the tools in the new path or the old path? Please click on the appropriate button.\nNew Path: {0}\nOld Path: {1}
25-26: “Yes/No” here is ambiguous. Suggest explicit options.Users can’t tell which path “Yes” maps to. Either revert to “Use New Path”/“Use Old Path” or rephrase the prompt to a single-path yes/no question.
Two options:
-ToolsInitializationDifferentPathMessageBoxOptionYes=Yes -ToolsInitializationDifferentPathMessageBoxOptionNo=No +ToolsInitializationDifferentPathMessageBoxOptionYes=Use New Path +ToolsInitializationDifferentPathMessageBoxOptionNo=Use Old PathOr adjust the message to align with Yes/No:
-ToolsInitializationDifferentPathMessageBoxMessage=A different ESP-IDF path was found... +ToolsInitializationDifferentPathMessageBoxMessage=A different ESP-IDF path was found...\n\nUse the new path?\nNew Path: {0}\nOld Path: {1}
29-29: Minor grammar.-IDFToolChainsMissingErrorMsg=Toolchains are missing. Please use ESP-IDF Manager for configuring +IDFToolChainsMissingErrorMsg=Toolchains are missing. Please use ESP-IDF Manager to configure.
37-44: Export/import terminology is inconsistent.Keys say “Export…”, but success/fail messages say “Import…”. Align to “Export” (or “Migration”) to avoid user confusion.
Apply this diff:
-OldConfigExportCompleteSuccessMsgTitle=Import Successful -OldConfigExportCompleteSuccessMsg=The old configuration has been successfully imported. -OldConfigExportCompleteFailMsgTitle=Conversion Failed -OldConfigExportCompleteFailMsg=An error occurred while importing old configuration. +OldConfigExportCompleteSuccessMsgTitle=Export Successful +OldConfigExportCompleteSuccessMsg=The old configuration has been successfully exported. +OldConfigExportCompleteFailMsgTitle=Export Failed +OldConfigExportCompleteFailMsg=An error occurred while exporting the old configuration.
39-40: Rename keys OldConfigExportDirectorSelectionDialogTitle/Info → OldConfigExportDirectorySelectionDialogTitle/Info.
Occurrences: bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties (lines 39–40) and bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java (lines 47–48). Update both and any callers/translations, or keep the current spelling to avoid breaking consumers.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties (2)
12-12: Fix typo and spacing.-EimJsonChangedMsgDetail=It seems that the ESP-IDF tools have been modified. If you just installed ESP-IDF we recommend refereshing via ESP-IDF Manager . Would you like to proceed with the update? +EimJsonChangedMsgDetail=It seems that the ESP-IDF tools have been modified. If you just installed ESP-IDF, we recommend refreshing via ESP-IDF Manager. Would you like to proceed with the update?
13-13: Minor grammar tweak.-EimJsonStateChangedMsgDetail=It looks like the ESP-IDF installation was modified since last start. The Espressif IDE cannot guarantee the consistency. Kindly refresh the installation via ESP-IDF Manager. +EimJsonStateChangedMsgDetail=It looks like the ESP-IDF installation was modified since the last start. The Espressif IDE cannot guarantee consistency. Kindly refresh the installation via ESP-IDF Manager.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/NewProjectHandlerUtil.java (1)
34-36: Tiny cleanup: use method reference.Equivalent and a bit cleaner.
-String path = Optional.ofNullable(pathEnv).map(o -> o.getValue()).orElse(null); +String path = Optional.ofNullable(pathEnv).map(IEnvironmentVariable::getValue).orElse(null);bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/ESPIDFManagerEditor.java (1)
79-83: Implement setFocus on a concrete control.Store the created Composite and forward focus for better UX and accessibility.
public class ESPIDFManagerEditor extends EditorPart { public static final String EDITOR_ID = "com.espressif.idf.ui.manageespidf"; + private Composite control; @@ - public void createPartControl(Composite parent) + public void createPartControl(Composite parent) { - IEditorInput input = getEditorInput(); + IEditorInput input = getEditorInput(); if (input instanceof EimEditorInput eimInput) { - ESPIDFMainTablePage espidfMainTablePage = ESPIDFMainTablePage.getInstance(eimInput.getEimJson()); - espidfMainTablePage.createPage(parent); + ESPIDFMainTablePage espidfMainTablePage = ESPIDFMainTablePage.getInstance(eimInput.getEimJson()); + this.control = espidfMainTablePage.createPage(parent); @@ - public void setFocus() + public void setFocus() { - + if (control != null && !control.isDisposed()) + { + control.setFocus(); + } }bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/DownloadListener.java (3)
7-12: Tighten Javadoc (grammar, threading, contract).Clarify callback semantics and remove personal email to avoid PII in source control.
-/** - * Interface to use for the download listening this can be used in your own classes. - * Added specifically for {@link EimLoader} - * @author Ali Azam Rana <[email protected]> - * - */ +/** + * Listener for download progress and completion events. + * Intended for use with {@link EimLoader}. + * + * Threading: Callbacks may be invoked from a background worker thread. + * Implementations should marshal to the UI thread if updating UI components. + * + * Contract: + * - onProgress is called with percent in [0, 100]. + * - onCompleted is called once on success with the local file path. + * - onError is called once on failure with a message and the underlying exception (may be null). + * + * @since 4.0.0 + */
15-17: Consider an adapter or default methods.To reduce boilerplate for implementers who only care about one callback, provide defaults.
-public interface DownloadListener +public interface DownloadListener { - public void onProgress(int percent); - public void onCompleted(String filePath); - public void onError(String message, Exception e); + default void onProgress(int percent) {} + default void onCompleted(String filePath) {} + default void onError(String message, Exception e) {} }
15-15: Document percent bounds.Add “0–100” to the parameter Javadoc or validate in caller.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.java (2)
16-17: Lightweight thread-safety for cached value.This cache may be read from background/watchers and UI; mark as volatile.
- private EimJson eimJson; + private volatile EimJson eimJson;
43-51: Optionally avoid nulls by returning an empty model.Returning null propagates checks to all callers; consider returning new EimJson() when file is missing.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java (1)
39-47: Never return null for idfInstalled.Guard against null to simplify callers.
- public List<IdfInstalled> getIdfInstalled() - { - return idfInstalled; - } + public List<IdfInstalled> getIdfInstalled() + { + return idfInstalled != null ? idfInstalled : java.util.Collections.emptyList(); + }bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.java (1)
53-57: Guard active window/page before opening editor.This mirrors your other UI code and avoids NPEs on startup edge cases.
- IDE.openEditor(activeww.getActivePage(), new EimEditorInput(eimJson), ESPIDFManagerEditor.EDITOR_ID, - true); + if (activeww != null && activeww.getActivePage() != null) { + IDE.openEditor(activeww.getActivePage(), new EimEditorInput(eimJson), + ESPIDFManagerEditor.EDITOR_ID, true); + } else { + Logger.log("No active workbench window/page to open ESP-IDF Manager."); //$NON-NLS-1$ + }bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EimButtonLaunchListener.java (5)
115-131: Don’t declare throws when you handle exceptions inside; use the instance display.Tighten the signature and use the field display for consistency.
- private void launchEspIdfManager() throws PartInitException + private void launchEspIdfManager() { - Display.getDefault().asyncExec(() -> { + display.asyncExec(() -> { IWorkbenchWindow activeww = EclipseHandler.getActiveWorkbenchWindow(); if (activeww == null || activeww.getActivePage() == null) { Logger.log("Cannot open ESP-IDF Manager. No active workbench window or active page."); return; } try { - EimIdfConfiguratinParser eimIdfConfiguratinParser = new EimIdfConfiguratinParser(); + EimIdfConfigurationParser eimIdfConfiguratinParser = new EimIdfConfigurationParser(); EimJson eimJson = eimIdfConfiguratinParser.getEimJson(true); IDE.openEditor(activeww.getActivePage(), new EimEditorInput(eimJson), ESPIDFManagerEditor.EDITOR_ID, true); }
63-64: Typo: EimDownlaodListener → EimDownloadListener.Keep names clean; update ctor usage and inner class name.
- eimLoader = new EimLoader(new EimDownlaodListener(), standardConsoleStream, errorConsoleStream, display); + eimLoader = new EimLoader(new EimDownloadListener(), standardConsoleStream, errorConsoleStream, display);
141-146: Rename inner listener class accordingly.- private class EimDownlaodListener implements DownloadListener + private class EimDownloadListener implements DownloadListener
144-152: Bound percent to [0,100] for progress bar.Avoid IllegalArgumentException from String.repeat on unexpected values.
- try - { - int blocks = percent / 10; - String bar = "[" + "#".repeat(blocks) + " ".repeat(10 - blocks) + "] " + percent + "%"; + try + { + int p = Math.max(0, Math.min(100, percent)); + int blocks = p / 10; + String bar = "[" + "#".repeat(blocks) + " ".repeat(10 - blocks) + "] " + p + "%"; standardConsoleStream.write("\r" + bar); }
206-216: Surface the error message text as well.Include the provided message to aid debugging.
- errorConsoleStream.write("Download Failed: " + e.getMessage()); + errorConsoleStream.write("Download Failed: " + message + " - " + e.getMessage());bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/EimEditorInput.java (1)
25-30: Remove TODO or delegate to adapter manager.Optional: return Platform.getAdapterManager().getAdapter(this, adapter) instead of null.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/EspressifGeneralStartup.java (3)
82-92: Avoid calling static methods via instance; drop unnecessary suppression.Call LaunchBarListener.setIgnoreTargetChange(...) directly.
Apply:
- @SuppressWarnings("static-access") private void disableLaunchBarEvents() { - launchBarListener.setIgnoreTargetChange(true); + LaunchBarListener.setIgnoreTargetChange(true); } @@ - @SuppressWarnings("static-access") private void enableLaunchBarEvents() { - launchBarListener.setIgnoreTargetChange(false); + LaunchBarListener.setIgnoreTargetChange(false); }
119-127: Null-safety on workbench window/page before findView.Avoid potential NPE when no window/page is active.
Apply:
- BuildView view = (BuildView) PlatformUI.getWorkbench().getActiveWorkbenchWindow().getActivePage() - .findView(BUILDHINTS_ID); + var window = PlatformUI.getWorkbench().getActiveWorkbenchWindow(); + if (window == null || window.getActivePage() == null) { + return; + } + BuildView view = (BuildView) window.getActivePage().findView(BUILDHINTS_ID);
11-11: Internal CDT API usage.org.eclipse.cdt.cmake.core.internal.Activator is internal; consider IDFCorePlugin.getService(ILaunchBarManager.class) for stability.
Also applies to: 78-80
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.java (2)
54-61: Persist preference updates with flush().Without flush(), the timestamp may not survive restart.
Apply:
public void updateLastSeenTimestamp() { File jsonFile = new File(getEimJsonPath()); if (jsonFile.exists()) { - preferences.putLong(LAST_MODIFIED_PREF_KEY, jsonFile.lastModified()); + preferences.putLong(LAST_MODIFIED_PREF_KEY, jsonFile.lastModified()); + try { + preferences.flush(); + } catch (org.osgi.service.prefs.BackingStoreException e) { + Logger.log(e); + } } }
45-48: Minor: align wording “eim_idf.json” vs actual filename.Log message uses “eim_idf.json”; constants likely point to “idf_eim.json”. Align to avoid confusion.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java (2)
175-186: Flush preference after setting OLD_CONFIG_EXPORTED_FLAG.Ensure it’s persisted across restarts.
Apply:
if (status.getSeverity() != IStatus.ERROR) { preferences.putBoolean(EimConstants.OLD_CONFIG_EXPORTED_FLAG, true); + try { + preferences.flush(); + } catch (org.osgi.service.prefs.BackingStoreException e) { + Logger.log(e); + } writeToStandardConsoleStream(Messages.OldConfigExportCompleteSuccessMsg); }
23-33: Minor: prune unused imports.Several imports appear unused (e.g., IWorkbench, IIntroManager). Clean up to reduce noise.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java (2)
101-103: Flush preference after updating last-seen timestamp.Persist change immediately.
Apply:
EimJsonStateChecker checker = new EimJsonStateChecker(preferences); - checker.updateLastSeenTimestamp(); + checker.updateLastSeenTimestamp(); + try { + preferences.flush(); + } catch (org.osgi.service.prefs.BackingStoreException e) { + Logger.log(e); + }
53-56: Reduce log noise in normal operation.“Listener is paused” is informational; log only in debug mode.
Apply:
- Logger.log("Listener is paused"); + Logger.log("Listener is paused", true);bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (2)
27-66: Handle null activation script path gracefullyThe method doesn't validate if the activation script path is null or if the file exists before attempting to execute it. This could lead to runtime exceptions.
public static String getIdfVersion(IdfInstalled idfInstalled, String gitPath) { String activationScript = idfInstalled.getActivationScript(); + if (activationScript == null || !new File(activationScript).exists()) + { + Logger.log("Activation script not found: " + activationScript); + return null; + } String espIdfVersion = null;
68-93: Consider Platform.OS_AIX and other Unix variantsThe method assumes non-Windows, non-Linux systems are macOS with
/bin/zsh. This could fail on other Unix variants like AIX or Solaris.else if (Platform.getOS().equals(Platform.OS_LINUX)) { command.add("/bin/bash"); //$NON-NLS-1$ command.add(activationScriptPath); command.add("-e"); //$NON-NLS-1$ } -else +else if (Platform.getOS().equals(Platform.OS_MACOSX)) { command.add("/bin/zsh"); //$NON-NLS-1$ command.add(activationScriptPath); command.add("-e"); //$NON-NLS-1$ } +else +{ + // Fallback to sh for other Unix-like systems + command.add("/bin/sh"); //$NON-NLS-1$ + command.add(activationScriptPath); + command.add("-e"); //$NON-NLS-1$ +}bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (1)
453-472: Git path separator inconsistencyLines 463 and 468 use hardcoded ";" as path separator, but this should be
File.pathSeparatorfor cross-platform compatibility.-path1 = gitDir.concat(";").concat(path1); //$NON-NLS-1$ +path1 = gitDir.concat(File.pathSeparator).concat(path1);-path2 = gitDir.concat(";").concat(path2); //$NON-NLS-1$ +path2 = gitDir.concat(File.pathSeparator).concat(path2);bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.java (2)
10-14: Path construction should use Paths.get() for consistencyThe path construction mixes string concatenation with
File.separator, which could be simplified usingPaths.get().-String EIM_POSIX_DIR = System.getProperty("user.home").concat(File.separator + ".espressif" + File.separator + "tools" + File.separator); +String EIM_POSIX_DIR = Paths.get(System.getProperty("user.home"), ".espressif", "tools").toString() + File.separator; -String EIM_WIN_DIR = EIM_WIN_ESPRESSIF_DIR + File.separator + "tools" + File.separator; +String EIM_WIN_DIR = Paths.get(EIM_WIN_ESPRESSIF_DIR, "tools").toString() + File.separator;
12-12: Hardcoded C: drive assumption for WindowsThe Windows path assumes C: drive, which may not be appropriate for all systems. Consider using environment variables or system properties.
Should we use
System.getenv("ProgramFiles")orSystem.getenv("LOCALAPPDATA")as the base path instead of hardcoding C:?bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.java (3)
151-171: Handle DELETE events and tighten dedupe granularity.DELETE currently triggers IOException on lastModifiedTime. Also, deduping at seconds can drop rapid updates.
Apply this diff:
- for (WatchEvent<?> event : key.pollEvents()) + for (WatchEvent<?> event : key.pollEvents()) { if (event.kind() == StandardWatchEventKinds.OVERFLOW) { continue; } Object context = event.context(); if (context instanceof Path path && path.toString().equals(EimConstants.EIM_JSON)) { Path fullPath = watchDirectoryPath.resolve(path); try { - Instant currentModified = Files.getLastModifiedTime(fullPath).toInstant() - .truncatedTo(ChronoUnit.SECONDS); + if (event.kind() == StandardWatchEventKinds.ENTRY_DELETE) + { + // Notify listeners about deletion without touching the filesystem. + lastModifiedTime = null; + for (EimJsonChangeListener listener : eimJsonChangeListeners) + { + listener.onJsonFileChanged(fullPath, paused); + } + continue; + } + Instant currentModified = Files.getLastModifiedTime(fullPath).toInstant() + .truncatedTo(ChronoUnit.MILLIS); - if (lastModifiedTime != null && currentModified.compareTo(lastModifiedTime) <= 0) + if (lastModifiedTime != null && !currentModified.isAfter(lastModifiedTime)) { continue; // skip duplicate or same-second event } lastModifiedTime = currentModified; for (EimJsonChangeListener listener : eimJsonChangeListeners) { listener.onJsonFileChanged(fullPath, paused); } } catch (IOException e) { Logger.log(e); }Also applies to: 172-178, 179-183
194-201: Minor log message polish.Past tense reads better for shutdown logs.
Apply this diff:
- Logger.log("File Watch Service close"); //$NON-NLS-1$ + Logger.log("File Watch Service closed."); //$NON-NLS-1$
42-60: Prefer factory start over starting thread in constructor.Starting the thread in the constructor can leak “this” before fully constructed. Consider a private ctor + start() in Holder after successful construction.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (5)
119-128: Robust link opening on all OSes.Wrap Desktop.browse with capability checks to avoid failures on systems without Desktop, and fall back to SWT Program.
Apply this diff:
- try - { - java.awt.Desktop.getDesktop() - .browse(new java.net.URI("https://dl.espressif.com/dl/esp-idf/support-periods.svg")); //$NON-NLS-1$ - } + try + { + java.net.URI uri = new java.net.URI("https://dl.espressif.com/dl/esp-idf/support-periods.svg"); //$NON-NLS-1$ + if (java.awt.Desktop.isDesktopSupported() && java.awt.Desktop.getDesktop().isSupported(java.awt.Desktop.Action.BROWSE)) { + java.awt.Desktop.getDesktop().browse(uri); + } else { + org.eclipse.swt.program.Program.launch(uri.toString()); + } + }
228-231: Remove duplicate MeasureItem listeners.Two handlers set the row height; keep one.
Apply this diff:
- table.addListener(SWT.MeasureItem, event -> { - event.height = 25; - }); ... table.addListener(SWT.MeasureItem, e -> { e.height = 30; });Also applies to: 234-236
241-246: Remove unused buttonComposite (dead UI).This composite has no children and wastes layout space.
Apply this diff:
- Composite buttonComposite = new Composite(idfToolsGroup, SWT.NONE); - GridData buttonCompositeGridData = new GridData(SWT.RIGHT, SWT.CENTER, false, false); - buttonCompositeGridData.verticalAlignment = SWT.TOP; // Aligns the button composite at the top - buttonComposite.setLayoutData(buttonCompositeGridData); - buttonComposite.setLayout(new GridLayout(1, true)); + // (removed unused 'Add' placeholder)
130-135: Avoid clearing the console multiple times per action.getConsoleStream creates a new console each time and clears it; pass shared streams or defer clearing.
Also applies to: 205-207, 516-520
412-416: Avoid repeated process execution for version retrieval in hot UI paths.getIdfVersion spawns a process; called per row update. Cache versions once per input refresh.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java (2)
162-168: Only set EIM_PATH if the default path exists.Current code may persist a non‑existent path in env vars.
Apply this diff:
public void findAndSetEimPath() { Path defaultEimPath = getDefaultEimPath(); - if (defaultEimPath != null) - setEimPathInEnvVar(defaultEimPath.toString()); + if (defaultEimPath != null && Files.exists(defaultEimPath)) + { + setEimPathInEnvVar(defaultEimPath.toString()); + } + else + { + Logger.log("EIM not found at default path: " + String.valueOf(defaultEimPath)); //$NON-NLS-1$ + } }
48-60: Optional: auto‑discover EIM and persist when found.If EIM_PATH is empty but default exists, consider calling findAndSetEimPath() here to improve UX.
| tags: | ||
| - "v[0-9]+.[0-9]+.[0-9]+" | ||
| - "v[0-9]+\\.[0-9]+\\.[0-9]+-beta" | ||
| - "v[0-9]+.[0-9]+.[0-9]+-beta.*" |
There was a problem hiding this comment.
Tag pattern is regex-like but GHA uses glob — will not match.
Use glob patterns; “+” and “.” aren’t regex here. This won’t trigger on beta tags.
Apply:
push:
tags:
- - "v[0-9]+.[0-9]+.[0-9]+"
- - "v[0-9]+.[0-9]+.[0-9]+-beta.*"
+ - "v*.*.*"
+ - "v*.*.*-beta*"📝 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.
| - "v[0-9]+.[0-9]+.[0-9]+-beta.*" | |
| push: | |
| tags: | |
| - "v*.*.*" | |
| - "v*.*.*-beta*" |
🤖 Prompt for AI Agents
In .github/workflows/ci_release.yml around line 7, the tag pattern uses regex
syntax ("v[0-9]+.[0-9]+.[0-9]+-beta.*") but GitHub Actions expects a glob, so it
won't match beta tags; replace the regex with a glob such as
"v[0-9]*.[0-9]*.[0-9]*-beta*" (or the simpler "v*.*.*-beta*") to use * instead
of + and ensure proper matching of beta tags.
| uses: actions/setup-python@v2 | ||
| with: | ||
| python-version: '3.10' | ||
| python-version: '3.10' | ||
|
|
There was a problem hiding this comment.
setup-python@v2 is deprecated on GH runners.
Upgrade to v5 to avoid Node runtime breakage.
- uses: actions/setup-python@v2
+ uses: actions/setup-python@v5
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/setup-python@v2 | |
| with: | |
| python-version: '3.10' | |
| python-version: '3.10' | |
| uses: actions/setup-python@v5 | |
| with: | |
| python-version: '3.10' |
🧰 Tools
🪛 actionlint (1.7.7)
25-25: 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)
🤖 Prompt for AI Agents
In .github/workflows/ci.yml around lines 25 to 28, the workflow uses the
deprecated actions/setup-python@v2; update the action to actions/setup-python@v5
to avoid Node runtime breakage on GH runners, keeping the same inputs (e.g.,
python-version: '3.10') and update any syntax if required by v5 release notes
before committing.
| public class EimIdfConfiguratinParser | ||
| { | ||
| private EimJson eimJson; | ||
| private Gson gson; | ||
|
|
||
| public EimIdfConfiguratinParser() | ||
| { | ||
| gson = new GsonBuilder().setPrettyPrinting().enableComplexMapKeySerialization() | ||
| .excludeFieldsWithoutExposeAnnotation().create(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Public class name typo: rename to EimIdfConfigurationParser (and constructor).
Fix the spelling to avoid public API churn later; update all references.
-public class EimIdfConfiguratinParser
+public class EimIdfConfigurationParser
{
- private EimJson eimJson;
- private Gson gson;
+ private EimJson eimJson;
+ private Gson gson;
- public EimIdfConfiguratinParser()
+ public EimIdfConfigurationParser()
{
gson = new GsonBuilder().setPrettyPrinting().enableComplexMapKeySerialization()
.excludeFieldsWithoutExposeAnnotation().create();
}📝 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 | |
| { | |
| private EimJson eimJson; | |
| private Gson gson; | |
| public EimIdfConfiguratinParser() | |
| { | |
| gson = new GsonBuilder().setPrettyPrinting().enableComplexMapKeySerialization() | |
| .excludeFieldsWithoutExposeAnnotation().create(); | |
| } | |
| public class EimIdfConfigurationParser | |
| { | |
| private EimJson eimJson; | |
| private Gson gson; | |
| public EimIdfConfigurationParser() | |
| { | |
| gson = new GsonBuilder().setPrettyPrinting().enableComplexMapKeySerialization() | |
| .excludeFieldsWithoutExposeAnnotation().create(); | |
| } |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.java
around lines 14 to 23, the public class and its constructor are misspelled as
EimIdfConfiguratinParser; rename the class to EimIdfConfigurationParser and
update the constructor name to match, then update all references (imports,
usages, tests, build files) across the codebase to the new class name to keep
the public API consistent; ensure the filename is updated to
EimIdfConfigurationParser.java to match the public class and run a quick compile
to catch remaining references.
| try (FileReader fileReader = new FileReader(file)) | ||
| { | ||
| eimJson = gson.fromJson(fileReader, EimJson.class); | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle UTF‑8 and malformed JSON; prevent runtime crash.
Gson can throw JsonSyntaxException/JsonIOException; also prefer explicit UTF‑8.
- try (FileReader fileReader = new FileReader(file))
- {
- eimJson = gson.fromJson(fileReader, EimJson.class);
- }
+ try (java.io.Reader reader = new java.io.InputStreamReader(
+ new java.io.FileInputStream(file), java.nio.charset.StandardCharsets.UTF_8))
+ {
+ eimJson = gson.fromJson(reader, EimJson.class);
+ }
+ catch (com.google.gson.JsonSyntaxException | com.google.gson.JsonIOException e)
+ {
+ Logger.log("Failed to parse EIM JSON: " + file.getAbsolutePath(), e); //$NON-NLS-1$
+ eimJson = null; // keep null so callers can fallback
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try (FileReader fileReader = new FileReader(file)) | |
| { | |
| eimJson = gson.fromJson(fileReader, EimJson.class); | |
| } | |
| } | |
| try (java.io.Reader reader = new java.io.InputStreamReader( | |
| new java.io.FileInputStream(file), java.nio.charset.StandardCharsets.UTF_8)) | |
| { | |
| eimJson = gson.fromJson(reader, EimJson.class); | |
| } | |
| catch (com.google.gson.JsonSyntaxException | com.google.gson.JsonIOException e) | |
| { | |
| Logger.log("Failed to parse EIM JSON: " + file.getAbsolutePath(), e); //$NON-NLS-1$ | |
| eimJson = null; // keep null so callers can fallback | |
| } | |
| } |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.java
around lines 37 to 41, replace the FileReader with an InputStreamReader using
StandardCharsets.UTF_8 to ensure UTF‑8 decoding, and wrap the gson.fromJson call
in a try/catch that catches com.google.gson.JsonSyntaxException and
com.google.gson.JsonIOException (and optionally IOException) to prevent a
runtime crash; in the catch block log the error with context and either return a
safe default/null or throw a checked exception so the caller can handle
malformed JSON instead of letting the method fail silently or crash.
| public String installAndLaunchDmg(Path dmgPath) throws IOException, InterruptedException | ||
| { | ||
| logMessage("Mounting DMG…\n"); //$NON-NLS-1$ | ||
| Process mountProcess = new ProcessBuilder("hdiutil", "attach", dmgPath.toString()) //$NON-NLS-1$ //$NON-NLS-2$ | ||
| .redirectErrorStream(true).start(); | ||
|
|
||
| int mountExit = mountProcess.waitFor(); | ||
| if (mountExit != 0) | ||
| throw new IOException("hdiutil attach failed (exit " + mountExit + "): " //$NON-NLS-1$ //$NON-NLS-2$ | ||
| + readProcessOutput(mountProcess)); | ||
|
|
||
| String volumePath = parseVolumePath(mountProcess.getInputStream()); | ||
| if (volumePath == null) | ||
| throw new IOException("Failed to mount DMG: volume path not found."); //$NON-NLS-1$ | ||
|
|
||
| File[] apps = new File(volumePath).listFiles((dir, name) -> name.endsWith(".app")); //$NON-NLS-1$ | ||
| if (apps == null || apps.length == 0) | ||
| throw new FileNotFoundException("No .app found inside DMG."); //$NON-NLS-1$ | ||
|
|
||
| File appBundle = apps[0]; | ||
| Path targetAppPath = Paths.get("/Applications", appBundle.getName()); //$NON-NLS-1$ | ||
|
|
||
| logMessage("Copying app to /Applications…\n"); //$NON-NLS-1$ | ||
| Process copyProcess = new ProcessBuilder("cp", "-R", appBundle.getAbsolutePath(), //$NON-NLS-1$ //$NON-NLS-2$ | ||
| targetAppPath.toString()).redirectErrorStream(true).start(); | ||
|
|
||
| int copyExit = copyProcess.waitFor(); | ||
| if (copyExit != 0) | ||
| throw new IOException("Copy failed (exit " + copyExit + "): " //$NON-NLS-1$ //$NON-NLS-2$ | ||
| + readProcessOutput(copyProcess)); | ||
|
|
||
| logMessage("Unmounting DMG…\n"); //$NON-NLS-1$ | ||
| Process detachProcess = new ProcessBuilder("hdiutil", "detach", volumePath) //$NON-NLS-1$ //$NON-NLS-2$ | ||
| .redirectErrorStream(true).start(); | ||
|
|
||
| int detachExit = detachProcess.waitFor(); | ||
| if (detachExit != 0) | ||
| throw new IOException("hdiutil detach failed (exit " + detachExit + "): " //$NON-NLS-1$ //$NON-NLS-2$ | ||
| + readProcessOutput(detachProcess)); | ||
|
|
||
| String eimPath = targetAppPath.resolve("Contents/MacOS/eim").toString(); //$NON-NLS-1$ | ||
| new IDFEnvironmentVariables().addEnvVariable(IDFEnvironmentVariables.EIM_PATH, eimPath); | ||
| return eimPath; | ||
| } |
There was a problem hiding this comment.
DMG installation requires elevated privileges
Line 201 attempts to copy to /Applications which may require admin privileges on macOS. This could fail with permission errors.
Consider checking permissions first or providing fallback to user's Applications folder:
File appBundle = apps[0];
-Path targetAppPath = Paths.get("/Applications", appBundle.getName());
+// Try system Applications first, fall back to user's
+Path targetAppPath = Paths.get("/Applications", appBundle.getName());
+if (!Files.isWritable(Paths.get("/Applications")))
+{
+ targetAppPath = Paths.get(System.getProperty("user.home"), "Applications", appBundle.getName());
+ Files.createDirectories(targetAppPath.getParent());
+}📝 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 String installAndLaunchDmg(Path dmgPath) throws IOException, InterruptedException | |
| { | |
| logMessage("Mounting DMG…\n"); //$NON-NLS-1$ | |
| Process mountProcess = new ProcessBuilder("hdiutil", "attach", dmgPath.toString()) //$NON-NLS-1$ //$NON-NLS-2$ | |
| .redirectErrorStream(true).start(); | |
| int mountExit = mountProcess.waitFor(); | |
| if (mountExit != 0) | |
| throw new IOException("hdiutil attach failed (exit " + mountExit + "): " //$NON-NLS-1$ //$NON-NLS-2$ | |
| + readProcessOutput(mountProcess)); | |
| String volumePath = parseVolumePath(mountProcess.getInputStream()); | |
| if (volumePath == null) | |
| throw new IOException("Failed to mount DMG: volume path not found."); //$NON-NLS-1$ | |
| File[] apps = new File(volumePath).listFiles((dir, name) -> name.endsWith(".app")); //$NON-NLS-1$ | |
| if (apps == null || apps.length == 0) | |
| throw new FileNotFoundException("No .app found inside DMG."); //$NON-NLS-1$ | |
| File appBundle = apps[0]; | |
| Path targetAppPath = Paths.get("/Applications", appBundle.getName()); //$NON-NLS-1$ | |
| logMessage("Copying app to /Applications…\n"); //$NON-NLS-1$ | |
| Process copyProcess = new ProcessBuilder("cp", "-R", appBundle.getAbsolutePath(), //$NON-NLS-1$ //$NON-NLS-2$ | |
| targetAppPath.toString()).redirectErrorStream(true).start(); | |
| int copyExit = copyProcess.waitFor(); | |
| if (copyExit != 0) | |
| throw new IOException("Copy failed (exit " + copyExit + "): " //$NON-NLS-1$ //$NON-NLS-2$ | |
| + readProcessOutput(copyProcess)); | |
| logMessage("Unmounting DMG…\n"); //$NON-NLS-1$ | |
| Process detachProcess = new ProcessBuilder("hdiutil", "detach", volumePath) //$NON-NLS-1$ //$NON-NLS-2$ | |
| .redirectErrorStream(true).start(); | |
| int detachExit = detachProcess.waitFor(); | |
| if (detachExit != 0) | |
| throw new IOException("hdiutil detach failed (exit " + detachExit + "): " //$NON-NLS-1$ //$NON-NLS-2$ | |
| + readProcessOutput(detachProcess)); | |
| String eimPath = targetAppPath.resolve("Contents/MacOS/eim").toString(); //$NON-NLS-1$ | |
| new IDFEnvironmentVariables().addEnvVariable(IDFEnvironmentVariables.EIM_PATH, eimPath); | |
| return eimPath; | |
| } | |
| public String installAndLaunchDmg(Path dmgPath) throws IOException, InterruptedException | |
| { | |
| logMessage("Mounting DMG…\n"); //$NON-NLS-1$ | |
| Process mountProcess = new ProcessBuilder("hdiutil", "attach", dmgPath.toString()) //$NON-NLS-1$ //$NON-NLS-2$ | |
| .redirectErrorStream(true).start(); | |
| int mountExit = mountProcess.waitFor(); | |
| if (mountExit != 0) | |
| throw new IOException("hdiutil attach failed (exit " + mountExit + "): " //$NON-NLS-1$ //$NON-NLS-2$ | |
| readProcessOutput(mountProcess)); | |
| String volumePath = parseVolumePath(mountProcess.getInputStream()); | |
| if (volumePath == null) | |
| throw new IOException("Failed to mount DMG: volume path not found."); //$NON-NLS-1$ | |
| File[] apps = new File(volumePath).listFiles((dir, name) -> name.endsWith(".app")); //$NON-NLS-1$ | |
| if (apps == null || apps.length == 0) | |
| throw new FileNotFoundException("No .app found inside DMG."); //$NON-NLS-1$ | |
| File appBundle = apps[0]; | |
| // Try system Applications first, fall back to user's | |
| Path targetAppPath = Paths.get("/Applications", appBundle.getName()); //$NON-NLS-1$ | |
| if (!Files.isWritable(Paths.get("/Applications"))) | |
| { | |
| targetAppPath = Paths.get(System.getProperty("user.home"), "Applications", appBundle.getName()); | |
| Files.createDirectories(targetAppPath.getParent()); | |
| } | |
| logMessage("Copying app to /Applications…\n"); //$NON-NLS-1$ | |
| Process copyProcess = new ProcessBuilder("cp", "-R", appBundle.getAbsolutePath(), //$NON-NLS-1$ //$NON-NLS-2$ | |
| targetAppPath.toString()).redirectErrorStream(true).start(); | |
| int copyExit = copyProcess.waitFor(); | |
| if (copyExit != 0) | |
| throw new IOException("Copy failed (exit " + copyExit + "): " //$NON-NLS-1$ //$NON-NLS-2$ | |
| readProcessOutput(copyProcess)); | |
| logMessage("Unmounting DMG…\n"); //$NON-NLS-1$ | |
| Process detachProcess = new ProcessBuilder("hdiutil", "detach", volumePath) //$NON-NLS-1$ //$NON-NLS-2$ | |
| .redirectErrorStream(true).start(); | |
| int detachExit = detachProcess.waitFor(); | |
| if (detachExit != 0) | |
| throw new IOException("hdiutil detach failed (exit " + detachExit + "): " //$NON-NLS-1$ //$NON-NLS-2$ | |
| readProcessOutput(detachProcess)); | |
| String eimPath = targetAppPath.resolve("Contents/MacOS/eim").toString(); //$NON-NLS-1$ | |
| new IDFEnvironmentVariables().addEnvVariable(IDFEnvironmentVariables.EIM_PATH, eimPath); | |
| return eimPath; | |
| } |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimLoader.java
around lines 178-221, the install copies the .app into /Applications which can
fail due to macOS permission restrictions; update the logic to first check if
/Applications is writable and if not fall back to the current user's
Applications folder (~/Applications) by constructing the target path there,
ensuring that directory exists (create it if necessary) before copying, and then
use that resulting targetAppPath when setting the eim path and environment
variable; keep existing error handling for the copy process and ensure the
fallback path is used consistently for logging and return value.
|
|
||
| Map<String, String> environment = new HashMap<>(IDFUtil.getSystemEnv()); | ||
| Map<String, String> environment = new HashMap<>(System.getenv()); | ||
| Logger.log(environment.toString()); |
There was a problem hiding this comment.
Do not log the full environment (secrets/PII leakage).
Dumping environment variables can leak tokens, credentials, and proxy settings to logs.
- Logger.log(environment.toString());
+ Logger.log(String.format(
+ "Prepared environment: PATH present=%s, PYTHONUNBUFFERED=%s, IDF_PATH=%s",
+ (environment.containsKey("PATH") || environment.containsKey("Path")),
+ environment.get("PYTHONUNBUFFERED"),
+ environment.getOrDefault("IDF_PATH", "<not set>")
+ ));📝 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.
| Logger.log(environment.toString()); | |
| Logger.log(String.format( | |
| "Prepared environment: PATH present=%s, PYTHONUNBUFFERED=%s, IDF_PATH=%s", | |
| (environment.containsKey("PATH") || environment.containsKey("Path")), | |
| environment.get("PYTHONUNBUFFERED"), | |
| environment.getOrDefault("IDF_PATH", "<not set>") | |
| )); |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/AbstractToolsHandler.java
around line 145, remove the Logger.log(environment.toString()) call because
dumping the full environment can expose secrets/PII; replace it with a safe
alternative that either logs no environment variables or logs a vetted list of
non-sensitive keys (or keys with their values redacted), e.g., iterate only over
an allowlist of safe variable names or produce a count/summary of variables,
ensuring any sensitive values are never printed.
|
|
||
| #Github Actions Path | ||
| default.env.esp.idf.path=/opt/actions-runner/_work/idf-eclipse-plugin/idf-eclipse-plugin/dependencies/idf-tools | ||
| default.env.esp.idf.path=/opt/actions-runner/_work/idf-eclipse-plugin/idf-eclipse-plugin/dependencies/idf-tools/v5.3/esp-idf |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Align ESP‑IDF path with CI/EIM and avoid hardcoded self‑hosted absolute path
- Path pins v5.3, while the PR mentions CI using v5.4; this will skew test runs and can break the release build validation.
- The absolute self‑hosted runner path is brittle (repo name and runner layout dependent). Prefer deriving from env (GITHUB_WORKSPACE, IDF_VERSION) or resolving via the new EIM logic.
Quick fixes (pick one):
- default.env.esp.idf.path=/opt/actions-runner/_work/idf-eclipse-plugin/idf-eclipse-plugin/dependencies/idf-tools/v5.3/esp-idf
+ default.env.esp.idf.path=/opt/actions-runner/_work/idf-eclipse-plugin/idf-eclipse-plugin/dependencies/idf-tools/v5.4/esp-idfBetter (if your test loader supports env/property expansion) — version- and workspace-agnostic:
- default.env.esp.idf.path=/opt/actions-runner/_work/idf-eclipse-plugin/idf-eclipse-plugin/dependencies/idf-tools/v5.3/esp-idf
+ default.env.esp.idf.path=${GITHUB_WORKSPACE}/dependencies/idf-tools/${IDF_VERSION}/esp-idfOr, if EIM is the source of truth, point tests to the EIM‑resolved IDF path (name per your codebase):
- default.env.esp.idf.path=/opt/actions-runner/_work/idf-eclipse-plugin/idf-eclipse-plugin/dependencies/idf-tools/v5.3/esp-idf
+ default.env.esp.idf.path=${EIM_IDF_PATH}Run to confirm versions and usages:
🏁 Script executed:
#!/bin/bash
# 1) What IDF version(s) do workflows install?
rg -nP -C2 --glob '.github/workflows/**/*.yml' '(IDF_VERSION|v5\.\d|esp-idf)'
# 2) Any other hard pins to v5.3/v5.4 in repo?
rg -nP 'dependencies/idf-tools/v5\.(3|4)'
# 3) Where is this property consumed and do we support env expansion?
rg -n -C3 'default\.env\.esp\.idf\.path|default-test-linux\.properties'
rg -nP --type=java -C3 'getProperty\("default\.env\.esp\.idf\.path"\)|System\.getenv\(|Properties'Length of output: 62805
Replace hard‑coded Linux IDF path with a workspace/env-based value
- Issue: tests/com.espressif.idf.ui.test/configs/default-test-linux.properties:10 currently hard‑codes .../dependencies/idf-tools/v5.3/esp-idf (pins version and uses a brittle self‑hosted absolute path).
- Evidence: DefaultPropertyFetcher formats properties with GITHUB_WORKSPACE (tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/common/configs/DefaultPropertyFetcher.java:75) and tests/.../default-test-win.properties already uses {0}/dependencies/idf-tools (tests/com.espressif.idf.ui.test/configs/default-test-win.properties:10).
- Action (recommended): change the Linux entry to a workspace‑agnostic value to match Windows, e.g.:
default.env.esp.idf.path={0}/dependencies/idf-tools - Alternate: point tests at the runtime IDF path used by the product (IDF_PATH), which is consumed by IDFUtil (bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java:175–179) if you want EIM/IDF-managed resolution.
🤖 Prompt for AI Agents
In tests/com.espressif.idf.ui.test/configs/default-test-linux.properties around
line 10, the Linux IDF path is hard‑coded to an absolute self‑hosted location
which pins a version and is brittle; update the property to be
workspace/env‑agnostic to match the Windows file (e.g. use the
{0}/dependencies/idf-tools pattern) or alternatively point it to the runtime
IDF_PATH used by the product; replace the current absolute path value with
{0}/dependencies/idf-tools (or the appropriate IDF_PATH placeholder) so
DefaultPropertyFetcher can substitute the workspace at runtime.
|
|
||
| SWTBotEditor espIdfManagerView = bot.editorByTitle("ESP-IDF Manager"); | ||
| espIdfManagerView.bot().radio(0).click(); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Wait for the ESP‑IDF Manager editor before accessing it.
Accessing the editor immediately is flaky if it hasn’t opened yet.
Apply this diff to add a robust wait:
-
- SWTBotEditor espIdfManagerView = bot.editorByTitle("ESP-IDF Manager");
+
+ bot.waitUntil(org.eclipse.swtbot.eclipse.finder.waits.Conditions.waitForEditor(withPartName("ESP-IDF Manager")));
+ SWTBotEditor espIdfManagerView = bot.editorByTitle("ESP-IDF Manager");🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java"
echo "Checking $FILE"
if [ -f "$FILE" ]; then
sed -n '1,240p' "$FILE" || true
else
echo "File not found: $FILE"
rg -n --hidden --no-ignore "EnvSetupOperations" || true
fi
echo "---- search for withPartName occurrences ----"
rg -n --hidden --no-ignore "withPartName\\(" || true
echo "---- search for waitForEditor occurrences ----"
rg -n --hidden --no-ignore "waitForEditor\\(" || true
echo "---- search for waitUntil occurrences ----"
rg -n --hidden --no-ignore "waitUntil\\(" || true
echo "---- search for editorByTitle occurrences ----"
rg -n --hidden --no-ignore "editorByTitle\\(" || trueLength of output: 9245
Wait for the ESP‑IDF Manager editor before accessing it.
Accessing the editor immediately is flaky; wait for the editor to open before calling editorByTitle.
Location: tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java: lines 31–33
-
- SWTBotEditor espIdfManagerView = bot.editorByTitle("ESP-IDF Manager");
+
+ bot.waitUntil(org.eclipse.swtbot.eclipse.finder.waits.Conditions.waitForEditor(withPartName("ESP-IDF Manager")));
+ SWTBotEditor espIdfManagerView = bot.editorByTitle("ESP-IDF Manager");📝 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.
| SWTBotEditor espIdfManagerView = bot.editorByTitle("ESP-IDF Manager"); | |
| espIdfManagerView.bot().radio(0).click(); | |
| bot.waitUntil(org.eclipse.swtbot.eclipse.finder.waits.Conditions.waitForEditor(withPartName("ESP-IDF Manager"))); | |
| SWTBotEditor espIdfManagerView = bot.editorByTitle("ESP-IDF Manager"); | |
| espIdfManagerView.bot().radio(0).click(); |
🤖 Prompt for AI Agents
In
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java
around lines 31–33, accessing the "ESP-IDF Manager" editor immediately is flaky;
before calling bot.editorByTitle("ESP-IDF Manager") add a wait for the editor to
open (e.g., bot.waitUntil(Conditions.editorIsActive("ESP-IDF Manager")) or
equivalent waitForEditor condition), then retrieve the editor and interact with
its widgets as before to ensure the editor is present and stable.
| } | ||
|
|
||
| SWTBotEditor espIdfManagerView = bot.editorByTitle("ESP-IDF Manager"); | ||
| espIdfManagerView.bot().radio(0).click(); |
There was a problem hiding this comment.
Avoid index‑based radio selection; use a stable label.
Index 0 is brittle across UI changes.
Apply this diff (adjust label to the exact text):
- espIdfManagerView.bot().radio(0).click();
+ espIdfManagerView.bot().radio("Install recommended tools").click(); // TODO: confirm exact label📝 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.
| espIdfManagerView.bot().radio(0).click(); | |
| espIdfManagerView.bot().radio("Install recommended tools").click(); // TODO: confirm exact label |
🤖 Prompt for AI Agents
In
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java
around line 33, the test uses index-based radio selection
(espIdfManagerView.bot().radio(0).click()) which is brittle; replace it with a
label-based selection using the exact radio label string (e.g.,
espIdfManagerView.bot().radio("Exact Radio Label").click()), adjusting the label
to match the UI text so the selector is stable across UI changes.
| SWTBotView consoleView = bot.viewById("org.eclipse.ui.console.ConsoleView"); | ||
| consoleView.show(); | ||
| consoleView.setFocus(); | ||
| TestWidgetWaitUtility.waitUntilViewContains(bot, "Tools Setup complete", consoleView, 99000000); |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Excessive timeout (≈27.5 hours) risks wedging CI; make it sane and configurable.
Use a reasonable default (e.g., 15 min) and allow override via property.
Apply this minimal fix now:
- TestWidgetWaitUtility.waitUntilViewContains(bot, "Tools Setup complete", consoleView, 99000000);
+ TestWidgetWaitUtility.waitUntilViewContains(bot, "Tools Setup complete", consoleView, 900_000); // 15 minOptionally, make it configurable with DefaultPropertyFetcher (outside the changed hunk):
// Add inside the class (helper)
private static int toolsSetupTimeoutMs() {
String v = DefaultPropertyFetcher.getProperty("tools.setup.timeout.ms", "900000");
try { return Integer.parseInt(v); } catch (NumberFormatException e) { return 900_000; }
}Then replace the wait call with:
TestWidgetWaitUtility.waitUntilViewContains(bot, "Tools Setup complete", consoleView, toolsSetupTimeoutMs());Reduce excessive timeout (≈27.5h) — use a sane default (15 min) and make it configurable.
99000000 ms will likely wedge CI; replace with 900_000 (15 min) and allow override.
- TestWidgetWaitUtility.waitUntilViewContains(bot, "Tools Setup complete", consoleView, 99000000);
+ TestWidgetWaitUtility.waitUntilViewContains(bot, "Tools Setup complete", consoleView, 900_000); // 15 minOptional (make configurable):
private static int toolsSetupTimeoutMs() {
String v = DefaultPropertyFetcher.getProperty("tools.setup.timeout.ms", "900000");
try { return Integer.parseInt(v); } catch (NumberFormatException e) { return 900_000; }
}Then call:
TestWidgetWaitUtility.waitUntilViewContains(bot, "Tools Setup complete", consoleView, toolsSetupTimeoutMs());🤖 Prompt for AI Agents
In
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java
around lines 35 to 38, the wait timeout is set to 99,000,000 ms (≈27.5h) which
is excessive for CI; change the hardcoded timeout to a sane default of 900,000
ms (15 minutes) and refactor to make it configurable via a property (e.g., read
"tools.setup.timeout.ms" with a default of "900000" and parse safely, falling
back to 900000 on parse errors); then call
TestWidgetWaitUtility.waitUntilViewContains(...) using that helper method
instead of the fixed literal.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (13)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/exceptions/EimVersionMismatchException.java (1)
21-27: Prefer error-level logging for version mismatchesThis condition represents an error state; consider using Logger.logError (or log with an error IStatus) instead of info-level Logger.log to surface it properly.
- Logger.log(String.format("Invalid eim_idf.json version. Expected: %s, but found: %s.", expectedVersion, foundVersion)); //$NON-NLS-1$ + Logger.logError(String.format("Invalid eim_idf.json version. Expected: %s, but found: %s.", expectedVersion, foundVersion)); //$NON-NLS-1$bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.java (2)
39-41: Guard against null workbench window/page before UI actionsAvoid NPEs if no active workbench window or page is present (e.g., early startup).
- IWorkbenchWindow activeww = EclipseHandler.getActiveWorkbenchWindow(); - IDFUtil.closeWelcomePage(activeww); + IWorkbenchWindow activeww = EclipseHandler.getActiveWorkbenchWindow(); + if (activeww != null) { + IDFUtil.closeWelcomePage(activeww); + } ... - IDE.openEditor(activeww.getActivePage(), new EimEditorInput(eimJson), ESPIDFManagerEditor.EDITOR_ID, - true); + if (activeww == null || activeww.getActivePage() == null) { + Logger.log("Cannot open ESP-IDF Manager. No active workbench window or active page."); //$NON-NLS-1$ + return; + } + IDE.openEditor(activeww.getActivePage(), new EimEditorInput(eimJson), + ESPIDFManagerEditor.EDITOR_ID, true);Also applies to: 59-63
14-21: Typo in class name: EimIdfConfiguratinParserConsider renaming to EimIdfConfigurationParser across the codebase for clarity and consistency.
Would you like a follow-up PR to rename this type and update references?
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (3)
422-426: Avoid repeated process invocations to compute versionToolsUtility.getIdfVersion likely spawns processes; cache per IdfInstalled to prevent frequent calls during table redraw/refresh.
I can add a simple Map<String,String> cache keyed by IdfInstalled.getId() and invalidate on refreshEditorUI().
92-101: Singleton with static mutable state can cause stale UI datagetInstance(EimJson) sets a static eimJson on a shared singleton, which can bleed state across editors. Consider removing the singleton or scoping EimJson to the editor instance.
Would you like a small refactor to make ESPIDFMainTablePage non-singleton and pass EimJson via constructor?
223-226: Externalize UI string"IDF Tools" should be in Messages to support i18n like other labels.
- idfToolsGroup.setText("IDF Tools"); //$NON-NLS-1$ + idfToolsGroup.setText(Messages.IDFToolsHandler_ToolsManagerConsole);If a distinct label is desired, add a new key to ui.tools.messages.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java (7)
103-104: Clarify the error message when EIM is not available.Current text is grammatically off and vague. Improve for users and logs.
- return new Status(IStatus.ERROR, IDFCorePlugin.getId(), -1, "Cannot Convert EIM is not installed", null); //$NON-NLS-1$ + return new Status(IStatus.ERROR, IDFCorePlugin.getId(), -1, + "EIM is not installed; cannot import legacy configuration.", null); //$NON-NLS-1$
119-120: Return a precise reason when legacy config is missing.Helps users understand what failed and where.
- return new Status(IStatus.ERROR, IDFCorePlugin.getId(), -1, "Error in conversion", null); //$NON-NLS-1$ + return new Status(IStatus.ERROR, IDFCorePlugin.getId(), -1, + "Legacy configuration file not found: " + oldConfig.getAbsolutePath(), null); //$NON-NLS-1$
56-59: Tighten the existence check for the default EIM binary.Use isRegularFile/isExecutable to reduce false positives (e.g., directories).
- if (defaultEimPath != null) - exists = Files.exists(defaultEimPath); + if (defaultEimPath != null) { + exists = Files.isRegularFile(defaultEimPath) || Files.isExecutable(defaultEimPath); + }
163-169: Set EIM_PATH only if the discovered path actually exists.Prevents polluting the IDE env with a bogus path.
- if (defaultEimPath != null) - setEimPathInEnvVar(defaultEimPath.toString()); + if (defaultEimPath != null && Files.exists(defaultEimPath)) { + setEimPathInEnvVar(defaultEimPath.toString()); + }
70-81: Avoid null returns from loadEimJson() on IO errors.Null forces all callers to remember a null-check and obscures the error. Prefer surfacing an IStatus/CoreException or returning Optional. If changing the signature isn’t viable now, at least document the null contract in Javadoc and centralize handling at call sites.
Can you confirm call sites handle null defensively? If not, I can propose a follow-up that wraps this in a Result/IStatus.
40-40: Make idfEnvironmentVariables final.It’s initialized once and not reassigned; marking final clarifies intent.
- private IDFEnvironmentVariables idfEnvironmentVariables; + private final IDFEnvironmentVariables idfEnvironmentVariables;
39-46: Typo in type name: EimIdfConfiguratinParser.If feasible in this release line, consider renaming to EimIdfConfigurationParser for consistency. Otherwise, note a tech-debt task for later to avoid propagating the typo across APIs.
Would you like me to raise a follow-up issue to track this rename and the ripple changes?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
bundles/com.espressif.idf.core/META-INF/MANIFEST.MF(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/Messages.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/exceptions/EimVersionMismatchException.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/vo/EimJson.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EimButtonLaunchListener.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java(1 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/manager/pages/ESPIDFMainTablePage.java(11 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- bundles/com.espressif.idf.core/META-INF/MANIFEST.MF
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/messages.properties
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.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/EimButtonLaunchListener.java
🧰 Additional context used
🧬 Code graph analysis (6)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/exceptions/EimVersionMismatchException.java (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-141)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/Messages.java (1)
Messages(6-30)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java (11)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.java (1)
EimIdfConfiguratinParser(15-58)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (1)
SetupToolsInIde(53-474)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/exceptions/EimVersionMismatchException.java (1)
EimVersionMismatchException(15-43)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java (1)
EimJson(8-71)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.java (1)
EimJsonStateChecker(22-67)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/GlobalModalLock.java (1)
GlobalModalLock(13-44)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/IDFConsole.java (1)
IDFConsole(27-100)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/EclipseHandler.java (1)
EclipseHandler(21-79)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/ESPIDFManagerEditor.java (1)
ESPIDFManagerEditor(23-84)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/EimEditorInput.java (1)
EimEditorInput(9-93)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (1)
ESPIDFMainTablePage(61-589)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/Messages.java (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java (1)
Messages(15-47)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/Messages.java (1)
Messages(9-86)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (11)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-141)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.java (1)
EimIdfConfiguratinParser(15-58)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (1)
SetupToolsInIde(53-474)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java (1)
ToolInitializer(36-175)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/exceptions/EimVersionMismatchException.java (1)
EimVersionMismatchException(15-43)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (1)
ToolsUtility(25-101)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java (1)
EimJson(8-71)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IdfInstalled.java (1)
IdfInstalled(5-80)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/IDFConsole.java (1)
IDFConsole(27-100)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EimButtonLaunchListener.java (1)
EimButtonLaunchListener(45-227)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/SetupToolsJobListener.java (1)
SetupToolsJobListener(28-88)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.java (7)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-141)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.java (1)
EimIdfConfiguratinParser(15-58)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/exceptions/EimVersionMismatchException.java (1)
EimVersionMismatchException(15-43)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java (1)
EimJson(8-71)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/EclipseHandler.java (1)
EclipseHandler(21-79)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/ESPIDFManagerEditor.java (1)
ESPIDFManagerEditor(23-84)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/EimEditorInput.java (1)
EimEditorInput(9-93)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java (6)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePlugin.java (1)
IDFCorePlugin(20-77)bundles/com.espressif.idf.core/src/com/espressif/idf/core/ProcessBuilderFactory.java (1)
ProcessBuilderFactory(16-163)bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-141)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/exceptions/EimVersionMismatchException.java (1)
EimVersionMismatchException(15-43)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java (1)
EimJson(8-71)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/StringUtil.java (1)
StringUtil(11-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_linux
- GitHub Check: build_windows
🔇 Additional comments (5)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.java (1)
42-48: Preserve fallback EimJson; avoid overwriting with null from parserParser may return null (missing file). Keep the pre-created EimJson unless parsed != null.
- EimIdfConfiguratinParser eimIdfConfiguratinParser = new EimIdfConfiguratinParser(); - eimJson = eimIdfConfiguratinParser.getEimJson(true); + EimIdfConfiguratinParser parser = new EimIdfConfiguratinParser(); + EimJson parsed = parser.getEimJson(true); + if (parsed != null) { + eimJson = parsed; + }bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java (1)
156-160: Console icon URL null → NPE in IDFConsolePassing null imageUrl leads to ImageDescriptor.createFromURL(null) NPE in IDFConsole.findConsole. Provide a non-null icon URL or make IDFConsole null‑tolerant.
Two options:
- Pass a valid URL here (e.g., UIPlugin.getDefault().getBundle().getEntry("icons/...")).
- Or update IDFConsole to guard null:
Additional change outside this file (IDFConsole.findConsole):
MessageConsole myConsole = new MessageConsole( name, imageURL != null ? ImageDescriptor.createFromURL(imageURL) : ImageDescriptor.getMissingImageDescriptor() );I can prep a patch for IDFConsole if you prefer that route.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (1)
398-404: Fix unsafe type/null check before castCurrent guard allows non-IdfInstalled through, causing ClassCastException.
- if ((!(cell.getElement() instanceof IdfInstalled)) && cell.getElement() == null) - return; + if (!(cell.getElement() instanceof IdfInstalled)) + return;bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/exceptions/EimVersionMismatchException.java (1)
23-24: NLS keys present and wired — resolved
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/messages.properties defines EimVersionMismatchExceptionMessage and EimVersionMismatchExceptionMessageTitle; bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/Messages.java exposes the fields and EimVersionMismatchException references them.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/Messages.java (1)
10-20: LGTM — properties keys presentClass wiring looks correct; all declared keys in bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/Messages.java are present in bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/messages.properties.
| ProcessBuilderFactory processBuilderFactory = new ProcessBuilderFactory(); | ||
| IStatus status = processBuilderFactory.runInBackground(commands, org.eclipse.core.runtime.Path.ROOT, | ||
| System.getenv()); | ||
|
|
||
| Logger.log(status.getMessage()); | ||
| return status; |
There was a problem hiding this comment.
Fix export flow: use a sane working dir, pass merged env, and persist the “exported” flag.
- Path.ROOT resolves to an empty string (“”) as cwd; prefer null to inherit the current process cwd.
- Child process env ignores plugin-managed vars; merge EIM_PATH from IDFEnvironmentVariables.
- After a successful import, set and flush OLD_CONFIG_EXPORTED_FLAG; otherwise, isOldConfigExported() will never turn true.
Apply this diff:
- ProcessBuilderFactory processBuilderFactory = new ProcessBuilderFactory();
- IStatus status = processBuilderFactory.runInBackground(commands, org.eclipse.core.runtime.Path.ROOT,
- System.getenv());
-
- Logger.log(status.getMessage());
- return status;
+ ProcessBuilderFactory processBuilderFactory = new ProcessBuilderFactory();
+ // Merge system env with plugin-managed EIM_PATH so the child sees IDE-set values
+ Map<String, String> env = new HashMap<>(System.getenv());
+ String eimPathEnv = idfEnvironmentVariables.getEnvValue(IDFEnvironmentVariables.EIM_PATH);
+ if (!StringUtil.isEmpty(eimPathEnv)) {
+ env.put(IDFEnvironmentVariables.EIM_PATH, eimPathEnv);
+ }
+ IStatus status = processBuilderFactory.runInBackground(commands, null, env);
+ Logger.log(status.getMessage());
+ if (status.isOK()) {
+ preferences.putBoolean(EimConstants.OLD_CONFIG_EXPORTED_FLAG, true);
+ try {
+ preferences.flush();
+ } catch (BackingStoreException e) {
+ Logger.log(e);
+ }
+ }
+ return status;And add required imports:
+import java.util.HashMap;
+import java.util.Map;
+import org.osgi.service.prefs.BackingStoreException;📝 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.
| ProcessBuilderFactory processBuilderFactory = new ProcessBuilderFactory(); | |
| IStatus status = processBuilderFactory.runInBackground(commands, org.eclipse.core.runtime.Path.ROOT, | |
| System.getenv()); | |
| Logger.log(status.getMessage()); | |
| return status; | |
| ProcessBuilderFactory processBuilderFactory = new ProcessBuilderFactory(); | |
| // Merge system env with plugin-managed EIM_PATH so the child sees IDE-set values | |
| Map<String, String> env = new HashMap<>(System.getenv()); | |
| String eimPathEnv = idfEnvironmentVariables.getEnvValue(IDFEnvironmentVariables.EIM_PATH); | |
| if (!StringUtil.isEmpty(eimPathEnv)) { | |
| env.put(IDFEnvironmentVariables.EIM_PATH, eimPathEnv); | |
| } | |
| IStatus status = processBuilderFactory.runInBackground(commands, null, env); | |
| Logger.log(status.getMessage()); | |
| if (status.isOK()) { | |
| preferences.putBoolean(EimConstants.OLD_CONFIG_EXPORTED_FLAG, true); | |
| try { | |
| preferences.flush(); | |
| } catch (BackingStoreException e) { | |
| Logger.log(e); | |
| } | |
| } | |
| return status; |
| ProcessBuilderFactory processBuilderFactory = new ProcessBuilderFactory(); | |
| IStatus status = processBuilderFactory.runInBackground(commands, org.eclipse.core.runtime.Path.ROOT, | |
| System.getenv()); | |
| Logger.log(status.getMessage()); | |
| return status; | |
| import java.util.HashMap; | |
| import java.util.Map; | |
| import org.osgi.service.prefs.BackingStoreException; |
| versionColumn = new TableViewerColumn(tableViewer, SWT.NONE); | ||
| versionColumn.getColumn().setText(Messages.EspIdfManagerVersionCol); | ||
| versionColumn.setLabelProvider(new IdfManagerTableColumnLabelProvider()); | ||
| setComparatorForCols(versionColumn, colIndex++); | ||
| tableColumnLayout.setColumnData(versionColumn.getColumn(), new ColumnWeightData(3, 50, true)); | ||
|
|
||
| nameColumn = new TableViewerColumn(tableViewer, SWT.NONE); | ||
| nameColumn.getColumn().setText(Messages.EspIdfManagerNameCol); | ||
| nameColumn.setLabelProvider(new IdfManagerTableColumnLabelProvider()); | ||
| setComparatorForCols(nameColumn, colIndex++); | ||
| tableColumnLayout.setColumnData(nameColumn.getColumn(), new ColumnWeightData(3, 50, true)); | ||
|
|
||
| locationColumn = new TableViewerColumn(tableViewer, SWT.NONE); | ||
| locationColumn.getColumn().setText(Messages.EspIdfManagerLocationCol); | ||
| locationColumn.setLabelProvider(new IdfManagerTableColumnLabelProvider()); | ||
| setComparatorForCols(locationColumn, colIndex++); | ||
| tableColumnLayout.setColumnData(locationColumn.getColumn(), new ColumnWeightData(10, 100, true)); | ||
|
|
There was a problem hiding this comment.
Sorting mismapped: headers sort wrong fields; disable Version sort
Clicks on Version/Name/Location currently sort Name/Path/Active. Map indices correctly and avoid Version sorting to prevent heavy calls.
versionColumn = new TableViewerColumn(tableViewer, SWT.NONE);
versionColumn.getColumn().setText(Messages.EspIdfManagerVersionCol);
versionColumn.setLabelProvider(new IdfManagerTableColumnLabelProvider());
- setComparatorForCols(versionColumn, colIndex++);
+ // Intentionally not sortable to avoid expensive version resolution during compare
tableColumnLayout.setColumnData(versionColumn.getColumn(), new ColumnWeightData(3, 50, true));
nameColumn = new TableViewerColumn(tableViewer, SWT.NONE);
nameColumn.getColumn().setText(Messages.EspIdfManagerNameCol);
nameColumn.setLabelProvider(new IdfManagerTableColumnLabelProvider());
- setComparatorForCols(nameColumn, colIndex++);
+ setComparatorForCols(nameColumn, 0);
tableColumnLayout.setColumnData(nameColumn.getColumn(), new ColumnWeightData(3, 50, true));
locationColumn = new TableViewerColumn(tableViewer, SWT.NONE);
locationColumn.getColumn().setText(Messages.EspIdfManagerLocationCol);
locationColumn.setLabelProvider(new IdfManagerTableColumnLabelProvider());
- setComparatorForCols(locationColumn, colIndex++);
+ setComparatorForCols(locationColumn, 1);
tableColumnLayout.setColumnData(locationColumn.getColumn(), new ColumnWeightData(10, 100, true)); switch (propertyIndex)
{
- case 0:
- rc = p1.getName().compareTo(p2.getName());
+ case 0:
+ rc = p1.getName().compareTo(p2.getName());
break;
- case 1:
- rc = p1.getPath().compareTo(p2.getPath());
+ case 1:
+ rc = p1.getPath().compareTo(p2.getPath());
break;
- case 2:
- Boolean p1State = ToolsUtility.isIdfInstalledActive(p1);
- Boolean p2State = ToolsUtility.isIdfInstalledActive(p2);
- rc = p1State.compareTo(p2State);
- break;
default:
break;
}Follow-up: if you want Version sorting later, compute and cache versions in the model to avoid UI freezes.
Also applies to: 559-586
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java
around lines 298-315 (and similarly 559-586), the table column comparators are
mismapped so clicking Version/Name/Location sorts the wrong fields and the
Version column triggers expensive work; fix by remapping the
setComparatorForCols calls to the correct model field indices (ensure Name ->
name index, Location -> path/location index, Active -> active index) and remove
or disable the comparator registration for the Version column (do not call
setComparatorForCols for version) to prevent heavy UI calls; if Version sorting
is desired later, compute and cache version values in the model and use those
cached values in the comparator to avoid UI freezes.
| private MessageConsoleStream getConsoleStream(boolean errorStream) | ||
| { | ||
| IDFConsole idfConsole = new IDFConsole(); | ||
| return idfConsole.getConsoleStream(Messages.IDFToolsHandler_ToolsManagerConsole, null, errorStream, true); | ||
| } |
There was a problem hiding this comment.
Console icon URL null → NPE in IDFConsole (same issue here)
Provide a non-null icon URL to avoid NPE in IDFConsole.
- return idfConsole.getConsoleStream(Messages.IDFToolsHandler_ToolsManagerConsole, null, errorStream, true);
+ return idfConsole.getConsoleStream(
+ Messages.IDFToolsHandler_ToolsManagerConsole,
+ UIPlugin.getDefault().getBundle().getEntry(RELOAD_ICON),
+ errorStream,
+ true
+ );Alternatively, harden IDFConsole to handle null URLs as noted in the other comment.
📝 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 MessageConsoleStream getConsoleStream(boolean errorStream) | |
| { | |
| IDFConsole idfConsole = new IDFConsole(); | |
| return idfConsole.getConsoleStream(Messages.IDFToolsHandler_ToolsManagerConsole, null, errorStream, true); | |
| } | |
| private MessageConsoleStream getConsoleStream(boolean errorStream) | |
| { | |
| IDFConsole idfConsole = new IDFConsole(); | |
| return idfConsole.getConsoleStream( | |
| Messages.IDFToolsHandler_ToolsManagerConsole, | |
| UIPlugin.getDefault().getBundle().getEntry(RELOAD_ICON), | |
| errorStream, | |
| true | |
| ); | |
| } |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java
around lines 526 to 530, the call to idfConsole.getConsoleStream(...) passes
null for the icon URL which leads to an NPE in IDFConsole; replace the null with
a valid non-null URL to an icon resource (e.g., obtain the URL from the plugin
bundle or an ImageDescriptor from your Activator or plugin classloader) so the
console receives a real icon URL, or alternatively modify IDFConsole to safely
handle a null icon URL—prefer supplying a concrete resource URL from the plugin
to fix the immediate NPE.
| loadEimJson(); | ||
| if (eimJson.getIdfInstalled().size() == 1) | ||
| { | ||
| // only one entry in eimJson so we can simply refresh the IDE environment with that. | ||
| setupToolsInIde(); | ||
| } | ||
| else |
There was a problem hiding this comment.
Null-safety: eimJson may be null when file is missing
Parser can return null; guard before dereferencing getIdfInstalled().
- loadEimJson();
- if (eimJson.getIdfInstalled().size() == 1)
+ loadEimJson();
+ if (eimJson == null || eimJson.getIdfInstalled() == null)
+ {
+ return;
+ }
+ if (eimJson.getIdfInstalled().size() == 1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| loadEimJson(); | |
| if (eimJson.getIdfInstalled().size() == 1) | |
| { | |
| // only one entry in eimJson so we can simply refresh the IDE environment with that. | |
| setupToolsInIde(); | |
| } | |
| else | |
| loadEimJson(); | |
| if (eimJson == null || eimJson.getIdfInstalled() == null) | |
| { | |
| return; | |
| } | |
| if (eimJson.getIdfInstalled().size() == 1) | |
| { | |
| // only one entry in eimJson so we can simply refresh the IDE environment with that. | |
| setupToolsInIde(); | |
| } | |
| else |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java
around lines 84-90, the code calls loadEimJson() and immediately dereferences
eimJson.getIdfInstalled() but loadEimJson() can return null; add a null-check
right after loadEimJson() and handle the null case (e.g., log a warning and
return/skip setup, or treat as empty configuration) before accessing
getIdfInstalled() so you never dereference a null eimJson.
* Initial integration 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. * Initial first flow with successful installation and loading of env in IDE with EIM * constants moved to single place and added refresh function * minor cleanup and logging for user * IEP-1334: code cleanup (Active Review) (#1125) * cleanup of unused classes and code * String internalization * update to ci file * ci error fixed * ci error fix * ci fix * fixing ci error * ci error fix * ci error fix * ci syntax fix * ci syntax indentation fix * ci syntax fix * ci syntax fix * ci fixed * ci fixed2 * ci trigger * workflow trigger fix * cleanup * ci deps isntalled * eim fix * using eim action to install * using Petr's fork * switched back to official action repo * Revert "switched back to official action repo" This reverts commit f8cd7a7. * trying to fix action for eim * trying with petrs fork again * ci fix * back to espressif * name fixed * updated url for eim * old config export handling * fixing tests env setup * logging to verify skip tests * fixing POSIX path resolution * activation script properly sourced * added test run variable in test pom directly * adding cache option for the maven * file name updated * increasing timeout for tests * test fixes and removing redundant and outdated tests * cleanup and update for user messages * updated to handle activation of single esp-idf from eim * removing unwanted test for fixing failures * increased timeout for tests * ci updated for the release branch trigger as well * ci error fix * fix for sdkconfig server * cleaning up the idf_tools_path configuration in the IDE * added the option to include homebrew paths on macos * changes for guide link * Guidance link added to NLS * fix: Launch ESP-IDF Manager view even there is no eim_idf.json file found * fix: Don't launch multiple ESP-IDF Manager editors * fix: Update the msg and convert it to MessageDialog * fix: notify IDF not found while opening Manager view * fix: java.lang.UnsupportedOperationException * fix: File Not found issue and others * updated code for eim watcher added Next commits will do some refactoring and then implement the logic to handle these changes * refactored startup classes * initial base level logic for file watcher 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 * missing copyright added --------- Co-authored-by: Kondal Kolipaka <[email protected]>
* automatic refresh added * switched to message dialog * improved null checks * file watcher added first
…df.json (#1218) * eim auto import for old config added
* IEP-1552: Fix for system python executable
IEP-1554: Launch and Download of EIM added --------- Co-authored-by: Kondal Kolipaka <[email protected]>
* IEP-1557: Documentation for the ESP-IDF Manager * version.js added
* IEP-1585: Fixed eim old config import handling
fix: added global modal lock to avoid multiple modals
* IEP-1583: Closing old editor window when old workspace is found * fix: address NPE while launching --------- Co-authored-by: Kondal Kolipaka <[email protected]>
This reverts commit 8b6303a.
* IEP-1603: version handling for json file from eim
alirana01
force-pushed
the
release/v4.0.0
branch
from
3875d67 to
f565735
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (31)
tests/com.espressif.idf.ui.test/configs/default-test-linux.properties (1)
10-10: Unresolved: Hardcoded IDF path inconsistent with v5.4 in PR and brittle vs Windows config.This issue was flagged in the previous review but remains unaddressed. The change has actually worsened it:
- Path pinned to v5.3, but PR objectives mention CI using v5.4—version mismatch will skew test runs and may break release build validation.
- Absolute self-hosted runner path is fragile and repo/runner layout-dependent, unlike the Windows config which uses a workspace-agnostic pattern.
- The
/v5.3/esp-idfsuffix added in this rebase increases brittleness rather than fixing it.Align with the Windows file pattern or use env expansion. Recommend one of:
- default.env.esp.idf.path=/opt/actions-runner/_work/idf-eclipse-plugin/idf-eclipse-plugin/dependencies/idf-tools/v5.3/esp-idf + default.env.esp.idf.path={0}/dependencies/idf-tools/v5.4/esp-idfOr, for workspace/version-agnostic resolution (preferred):
- default.env.esp.idf.path=/opt/actions-runner/_work/idf-eclipse-plugin/idf-eclipse-plugin/dependencies/idf-tools/v5.3/esp-idf + default.env.esp.idf.path={0}/dependencies/idf-tools/v5.4/esp-idfVerify: Does DefaultPropertyFetcher (referenced in prior review at DefaultPropertyFetcher.java:75) support
{0}expansion for GITHUB_WORKSPACE? If so, this pattern also makes the version number explicit without hardcoding the full path.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/GlobalModalLock.java (1)
21-43: Semaphore leak on UI/display errors; make release unconditional and thread daemon.The semaphore is only released inside the
syncExeccallback's finally block (line 34). IfDisplay.getDefault()returns null, is disposed, or ifsyncExec()throws before executing the lambda, the lock is never released, causing permanent deadlock for all subsequentshowModalcalls. Additionally, the thread should be marked as daemon to prevent blocking JVM shutdown.Apply this diff to fix the semaphore leak and daemon thread issues:
- public static <T> void showModal(Supplier<T> dialogSupplier, Consumer<T> callback) - { - new Thread(() -> { - try - { - lock.acquire(); - Display.getDefault().syncExec(() -> { - try - { - T result = dialogSupplier.get(); - callback.accept(result); - } finally - { - lock.release(); - } - }); - } - catch (InterruptedException e) - { - Thread.currentThread().interrupt(); - } - }, "GlobalModalLock-DialogThread").start(); //$NON-NLS-1$ - } + public static <T> void showModal(Supplier<T> dialogSupplier, Consumer<T> callback) + { + Thread t = new Thread(() -> { + boolean acquired = false; + try + { + lock.acquire(); + acquired = true; + Display display = Display.getDefault(); + if (display == null || display.isDisposed()) + { + return; + } + display.syncExec(() -> { + T result = dialogSupplier.get(); + callback.accept(result); + }); + } + catch (InterruptedException e) + { + Thread.currentThread().interrupt(); + } + finally + { + if (acquired) + { + lock.release(); + } + } + }, "GlobalModalLock-DialogThread"); //$NON-NLS-1$ + t.setDaemon(true); + t.start(); + }.github/workflows/ci_release.yml (1)
6-7: Tag patterns use regex syntax but GHA expects glob — will not match intended tags.This is the same issue flagged in the prior review. GitHub Actions tag triggers use glob patterns, not regex. The literal dots and plus signs won't match version tags as intended.
Apply this fix:
push: tags: - - "v[0-9]+.[0-9]+.[0-9]+" - - "v[0-9]+.[0-9]+.[0-9]+-beta.*" + - "v*.*.*" + - "v*.*.*-beta*".github/workflows/ci.yml (1)
25-27: Upgrade deprecated setup-python@v2 to v5.This is the same issue flagged in the prior review. The
actions/setup-python@v2action is deprecated and will break on GitHub runners. Upgrade tov5to ensure compatibility.Apply this fix:
- name: Set up Python - uses: actions/setup-python@v2 + uses: actions/setup-python@v5 with: - python-version: '3.10' + python-version: '3.10'tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java (3)
47-48: Wait for the ESP-IDF Manager editor before accessing it.This issue was previously flagged: accessing the editor immediately is flaky if it hasn't opened yet. The race condition can cause intermittent test failures.
Apply this diff to add a robust wait:
- - SWTBotEditor espIdfManagerView = bot.editorByTitle("ESP-IDF Manager"); + + bot.waitUntil(org.eclipse.swtbot.eclipse.finder.waits.Conditions.waitForEditor(withPartName("ESP-IDF Manager"))); + SWTBotEditor espIdfManagerView = bot.editorByTitle("ESP-IDF Manager");
49-49: Avoid index-based radio selection; use a stable label.This issue was previously flagged: index 0 is brittle and will break if radio buttons are reordered or new ones are added.
Apply this diff (adjust the label to match the exact UI text):
- espIdfManagerView.bot().radio(0).click(); + espIdfManagerView.bot().radio("Use existing ESP-IDF directory from file system").click(); // TODO: verify exact label textIf the exact label text differs, inspect the ESP-IDF Manager UI to determine the correct string.
51-54: Reduce excessive timeout (≈27.5 hours) to a sane default.This issue was previously flagged: the timeout of 99000000 ms (approximately 27.5 hours) is excessive and will wedge CI if the tools setup hangs or fails silently.
Apply this diff to use a reasonable timeout:
- TestWidgetWaitUtility.waitUntilViewContains(bot, "Tools Setup complete", consoleView, 99000000); + TestWidgetWaitUtility.waitUntilViewContains(bot, "Tools Setup complete", consoleView, 900_000); // 15 minOptional: Make it configurable via a property:
private static int getToolsSetupTimeoutMs() { String value = DefaultPropertyFetcher.getProperty("tools.setup.timeout.ms", "900000"); try { return Integer.parseInt(value); } catch (NumberFormatException e) { return 900_000; // fallback } }Then use:
TestWidgetWaitUtility.waitUntilViewContains(bot, "Tools Setup complete", consoleView, getToolsSetupTimeoutMs());bundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/JsonConfigServer.java (2)
108-109: Handle missing Python executable explicitly.
pythonPathcan be null or empty, which will cause a runtime failure when launching the process. This issue was flagged in a previous review but remains unresolved.Apply this diff to validate pythonPath:
String pythonPath = IDFUtil.getIDFPythonEnvPath(); + if (StringUtil.isEmpty(pythonPath)) + { + throw new FileNotFoundException("Python executable not found. Check IDF_PYTHON_ENV_PATH / PATH."); //$NON-NLS-1$ + } arguments.add(pythonPath);
141-171: Null-safety in PATH preparation.
idfExportPath(line 144) may be null or empty. Callingreplace()on lines 159-160 will throw an NPE, and putting a null/empty value intosystemEnvat line 163 will corrupt the PATH entirely. This issue was flagged in a previous review but remains unresolved.Apply this diff to guard against null:
private void loadIdfPathWithSystemPath(Map<String, String> systemEnv) { IDFEnvironmentVariables idfEnvironmentVariables = new IDFEnvironmentVariables(); String idfExportPath = idfEnvironmentVariables.getEnvValue(IDFEnvironmentVariables.PATH); String pathVar = "PATH"; // for Windows //$NON-NLS-1$ String pathEntry = systemEnv.get(pathVar); // $NON-NLS-1$ if (pathEntry == null) { pathVar = "Path"; //$NON-NLS-1$ pathEntry = systemEnv.get(pathVar); if (pathEntry == null) // no idea { Logger.log(new Exception("No PATH found in the system environment variables")); //$NON-NLS-1$ } } + // If no export PATH from EIM, keep the system PATH as-is + if (StringUtil.isEmpty(idfExportPath)) + { + if (!StringUtil.isEmpty(pathEntry)) + { + systemEnv.put(pathVar, pathEntry); + } + for (Entry<String, String> entry : idfEnvironmentVariables.getEnvMap().entrySet()) + { + if (entry.getKey().equals(IDFEnvironmentVariables.PATH)) + continue; + systemEnv.put(entry.getKey(), entry.getValue()); + } + return; + } + if (!StringUtil.isEmpty(pathEntry)) { idfExportPath = idfExportPath.replace("$PATH", pathEntry); // macOS //$NON-NLS-1$ idfExportPath = idfExportPath.replace("%PATH%", pathEntry); // Windows //$NON-NLS-1$ } systemEnv.put(pathVar, idfExportPath); for (Entry<String, String> entry : idfEnvironmentVariables.getEnvMap().entrySet()) { if (entry.getKey().equals(IDFEnvironmentVariables.PATH)) continue; systemEnv.put(entry.getKey(), entry.getValue()); } }bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/EspressifGeneralStartup.java (1)
51-70: WrapPopupDialog.valueOfto avoid listener crashesUnrecognized property names still blow up the listener with
IllegalArgumentException, reproducing the previously raised issue. Please guard the conversion so unknown events are ignored instead of crashing the UI.- OpenDialogListenerSupport.getSupport().addPropertyChangeListener(evt -> { - PopupDialog popupDialog = PopupDialog.valueOf(evt.getPropertyName()); + OpenDialogListenerSupport.getSupport().addPropertyChangeListener(evt -> { + PopupDialog popupDialog; + try + { + popupDialog = PopupDialog.valueOf(evt.getPropertyName()); + } + catch (IllegalArgumentException ex) + { + Logger.log("Ignoring unknown dialog event: " + evt.getPropertyName(), true); //$NON-NLS-1$ + return; + } switch (popupDialog)bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/core/IDFMonitor.java (1)
165-186: Remove full-environment logging.Switching to
System.getenv()means we now dump the entire OS environment (tokens, secrets, PII) twice. Please drop or heavily redact these logs. This was flagged earlier and remains unresolved.Apply:
- Logger.log(environment.toString()); ... - Logger.log(environment.toString()); + Logger.log("Launching IDF Monitor with environment overrides: " + idfEnvMap.keySet());bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (2)
215-236: Guard against null and join paths safely.
idfPyEnvPath.strip()still NPEs when callers pass null/blank, and string concatenation ignores OS separators. Keep the safe fallback and build the search dir withPaths.get(...).- 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); + public static String getIDFPythonEnvPath(String idfPyEnvPath) + { + if (StringUtil.isEmpty(idfPyEnvPath)) + { + return findCommandFromBuildEnvPath(IDFConstants.PYTHON_CMD); + } + idfPyEnvPath = idfPyEnvPath.trim(); + String subdir = Platform.OS_WIN32.equals(Platform.getOS()) ? "Scripts" : "bin"; //$NON-NLS-1$ //$NON-NLS-2$ + String searchDir = Paths.get(idfPyEnvPath, subdir).toString(); + java.nio.file.Path commandPath = findCommand(IDFConstants.PYTHON_CMD, searchDir); + if (commandPath != null) + { + return commandPath.toFile().getAbsolutePath(); + } + return findCommandFromBuildEnvPath(IDFConstants.PYTHON_CMD); }
253-257: Reinstate python executable fallback.Only returning
PYTHON_EXE_PATHregresses all users relying on PATH discovery. Keep the old fallback togetIDFPythonEnvPath()when the env var is absent.- IDFEnvironmentVariables idfEnvironmentVariables = new IDFEnvironmentVariables(); - return idfEnvironmentVariables.getEnvValue(IDFEnvironmentVariables.PYTHON_EXE_PATH); + IDFEnvironmentVariables idfEnvironmentVariables = new IDFEnvironmentVariables(); + String pythonExe = idfEnvironmentVariables.getEnvValue(IDFEnvironmentVariables.PYTHON_EXE_PATH); + if (!StringUtil.isEmpty(pythonExe)) + { + return pythonExe; + } + return getIDFPythonEnvPath();bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.java (1)
42-63: Keep the default EimJson fallback.
getEimJson(true)can return null when the file is missing; assigning that toeimJsondrops the fallback and later code will NPE. Only replace the default when parsing succeeds.- EimIdfConfiguratinParser eimIdfConfiguratinParser = new EimIdfConfiguratinParser(); - eimJson = eimIdfConfiguratinParser.getEimJson(true); + EimIdfConfiguratinParser parser = new EimIdfConfiguratinParser(); + EimJson parsed = parser.getEimJson(true); + if (parsed != null) + { + eimJson = parsed; + }bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (4)
312-340: PATH variable replacement is fragile.Lines 329-330 use hardcoded string replacements for
$PATHand%PATH%which may not handle all edge cases (e.g., escaped variables, multiple occurrences).Make the replacement OS-specific and more robust:
if (!StringUtil.isEmpty(pathEntry)) { - idfExportPath = idfExportPath.replace("$PATH", pathEntry); // macOS //$NON-NLS-1$ - idfExportPath = idfExportPath.replace("%PATH%", pathEntry); // Windows //$NON-NLS-1$ + if (Platform.getOS().equals(Platform.OS_WIN32)) + { + idfExportPath = idfExportPath.replace("%PATH%", pathEntry); + } + else + { + idfExportPath = idfExportPath.replace("$PATH", pathEntry); + } }Based on past review comments.
164-222: Files.copy to /etc/udev/rules.d will fail without elevated privileges.Lines 198 and 208 attempt to write to
/etc/udev/rules.d/, which requires root privileges on Linux. This will throwAccessDeniedExceptionfor non-root users.Instead of direct copy, provide instructions for manual installation:
- Files.copy(source, target, StandardCopyOption.REPLACE_EXISTING); + // Provide instructions for manual copy + String copyCommand = String.format("sudo cp %s %s", source.toString(), target.toString()); + log("Please run the following command manually:\n" + copyCommand, IStatus.INFO); + MessageBox infoBox = new MessageBox(Display.getDefault().getActiveShell(), + SWT.ICON_INFORMATION | SWT.OK); + infoBox.setText("Manual Action Required"); + infoBox.setMessage("OpenOCD rules require elevated privileges.\nPlease run:\n" + copyCommand); + infoBox.open(); + return;Based on past review comments.
238-303: Process timeout is too short and doesn't terminate hung processes.The timeout at lines 268-285 uses a fixed 3-second wait (10 × 300ms) which may be insufficient for slow systems. Additionally, the process is not terminated if it times out, leaving it running indefinitely.
Use a proper timeout mechanism and terminate hung processes:
- int waitCount = 10; + int timeoutSeconds = 30; 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()); } - while (process.isAlive() && waitCount > 0) - { - try - { - Thread.sleep(300); - } - catch (InterruptedException e) - { - Logger.log(e); - } - waitCount--; - } - - if (waitCount == 0) + boolean completed = process.waitFor(timeoutSeconds, java.util.concurrent.TimeUnit.SECONDS); + if (!completed) { log("Process possibly stuck", IStatus.ERROR); //$NON-NLS-1$ + process.destroy(); + process.waitFor(5, java.util.concurrent.TimeUnit.SECONDS); + if (process.isAlive()) + { + process.destroyForcibly(); + } return Status.CANCEL_STATUS; }Based on past review comments.
406-413: Environment parser discards legitimate empty values.Line 411 filters out entries where
parts[1].isEmpty(), but environment variables can legitimately have empty values (e.g.,VAR=). This could lead to incorrect environment setup.Remove the empty-value filter:
private Map<String, String> parseEnvKeys(String activationScriptOutput) { return Arrays.stream(activationScriptOutput.split(System.lineSeparator())).map(String::trim) .filter(line -> line.contains("=")) //$NON-NLS-1$ .map(line -> line.split("=", 2)) //$NON-NLS-1$ - .filter(parts -> parts.length == 2 && !parts[0].isEmpty() && !parts[1].isEmpty()) + .filter(parts -> parts.length == 2 && !parts[0].isEmpty()) .collect(Collectors.toMap(parts -> parts[0].trim(), parts -> parts[1].trim())); }Based on past review comments.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EimButtonLaunchListener.java (1)
86-97: Null EIM_PATH can cause FileNotFoundException when launching.The code retrieves
EIM_PATHfrom environment variables but doesn't validate it before passing tolaunchEim(). TheisEimInstalled()check may return true based on default path existence, but the env var could still be null or blank, causinglaunchEim()to throwFileNotFoundException.Retrieve and validate the path before launching:
else { + String eimPath = idfEnvironmentVariables.getEnvValue(IDFEnvironmentVariables.EIM_PATH); + if (eimPath == null || eimPath.isBlank()) + { + java.nio.file.Path defaultPath = toolInitializer.getDefaultEimPath(); + if (defaultPath != null && java.nio.file.Files.exists(defaultPath)) + { + eimPath = defaultPath.toString(); + idfEnvironmentVariables.addEnvVariable(IDFEnvironmentVariables.EIM_PATH, eimPath); + } + } + if (eimPath == null || eimPath.isBlank()) + { + Logger.log("EIM path is not set and no default could be found."); //$NON-NLS-1$ + return; + } try { - Process process = eimLoader.launchEim(idfEnvironmentVariables.getEnvValue(IDFEnvironmentVariables.EIM_PATH)); + Process process = eimLoader.launchEim(eimPath); eimLoader.waitForEimClosure(process, EimButtonLaunchListener.this::refreshAfterEimClose); } catch (IOException e) { Logger.log(e); } }Based on past review comments.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/EimEditorInput.java (2)
13-13: Make FILE_NAME a proper constant.The field is used as a constant for identity in
equals()/hashCode(), but is declared as a mutable instance field. This is misleading and could cause issues if accidentally modified.Apply this fix:
- private String FILE_NAME = "idf_eim.json"; //$NON-NLS-1$ + private static final String FILE_NAME = "idf_eim.json"; //$NON-NLS-1$Based on past review comments.
44-60: Return non-null values from IEditorInput methods.
getName()andgetToolTipText()return null, which can cause NPEs in Eclipse framework code that assumes non-null return values.Apply this fix:
@Override public String getName() { - return null; + return FILE_NAME; } @Override public IPersistableElement getPersistable() { return null; } @Override public String getToolTipText() { - return null; + return "ESP-IDF Manager (" + FILE_NAME + ')'; //$NON-NLS-1$ }Based on past review comments.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java (2)
284-288: Console creation will NPE with null icon URL.
IDFConsole.findConsole()internally callsImageDescriptor.createFromURL(imageURL)which throws NPE when passed null. Multiple call sites in the codebase pass null.The preferred fix is to handle null defensively in
IDFConsole.findConsole()at line 83:- MessageConsole myConsole = new MessageConsole(name, ImageDescriptor.createFromURL(imageURL)); + MessageConsole myConsole = new MessageConsole(name, imageURL == null ? null : ImageDescriptor.createFromURL(imageURL));This fixes the issue for all callers. Alternatively, pass a valid icon URL here.
Based on past review comments.
326-342: Guard against null workbench window and page.
getActiveWorkbenchWindow()can return null, andgetActivePage()can also return null, leading to NPE when callingIDE.openEditor().Apply this fix:
private void openEspIdfManager(EimJson eimJson) { Display.getDefault().asyncExec(() -> { IWorkbenchWindow window = EclipseHandler.getActiveWorkbenchWindow(); + if (window == null || window.getActivePage() == null) + { + Logger.log("Cannot open ESP-IDF Manager. No active workbench window or active page."); + return; + } try { EimEditorInput input = new EimEditorInput(eimJson); input.setFirstStartup(true); IDE.openEditor(window.getActivePage(), input, ESPIDFManagerEditor.EDITOR_ID); IDFUtil.closeWelcomePage(window); } catch (PartInitException e) { Logger.log(e); } }); }Based on past review comments.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/SetupToolsJobListener.java (1)
39-41: No-arg constructor causes NPE in rollback logic.The no-arg constructor at line 39 leaves
setupToolsInIdenull, but line 58 unconditionally callssetupToolsInIde.rollback(), causing an NPE if the job fails.Either remove the no-arg constructor to force proper initialization, or null-guard the rollback:
Option 1 (preferred): Remove the no-arg constructor
- public SetupToolsJobListener() - { - }Option 2: Add null-guard
if (event.getResult().getSeverity() != IStatus.OK) { - // Rollback all the changes - setupToolsInIde.rollback(); + if (setupToolsInIde != null) + { + setupToolsInIde.rollback(); + } + else + { + Logger.log("Cannot rollback: setupToolsInIde is null"); + } }Based on past review comments.
Also applies to: 55-59
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java (2)
82-89: Null-check eimJson after loading.
loadEimJson()can leaveeimJsonnull if the file doesn't exist or parsing fails (seeEimIdfConfiguratinParser.load()which logs and returns early on missing file). DereferencingeimJson.getIdfInstalled()at line 85 will NPE.Apply this fix:
try { loadEimJson(); + if (eimJson == null || eimJson.getIdfInstalled() == null) + { + Logger.log("EIM JSON is null or has no installed IDFs"); + return; + } if (eimJson.getIdfInstalled().size() == 1) {Based on past review comments.
156-160: Console creation will NPE with null icon URL.Same issue as in
EspressifToolStartup.java:IDFConsole.findConsole()internally callsImageDescriptor.createFromURL(imageURL)which throws NPE when passed null.The preferred fix is to handle null defensively in
IDFConsole.findConsole(). See the comment onEspressifToolStartup.javafor the fix location.Based on past review comments.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (3)
400-405: Fix theIdfInstalledguard before casting.Line 400 uses
&&, so any non-null element that isn’t anIdfInstalledstill falls through and the cast on Line 404 throwsClassCastExceptionthe first time the viewer paints unexpected data. Replace the guard with an early return for every non-IdfInstalled(or null) element before the cast.- if ((!(cell.getElement() instanceof IdfInstalled)) && cell.getElement() == null) - return; - - IdfInstalled idfInstalled = (IdfInstalled) cell.getElement(); + Object element = cell.getElement(); + if (!(element instanceof IdfInstalled)) + { + return; + } + + IdfInstalled idfInstalled = (IdfInstalled) element;
526-530: Pass a real console icon to prevent NPE.
IDFConsole.findConsolecallsImageDescriptor.createFromURL(imageURL); handing itnull(Line 529) immediately throws, so the editor dies as soon as it tries to open the console. Supply an actual bundle entry (or guard for null inside IDFConsole).-import java.io.IOException; -import java.util.List; +import java.io.IOException; +import java.net.URL; +import java.util.List; @@ - IDFConsole idfConsole = new IDFConsole(); - return idfConsole.getConsoleStream(Messages.IDFToolsHandler_ToolsManagerConsole, null, errorStream, true); + IDFConsole idfConsole = new IDFConsole(); + URL consoleIcon = UIPlugin.getDefault().getBundle().getEntry(RELOAD_ICON); + return idfConsole.getConsoleStream(Messages.IDFToolsHandler_ToolsManagerConsole, consoleIcon, errorStream, true);
298-315: Re-align comparator indices (and skip Version sorting).With the current
colIndex++wiring, clicking “Version” sorts by Name, “Name” sorts by Path, and “Location” calls the Active-state comparator (case 2). Besides being wrong, Version sorting also re-triggers the heavy activation-script lookup discussed above. Please map explicit indices for Name/Location and leave Version unsortable until the value is cached.- int colIndex = 0; + final int nameSortIndex = 0; + final int locationSortIndex = 1; @@ - setComparatorForCols(versionColumn, colIndex++); + // Version column intentionally left unsortable to avoid blocking activation script calls @@ - setComparatorForCols(nameColumn, colIndex++); + setComparatorForCols(nameColumn, nameSortIndex); @@ - setComparatorForCols(locationColumn, colIndex++); + setComparatorForCols(locationColumn, locationSortIndex);- case 2: - Boolean p1State = ToolsUtility.isIdfInstalledActive(p1); - Boolean p2State = ToolsUtility.isIdfInstalledActive(p2); - rc = p1State.compareTo(p2State); - break;Also applies to: 565-576
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.java (1)
98-115: GuardwithPausedListenerswhen the watcher failed to start.If the constructor throws (e.g., directory creation fails),
Holder.INSTANCEstays null. The next call towithPausedListenershits Line 101 and dereferenceswatchService, instantly raising aNullPointerException. Add a null guard and just run the task (still logged) so clients don’t crash.public static void withPausedListeners(Runnable task) { EimJsonWatchService watchService = getInstance(); + if (watchService == null) + { + try + { + task.run(); + } + catch (Exception e) + { + Logger.log(e); + } + return; + } boolean wasPaused = watchService.paused;bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimLoader.java (1)
197-221: Handle non-admin macOS installs gracefullyCopying into
/Applicationsrequires admin rights; for a standard user this fails, leaving EIM unusable. Detect the writable target and fall back to~/Applications(creating it if necessary) so the install succeeds, and use that resolved path for logging, copying, and theEIM_PATHenvironment update.- Path targetAppPath = Paths.get("/Applications", appBundle.getName()); //$NON-NLS-1$ - - logMessage("Copying app to /Applications…\n"); //$NON-NLS-1$ - Process copyProcess = new ProcessBuilder("cp", "-R", appBundle.getAbsolutePath(), //$NON-NLS-1$ //$NON-NLS-2$ - targetAppPath.toString()).redirectErrorStream(true).start(); + Path systemApplications = Paths.get("/Applications"); //$NON-NLS-1$ + Path targetAppPath; + if (Files.isWritable(systemApplications)) + { + targetAppPath = systemApplications.resolve(appBundle.getName()); + } + else + { + Path userApplications = Paths.get(System.getProperty("user.home"), "Applications"); //$NON-NLS-1$ //$NON-NLS-2$ + Files.createDirectories(userApplications); + targetAppPath = userApplications.resolve(appBundle.getName()); + } + + logMessage("Copying app to " + targetAppPath.getParent() + "…\n"); //$NON-NLS-1$ + Process copyProcess = new ProcessBuilder("cp", "-R", appBundle.getAbsolutePath(), //$NON-NLS-1$ //$NON-NLS-2$ + targetAppPath.toString()).redirectErrorStream(true).start(); @@ - String eimPath = targetAppPath.resolve("Contents/MacOS/eim").toString(); //$NON-NLS-1$ + String eimPath = targetAppPath.resolve("Contents/MacOS/eim").toString(); //$NON-NLS-1$
🧹 Nitpick comments (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java (1)
55-56: Consider if activation is necessary before closing.The code activates the ESP-IDF Manager tab and then immediately closes it. If the activation is solely to ensure the tab can be closed reliably, verify whether it's needed or if
close()alone suffices.If activation is not required:
- bot.cTabItem("ESP-IDF Manager").activate(); - bot.cTabItem("ESP-IDF Manager").close(); + bot.cTabItem("ESP-IDF Manager").close();
| - name: Install ESP-IDF via eim | ||
| uses: espressif/install-esp-idf-action@v1 | ||
| with: | ||
| version: 'v5.4' |
There was a problem hiding this comment.
Fix YAML syntax error: remove tab character from step name.
Line 29 contains a tab character (\t) after the step name, which breaks YAML parsing. Replace with spaces.
- - name: Install ESP-IDF via eim
+ - name: Install ESP-IDF via eim
uses: espressif/install-esp-idf-action@v1
with:
version: 'v5.4'The functional change (EIM-driven ESP-IDF v5.4 installation) is good and aligns with the v4.0 release objectives. After fixing the syntax error, this step will replace the legacy manual cloning approach.
📝 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 | |
| uses: espressif/install-esp-idf-action@v1 | |
| with: | |
| version: 'v5.4' | |
| - name: Install ESP-IDF via eim | |
| uses: espressif/install-esp-idf-action@v1 | |
| with: | |
| version: 'v5.4' |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 29-29: syntax error: found character '\t' that cannot start any token
(syntax)
🤖 Prompt for AI Agents
In .github/workflows/ci.yml around lines 29 to 32, the step name line contains a
tab character after "Install ESP-IDF via eim" which breaks YAML parsing; replace
the tab with spaces so the "name:" value is a normal string, ensure all
indentation in this step uses spaces (not tabs), then save and re-run a YAML
lint/CI to confirm the file parses correctly.
| Map<String, String> envMap = new HashMap<String, String>(System.getenv()); | ||
| if (variables != null) | ||
| { | ||
| for (IEnvironmentVariable iEnvironmentVariable : variables) | ||
| { | ||
| String key = iEnvironmentVariable.getName(); | ||
| if (key.equals(IDFCorePreferenceConstants.IDF_TOOLS_PATH)) | ||
| { | ||
| continue; | ||
| } | ||
| String value = iEnvironmentVariable.getValue(); | ||
| envMap.put(key, value); | ||
| } |
There was a problem hiding this comment.
Restore IDF-specific environment enrichments.
Replacing the old base map with System.getenv() drops the IDF-specific additions from IDFUtil.getSystemEnv() (IDF_TOOLS_PATH preference and Homebrew PATH merge). Tools launched via this map will now miss the installed toolchain. Seed from IDFUtil.getSystemEnv() instead.
- Map<String, String> envMap = new HashMap<String, String>(System.getenv());
+ Map<String, String> envMap = new HashMap<>(IDFUtil.getSystemEnv());📝 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.
| Map<String, String> envMap = new HashMap<String, String>(System.getenv()); | |
| if (variables != null) | |
| { | |
| for (IEnvironmentVariable iEnvironmentVariable : variables) | |
| { | |
| String key = iEnvironmentVariable.getName(); | |
| if (key.equals(IDFCorePreferenceConstants.IDF_TOOLS_PATH)) | |
| { | |
| continue; | |
| } | |
| String value = iEnvironmentVariable.getValue(); | |
| envMap.put(key, value); | |
| } | |
| Map<String, String> envMap = new HashMap<>(IDFUtil.getSystemEnv()); | |
| if (variables != null) | |
| { | |
| for (IEnvironmentVariable iEnvironmentVariable : variables) | |
| { | |
| String key = iEnvironmentVariable.getName(); | |
| String value = iEnvironmentVariable.getValue(); | |
| envMap.put(key, value); | |
| } |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFEnvironmentVariables.java
around lines 137 to 145, the code initializes envMap from System.getenv() which
loses IDF-specific enrichments produced by IDFUtil.getSystemEnv(); change the
seed to new HashMap<String,String>(IDFUtil.getSystemEnv()) so the IDF_TOOLS_PATH
and Homebrew PATH merge are preserved, then continue to merge the provided
IEnvironmentVariable entries into that map as before.
| public String installAndLaunchDmg(Path dmgPath) throws IOException, InterruptedException | ||
| { | ||
| logMessage("Mounting DMG…\n"); //$NON-NLS-1$ | ||
| Process mountProcess = new ProcessBuilder("hdiutil", "attach", dmgPath.toString()) //$NON-NLS-1$ //$NON-NLS-2$ | ||
| .redirectErrorStream(true).start(); | ||
|
|
||
| int mountExit = mountProcess.waitFor(); | ||
| if (mountExit != 0) | ||
| throw new IOException("hdiutil attach failed (exit " + mountExit + "): " //$NON-NLS-1$ //$NON-NLS-2$ | ||
| + readProcessOutput(mountProcess)); | ||
|
|
||
| String volumePath = parseVolumePath(mountProcess.getInputStream()); | ||
| if (volumePath == null) | ||
| throw new IOException("Failed to mount DMG: volume path not found."); //$NON-NLS-1$ | ||
|
|
||
| File[] apps = new File(volumePath).listFiles((dir, name) -> name.endsWith(".app")); //$NON-NLS-1$ | ||
| if (apps == null || apps.length == 0) | ||
| throw new FileNotFoundException("No .app found inside DMG."); //$NON-NLS-1$ | ||
|
|
||
| File appBundle = apps[0]; | ||
| Path targetAppPath = Paths.get("/Applications", appBundle.getName()); //$NON-NLS-1$ | ||
|
|
||
| logMessage("Copying app to /Applications…\n"); //$NON-NLS-1$ | ||
| Process copyProcess = new ProcessBuilder("cp", "-R", appBundle.getAbsolutePath(), //$NON-NLS-1$ //$NON-NLS-2$ | ||
| targetAppPath.toString()).redirectErrorStream(true).start(); | ||
|
|
||
| int copyExit = copyProcess.waitFor(); | ||
| if (copyExit != 0) | ||
| throw new IOException("Copy failed (exit " + copyExit + "): " //$NON-NLS-1$ //$NON-NLS-2$ | ||
| + readProcessOutput(copyProcess)); | ||
|
|
||
| logMessage("Unmounting DMG…\n"); //$NON-NLS-1$ | ||
| Process detachProcess = new ProcessBuilder("hdiutil", "detach", volumePath) //$NON-NLS-1$ //$NON-NLS-2$ | ||
| .redirectErrorStream(true).start(); | ||
|
|
||
| int detachExit = detachProcess.waitFor(); | ||
| if (detachExit != 0) | ||
| throw new IOException("hdiutil detach failed (exit " + detachExit + "): " //$NON-NLS-1$ //$NON-NLS-2$ | ||
| + readProcessOutput(detachProcess)); |
There was a problem hiding this comment.
Always detach the DMG even on failure
If any step after hdiutil attach fails (no .app, copy error, etc.), we throw before calling hdiutil detach, leaving the volume mounted and blocking subsequent installs. Wrap the body in try/finally (storing the volumePath) so detach runs whenever the mount succeeds, and surface any detach failure in the error path.
- String volumePath = parseVolumePath(mountProcess.getInputStream());
- if (volumePath == null)
- throw new IOException("Failed to mount DMG: volume path not found."); //$NON-NLS-1$
-
- File[] apps = new File(volumePath).listFiles((dir, name) -> name.endsWith(".app")); //$NON-NLS-1$
- if (apps == null || apps.length == 0)
- throw new FileNotFoundException("No .app found inside DMG."); //$NON-NLS-1$
-
- File appBundle = apps[0];
- Path targetAppPath = Paths.get("/Applications", appBundle.getName()); //$NON-NLS-1$
-
- logMessage("Copying app to /Applications…\n"); //$NON-NLS-1$
- Process copyProcess = new ProcessBuilder("cp", "-R", appBundle.getAbsolutePath(), //$NON-NLS-1$ //$NON-NLS-2$
- targetAppPath.toString()).redirectErrorStream(true).start();
-
- int copyExit = copyProcess.waitFor();
- if (copyExit != 0)
- throw new IOException("Copy failed (exit " + copyExit + "): " //$NON-NLS-1$ //$NON-NLS-2$
- + readProcessOutput(copyProcess));
-
- logMessage("Unmounting DMG…\n"); //$NON-NLS-1$
- Process detachProcess = new ProcessBuilder("hdiutil", "detach", volumePath) //$NON-NLS-1$ //$NON-NLS-2$
- .redirectErrorStream(true).start();
-
- int detachExit = detachProcess.waitFor();
- if (detachExit != 0)
- throw new IOException("hdiutil detach failed (exit " + detachExit + "): " //$NON-NLS-1$ //$NON-NLS-2$
- + readProcessOutput(detachProcess));
+ String volumePath = parseVolumePath(mountProcess.getInputStream());
+ if (volumePath == null)
+ throw new IOException("Failed to mount DMG: volume path not found."); //$NON-NLS-1$
+
+ try
+ {
+ File[] apps = new File(volumePath).listFiles((dir, name) -> name.endsWith(".app")); //$NON-NLS-1$
+ if (apps == null || apps.length == 0)
+ throw new FileNotFoundException("No .app found inside DMG."); //$NON-NLS-1$
+
+ File appBundle = apps[0];
+ // copy logic (updated as per separate comment) here…
+ // ...
+ int copyExit = copyProcess.waitFor();
+ if (copyExit != 0)
+ throw new IOException("Copy failed (exit " + copyExit + "): " //$NON-NLS-1$ //$NON-NLS-2$
+ + readProcessOutput(copyProcess));
+ }
+ finally
+ {
+ logMessage("Unmounting DMG…\n"); //$NON-NLS-1$
+ Process detachProcess = new ProcessBuilder("hdiutil", "detach", volumePath) //$NON-NLS-1$ //$NON-NLS-2$
+ .redirectErrorStream(true).start();
+
+ int detachExit = detachProcess.waitFor();
+ if (detachExit != 0)
+ throw new IOException("hdiutil detach failed (exit " + detachExit + "): " //$NON-NLS-1$ //$NON-NLS-2$
+ + readProcessOutput(detachProcess));
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimLoader.java
around lines 178 to 216, the method mounts a DMG and can throw before calling
hdiutil detach, leaving the volume mounted; wrap the steps after successful
attach in a try/finally where the parsed volumePath is stored in a local
variable visible to the finally, and in finally call hdiutil detach if
volumePath != null (or if mount succeeded); if detach fails when cleaning up,
include its output/exit code in the thrown IOException (or add it as a
suppressed exception) so cleanup errors are surfaced while ensuring the DMG is
always attempted to be unmounted.
| private Path unzip(Path zipPath, Path destDir) throws IOException | ||
| { | ||
| Files.createDirectories(destDir); | ||
| Path firstExecutable = null; | ||
|
|
||
| try (ZipInputStream zis = new ZipInputStream(new FileInputStream(zipPath.toFile()))) | ||
| { | ||
| ZipEntry entry; | ||
| while ((entry = zis.getNextEntry()) != null) | ||
| { | ||
| Path newPath = destDir.resolve(entry.getName()); | ||
| if (entry.isDirectory()) | ||
| { | ||
| Files.createDirectories(newPath); | ||
| } | ||
| else | ||
| { | ||
| Files.createDirectories(newPath.getParent()); | ||
| Files.copy(zis, newPath, StandardCopyOption.REPLACE_EXISTING); | ||
| if (firstExecutable == null && Files.isRegularFile(newPath)) | ||
| { | ||
| newPath.toFile().setExecutable(true); | ||
| firstExecutable = newPath; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return firstExecutable != null ? firstExecutable : destDir; |
There was a problem hiding this comment.
Do not rely on ZIP entry order to find the executable
unzip() returns the first regular file it sees, assumes it’s the EIM binary, and marks it executable. ZIP entry order is undefined; on some builds the first file is a README or resource, so we would return the wrong path and the launch fails. Locate the actual binary by name (e.g., file name eim or .AppImage) and only then mark it executable, otherwise signal an error.
- Path firstExecutable = null;
+ Path executable = null;
@@
- if (firstExecutable == null && Files.isRegularFile(newPath))
- {
- newPath.toFile().setExecutable(true);
- firstExecutable = newPath;
- }
+ if (Files.isRegularFile(newPath))
+ {
+ String fileName = newPath.getFileName().toString();
+ if ("eim".equals(fileName) || fileName.endsWith(".AppImage")) //$NON-NLS-1$
+ {
+ newPath.toFile().setExecutable(true);
+ executable = newPath;
+ }
+ }
}
}
}
- return firstExecutable != null ? firstExecutable : destDir;
+ if (executable == null)
+ {
+ throw new IOException("Failed to locate extracted EIM executable in archive: " + zipPath); //$NON-NLS-1$
+ }
+ return executable;📝 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 Path unzip(Path zipPath, Path destDir) throws IOException | |
| { | |
| Files.createDirectories(destDir); | |
| Path firstExecutable = null; | |
| try (ZipInputStream zis = new ZipInputStream(new FileInputStream(zipPath.toFile()))) | |
| { | |
| ZipEntry entry; | |
| while ((entry = zis.getNextEntry()) != null) | |
| { | |
| Path newPath = destDir.resolve(entry.getName()); | |
| if (entry.isDirectory()) | |
| { | |
| Files.createDirectories(newPath); | |
| } | |
| else | |
| { | |
| Files.createDirectories(newPath.getParent()); | |
| Files.copy(zis, newPath, StandardCopyOption.REPLACE_EXISTING); | |
| if (firstExecutable == null && Files.isRegularFile(newPath)) | |
| { | |
| newPath.toFile().setExecutable(true); | |
| firstExecutable = newPath; | |
| } | |
| } | |
| } | |
| } | |
| return firstExecutable != null ? firstExecutable : destDir; | |
| private Path unzip(Path zipPath, Path destDir) throws IOException | |
| { | |
| Files.createDirectories(destDir); | |
| Path executable = null; | |
| try (ZipInputStream zis = new ZipInputStream(new FileInputStream(zipPath.toFile()))) | |
| { | |
| ZipEntry entry; | |
| while ((entry = zis.getNextEntry()) != null) | |
| { | |
| Path newPath = destDir.resolve(entry.getName()); | |
| if (entry.isDirectory()) | |
| { | |
| Files.createDirectories(newPath); | |
| } | |
| else | |
| { | |
| Files.createDirectories(newPath.getParent()); | |
| Files.copy(zis, newPath, StandardCopyOption.REPLACE_EXISTING); | |
| if (Files.isRegularFile(newPath)) | |
| { | |
| String fileName = newPath.getFileName().toString(); | |
| if ("eim".equals(fileName) || fileName.endsWith(".AppImage")) //$NON-NLS-1$ | |
| { | |
| newPath.toFile().setExecutable(true); | |
| executable = newPath; | |
| } | |
| } | |
| } | |
| } | |
| } | |
| if (executable == null) | |
| { | |
| throw new IOException("Failed to locate extracted EIM executable in archive: " + zipPath); //$NON-NLS-1$ | |
| } | |
| return executable; |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimLoader.java
around lines 522-549, the unzip method currently returns the first regular file
encountered (relying on ZIP entry order) and marks it executable; instead, after
extracting all entries scan the extracted tree for the actual runtime binary by
name (for example a file named exactly "eim" or files ending with ".AppImage"),
mark only that file executable and return its Path; if no matching binary is
found throw or propagate an IOException with a clear message; do not rely on
entry order or return a README/resource as the executable.
| t.start(); | ||
| } | ||
| catch (CoreException e) | ||
| catch (Exception e) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid catching overly broad Exception.
Catching Exception instead of specific exceptions like CoreException or IOException may hide bugs and make debugging harder.
Apply this diff to catch specific exceptions:
- catch (Exception e)
+ catch (CoreException | IOException e)
{
Logger.log(e);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/JsonConfigServer.java
around line 128, the code currently catches a broad Exception; replace this with
targeted catches for the specific exceptions that can be thrown (for example
CoreException and IOException — add separate catch blocks for each to handle/log
appropriately), and remove the catch(Exception e) blanket handler (optionally
keep a final catch for RuntimeException or Throwable only if you rethrow or log
critically). Ensure each catch logs meaningful context and the exception
details.
| @Override | ||
| public void earlyStartup() | ||
| { | ||
| hookDialogListeners(); | ||
| hookLaunchBarListeners(); | ||
| } | ||
|
|
||
| private void hookDialogListeners() | ||
| { | ||
| OpenDialogListenerSupport.getSupport().addPropertyChangeListener(evt -> { | ||
| PopupDialog popupDialog = PopupDialog.valueOf(evt.getPropertyName()); | ||
| switch (popupDialog) | ||
| { | ||
| case LOW_PARTITION_SIZE: | ||
| openLowPartitionSizeDialog(evt); | ||
| break; | ||
| case AVAILABLE_HINTS: | ||
| openAvailableHintsDialog(evt); | ||
| break; | ||
| case DISABLE_LAUNCHABAR_EVENTS: | ||
| disableLaunchBarEvents(); | ||
| break; | ||
| case ENABLE_LAUNCHBAR_EVENTS: | ||
| enableLaunchBarEvents(); | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| private void hookLaunchBarListeners() | ||
| { | ||
| launchBarListener = new LaunchBarListener(); | ||
| ResourcesPlugin.getWorkspace().addResourceChangeListener(new ResourceChangeListener(launchBarListener)); | ||
|
|
||
| ILaunchBarManager launchBarManager = Activator.getService(ILaunchBarManager.class); | ||
| launchBarManager.addListener(launchBarListener); | ||
| } | ||
|
|
||
| @SuppressWarnings("static-access") | ||
| private void disableLaunchBarEvents() | ||
| { | ||
| launchBarListener.setIgnoreTargetChange(true); | ||
| } | ||
|
|
||
| @SuppressWarnings("static-access") | ||
| private void enableLaunchBarEvents() | ||
| { | ||
| launchBarListener.setIgnoreTargetChange(false); | ||
| } | ||
|
|
There was a problem hiding this comment.
Initialize launchBarListener before handling dialog events
launchBarListener stays null until hookLaunchBarListeners runs. If a dialog event (e.g., DISABLE_LAUNCHABAR_EVENTS) fires in that window, disableLaunchBarEvents() dereferences null and crashes startup. Create the listener before subscribing, or guard against null, so early events cannot NPE.
@Override
public void earlyStartup()
{
- hookDialogListeners();
- hookLaunchBarListeners();
+ launchBarListener = new LaunchBarListener();
+ hookDialogListeners();
+ hookLaunchBarListeners();
}
private void hookLaunchBarListeners()
{
- launchBarListener = new LaunchBarListener();
ResourcesPlugin.getWorkspace().addResourceChangeListener(new ResourceChangeListener(launchBarListener));
ILaunchBarManager launchBarManager = Activator.getService(ILaunchBarManager.class);
- launchBarManager.addListener(launchBarListener);
+ if (launchBarManager != null)
+ {
+ launchBarManager.addListener(launchBarListener);
+ }
}📝 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.
| @Override | |
| public void earlyStartup() | |
| { | |
| hookDialogListeners(); | |
| hookLaunchBarListeners(); | |
| } | |
| private void hookDialogListeners() | |
| { | |
| OpenDialogListenerSupport.getSupport().addPropertyChangeListener(evt -> { | |
| PopupDialog popupDialog = PopupDialog.valueOf(evt.getPropertyName()); | |
| switch (popupDialog) | |
| { | |
| case LOW_PARTITION_SIZE: | |
| openLowPartitionSizeDialog(evt); | |
| break; | |
| case AVAILABLE_HINTS: | |
| openAvailableHintsDialog(evt); | |
| break; | |
| case DISABLE_LAUNCHABAR_EVENTS: | |
| disableLaunchBarEvents(); | |
| break; | |
| case ENABLE_LAUNCHBAR_EVENTS: | |
| enableLaunchBarEvents(); | |
| break; | |
| default: | |
| break; | |
| } | |
| }); | |
| } | |
| private void hookLaunchBarListeners() | |
| { | |
| launchBarListener = new LaunchBarListener(); | |
| ResourcesPlugin.getWorkspace().addResourceChangeListener(new ResourceChangeListener(launchBarListener)); | |
| ILaunchBarManager launchBarManager = Activator.getService(ILaunchBarManager.class); | |
| launchBarManager.addListener(launchBarListener); | |
| } | |
| @SuppressWarnings("static-access") | |
| private void disableLaunchBarEvents() | |
| { | |
| launchBarListener.setIgnoreTargetChange(true); | |
| } | |
| @SuppressWarnings("static-access") | |
| private void enableLaunchBarEvents() | |
| { | |
| launchBarListener.setIgnoreTargetChange(false); | |
| } | |
| @Override | |
| public void earlyStartup() | |
| { | |
| launchBarListener = new LaunchBarListener(); | |
| hookDialogListeners(); | |
| hookLaunchBarListeners(); | |
| } | |
| private void hookDialogListeners() | |
| { | |
| OpenDialogListenerSupport.getSupport().addPropertyChangeListener(evt -> { | |
| PopupDialog popupDialog = PopupDialog.valueOf(evt.getPropertyName()); | |
| switch (popupDialog) | |
| { | |
| case LOW_PARTITION_SIZE: | |
| openLowPartitionSizeDialog(evt); | |
| break; | |
| case AVAILABLE_HINTS: | |
| openAvailableHintsDialog(evt); | |
| break; | |
| case DISABLE_LAUNCHABAR_EVENTS: | |
| disableLaunchBarEvents(); | |
| break; | |
| case ENABLE_LAUNCHBAR_EVENTS: | |
| enableLaunchBarEvents(); | |
| break; | |
| default: | |
| break; | |
| } | |
| }); | |
| } | |
| private void hookLaunchBarListeners() | |
| { | |
| ResourcesPlugin.getWorkspace().addResourceChangeListener(new ResourceChangeListener(launchBarListener)); | |
| ILaunchBarManager launchBarManager = Activator.getService(ILaunchBarManager.class); | |
| if (launchBarManager != null) | |
| { | |
| launchBarManager.addListener(launchBarListener); | |
| } | |
| } | |
| @SuppressWarnings("static-access") | |
| private void disableLaunchBarEvents() | |
| { | |
| launchBarListener.setIgnoreTargetChange(true); | |
| } | |
| @SuppressWarnings("static-access") | |
| private void enableLaunchBarEvents() | |
| { | |
| launchBarListener.setIgnoreTargetChange(false); | |
| } |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/EspressifGeneralStartup.java
around lines 42-93, launchBarListener can be null when dialog events fire,
causing an NPE in disableLaunchBarEvents()/enableLaunchBarEvents(); initialize
launchBarListener before any dialog listeners are hooked (i.e., create the
LaunchBarListener instance and register it with the workspace/ILaunchBarManager
prior to calling hookDialogListeners()), or alternatively add a null-check in
disableLaunchBarEvents()/enableLaunchBarEvents() to return early if
launchBarListener is null; prefer moving the initialization so dialog handlers
can safely call the listener.
| private String getIdfVersion(IdfInstalled idfInstalled) | ||
| { | ||
| String version = ToolsUtility.getIdfVersion(idfInstalled, eimJson.getGitPath()); | ||
| return version; | ||
| } |
There was a problem hiding this comment.
Avoid running the activation script on the UI thread.
getIdfVersion (Line 424) calls ToolsUtility.getIdfVersion, which spawns an external process and waits for it to finish. Because update(ViewerCell) executes on the SWT UI thread, every table paint (initial load, hover, sort, refresh) now blocks on that process—effectively freezing the IDE for seconds. Pre-compute/cache versions off the UI thread (e.g., in a background Job when the table input loads) and only render the cached value here.
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java
around lines 422-426, getIdfVersion currently calls ToolsUtility.getIdfVersion
which spawns a blocking external process and runs on the SWT UI thread; change
the approach to avoid blocking by removing the direct call here and instead read
from a precomputed cache (e.g., a ConcurrentHashMap keyed by IdfInstalled or
path) and return the cached value or a placeholder; when the table input loads
or when an uncached entry is encountered, schedule a background Job/Executor
task to run ToolsUtility.getIdfVersion off the UI thread, store the result in
the cache, and then refresh the viewer cell/update the UI using
Display.asyncExec so the UI updates without blocking; ensure the cache is
thread-safe and handle errors/timeouts from the external process gracefully
(store an error/unknown marker) to avoid repeated blocking calls.
| EIMButtonDownloadText=Download and Launch EIM | ||
| EIMButtonLaunchText=Launch EIM | ||
| EimJsonChangedMsgTitle=ESP-IDF Installation Changed | ||
| EimJsonChangedMsgDetail=It seems that the ESP-IDF tools have been modified. If you just installed ESP-IDF we recommend refereshing via ESP-IDF Manager . Would you like to proceed with the update? |
There was a problem hiding this comment.
Fix typo in user-facing message
“refereshing” is misspelled and there’s an extra space before the period. Please correct it to keep the UI text polished.
-EimJsonChangedMsgDetail=It seems that the ESP-IDF tools have been modified. If you just installed ESP-IDF we recommend refereshing via ESP-IDF Manager . Would you like to proceed with the update?
+EimJsonChangedMsgDetail=It seems that the ESP-IDF tools have been modified. If you just installed ESP-IDF we recommend refreshing via ESP-IDF Manager. Would you like to proceed with the update?📝 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.
| EimJsonChangedMsgDetail=It seems that the ESP-IDF tools have been modified. If you just installed ESP-IDF we recommend refereshing via ESP-IDF Manager . Would you like to proceed with the update? | |
| EimJsonChangedMsgDetail=It seems that the ESP-IDF tools have been modified. If you just installed ESP-IDF we recommend refreshing via ESP-IDF Manager. Would you like to proceed with the update? |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties
around line 12, the user-facing message has a typo and extra space: change
"refereshing" to "refreshing" and remove the stray space before the period after
"ESP-IDF Manager" so the line reads with proper spelling and punctuation (e.g.,
"...we recommend refreshing via ESP-IDF Manager. Would you like to proceed with
the update?").
* bug fix after rebase * fix: fix unit failing unit test --------- Co-authored-by: Denys Almazov <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/zh_CN/additionalfeatures/writebinarytoflash.rst (1)
1-1: Clarify the localization strategy for Chinese documentation.This file includes the English documentation content, which means Chinese-speaking users will read English text. For a v4.0.0 release, please clarify whether this is:
- A temporary placeholder pending Chinese translation, or
- An intentional fallback to English documentation for this topic.
If this is a temporary measure, consider opening a follow-up issue to track the Chinese translation work. If other Chinese documentation files in the repository use the same include pattern, note this in the PR description for consistency.
Do you want me to verify whether the English source file exists, check if other Chinese documentation files follow this include pattern, or help open a tracking issue for future translation work?
docs/zh_CN/conf.py (1)
10-16: Import fallback mechanism looks good.The try-except pattern correctly handles importing
conf_commonwith a fallback path adjustment. This is a robust approach for the Sphinx configuration.Optionally, you can clean up the
noqadirectives sinceF401is not being triggered:try: - from conf_common import * # noqa: F403,F401 + from conf_common import * # noqa: F403 except ImportError: import os import sys sys.path.insert(0, os.path.abspath('../')) - from conf_common import * # noqa: F403,F401 + from conf_common import * # noqa: F403
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
.github/workflows/docs_build.yml(1 hunks).github/workflows/docs_production.yml(1 hunks)docs/en/installation.rst(1 hunks)docs/en/prerequisites.rst(1 hunks)docs/en/troubleshooting.rst(3 hunks)docs/zh_CN/additionalfeatures.rst(1 hunks)docs/zh_CN/additionalfeatures/appleveltracing.rst(1 hunks)docs/zh_CN/additionalfeatures/application-size-analysis.rst(1 hunks)docs/zh_CN/additionalfeatures/clangd_cdt_support.rst(1 hunks)docs/zh_CN/additionalfeatures/clangtoolchain.rst(1 hunks)docs/zh_CN/additionalfeatures/cmakeeditor.rst(1 hunks)docs/zh_CN/additionalfeatures/configureenvvariables.rst(1 hunks)docs/zh_CN/additionalfeatures/configuretoolchain.rst(1 hunks)docs/zh_CN/additionalfeatures/coredumpdebugging.rst(1 hunks)docs/zh_CN/additionalfeatures/dfu.rst(1 hunks)docs/zh_CN/additionalfeatures/esp-terminal.rst(1 hunks)docs/zh_CN/additionalfeatures/gdbstubdebugging.rst(1 hunks)docs/zh_CN/additionalfeatures/heaptracing.rst(1 hunks)docs/zh_CN/additionalfeatures/install-esp-components.rst(1 hunks)docs/zh_CN/additionalfeatures/lspeditor.rst(1 hunks)docs/zh_CN/additionalfeatures/nvspartitioneditor.rst(1 hunks)docs/zh_CN/additionalfeatures/partitiontableeditor.rst(1 hunks)docs/zh_CN/additionalfeatures/switchlanguage.rst(1 hunks)docs/zh_CN/additionalfeatures/wokwisimulator.rst(1 hunks)docs/zh_CN/additionalfeatures/writebinarytoflash.rst(1 hunks)docs/zh_CN/buildproject.rst(1 hunks)docs/zh_CN/conf.py(1 hunks)docs/zh_CN/configureproject.rst(1 hunks)docs/zh_CN/connectdevice.rst(1 hunks)docs/zh_CN/debugproject.rst(1 hunks)docs/zh_CN/downloads.rst(1 hunks)docs/zh_CN/faqs.rst(1 hunks)docs/zh_CN/flashdevice.rst(1 hunks)docs/zh_CN/index.rst(1 hunks)docs/zh_CN/installation.rst(1 hunks)docs/zh_CN/marketplaceupdate.rst(1 hunks)docs/zh_CN/monitoroutput.rst(1 hunks)docs/zh_CN/openocddebugging.rst(1 hunks)docs/zh_CN/prerequisites.rst(1 hunks)docs/zh_CN/startproject.rst(1 hunks)docs/zh_CN/troubleshooting.rst(1 hunks)docs/zh_CN/windowsofflineinstaller.rst(1 hunks)
✅ Files skipped from review due to trivial changes (31)
- docs/zh_CN/additionalfeatures/clangd_cdt_support.rst
- docs/zh_CN/buildproject.rst
- docs/zh_CN/additionalfeatures/coredumpdebugging.rst
- docs/zh_CN/additionalfeatures/install-esp-components.rst
- docs/zh_CN/openocddebugging.rst
- docs/zh_CN/monitoroutput.rst
- docs/zh_CN/additionalfeatures/configuretoolchain.rst
- docs/zh_CN/configureproject.rst
- docs/zh_CN/connectdevice.rst
- docs/zh_CN/additionalfeatures/cmakeeditor.rst
- docs/zh_CN/additionalfeatures/heaptracing.rst
- docs/zh_CN/additionalfeatures/configureenvvariables.rst
- docs/zh_CN/additionalfeatures/nvspartitioneditor.rst
- docs/zh_CN/additionalfeatures/dfu.rst
- docs/zh_CN/additionalfeatures/switchlanguage.rst
- docs/zh_CN/flashdevice.rst
- docs/zh_CN/additionalfeatures/wokwisimulator.rst
- docs/zh_CN/additionalfeatures/partitiontableeditor.rst
- docs/zh_CN/marketplaceupdate.rst
- docs/zh_CN/additionalfeatures.rst
- docs/zh_CN/additionalfeatures/application-size-analysis.rst
- docs/zh_CN/debugproject.rst
- docs/zh_CN/windowsofflineinstaller.rst
- docs/en/prerequisites.rst
- docs/zh_CN/troubleshooting.rst
- docs/zh_CN/additionalfeatures/gdbstubdebugging.rst
- docs/zh_CN/additionalfeatures/clangtoolchain.rst
- docs/zh_CN/additionalfeatures/esp-terminal.rst
- docs/zh_CN/downloads.rst
- docs/zh_CN/additionalfeatures/appleveltracing.rst
- docs/zh_CN/additionalfeatures/lspeditor.rst
🧰 Additional context used
🪛 Ruff (0.14.4)
docs/zh_CN/conf.py
11-11: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
16-16: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
20-20: String contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF001)
20-20: String contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Espressif-IDE Docs
- GitHub Check: build_macos
- GitHub Check: Deploy Documentation on Production
- GitHub Check: build_linux
- GitHub Check: build_macos
🔇 Additional comments (12)
docs/en/troubleshooting.rst (4)
6-6: Added translation link for Chinese documentation.The translation linkage for Chinese is a good addition for international user support, aligning with the PR's documentation harmonization objectives.
16-38: Terminology harmonized: "Hints Viewer" → "Hints View".The section title and all references now consistently use "Hints View" terminology (lines 16, 23, 26, 33), which aligns with the PR's goal of standardizing terminology across the documentation. Image alt text has been updated for accessibility. These changes are consistent with the broader terminology harmonization effort described in the PR summary.
45-54: Terminology harmonized: "Error Log" → "Error Log View".Updated section title and references (lines 45, 47, 51, 54) now consistently use "Error Log View" and "Export Log" terminology. Alt text for the image has been updated. These changes maintain consistency with the terminology standardization across the documentation set.
65-88: Terminology harmonized: "Console" → "Console View".Updated references throughout this section (lines 65, 67, 84, 88) now consistently use "Console View" terminology. This aligns with the broader terminology harmonization effort across the documentation, ensuring users encounter consistent naming conventions regardless of which section they reference.
docs/zh_CN/startproject.rst (1)
1-1: Documentation localization approach aligns with pattern.The include directive for reusing English content is consistent with other zh_CN documentation files in the PR and is an appropriate localization strategy.
.github/workflows/docs_production.yml (1)
49-49: Verifybuild-docsbehavior without language flag.The language flag (
-l en) has been removed from the build command. Confirm thatbuild-docswithout flags produces the expected documentation output and successfully handles all configured languages.docs/en/installation.rst (1)
1-107: Well-structured installation guide with comprehensive EIM workflow.The restructured documentation clearly presents the three-step installation process, emphasizes EIM as the primary tool manager, and provides migration guidance for legacy workspaces. The narrative progression and instructions are clear.
Verify that all referenced images and cross-references are accessible and valid:
- Images:
ToolsManager/ESP-IDF_Manager_Editor_Screen.png,ToolsManager/Tool_installed_and_activated.png,ToolsManager/ESP-IDF_Manager_Conversion.png- Cross-references:
:ref:tags likedownloads,windowsofflineinstaller,marketplaceupdate, etc.You can verify this by checking image file paths and running a docs build to detect broken references.
docs/zh_CN/index.rst (1)
1-1: Localization pattern applied consistently.The include directive approach for providing Chinese documentation variants is consistent across the PR.
docs/zh_CN/faqs.rst (1)
1-1: Localization pattern applied consistently.The include directive for FAQ localization follows the established pattern in the PR.
docs/zh_CN/prerequisites.rst (1)
1-31: Comprehensive prerequisites documentation with clear requirements.The Chinese prerequisites guide clearly specifies hardware (boards, cables, OS) and software requirements (Java 21+, Python 3.12+, Git) with proper emphasis on PATH configuration. Structure and content are well-organized.
Verify that external links are valid and current:
- Oracle Java downloads (line 24)
- Python downloads (line 25)
- Git downloads (line 26)
- ESP-IDF dependencies documentation (line 27)
docs/zh_CN/installation.rst (1)
1-107: Comprehensive Chinese installation guide with complete EIM workflow documentation.The Chinese guide provides a faithful translation of the English installation guide, maintaining the three-step workflow, EIM-centric guidance, and migration pathway. Structure and terminology are appropriate for Chinese technical documentation.
Verify image and cross-reference validity (same as English version):
- Images:
ToolsManager/ESP-IDF_Manager_Editor_Screen.png,ToolsManager/Tool_installed_and_activated.png,ToolsManager/ESP-IDF_Manager_Conversion.png- Cross-references:
:ref:tags such asdownloads,windowsofflineinstaller,marketplaceupdate, etc.docs/zh_CN/conf.py (1)
18-25: LGTM! Project metadata and language configuration are correct.The project name, copyright (with proper Chinese characters and fullwidth parentheses, which are standard in Chinese typography), PDF title, and language setting are all appropriate for the Chinese documentation configuration.
Note: The static analysis warning about fullwidth parentheses on line 20 is a false positive—fullwidth parentheses are the correct typographic convention for Chinese text.
| cd ./docs | ||
| PATH=/home/runner/.local/bin:$PATH pip3 install -r requirements.txt --prefer-binary | ||
| PATH=/home/runner/.local/bin:$PATH SPHINXOPTS="-W" build-docs -l en | ||
| PATH=/home/runner/.local/bin:$PATH SPHINXOPTS="-W" build-docs |
There was a problem hiding this comment.
🧩 Analysis chain
Inconsistent language flag handling across build steps.
Line 41 removes the -l en language flag from build-docs, but line 65 retains it. This inconsistency could cause the build step and the deploy step to produce different outputs. Clarify whether both steps should use the same invocation or if the different flag usage is intentional.
Also applies to: 65-65
🏁 Script executed:
#!/bin/bash
# Examine the workflow file to verify the language flag inconsistency
cat .github/workflows/docs_build.yml | head -80 | tail -45Length of output: 1800
🏁 Script executed:
#!/bin/bash
# Get specific lines around 41 and 65 with context
echo "=== Context around line 41 ==="
sed -n '35,50p' .github/workflows/docs_build.yml
echo -e "\n=== Context around line 65 ==="
sed -n '60,70p' .github/workflows/docs_build.ymlLength of output: 1160
🏁 Script executed:
#!/bin/bash
# Search for all build-docs invocations in the file
echo "=== All build-docs invocations ==="
grep -n "build-docs" .github/workflows/docs_build.ymlLength of output: 165
Inconsistent language flag handling is confirmed; clarify intent or standardize both invocations.
The -l en flag was removed from line 41 (build-docs without flag) but retained at line 65 (build-docs -l en). Since line 41 builds the docs for testing and line 65 builds before deployment to the preview server, this inconsistency means the tested build differs from the deployed build. Either both steps should use the same invocation, or document why the flags differ.
Also applies to: 65-65
🤖 Prompt for AI Agents
In .github/workflows/docs_build.yml around lines 41 and 65, the build-docs
invocation is inconsistent (line 41 calls "build-docs" while line 65 calls
"build-docs -l en"); make them consistent by either adding "-l en" to the line
41 step or removing "-l en" from line 65 so the tested and deployed builds
match, or if the difference is intentional, add a clear comment above each step
explaining the reason for the differing language flag; update both lines
accordingly so the workflow is unambiguous.
| @@ -0,0 +1,25 @@ | |||
| # -*- coding: utf-8 -*- | |||
| # | |||
| # English Language RTD & Sphinx config file | |||
There was a problem hiding this comment.
Correct the comment to reflect Chinese documentation.
The comment states "English Language RTD & Sphinx config file" but this is the Chinese (zh_CN) configuration. This appears to be a copy-paste oversight.
Apply this diff to correct the comment:
-# English Language RTD & Sphinx config file
+# Chinese Language RTD & Sphinx config file📝 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.
| # English Language RTD & Sphinx config file | |
| # Chinese Language RTD & Sphinx config file |
🤖 Prompt for AI Agents
In docs/zh_CN/conf.py around line 3, the file-level comment incorrectly reads
"English Language RTD & Sphinx config file"; update that single-line comment to
reflect that this is the Chinese documentation configuration (for example:
"Chinese Language RTD & Sphinx config file" or "中文 RTD 与 Sphinx 配置文件") so the
comment matches the zh_CN context.
* IEP-1549: Updating java version for release and builds (#1241) Co-authored-by: Kondal Kolipaka <[email protected]> * IEP-1650 Convert NVS Title Area dialog to the editor (#1339) * feat: convertin NVS dialog to the editor * fix: replace enum with EnumMap * feat: optimizing performance * feat: improving tooltip performance * feat: move to the new API, improving tooltip * fix: fixed potential NPE is there is no selected project * fix: addressing coderabbit point about NPE * fix: remove duplicate editor in the plugin.xml * feat: improved status messaging * feat: setting the default partition size to improve the UX * fix: set dirty false after save action * fix: addressing coderabbit comment about possible NPE * fix: addressing minor fix about filter path * feat: saving the status of fields based on the project * feat: improving UX by utilizing ColumnLabelProvider * fix: return non editable for first row * feat: refactoring to reduce size of the EditorPage and make it easier to read * feat: refactoring - moved storing preference logic to the separate class * fix: fixed typo in the methods names * IEP-1652 Add project selector to ESP-IDF Terminal launch workflow (#1343) * feat: added a project selector on the terminal page * feat: update terminal title for better UX on windows * fix: added Project name label to the Messages * fix: allowing empty project --------- Co-authored-by: Ali Azam Rana <[email protected]> Co-authored-by: Denys Almazov <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/controls/IDFConsoleWizardConfigurationPanel.java (1)
60-116: Wizard never validates project selection or surfaces the new "missing project" errorThe validation gap identified in the review is confirmed. The panel's
isValid()method (lines 154–156) unconditionally returnstrue, allowing the wizard to proceed even when:
- No IDF projects exist in the workspace, or
- The project combo has no selection or empty text.
In those cases
extractData()(lines 129–143) will silently skip settingPROP_PROCESS_WORKING_DIRandPROP_TITLE, leaving the terminal unconfigured without user feedback. The error message keyIDFConsoleWizardConfigurationPanel_MissingProjectErrorMsgexists inmessages.propertiesbut is never used.The suggested fix is valid and follows the pattern used elsewhere in the codebase (e.g.,
NewComponentWizardPage.java):@@ - @Override - public boolean isValid() { - return true; - } + @Override + public boolean isValid() { + // Require a selected IDF project to launch the console + if (projectCombo == null || projectCombo.isDisposed() || projectCombo.getItemCount() == 0 + || projectCombo.getText().isEmpty()) { + setErrorMessage(Messages.IDFConsoleWizardConfigurationPanel_MissingProjectErrorMsg); + return false; + } + + setErrorMessage(null); + return true; + }This prevents finishing the wizard with an invalid state and reuses the localized error message for clear user feedback.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorDialog.java (1)
314-344: Missing label provider setup confirmed; the table cells will display incorrect or blank content.The commented-out line at 333 cannot be simply uncommented because
NvsTableEditorLabelProvideris an abstract class and cannot be instantiated directly. However, the underlying issue is real: no label provider is set ontableViewerin thecreateTableViewer()method.The codebase has a factory (
NvsEditorSupportFactory.createLabelProvider(NvsColumn column)) that creates concrete per-column label provider implementations, but this factory is never invoked inNvsEditorDialog.java. Without a label provider, each table cell will display defaulttoString()output or be empty, making the NVS editor dialog non-functional for viewing key/type/encoding/value columns properly.The proper fix requires either:
- Creating concrete per-column label provider instances via the factory and attaching them to individual
TableViewerColumnobjects usingcolumn.setLabelProvider(), or- If using a single label provider for all columns, creating a unified implementation that delegates to the factory based on column index.
♻️ Duplicate comments (1)
bundles/com.espressif.idf.ui/plugin.xml (1)
642-647: Startup contributions: avoid relying on extension order and keep heavy work off the UI thread.You now register both
EspressifGeneralStartupandEspressifToolStartupasorg.eclipse.ui.startupimplementors. Startup order between them is not guaranteed; any sequencing assumptions should be handled viaJobs or explicit coordination instead of extension ordering. Also ensure any heavy initialization (e.g., tool export/import, filesystem I/O) runs in background jobs, with only short UI updates on the UI thread. This reiterates the concern from an earlier review; please confirm it still holds after the rebase.#!/bin/bash # Reconfirm IStartup implementations and check for heavy work in earlyStartup rg -n "class\s+EspressifGeneralStartup\b" bundles/com.espressif.idf.ui/src -C5 rg -n "class\s+EspressifToolStartup\b" bundles/com.espressif.idf.ui/src -C20 rg -n "earlyStartup\s*\(" bundles/com.espressif.idf.ui/src -C5
🧹 Nitpick comments (2)
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java (1)
166-179: Consider avoidingvarand tightening title handling assumptions
setTerminalTitlelooks functionally correct and centralizes the title behavior nicely. Two small points to consider:
- If this bundle still targets Java 8 (or any BREE < 10),
varwill not compile; an explicit type here (ObjectorString) avoids tying the code to newer language levels.- The method assumes the existing
PROP_TITLEvalue is directly usable; you might make that explicit (e.g., viaString.valueOf(...)) to ensure aStringis always stored back, even if callers pass non‑String values.A possible refactor that addresses both points and removes the branch:
- if (properties.containsKey(ITerminalsConnectorConstants.PROP_TITLE)) { - var projectName = properties.get(ITerminalsConnectorConstants.PROP_TITLE); - properties.put(ITerminalsConnectorConstants.PROP_TITLE, - String.format("%s (%s)", projectName, Messages.IDFConsoleLauncherDelegate_ESPIDFTerminal)); //$NON-NLS-1$ - } else { - properties.put(ITerminalsConnectorConstants.PROP_TITLE, Messages.IDFConsoleLauncherDelegate_ESPIDFTerminal); - } + Object projectName = properties.get(ITerminalsConnectorConstants.PROP_TITLE); + String effectiveTitle = projectName != null + ? String.format("%s (%s)", projectName, Messages.IDFConsoleLauncherDelegate_ESPIDFTerminal) //$NON-NLS-1$ + : Messages.IDFConsoleLauncherDelegate_ESPIDFTerminal; + properties.put(ITerminalsConnectorConstants.PROP_TITLE, effectiveTitle);bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (1)
110-138: Consider avoiding dirty state during initial settings load
loadSettingsToUi()is called after controls and listeners are wired. ProgrammaticsetText/setSelectiononsizeText,encryptAction, etc., will fire their listeners, which callmarkDirty()andvalidateGenerationState(). That can mark a freshly opened editor as dirty before the user changes anything.If you prefer “clean on open unless user edits”, you can introduce an initialization guard, e.g.:
public class NvsCsvEditorPage { @@ - private NvsEditorPreferenceService preferenceService; + private NvsEditorPreferenceService preferenceService; + private boolean initializing = true; @@ public void createControl() { @@ - loadSettingsToUi(); + loadSettingsToUi(); @@ - validateGenerationState(); // Set initial button state + validateGenerationState(); // Set initial button state + initializing = false; } @@ public void markDirty() { - @SuppressWarnings("unchecked") - List<NvsTableBean> beansToSave = (List<NvsTableBean>) tableViewer.getInput(); - validateBeansBeforeSaving(beansToSave); - updateErrorMessage(); - dirtyStateListener.accept(true); + if (initializing) + { + return; + } + @SuppressWarnings("unchecked") + List<NvsTableBean> beansToSave = (List<NvsTableBean>) tableViewer.getInput(); + validateBeansBeforeSaving(beansToSave); + updateErrorMessage(); + dirtyStateListener.accept(true); }That keeps the existing behavior once the page is live while preventing initial preference application from toggling the dirty flag.
Also applies to: 268-275, 718-743
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/controls/IDFConsoleWizardConfigurationPanel.java(4 hunks)bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/controls/Messages.java(1 hunks)bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/controls/messages.properties(1 hunks)bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java(5 hunks)bundles/com.espressif.idf.ui/plugin.xml(2 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsColumn.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditor.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorDialog.java(10 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorPreferenceService.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorSettings.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorSupportFactory.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsTableEditorLabelProvider.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/handlers/NvsEditorHandler.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-27T18:07:42.470Z
Learnt from: sigmaaa
Repo: espressif/idf-eclipse-plugin PR: 1339
File: bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/handlers/NvsEditorHandler.java:27-34
Timestamp: 2025-10-27T18:07:42.470Z
Learning: In the file bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/handlers/NvsEditorHandler.java, the NvsEditorHandler is tied to a project explorer right-click command, so EclipseUtil.getSelectedProjectInExplorer() will never return null due to the command's enablement conditions.
Applied to files:
bundles/com.espressif.idf.ui/plugin.xmlbundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorDialog.javabundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/controls/IDFConsoleWizardConfigurationPanel.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorPreferenceService.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/handlers/NvsEditorHandler.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditor.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorSupportFactory.java
🧬 Code graph analysis (10)
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java (2)
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/activator/UIPlugin.java (1)
UIPlugin(26-121)bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/Messages.java (1)
Messages(5-15)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorDialog.java (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/NvsTableDataService.java (1)
NvsTableDataService(14-80)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/Messages.java (1)
Messages(10-54)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorSettings.java (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/StringUtil.java (1)
StringUtil(11-25)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsTableEditorLabelProvider.java (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/NvsTableBean.java (1)
NvsTableBean(10-72)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/NvsBeanValidator.java (1)
NvsBeanValidator(15-173)
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/controls/IDFConsoleWizardConfigurationPanel.java (4)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFProjectNature.java (1)
IDFProjectNature(16-61)bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-141)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/EclipseUtil.java (1)
EclipseUtil(36-186)bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/controls/Messages.java (1)
Messages(5-17)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorPreferenceService.java (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-141)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/StringUtil.java (1)
StringUtil(11-25)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/handlers/NvsEditorHandler.java (4)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-141)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/NvsTableDataService.java (1)
NvsTableDataService(14-80)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/EclipseUtil.java (1)
EclipseUtil(36-186)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditor.java (1)
NvsEditor(14-104)
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/controls/Messages.java (2)
bundles/com.espressif.idf.wokwi/src/com/espressif/idf/wokwi/ui/Messages.java (1)
Messages(5-31)bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/nls/Messages.java (1)
Messages(20-46)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (6)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/NvsTableBean.java (1)
NvsTableBean(10-72)bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-141)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/NvsBeanValidator.java (1)
NvsBeanValidator(15-173)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/NvsPartitionGenerator.java (1)
NvsPartitionGenerator(22-75)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/NvsTableDataService.java (1)
NvsTableDataService(14-80)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/StringUtil.java (1)
StringUtil(11-25)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorSupportFactory.java (3)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/NvsTableBean.java (1)
NvsTableBean(10-72)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/NvsTableDataService.java (1)
NvsTableDataService(14-80)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/StringUtil.java (1)
StringUtil(11-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_macos
- GitHub Check: build_macos
🔇 Additional comments (12)
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/controls/Messages.java (1)
5-16: NLS helper looks consistent and ready for useThe NLS setup (bundle name, static initializer, private ctor, and public fields) matches existing patterns in the repo and aligns with the new messages.properties keys; nothing blocking here.
If you haven’t already, you may want to quickly confirm PDE NLS tooling sees these two keys as resolved (no missing/broken message warnings).
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/controls/messages.properties (1)
1-2: Properties keys align with Messages.java and read wellKeys match the Java fields and the strings are clear for end users; looks good as the base EN locale.
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java (2)
88-97: Encoding default behavior looks consistent with platform expectationsUsing UTF‑8 on macOS/Windows and falling back to the workbench default encoding otherwise is a reasonable, predictable strategy, and the null/empty guard before putting the encoding into
propertiesis correct. I don’t see functional issues here.
105-155: Working-directory initialization and wiring intoProcessSettingsThe new block that initializes
PROP_PROCESS_WORKING_DIRonly when the panel hasn’t set it is careful about preferences, Eclipse home/workspace, and dynamic variable resolution, and it validates directories before use. Explicitly passingworkingDirviaprocessSettings.setWorkingDir(workingDir)then cleanly wires this into the launched process. Behaviorally this looks sound and should avoid clobbering any explicitly provided working directory.Also applies to: 296-297, 371-371
bundles/com.espressif.idf.ui/plugin.xml (1)
720-729: NVS editor registration matches the new editor class and ID.The editor id
com.espressif.idf.ui.nvs.nvsEditormatchesNvsEditor.ID, and the localized name key%command.name.nvsTableEditorkeeps labels in sync. Wiring between command, handler, and editor looks consistent.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/handlers/NvsEditorHandler.java (1)
25-52: Handler flow for opening NvsEditor looks correct and robust.The handler gets the selected project, ensures
nvs.csvexists viaNvsTableDataService().saveCsv(...), then safely opensNvsEditorthrough the active workbench window/page, loggingPartInitExceptionif anything fails. Given this command is bound to a Project Explorer selection, relying onEclipseUtil.getSelectedProjectInExplorer()without extra null checks is appropriate. Based on learnings.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorDialog.java (1)
446-461: First-row protection and info messaging are consistent.Preventing edits/deletes on the first row and surfacing
NvsEditorDialog_FirstRowIsFixedInfoMsgboth incanModifyandgetDeleteRowAction()makes the constraint explicit to users while keeping the rest of the table editable as expected.Also applies to: 670-694
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsColumn.java (1)
1-75: NvsColumn enum cleanly centralizes column metadata.Defining columns with names, widths, and helpers like
getColumnProperties()/fromIndex(int)removes magic indices and string arrays and should simplify both label and editing support. No issues spotted.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorPreferenceService.java (1)
1-70: Project-scoped NVS editor preference service is symmetric and safe.
NvsEditorPreferenceServicecleanly maps between project-scoped preferences andNvsEditorSettings, with consistent keys, sensible defaults, and properflush()error handling viaLogger. The constructor’s use ofProjectScopekeeps settings per-project as expected.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsTableEditorLabelProvider.java (1)
13-66: Verification confirms refactor is sound.The abstract
NvsTableEditorLabelProvideris properly instantiated via an anonymous subclass inNvsEditorSupportFactory.createLabelProvider()that fully implements required methods:
getColumnIndex()delegates tocolumn.getIndex()getColumnText(NvsTableBean)switches on column enum and returns bean propertiesgetToolTipText(Object)chains first-row handling with parent's validation-message logicDesign is cohesive and ready for integration.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorSettings.java (1)
1-27: Record syntax is supported; bundle BREE is correctly configured.Records require JDK 16 or later, and the bundle is configured for JavaSE-17 (as specified in
META-INF/MANIFEST.MF). The record-based DTO will compile and load without issues. No changes are needed.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditor.java (1)
14-103: EditorPart lifecycle and dirty wiring look solidThe conversion to an
EditorPartwith explicitID,init(...)validation, delegation toNvsCsvEditorPage, and thedirtyStateListener→setDirtyflow (includingPROP_DIRTYfiring) is cohesive and matches Eclipse editor expectations. I don't see any local correctness issues here; the class cleanly coordinates save and focus with the page.
| } else if (Platform.OS_WIN32.equals(Platform.getOS()) && image.contains("cmd.exe")) { //$NON-NLS-1$ | ||
| // This is the new part that rewrites the arguments for cmd.exe | ||
| String title = (String) properties.get(ITerminalsConnectorConstants.PROP_TITLE); | ||
| if (title != null && !title.isEmpty()) { | ||
| String safeTitle = title.replaceAll("[\\r\\n\"&|<>^]", " ").trim(); //$NON-NLS-1$ //$NON-NLS-2$ | ||
| arguments = "/c \"title " + safeTitle + " && cmd.exe\""; //$NON-NLS-1$ //$NON-NLS-2$ | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Sanitize cmd.exe title more defensively to avoid shell injection via %VAR% expansion
The new cmd.exe branch is a good idea to push the terminal title down into the Windows shell, and the current replaceAll("[\\r\\n\"&|<>^]", " ") already strips several dangerous metacharacters. However, when the title comes from user‑controlled data (e.g., project name or other external sources), there is still a remaining injection vector:
- A title containing
%FOO%will causecmd.exeto expand the environment variableFOOinsidetitle .... IfFOO’s value contains characters like&or|, they are reintroduced after your sanitization and can split the command line (e.g.,title ... & <second command>).
To harden this, I’d recommend at least removing % as well, and optionally any other characters you don’t expect in titles:
- String safeTitle = title.replaceAll("[\\r\\n\"&|<>^]", " ").trim(); //$NON-NLS-1$ //$NON-NLS-2$
+ String safeTitle = title.replaceAll("[\\r\\n\"&|<>^%]", " ").trim(); //$NON-NLS-1$ //$NON-NLS-2$If you want to be extra strict, you could invert the logic and allow only a known-safe whitelist (letters, digits, a small set of punctuation) instead of blacklisting characters.
Given this code is constructing a cmd.exe command line, I’d treat tightening the sanitization as important before shipping.
Also applies to: 172-179
</web_search>
🏁 Script executed:
#!/bin/bash
# Locate the file and display code around both mentioned line ranges
find . -name "IDFConsoleLauncherDelegate.java" -type f 2>/dev/null | head -1 > /tmp/filepath.txt
FILEPATH=$(cat /tmp/filepath.txt)
if [ -n "$FILEPATH" ]; then
echo "=== File found at: $FILEPATH ==="
echo ""
echo "=== Lines 172-179 ==="
sed -n '172,179p' "$FILEPATH"
echo ""
echo "=== Lines 253-259 ==="
sed -n '253,259p' "$FILEPATH"
else
echo "File not found"
fiLength of output: 1201
🌐 Web query:
cmd.exe percent sign %VAR% environment variable expansion security shell injection
💡 Result:
Short answer
- cmd.exe expands %VAR% during parse-time (before execution), which means percent-sign expansions (and other metacharacters) in arguments can be interpreted and cause command injection if the argument comes from untrusted input. [1][2][3][4]
Why this is dangerous (brief)
- %...% is expanded early in cmd.exe’s multi-phase parser, so text containing % or other special chars (& | ^ > < " ` etc.) can be interpreted as variable expansions or shell metacharacters rather than literal data. That parsing behavior has led to real-world exploitable cases (for example CVE-2024-24576 where untrusted arguments to batch files allowed command execution). [1][4]
Concrete mitigations
- Don’t invoke .bat/.cmd files with untrusted arguments. If you must, run cmd.exe yourself with a carefully constructed command line (cmd /C "…") and fully escape/quote every argument. [1]
- Prefer APIs that pass argv arrays (CreateProcess with lpCommandLine built by proper quoting/ArgvQuote, or higher‑level spawn APIs that avoid invoking the shell). Avoid handing user data into a single shell string. [1]
- Sanitize/validate input: reject or remove % and other metacharacters when they are not allowed.
- If writing batch scripts:
- Use %% inside batch files to emit literal % and doubling to escape percent signs in batch files. Use delayed expansion (!VAR!) when you need runtime expansion inside command blocks. [2][3]
- Be careful: delayed expansion introduces ! as another special char and has its own parsing rules. [2]
- Consider using PowerShell (with --% StopParsing where appropriate) or other script hosts that let you pass arguments as data, not as shell-parsed text.
- Test thoroughly: any escaping scheme must be proven against cases with %, quotes, backslashes, newlines and shell metacharacters.
References
- CVE and advisory about unsafe argument escaping when spawning batch files (example: Rust advisory / CVE-2024-24576). [1]
- Delayed expansion vs percent expansion and !VAR! usage (SS64/EnableDelayedExpansion docs). [2]
- Practical escaping notes (how to emit literal % and differences between interactive vs batch-file escaping). [3][4]
If you want, I can:
- show short escaping examples (safe vs unsafe) and a small checklist to harden a specific command invocation, or
- analyze a specific command line you’re concerned about.
Add % to cmd.exe title sanitization to prevent environment variable expansion attacks
The concern is valid. cmd.exe expands %VAR% during parse-time, which means percent-sign expansions in arguments can be interpreted and cause command injection if the argument comes from untrusted input. The code at lines 172–179 constructs the title from user-controlled properties, and the current sanitization at line 257 does not remove %, allowing an attacker who controls an environment variable to inject shell metacharacters after expansion.
Update the regex at line 257:
- String safeTitle = title.replaceAll("[\\r\\n\"&|<>^]", " ").trim(); //$NON-NLS-1$ //$NON-NLS-2$
+ String safeTitle = title.replaceAll("[\\r\\n\"&|<>^%]", " ").trim(); //$NON-NLS-1$ //$NON-NLS-2$Apply the same fix to lines 172–179 if title sanitization occurs there as well.
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java
around lines 253 to 259, the cmd.exe title sanitization currently strips CR/LF,
quotes and other metacharacters but omits the percent sign, allowing %VAR%
expansions and potential command injection; update the replaceAll regex to also
remove '%' (e.g. include % in the character class) so percent signs are stripped
or neutralized, and make the same change to the earlier title sanitization block
around lines 172–179 to ensure consistent protection.
| public boolean getSaveAction() | ||
| { | ||
| boolean isPageValid; | ||
| @SuppressWarnings("unchecked") | ||
| List<NvsTableBean> beansToSave = (List<NvsTableBean>) tableViewer.getInput(); | ||
| isPageValid = validateBeansBeforeSaving(beansToSave); | ||
| updateErrorMessage(); | ||
| if (!isPageValid) |
There was a problem hiding this comment.
Avoid NPE when tableViewer has no input (e.g., CSV parse failure)
Both getSaveAction() and markDirty() assume tableViewer.getInput() is a non‑null List<NvsTableBean>, but createTableViewer() logs and bails on IOException without setting any input. In that case tableViewer.getInput() stays null, and validateBeansBeforeSaving(beansToSave) will throw when calling beansToSave.isEmpty().
You can make validateBeansBeforeSaving null‑safe and have it handle the “no data” case centrally:
- private boolean validateBeansBeforeSaving(List<NvsTableBean> beansToSave)
- {
- String errorMsg;
- if (beansToSave.isEmpty())
- {
- saveErrorMsg = Messages.NvsEditorDialog_EmptyTableErrorMsg;
- return false;
- }
+ private boolean validateBeansBeforeSaving(List<NvsTableBean> beansToSave)
+ {
+ String errorMsg;
+ if (beansToSave == null || beansToSave.isEmpty())
+ {
+ saveErrorMsg = Messages.NvsEditorDialog_EmptyTableErrorMsg;
+ return false;
+ }With this change, the existing calls in getSaveAction() and markDirty() become safe even when CSV parsing failed or produced no rows.
Also applies to: 268-275, 635-663
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java
around lines 226-233 (also apply to 268-275 and 635-663): getSaveAction() and
markDirty() assume tableViewer.getInput() returns a non-null List and pass it
directly to validateBeansBeforeSaving, causing an NPE when parsing failed and
input is null; make validateBeansBeforeSaving(List<NvsTableBean> beans)
null-safe by treating null or empty lists the same (e.g., return false or add a
validation error for "no data"), centralize the "no data" handling inside that
method, and remove any callers' null checks so callers can pass
tableViewer.getInput() directly without risking NPE.
| public void updateErrorMessage() | ||
| { | ||
| String newErrorMessage = StringUtil.EMPTY; | ||
|
|
||
| if (saveErrorMsg == null) | ||
| { | ||
| saveErrorMsg = StringUtil.EMPTY; | ||
| } | ||
|
|
||
| if (saveErrorMsg != null && !saveErrorMsg.isBlank()) | ||
| { | ||
| newErrorMessage = String.format(Messages.NvsEditorDialog_ComplexSaveErrorMsg, saveErrorMsg); | ||
| } | ||
| if (!validationErrors.isEmpty()) | ||
| { | ||
| if (newErrorMessage != null && !newErrorMessage.isBlank()) | ||
| newErrorMessage = newErrorMessage.concat(StringUtil.LINE_SEPARATOR).concat(" "); //$NON-NLS-1$ | ||
|
|
||
| List<String> valuesArr = new ArrayList<>(); | ||
| for (Entry<GeneratePartitionValidationError, String> errorEntry : validationErrors.entrySet()) | ||
| { | ||
| valuesArr.add(errorEntry.getValue()); | ||
| } | ||
| newErrorMessage += String.format(Messages.NvsEditorDialog_GenerateButtonComplexErrorMsg, | ||
| String.join("; ", valuesArr)); //$NON-NLS-1$ | ||
|
|
||
| } | ||
|
|
||
| setErrorMessage(newErrorMessage); | ||
| } |
There was a problem hiding this comment.
Clear status correctly when there are no validation errors
updateErrorMessage() builds newErrorMessage and then unconditionally calls setErrorMessage(newErrorMessage). When both saveErrorMsg and validationErrors are empty, newErrorMessage remains "", so setErrorMessage("") treats this as an error (red foreground) with an empty text instead of reverting to the default informational message.
You can treat a blank message as “no error”:
public void updateErrorMessage()
{
String newErrorMessage = StringUtil.EMPTY;
@@
- setErrorMessage(newErrorMessage);
+ if (newErrorMessage == null || newErrorMessage.isBlank())
+ {
+ // No errors: show the default info message
+ setErrorMessage(null);
+ }
+ else
+ {
+ setErrorMessage(newErrorMessage);
+ }
}This keeps the status area consistent: default info when everything is valid, and an error message only when real issues are present.
Also applies to: 196-209
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java
around lines 280-309 (and similarly lines 196-209), update updateErrorMessage()
so it does not call setErrorMessage("") when there are no errors; instead detect
that newErrorMessage is blank and call setErrorMessage(null) (or otherwise clear
the error state) to revert the status area to the default informational
appearance; only call setErrorMessage with a non-blank string when there is an
actual error to display.
| @Override | ||
| public String getToolTipText(Object element) | ||
| { | ||
| if (tableViewer.getElementAt(0).equals(element)) | ||
| { | ||
| return Messages.NvsEditorDialog_FirstRowIsFixedInfoMsg; | ||
| } | ||
| return super.getToolTipText(element); | ||
| } | ||
|
|
||
| @Override | ||
| public Color getBackground(Object element) | ||
| { | ||
| if (column != NvsColumn.KEY && tableViewer.getElementAt(0).equals(element)) | ||
| { | ||
| return Display.getCurrent().getSystemColor(SWT.COLOR_WIDGET_LIGHT_SHADOW); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Guard against null first element when using tableViewer.getElementAt(0)
getToolTipText, getBackground, and BaseNvsEditingSupport.canEdit all call tableViewer.getElementAt(0).equals(element). If the viewer has no input or no rows (e.g., CSV parse failure or an empty list), getElementAt(0) can return null, leading to an NPE.
Consider centralizing a null‑safe check for “is this the first row?” and reusing it:
@@
- @Override
- protected boolean canEdit(Object element)
- {
- return !tableViewer.getElementAt(0).equals(element);
- }
+ @Override
+ protected boolean canEdit(Object element)
+ {
+ return !isFirstRowElement(element);
+ }
@@
- public String getToolTipText(Object element)
- {
- if (tableViewer.getElementAt(0).equals(element))
- {
- return Messages.NvsEditorDialog_FirstRowIsFixedInfoMsg;
- }
- return super.getToolTipText(element);
- }
+ public String getToolTipText(Object element)
+ {
+ if (isFirstRowElement(element))
+ {
+ return Messages.NvsEditorDialog_FirstRowIsFixedInfoMsg;
+ }
+ return super.getToolTipText(element);
+ }
@@
- public Color getBackground(Object element)
- {
- if (column != NvsColumn.KEY && tableViewer.getElementAt(0).equals(element))
- {
- return Display.getCurrent().getSystemColor(SWT.COLOR_WIDGET_LIGHT_SHADOW);
- }
- return null;
- }
+ public Color getBackground(Object element)
+ {
+ if (column != NvsColumn.KEY && isFirstRowElement(element))
+ {
+ return Display.getCurrent().getSystemColor(SWT.COLOR_WIDGET_LIGHT_SHADOW);
+ }
+ return null;
+ }
@@
public class NvsEditorSupportFactory
{
@@
public EditingSupport createEditingSupport(NvsColumn column)
@@
}
}
+
+ private boolean isFirstRowElement(Object element)
+ {
+ Object first = tableViewer.getElementAt(0);
+ return first != null && first.equals(element);
+ }This keeps the first-row logic in one place and avoids possible NPEs.
Also applies to: 122-126
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorSupportFactory.java
around lines 73-91 (and similarly lines 122-126), the code calls
tableViewer.getElementAt(0).equals(element) and will NPE if getElementAt(0)
returns null; add a small private helper method like isFirstRow(Object element)
that fetches tableViewer.getElementAt(0), returns false if that value is null,
otherwise compares it to element, then replace the direct
getElementAt(0).equals(...) checks in getToolTipText, getBackground and
BaseNvsEditingSupport.canEdit with calls to this helper so the first-row logic
is centralized and null-safe.
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 (20)
docs/en/additionalfeatures.rst(1 hunks)docs/en/configureproject.rst(1 hunks)docs/en/connectdevice.rst(1 hunks)docs/en/downloads.rst(1 hunks)docs/en/faqs.rst(1 hunks)docs/en/flashdevice.rst(4 hunks)docs/en/index.rst(3 hunks)docs/en/monitoroutput.rst(2 hunks)docs/en/openocddebugging.rst(1 hunks)docs/en/startproject.rst(1 hunks)docs/zh_CN/additionalfeatures.rst(1 hunks)docs/zh_CN/configureproject.rst(1 hunks)docs/zh_CN/connectdevice.rst(1 hunks)docs/zh_CN/downloads.rst(1 hunks)docs/zh_CN/faqs.rst(1 hunks)docs/zh_CN/flashdevice.rst(1 hunks)docs/zh_CN/index.rst(1 hunks)docs/zh_CN/monitoroutput.rst(1 hunks)docs/zh_CN/openocddebugging.rst(1 hunks)docs/zh_CN/startproject.rst(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- docs/zh_CN/index.rst
- docs/en/flashdevice.rst
- docs/en/configureproject.rst
- docs/zh_CN/monitoroutput.rst
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/zh_CN/downloads.rst
- docs/zh_CN/additionalfeatures.rst
- docs/zh_CN/startproject.rst
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_macos
🔇 Additional comments (21)
docs/en/monitoroutput.rst (4)
3-4: Heading formatting is correct.The heading underline length matches the text length (18 characters), maintaining proper reStructuredText formatting.
6-6: Translation directive aligns with PR localization objectives.The addition of the zh_CN translation link is consistent with the PR's goal to broaden documentation support and add Chinese translations. This follows standard Sphinx translation conventions.
8-8: Content rewrite improves clarity.The reworded introduction is more direct and clearly conveys the purpose of configuring the ESP-IDF Serial Monitor while maintaining the technical accuracy of the reference link.
24-31: Step instructions are clearer and better structured.The reworded step 1 ("In Eclipse, go to
Espressif>Preferences") is more direct than the prior phrasing and matches common UI instruction patterns. The adjustments maintain instructional clarity and align the steps with the updated introduction.docs/zh_CN/flashdevice.rst (1)
1-120: Now I need to examine the dynamic variables directly from the codebase:Documentation content and RST formatting are sound; dynamic variables are implemented in the codebase.
The Chinese documentation follows proper reStructuredText conventions with correct anchors, cross-references, and formatting. The dynamic variables referenced (
${IDF_PY},${BUILD_DIR},${serial_port},${flash_command}) are part of the plugin's launch configuration system. OpenOCD v0.10.0-esp32-20201202 is a legitimate release from December 2, 2020, appropriate for this v4.0.0 release. The safety warnings about flash encryption irreversibility are clear and well-placed. No issues found.docs/en/additionalfeatures.rst (3)
6-24: Documentation structure is consistent and well-organized.The toctree entries are properly formatted with consistent indentation and spacing. The addition of the Chinese translation link (line 4) aligns with the v4.0 release's broader language support mentioned in the PR summary.
1-2: RST syntax error: Title underline must match title length.The title "Additional IDE Features" is 23 characters, but the underline on line 2 is only 22 characters. In reStructuredText, the underline must be at least as long as the title.
Apply this diff to fix the underline:
Additional IDE Features -======================= +=======================(Add one more
=character to make it 23 characters total.)Likely an incorrect or invalid review comment.
4-4: No issues found—translation link directive is properly configured.The
link_to_translationrole is provided by theesp-docs>=1.4.0package listed indocs/requirements.txt. The syntax on line 4 matches the consistent usage across all documentation files and is correctly configured.docs/en/connectdevice.rst (1)
1-24: Documentation improvements look good.The rewording is clear, translation link is properly formatted, and the step descriptions are enhanced without changing meaning. The updated structure for the Custom Target section improves user guidance.
docs/zh_CN/connectdevice.rst (1)
1-24: New Chinese translation file is well-structured.Translation link is properly formatted, image references are preserved, and RST structure is correct.
docs/en/openocddebugging.rst (1)
1-147: Comprehensive documentation improvements enhance clarity.Conversion of bullet points to numbered steps for sequential instructions is appropriate and improves user guidance. The expanded explanations in tab sections provide better context. Translation link is properly formatted. Notes and code blocks are well-placed.
docs/zh_CN/openocddebugging.rst (1)
1-147: New Chinese OpenOCD debugging guide is well-structured and complete.Translation link is properly formatted, RST structure is correct, and the translation mirrors the improved English version. Image references and external links are preserved.
docs/en/startproject.rst (1)
3-6: Documentation improvements are well-structured and clear.Title refinement, added translation link, toctree navigation links, and conversion to numbered steps for each workflow all enhance usability. Image references and expanded explanations (e.g., line 51 clarification) improve the user guidance flow.
Also applies to: 10-12, 21-27, 39-51, 61-67
docs/zh_CN/faqs.rst (1)
1-199: Comprehensive Chinese FAQ is well-organized and properly formatted.Translation link is correct, RST structure is sound, code blocks are properly formatted, and external references are preserved. The Q&A format is consistent throughout, making it easy to navigate.
docs/en/index.rst (1)
1-58: Index documentation updates are well-integrated.Translation link is properly formatted, the new platform support note provides important version guidance, expanded Features section reflects v4.0 improvements, and toctree structure is clean and organized.
docs/en/downloads.rst (1)
1-39: Download documentation updates improve clarity and platform support.Title underline and translation link are properly formatted. Converting to list-table format is superior for rendering. The new macOS Security Notice section provides important guidance with clear examples and code blocks. Release notes link reference enhances documentation navigation.
docs/en/faqs.rst (5)
4-4: Translation link addition improves i18n support.Adding the translation link header is consistent with the PR's broader documentation expansion (including Chinese translations). The formatting follows reStructuredText conventions correctly.
9-9: Code formatting standardization improves clarity.The consistent conversion from inline
-code`` to proper triple-backtickcodeformatting improves readability and rst rendering. This change is well-applied throughout the FAQ.Also applies to: 33-33, 66-66, 71-71, 76-76, 101-101, 108-108, 113-113
14-15: Procedural steps restructuring significantly improves UX.Converting bullet points to numbered step sequences for all procedural guidance (installation, configuration, troubleshooting, etc.) is a best practice for technical documentation. This makes multi-step procedures substantially clearer and easier to follow. The expanded sections like rollback (lines 89-96) and multiple configurations (lines 177-185) now provide comprehensive step-by-step guidance that will improve user onboarding.
Also applies to: 20-21, 38-39, 44-47, 52-55, 60-61, 83-84, 89-96, 150-155, 160-163, 170-172, 177-185
127-127: Verify Java and Eclipse CDT version requirements.Lines 127 and 132-143 make specific claims about Java 11+ being required for CDT 10.0 and Eclipse 2020-06+. Please confirm these version compatibility statements are still accurate with the current Eclipse/CDT releases supported by this v4.0.0 release, as these requirements may have evolved.
Also applies to: 132-143
76-76: Confirm system paths and directory conventions.Line 76 specifies
~/.espressiffor Linux/macOS and%USER_PROFILE%.espressiffor Windows. Line 101 specifies/projectName/build/compile_commands.jsonas the standard location. Please verify these paths remain correct and aligned with the EIM integration changes and any system environment variable shifts mentioned in the PR summary.Also applies to: 101-101
| --------------------------- | ||
| -------------------------- | ||
|
|
||
| To import an existing project into the Espressif-IDE, please make sure that is a CMake project. Follow the steps below: |
There was a problem hiding this comment.
Fix grammar error in import section introduction.
Line 59 has an incomplete phrase: "please make sure that is a CMake project" should read "please make sure that it is a CMake project."
Apply this fix:
-To import an existing project into the Espressif-IDE, please make sure that is a CMake project. Follow the steps below:
+To import an existing project into the Espressif-IDE, please make sure that it is a CMake project. Follow the steps below:📝 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.
| To import an existing project into the Espressif-IDE, please make sure that is a CMake project. Follow the steps below: | |
| To import an existing project into the Espressif-IDE, please make sure that it is a CMake project. Follow the steps below: |
🤖 Prompt for AI Agents
In docs/en/startproject.rst around line 59, fix the grammar in the import
section intro by changing the phrase "please make sure that is a CMake project"
to "please make sure that it is a CMake project" so the sentence is complete and
reads correctly.
| 配置项目 | ||
| ======== | ||
|
|
||
| :link_to_translation:`en:[English]` | ||
|
|
||
| 项目配置保存在项目根目录下一个名为 ``sdkconfig`` 的文件中。可以使用 SDK 配置编辑器修改该配置文件。 | ||
|
|
||
| .. note:: | ||
|
|
||
| ``sdkconfig`` 文件仅在构建之后才会生成。因此，建议在配置项目之前先构建一次项目。 | ||
|
|
||
| 可参照下列步骤，启动 SDK 配置编辑器： | ||
|
|
||
| #. 找到 ``sdkconfig`` 文件。 | ||
| #. 双击该文件以启动 SDK 配置编辑器。 | ||
| #. 根据操作系统使用 ``Ctrl+S`` 或 ``Command+S`` 保存更改。也可以使用 Eclipse 工具栏中的 ``Save`` 按钮来保存。 | ||
| #. 若想撤销在 SDK 配置编辑器中所做的更改，可以在不保存的情况下关闭编辑器，或右键单击 ``sdkconfig`` 文件并选择 ``Load sdkconfig``，在编辑器中还原更改。 | ||
|
|
||
| .. image:: ../../media/13_sdkconfig_editor.png | ||
| :alt: SDK 配置编辑器 |
There was a problem hiding this comment.
🧩 Analysis chain
Documentation file structure looks good; verify image path and consistency with English version.
This is a well-formatted Chinese documentation translation using proper ReStructuredText syntax. The file includes appropriate localization elements (translation link on line 4), clear structure with a note block and numbered steps, and descriptive content. However, before approval, please verify:
-
Image path validity (Line 19-20): Confirm that
../../media/13_sdkconfig_editor.pngresolves correctly relative todocs/zh_CN/configureproject.rst. The path should point todocs/media/13_sdkconfig_editor.png. -
Consistency with English version: Ensure the content, structure, and instructions in this Chinese version accurately mirror
docs/en/configureproject.rst, including any recent updates. -
Documentation build integration: Verify that this file is properly indexed in the Chinese documentation build configuration (typically an
index.rstortoctreeentry indocs/zh_CN/).
Run the following script to verify the image path and documentation structure:
🏁 Script executed:
#!/bin/bash
# Verify image path exists
if [ -f "docs/media/13_sdkconfig_editor.png" ]; then
echo "✓ Image file exists at docs/media/13_sdkconfig_editor.png"
else
echo "✗ Image file NOT found at docs/media/13_sdkconfig_editor.png"
fi
# Verify the file structure is valid RST
if grep -q "^配置项目$" docs/zh_CN/configureproject.rst; then
echo "✓ Chinese title found"
fi
# Check for English version for comparison
if [ -f "docs/en/configureproject.rst" ]; then
echo "✓ English version exists for comparison"
echo ""
echo "English version structure:"
head -20 docs/en/configureproject.rst
else
echo "✗ English version not found at docs/en/configureproject.rst"
fi
# Verify Chinese docs index references this file
if grep -r "configureproject" docs/zh_CN/index.rst 2>/dev/null; then
echo "✓ File is referenced in zh_CN documentation index"
else
echo "⚠ File may not be indexed in zh_CN documentation"
fiLength of output: 1230
Image file missing—documentation will not render correctly.
The Chinese documentation file is well-structured and properly indexed, with content that accurately translates the English version. However, verification reveals a critical issue:
The image file referenced on line 19 (../../media/13_sdkconfig_editor.png, which resolves to docs/media/13_sdkconfig_editor.png) does not exist in the repository. Both the English and Chinese versions reference this same missing image, which will cause documentation build failures or broken image references in the final output.
Action required: Either add the missing image file to docs/media/13_sdkconfig_editor.png or correct the image path if it points to a different location.
🤖 Prompt for AI Agents
In docs/zh_CN/configureproject.rst around lines 1 to 20, the referenced image
../../media/13_sdkconfig_editor.png (docs/media/13_sdkconfig_editor.png) is
missing and will break the docs build; fix by either adding the correct image
file at docs/media/13_sdkconfig_editor.png (use the original
13_sdkconfig_editor.png from the English docs or generate a matching screenshot)
or update the image path to the correct existing image location and ensure the
file is committed to the repo so the reference resolves.
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 (4)
docs/en/buildproject.rst(2 hunks)docs/en/marketplaceupdate.rst(2 hunks)docs/zh_CN/buildproject.rst(1 hunks)docs/zh_CN/marketplaceupdate.rst(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/en/buildproject.rst
- docs/en/marketplaceupdate.rst
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_macos
🔇 Additional comments (2)
docs/zh_CN/buildproject.rst (2)
22-37: Well-structured custom build directory section.The section is clearly organized with proper step-by-step instructions and includes a helpful note with image reference. RST syntax is correct.
39-46: Cross-references are valid—no issues found.Both referenced documentation files exist at the specified paths. The toctree directive is correctly configured and will not produce broken links.
| 如果你是初次使用 IDE，请参照下列步骤，完成项目的构建流程： | ||
|
|
||
| 1. 在 ``Project Explorer`` 中选择一个项目。 | ||
| 2. 在第一个下拉菜单 **启动模式** 中选择 ``Run``。。 |
There was a problem hiding this comment.
Fix double period punctuation on line 15.
Line 15 contains "。。" (two Chinese periods), which is incorrect punctuation. It should use a single period.
Apply this diff:
-2. 在第一个下拉菜单 **启动模式** 中选择 ``Run``。。
+2. 在第一个下拉菜单 **启动模式** 中选择 ``Run``。📝 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.
| 2. 在第一个下拉菜单 **启动模式** 中选择 ``Run``。。 | |
| 2. 在第一个下拉菜单 **启动模式** 中选择 ``Run``。 |
🤖 Prompt for AI Agents
In docs/zh_CN/buildproject.rst around line 15, the sentence currently ends with
double Chinese periods ("。。"); replace the double period with a single Chinese
period so the line reads "2. 在第一个下拉菜单 **启动模式** 中选择 ``Run``。" (only one period)
to fix the punctuation.
| .. _marketplaceupdate: | ||
|
|
||
| 更新站点安装指南 | ||
| ================ | ||
|
|
||
| :link_to_translation:`en:[English]` | ||
|
|
||
| 可通过以下三种方式安装 Espressif-IDE Eclipse 插件： | ||
|
|
||
| - :ref:`installUpdateSiteURL` | ||
| - :ref:`install_idf_eclipse_plugin_marketplace` | ||
| - :ref:`install_idf_eclipse_plugin_local_archive` | ||
|
|
||
| .. _installUpdateSiteURL: | ||
|
|
||
| 使用更新站点 URL 安装 IDF 插件 | ||
| ------------------------------ | ||
|
|
||
| 可以使用更新站点 URL 将 IDF Eclipse 插件安装到现有的 Eclipse CDT 或 Espressif-IDE 中。首先，按如下方式添加发布仓库的 URL： | ||
|
|
||
| 1. 进入 ``Help`` > ``Install New Software``。 | ||
| 2. 点击 ``Add``，在弹出窗口中： | ||
|
|
||
| * 在 ``Name`` 一栏填写 ``Espressif IDF Plugin for Eclipse``。 | ||
| * 在 ``Location`` 一栏，填写插件更新站点的 URL.（`稳定版 <https://dl.espressif.com/dl/idf-eclipse-plugin/updates/latest/>`_） | ||
| * 点击 ``Add``。 | ||
|
|
||
| 3. 全选列表中的组件并继续安装。 | ||
|
|
||
| 若要添加测试版和每日构建版插件，可以使用以下的更新站点 URL： | ||
|
|
||
| * `测试版本 <https://dl.espressif.com/dl/idf-eclipse-plugin/updates/beta/>`_ | ||
| * `每日构建版本 (Nightly Build) <https://dl.espressif.com/dl/idf-eclipse-plugin/updates/nightly/>`_ | ||
|
|
||
| .. note:: | ||
|
|
||
| 尽管本文中的截图来自 macOS 系统中的操作，但安装说明同样适用于 Windows 和 Linux。 | ||
|
|
||
| .. image:: ../../media/idf_update_site_install.png | ||
|
|
||
|
|
||
| .. _install_idf_eclipse_plugin_marketplace: | ||
|
|
||
| 通过 Eclipse 市场安装 IDF Eclipse 插件 | ||
| -------------------------------------- | ||
|
|
||
| 要通过 Eclipse 市场安装 ESP-IDF Eclipse 插件，请按以下步骤操作： | ||
|
|
||
| 1. 打开 Eclipse，前往 ``Help`` > ``Eclipse Marketplace...``。 | ||
| 2. 在搜索框中输入 **ESP‑IDF Eclipse Plugin** 找到该插件。 | ||
| 3. 点击 ``Install``，按照屏幕上的提示完成安装。 | ||
| 4. 安装完成后，重启 Eclipse 以启用插件。 | ||
|
|
||
|
|
||
| .. _install_idf_eclipse_plugin_local_archive: | ||
|
|
||
| 通过本地压缩包安装 IDF Eclipse 插件 | ||
| ----------------------------------- | ||
|
|
||
| 要通过本地压缩包安装 ESP-IDF Eclipse 插件，请按以下步骤操作： | ||
|
|
||
| 1. 点击 `此处 <https://github.com/espressif/idf-eclipse-plugin/releases>`_ 下载 IDF Eclipse 插件的最新更新站点压缩包。 | ||
| 2. 在 Eclipse 中，进入 ``Help`` > ``Install New Software``。 | ||
| 3. 点击 ``Add`` 按钮。 | ||
| 4. 在 ``Add Repository`` 对话框中，选择 ``Archive`` 并选择文件 ``com.espressif.idf.update-vxxxxxxx.zip``。 | ||
| 5. 点击 ``Add``。 | ||
| 6. 在列表中选择 ``Espressif IDF``，然后继续安装。 | ||
| 7. 完成安装后重启 Eclipse。 | ||
|
|
||
| .. _upgradePlugins: | ||
|
|
||
| 如何升级现有的 IDF Eclipse 插件 | ||
| ------------------------------- | ||
|
|
||
| 如果你是首次安装 IDF Eclipse 插件，请按以下步骤添加新版本的仓库： | ||
|
|
||
| 1. 前往 ``Window`` > ``Preferences`` > ``Install/Update`` > ``Available Software Sites``。 | ||
| 2. 点击 ``Add``。 | ||
| 3. 输入新仓库的 `URL <https://dl.espressif.com/dl/idf-eclipse-plugin/updates/latest/>`_。 | ||
| 4. 点击 ``Ok``。 | ||
|
|
||
| 如果已经通过更新站点 URL 安装了 IDF Eclipse 插件，可按以下步骤升级到最新版本： | ||
|
|
||
| 1. 进入 ``Help`` > ``Check for Updates``。 | ||
| 2. 如果发现有可用更新版本，选择 ``Espressif IDF Plugins for Eclipse``，并取消选择所有其他项目。 | ||
| 3. 点击 ``Next`` 继续安装。 | ||
|
|
||
| .. image:: ../../media/Update_plugins.png | ||
|
|
||
|
|
||
| .. _upgradeEspressifIdeDependencies: | ||
|
|
||
| 从 Espressif-IDE 3.3.0 升级到 3.4.0 | ||
| ----------------------------------- | ||
|
|
||
| 如果插件的新版本需要更新依赖项（例如最低 CDT 版本从 11.6 更新到 12），请在通过更新站点安装插件时一并更新这些依赖项。 | ||
|
|
||
| .. image:: ../../media/Update_dependencies.png | ||
|
|
||
| 如果所需依赖项未更新，安装程序会显示如下错误： | ||
|
|
||
| ``The installation cannot be completed as requested.`` | ||
|
|
||
| 解决方法： | ||
|
|
||
| 1. 点击 ``Show original error and build my own solution``。 | ||
| 2. 然后勾选以下两项： | ||
|
|
||
| - ``Update items already installed`` | ||
| - ``Remove items already installed`` | ||
|
|
||
| .. image:: ../../media/Resolve_update_error.png | ||
|
|
||
| 这样可以确保安装程序更新或替换任何存在冲突的组件，从而顺利完成安装。 | ||
|
|
||
| .. _troubleshooting_missing_items: | ||
|
|
||
| 故障排查 | ||
| -------- | ||
|
|
||
| 如果在安装过程中遇到错误提示 ``Cannot complete the install because one or more required items could not be found.``，通常是因为未启用 Eclipse 更新站点。 | ||
|
|
||
| 解决方法： | ||
|
|
||
| 1. 进入 ``Help`` > ``Install New Software``。 | ||
| 2. 点击 ``Manage``。 | ||
| 3. 确保已启用 ``Eclipse Platform - Latest Release Update Site`` 选项。 | ||
|
|
||
| .. image:: ../../media/Resolve_update_error_2.png | ||
|
|
||
| 4. 应用更改并关闭对话框。 | ||
| 5. 前往 ``Help`` > ``Check for Updates``，继续更新 IDE 及其依赖项。 | ||
|
|
||
| .. note:: | ||
|
|
||
| 启用 Eclipse Platform 更新站点可确保在安装或升级过程中解析所有必要的依赖项。 |
There was a problem hiding this comment.
🧩 Analysis chain
Verify parity with English documentation and validate referenced resources.
The Chinese documentation is well-structured with proper reStructuredText formatting, comprehensive installation instructions (update site, marketplace, local archive), clear upgrade guidance, and practical troubleshooting steps. To ensure completeness:
- Verify the corresponding English version (
docs/en/marketplaceupdate.rst) exists with matching structure and section coverage. - Confirm that all referenced image files (
../../media/idf_update_site_install.png,../../media/Update_plugins.png, etc.) are present and correctly located. - Validate that external URLs (GitHub releases, Espressif CDN update site links) are current and accessible.
🏁 Script executed:
#!/bin/bash
# Description: Verify parity between English and Chinese documentation, and validate image references.
# Test 1: Check if English version exists
echo "=== Checking for English documentation file ==="
if [ -f "docs/en/marketplaceupdate.rst" ]; then
echo "✓ English version found"
# Compare section structure (count main sections)
en_sections=$(grep -c "^-*$" docs/en/marketplaceupdate.rst)
zh_sections=$(grep -c "^-*$" docs/zh_CN/marketplaceupdate.rst)
echo "English sections: $en_sections, Chinese sections: $zh_sections"
else
echo "✗ English version NOT found"
fi
# Test 2: Check if all referenced images exist
echo ""
echo "=== Checking for referenced image files ==="
rg -o '../../media/[^\s]+\.png' docs/zh_CN/marketplaceupdate.rst | sort -u | while read img_ref; do
img_path="docs/${img_ref#../../}"
if [ -f "$img_path" ]; then
echo "✓ Found: $img_path"
else
echo "✗ Missing: $img_path"
fi
done
# Test 3: Check for broken references/anchors
echo ""
echo "=== Checking for reStructuredText reference definitions ==="
rg '^\.\. _[a-zA-Z_]+:' docs/zh_CN/marketplaceupdate.rst | head -10Length of output: 684
I need to verify whether the image files are also missing from the English version and check if they exist elsewhere in the repository.
🏁 Script executed:
#!/bin/bash
# Check if English version references the same images
echo "=== Checking image references in English version ==="
rg -o '../../media/[^\s]+\.png' docs/en/marketplaceupdate.rst | sort -u
# Search for these image files anywhere in the repo
echo ""
echo "=== Searching for referenced images in entire repo ==="
for img in "idf_update_site_install.png" "Update_plugins.png" "Update_dependencies.png" "Resolve_update_error.png" "Resolve_update_error_2.png"; do
echo "Searching for: $img"
fd -type f -name "$img" . || echo " Not found"
done
# Check if media directory exists and what's in it
echo ""
echo "=== Checking docs/media directory ==="
if [ -d "docs/media" ]; then
echo "Directory exists. Contents:"
ls -la docs/media/ | head -20
else
echo "docs/media directory does NOT exist"
fiLength of output: 1479
All referenced image files are missing from the repository.
The Chinese documentation properly mirrors the English version (identical structure, matching section counts), and reStructuredText syntax is valid. However, all 5 referenced image files are missing:
idf_update_site_install.pngUpdate_plugins.pngUpdate_dependencies.pngResolve_update_error.pngResolve_update_error_2.png
The docs/media directory does not exist. Both English (docs/en/marketplaceupdate.rst) and Chinese documentation reference these images, making the installation guide incomplete. The documentation instructs users to reference screenshots (e.g., line 44: ".. image:: ../../media/idf_update_site_install.png") but these visual aids are not present in the repository.
Required: Include the 5 missing PNG files in docs/media/ or remove the image references from the documentation.
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 (12)
docs/en/additionalfeatures/cmakeeditor.rst(1 hunks)docs/en/additionalfeatures/configuretoolchain.rst(1 hunks)docs/en/additionalfeatures/coredumpdebugging.rst(1 hunks)docs/en/additionalfeatures/install-esp-components.rst(1 hunks)docs/en/additionalfeatures/partitiontableeditor.rst(1 hunks)docs/en/additionalfeatures/switchlanguage.rst(1 hunks)docs/zh_CN/additionalfeatures/cmakeeditor.rst(1 hunks)docs/zh_CN/additionalfeatures/configuretoolchain.rst(1 hunks)docs/zh_CN/additionalfeatures/coredumpdebugging.rst(1 hunks)docs/zh_CN/additionalfeatures/install-esp-components.rst(1 hunks)docs/zh_CN/additionalfeatures/partitiontableeditor.rst(1 hunks)docs/zh_CN/additionalfeatures/switchlanguage.rst(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/en/additionalfeatures/partitiontableeditor.rst
- docs/en/additionalfeatures/cmakeeditor.rst
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/zh_CN/additionalfeatures/coredumpdebugging.rst
- docs/zh_CN/additionalfeatures/install-esp-components.rst
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_macos
🔇 Additional comments (11)
docs/en/additionalfeatures/coredumpdebugging.rst (4)
4-4: Documentation improvements well-executed.The title underline normalization and addition of the translation link follow standard RST conventions and align with the PR's broader localization efforts. The
:link_to_translation:directive is a good addition for multi-language support.Also applies to: 6-6
8-8: Clarity enhancements improve user guidance.The description now explicitly states the prerequisites and supported modes (UART-only), and the section heading is more action-oriented. These changes make the documentation more accessible to users.
Also applies to: 10-10
12-13: Steps formatting is clearer with proper code markup.The inline code formatting for UI elements (
sdkconfig,Core Dump,Data Destination,UART) improves visual distinction and usability. Numbered steps are now properly formatted.
17-17: Consistent RST formatting and wording improvements.The clarification about Eclipse automatically loading the core dump is helpful, and the inline code formatting for the
stopbutton enhances consistency with the rest of the documentation.Also applies to: 21-21
docs/zh_CN/additionalfeatures/partitiontableeditor.rst (1)
1-31: Well-structured Chinese documentation addition.The new Partition Table Editor documentation for zh_CN is properly formatted as reStructuredText with clear sections, step-by-step instructions, and appropriate image references with Chinese alt text. The content aligns with the PR's goal of expanding Chinese localization and pairs naturally with the existing English version and broader EIM/tooling enhancements.
docs/en/additionalfeatures/install-esp-components.rst (2)
4-4: Confirm translation link directive is properly configured.The translation link to the Chinese version is correctly formatted. Ensure your Sphinx theme or configuration supports the
:link_to_translation:directive; otherwise, it may not render as intended.
8-10: Numbered list formatting and command syntax look good.The conversion from bullet points to a numbered list is well-formatted, and the updated command syntax "ESP-IDF: Install ESP-IDF Components" aligns with the EIM integration described in the PR objectives.
docs/en/additionalfeatures/configuretoolchain.rst (1)
1-71: Well-structured documentation aligned with EIM integration.The English documentation is properly formatted with valid RST syntax, clear step-by-step instructions, concrete examples, accessible alt text for figures, and logical progression for setting up custom/preview ESP-IDF targets. This aligns well with the PR's focus on EIM-driven toolchain and custom target support.
docs/zh_CN/additionalfeatures/configuretoolchain.rst (2)
1-3: Translation link on Line 4 may need verification.Line 4 contains
:link_to_translation:zh_CN:[中文]`` which mirrors the English version's link. In typical reciprocal documentation patterns, the Chinese version should link back to the English version (en_USor similar) rather than pointing to itself. Verify whether this translation link pattern is a repo-wide convention or if it needs correction.
5-71: Well-executed Chinese translation maintaining structure and localization.The Chinese documentation accurately mirrors the English version's structure and content with appropriate translation and localization. RST formatting, inline code, figure captions, and alt text are all properly maintained. The step-by-step instructions, concrete examples (paths, target names), and visual aids make the guide accessible to Chinese-speaking users.
docs/en/additionalfeatures/switchlanguage.rst (1)
1-4: Documentation structure and translation link look good.The title simplification and added translation link directive are appropriate for a release that improves i18n/l10n support. RST syntax is correct.
| Install ESP-IDF Components | ||
| =============================== | ||
| ========================== |
There was a problem hiding this comment.
Fix title underline length mismatch.
The title "Install ESP-IDF Components" is 27 characters, but the underline has only 26 equal signs. RST requires the underline to match or exceed the title length, or the document will render incorrectly.
Apply this diff to fix the underline:
Install ESP-IDF Components
-==========================
+===========================📝 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.
| Install ESP-IDF Components | |
| =============================== | |
| ========================== | |
| Install ESP-IDF Components | |
| =========================== |
🤖 Prompt for AI Agents
In docs/en/additionalfeatures/install-esp-components.rst around lines 1 to 2,
the title underline is one character short (title is 27 characters, underline
has 26 "="), so update the underline to match the title length by replacing the
existing underline with 27 equal signs (i.e., ensure the underline length equals
or exceeds the title length).
| CMake 编辑器 | ||
| ============ | ||
|
|
||
| :link_to_translation:`zh_CN:[中文]` | ||
|
|
||
| CMake 编辑器插件与 IDF 插件集成，可用于编辑 CMake 文件（例如 ``CMakeLists.txt``）。该插件提供语法高亮、CMake 命令自动补全和代码模板等功能。 | ||
|
|
||
| .. image:: ../../../media/cmake_editor_ca.png | ||
| :alt: 带内容提示的 CMake 编辑器 | ||
|
|
||
| CMake 编辑器的首选项可通过 ``Eclipse`` > ``Preferences`` > ``CMakeEd`` 进行设置。 | ||
|
|
||
| .. image:: ../../../media/cmake_editor_preferences.png | ||
| :alt: CMake 编辑器首选项 |
There was a problem hiding this comment.
🧩 Analysis chain
Verify image references and English documentation counterpart exist.
This new Chinese documentation file references images and a translation link that need validation:
- Line 4 uses
:link_to_translation:- a non-standard Sphinx role/directive. Confirm this is defined in your project's Sphinx extensions (typically inconf.py). - Lines 8 and 13 reference images using relative paths (
../../../media/cmake_editor_ca.pngand../../../media/cmake_editor_preferences.png). Verify these images exist atdocs/media/before build. - Confirm the English documentation exists at
docs/en/additionalfeatures/cmakeeditor.rstto match the AI summary.
🏁 Script executed:
#!/bin/bash
# Verify image files and English documentation existence
echo "=== Checking English documentation file ==="
if [ -f docs/en/additionalfeatures/cmakeeditor.rst ]; then
echo "✓ English file exists: docs/en/additionalfeatures/cmakeeditor.rst"
else
echo "✗ Missing: docs/en/additionalfeatures/cmakeeditor.rst"
fi
echo ""
echo "=== Checking image files ==="
for img in "cmake_editor_ca.png" "cmake_editor_preferences.png"; do
if [ -f "docs/media/$img" ]; then
echo "✓ Found: docs/media/$img"
else
echo "✗ Missing: docs/media/$img"
fi
done
echo ""
echo "=== Searching for link_to_translation directive definition ==="
if grep -r "link_to_translation" docs/conf.py 2>/dev/null || grep -r "add_role\|add_directive" docs/conf.py 2>/dev/null | grep -i "translation"; then
echo "✓ Found custom role/directive definition"
else
echo "⚠ Warning: link_to_translation not found in conf.py. Check if it's defined in a Sphinx extension."
echo "Searching in extension files..."
find . -name "*.py" -path "*/docs/*" -type f | xargs grep -l "link_to_translation" 2>/dev/null || echo "Not found in extension files"
fiLength of output: 455
Let me search more comprehensively to understand the broader context:
Let me search more comprehensively in the codebase for the translation link pattern and verify by checking the English file:
Now let me check the English file and search more broadly for the translation link pattern and media directory:
Based on my thorough verification, I have identified critical build-breaking issues:
Fix missing image files and undefined translation link role.
This new Chinese documentation contains two critical issues that will cause build failures:
-
Missing image files (lines 8, 13): The referenced images
cmake_editor_ca.pngandcmake_editor_preferences.pngdo not exist atdocs/media/. Sphinx will fail to build with missing image references. Ensure these files are added todocs/media/before merging. -
Undefined
:link_to_translation:role (line 4): This custom role is not defined in the project's Sphinx configuration or extensions. Custom roles must be registered viaconf.pyusingapp.add_role(). Either define this role or verify it comes from an installed extension and check the role name for typos.
Confirm the English counterpart at docs/en/additionalfeatures/cmakeeditor.rst exists and uses the same translation link pattern to ensure consistency.
🤖 Prompt for AI Agents
In docs/zh_CN/additionalfeatures/cmakeeditor.rst around lines 1 to 14, the file
references two missing images and an undefined custom role which will break
Sphinx builds; add the two image files (cmake_editor_ca.png and
cmake_editor_preferences.png) into docs/media/ (or update the paths to the
correct existing images), and either register the :link_to_translation: role in
docs/conf.py using app.add_role(...) or replace it with a supported translation
link pattern (or the exact role used in
docs/en/additionalfeatures/cmakeeditor.rst) to ensure consistency with the
English counterpart.
| 在 IDE 中切换语言 | ||
| ================= | ||
|
|
||
| :link_to_translation:`zh_CN:[中文]` |
There was a problem hiding this comment.
Correct the translation link language code.
Line 4 contains :link_to_translation:zh_CN:[中文] which points back to Chinese. Since this is the Chinese documentation file, it should link back to the English version instead: `:link_to_translation:`en:[English] (or the appropriate English label).
Apply this diff:
-:link_to_translation:`zh_CN:[中文]`
+:link_to_translation:`en:[English]`📝 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.
| :link_to_translation:`zh_CN:[中文]` | |
| :link_to_translation:`en:[English]` |
🤖 Prompt for AI Agents
In docs/zh_CN/additionalfeatures/switchlanguage.rst around line 4, the
translation link incorrectly points to Chinese; replace the token
:link_to_translation:`zh_CN:[中文]` with :link_to_translation:`en:[English]` so
the Chinese page links to the English version (update both the language code and
the label).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/en/additionalfeatures/appleveltracing.rst(3 hunks)docs/zh_CN/additionalfeatures/appleveltracing.rst(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_macos
🔇 Additional comments (6)
docs/zh_CN/additionalfeatures/appleveltracing.rst (2)
4-4: Good: Translation placeholder added for Chinese documentation.The bidirectional translation link is properly formatted and aligns with the English version update. This supports accessibility for both language communities.
6-6: All external links in the documentation are reachable and valid.Verification confirms that the three external links referenced in the documentation return HTTP 200 OK responses:
- ESP-IDF app_trace documentation (zh_CN): accessible ✓
- GitHub app_trace_to_host example (release/v5.0): accessible ✓
- GitHub app_trace_basic example (release/v5.1): accessible ✓
No issues detected with external links.
docs/en/additionalfeatures/appleveltracing.rst (4)
4-4: Good: Bidirectional translation link added.Translation placeholder is properly formatted with the corresponding Chinese language code (zh_CN) and link text. This maintains consistency with i18n practices.
25-33: Command syntax and parameters look accurate.The trace command syntax and parameter descriptions appear consistent with ESP-IDF app_trace documentation. All parameters include units and default values, aiding user understanding.
1-50: Verify Chinese translation accuracy and completeness.The Chinese translation appears structurally sound and follows the English version. However, a detailed translation review is outside the typical code review scope. Recommend having a native Chinese speaker or translation reviewer verify:
- Technical terminology consistency with UI and ESP-IDF Chinese documentation
- Accuracy of parameter descriptions and command syntax explanations
- Proper Chinese conventions for inline code formatting and menu references
- Consistency of image captions between English and Chinese versions
Additionally, verify that both versions refer to the same image files at the same relative paths.
16-16: The menu path in the documentation is misleading; it appears as a submenu rather than a single item with colon notation.The actual menu structure (verified in
bundles/com.espressif.idf.ui/plugin.xmllines 142–241) shows:
- A submenu labeled "ESP-IDF" is created on the Project Explorer context menu
- "Application Level Tracing" appears as a command item within that "ESP-IDF" submenu
The documentation states users should "select
ESP-IDF: Application Level Tracing" (suggesting a single menu item), but the implementation creates a hierarchical menu: right-click project → select "ESP-IDF" submenu → select "Application Level Tracing" item.Update the documentation to clarify the correct menu navigation: "right-click on the project in the Project Explorer, select the
ESP-IDFsubmenu, and then selectApplication Level Tracing" — or use a format like "ESP-IDF>Application Level Tracing" if single-line formatting is preferred.
|
|
||
| ESP-IDF 提供了一项用于程序行为分析的实用功能，称为 `应用级跟踪库 <https://docs.espressif.com/projects/esp-idf/zh_CN/latest/esp32c3/api-guides/app_trace.html>`_。IDF-Eclipse 插件提供了 UI 界面，可用于启动和停止跟踪命令并处理接收的数据。如需熟悉该库，你可以参考 `app_trace_to_host <https://github.com/espressif/esp-idf/tree/release/v5.0/examples/system/app_trace_to_host>`_ 示例项目，或者 `app_trace_basic <https://github.com/espressif/esp-idf/tree/release/v5.1/examples/system/app_trace_basic>`_ 示例项目（要求使用 ESP-IDF 5.1 及以上）。这些示例项目可以直接通过插件创建。 | ||
|
|
||
| .. image:: ../../../media/AppLvlTracing_1.png |
There was a problem hiding this comment.
🧩 Analysis chain
Script executed:
#!/bin/bash
# Find all AppLvlTracing image files in the repository
fd -type f -name "AppLvlTracing_*.png"Repository: espressif/idf-eclipse-plugin
<shell_result>
Repository: espressif/idf-eclipse-plugin
Exit code: 2
stderr:
error: 'ype' isn't a valid value for '--type <filetype>'
[possible values: file, directory, symlink, executable, empty, socket, pipe]
For more information try '--help'
</shell_result>
Script executed:
#!/bin/bash
# Find all AppLvlTracing image files in the repository
fd "AppLvlTracing_.*\.png$"Repository: espressif/idf-eclipse-plugin
<shell_result>
Repository: espressif/idf-eclipse-plugin
Exit code: 0
stdout:
docs_readme/images/AppLvlTracing_1.png
docs_readme/images/AppLvlTracing_2.png
docs_readme/images/AppLvlTracing_3.png
docs_readme/images/AppLvlTracing_4.png
docs_readme/images/AppLvlTracing_5.png
media/AppLvlTracing_1.png
media/AppLvlTracing_2.png
media/AppLvlTracing_3.png
media/AppLvlTracing_4.png
media/AppLvlTracing_5.png
</shell_result>
Fix incorrect relative path for image references; images exist at repository root media/ not docs/media/.
The documentation file at docs/zh_CN/additionalfeatures/appleveltracing.rst references images using the path ../../../media/AppLvlTracing_*.png, which would resolve to docs/media/. However, the image files actually exist at the repository root: media/AppLvlTracing_1.png through media/AppLvlTracing_5.png.
Correct the relative paths to ../../../../media/AppLvlTracing_*.png to properly reference the images from the documentation's location.
Applies to lines: 8, 13, 18, 37, 49
🤖 Prompt for AI Agents
In docs/zh_CN/additionalfeatures/appleveltracing.rst around lines 8, 13, 18, 37
and 49 the image references use ../../../media/... which resolves to docs/media/
but the images live at the repository root media/; update each image path to use
../../../../media/AppLvlTracing_*.png (i.e. replace ../../../media/ with
../../../../media/) so the images correctly reference media/AppLvlTracing_1.png
through AppLvlTracing_5.png.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/en/additionalfeatures/coredumpdebugging.rst(1 hunks)docs/en/additionalfeatures/install-esp-components.rst(1 hunks)docs/en/debugproject.rst(1 hunks)docs/en/installation.rst(1 hunks)docs/en/windowsofflineinstaller.rst(0 hunks)docs/zh_CN/additionalfeatures/coredumpdebugging.rst(1 hunks)docs/zh_CN/debugproject.rst(1 hunks)docs/zh_CN/installation.rst(1 hunks)
💤 Files with no reviewable changes (1)
- docs/en/windowsofflineinstaller.rst
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/zh_CN/debugproject.rst
- docs/zh_CN/installation.rst
- docs/en/additionalfeatures/install-esp-components.rst
🔇 Additional comments (13)
docs/en/additionalfeatures/coredumpdebugging.rst (1)
1-21: Documentation improvements look good.The English documentation file reads well with clear, active phrasing. The changes improve clarity around UART-only support and the debugging workflow. Formatting is consistent with proper RST syntax and code formatting for UI elements.
docs/zh_CN/additionalfeatures/coredumpdebugging.rst (1)
1-21: Chinese translation is complete and accurate.The translation mirrors the English documentation structure and content precisely, with proper technical terminology (sdkconfig, Core Dump, UART, Eclipse debug perspective). Formatting is consistent with the English version, and the translation link back to English is properly formatted. Once the heading underline is corrected, this file will be ready.
docs/en/debugproject.rst (3)
2-35: Well-structured documentation improvements.The restructuring into numbered steps significantly improves clarity and usability. The addition of the Windows driver note, embedded screenshot, and cross-reference to related documentation provides comprehensive guidance. The changes maintain proper reStructuredText syntax throughout.
35-35: Cross-reference target verified — no issues found.The cross-reference target
OpenOCDDebuggingis properly defined indocs/en/openocddebugging.rst(line 1) anddocs/zh_CN/openocddebugging.rst(line 1). The:ref:link on line 35 ofdebugproject.rstwill resolve correctly.
4-4: Thelink_to_translationrole is properly configured through theesp-docs>=1.4.0dependency.The role is defined in the
esp-docspackage (as specified indocs/requirements.txt), which is imported viaesp_docs.conf_docsindocs/conf_common.py. This configuration is correct, and the role is used consistently across 69+ documentation files throughout the project, includingdocs/en/debugproject.rst.docs/en/installation.rst (8)
44-44: Clarify workspace auto-conversion behavior.Line 44 states: "When you open a workspace created with an old version of Espressif-IDE, the IDE will prompt you to convert its configuration to be compatible with EIM."
This auto-detection and prompting behavior is central to the user experience. Verify this is the actual behavior; if not, update the documentation or clarify the conditions under which prompts appear.
6-10: Structure and clarity of three-step process is clear.The reframing of the installation into three logical steps (IDE installation → ESP-IDF/tools installation → workspace migration) is well-organized and makes the flow intuitive for users transitioning from older versions.
39-75: Step-by-step EIM integration instructions are well-structured.The reworked ESP-IDF installation section provides clear, numbered steps with UI references and explanatory notes. The emphasis on EIM as the single source of truth for tool management is well-communicated, and the refresh/activation mechanics are clearly explained.
76-95: Migration section addresses old workspace conversion gap.The addition of a dedicated "Old-to-New Workspace Migration" section is essential for users upgrading from earlier versions. OS-specific guidance for EIM placement and step-by-step conversion instructions are appropriate, pending verification of paths and actual behavior.
48-48: I'll help you verify the menu structure and button names referenced in the documentation. Let me start by exploring the repository structure to understand the codebase and locate the relevant UI definitions.
<function_calls>
#!/bin/bashGet an overview of the repository structure
git ls-files | head -30
echo "---"Check if there's a README or version info
ls -la | grep -E "README|VERSION|package.json|version"
</function_calls>Let me search for the documentation file and understand the project better:
<function_calls>
#!/bin/bashFind and read the installation documentation file
find . -name "installation.rst" -type f
echo "---"Check repository root for key files
ls -la | head -20
</function_calls>Now let me search for UI definitions, menu paths, and button labels in the codebase:
<function_calls>
#!/bin/bashSearch for menu structure and button definitions
echo "=== Searching for 'Espressif' and 'ESP-IDF Manager' ==="
rg -i "espressif|esp-idf.?manager" -B 2 -A 2 --max-count=20echo ""
echo "=== Searching for 'Launch EIM' or 'Download' button labels ==="
rg -i "launch.?eim|download.*launch|download.*eim" -B 2 -A 2 --max-count=20echo ""
echo "=== Looking for UI definition files ==="
fd -e xml -e properties -e json | grep -i "ui|menu|button|label" | head -20
</function_calls>Let me check for version information and explore the project more thoroughly:
<function_calls>
#!/bin/bashCheck for version files and project metadata
echo "=== Version information ==="
find . -name "package.json" -o -name "version.txt" -o -name "VERSION" -o -name "_version.py" | head -10echo ""
echo "=== File structure overview ==="
git ls-files | cut -d/ -f1 | sort -u | head -20echo ""
echo "=== Looking for extension manifest or plugin definitions ==="
find . -name "extension.xml" -o -name "plugin.xml" -o -name "manifest.json" -o -name "package.json" | head -10
</function_calls><pending_user_response>
I'm ready to analyze the shell results to locate the relevant UI definitions and verify the menu paths and button labels. Please run these scripts so I can examine the codebase structure and search for the specific UI elements mentioned in the documentation.
</pending_user_response>
50-50: All referenced screenshot files exist and are correctly referenced.Verification confirms that all four PNG files mentioned in the documentation exist in the repository:
media/ToolsManager/ESP-IDF_Manager_Editor_Screen.pngmedia/ToolsManager/Tool_installed_and_activated.pngmedia/ToolsManager/ESP-IDF_Manager_Changed_Installation_Message.pngmedia/ToolsManager/ESP-IDF_Manager_Conversion.pngThe relative paths in
docs/en/installation.rstcorrectly resolve to these files in themedia/ToolsManager/directory. All files are tracked in git and available for the v4.0.0 release.
85-87: All OS-specific EIM installation paths documented in lines 85-87 are accurate and match the implementation.Verification against source code confirms:
- Windows
C:\Users\<username>\.espressif\eim_gui\eim.exe: Correctly reflectsToolInitializer.getDefaultEimPath()which constructsSystem.getProperty("user.home") + .espressif/eim_gui/eim.exe- Linux
~/.espressif/eim_gui/eim: Correctly reflects the path constructed for non-Windows/non-macOS systems, withEimLoader.extractAndPlaceLinux()confirming extraction to this location- macOS Copy
eim.appto/Applications/eim.app: Correctly reflects the bundle placement thatToolInitializer.getDefaultEimPath()andEimLoader.extractMacosApp()expectNo changes required.
42-42: All EIM links are accessible and current for v4.0.0 release.Verification confirms that the documentation links referenced are accurate and maintained:
- Line 23:
https://docs.espressif.com/projects/idf-im-ui/en/latest/— Official EIM documentation, confirmed accessible and current.- Lines 42 & 81:
https://dl.espressif.com/dl/eim/— Official Espressif download page for EIM, confirmed as current download endpoint for GUI/CLI installers.All referenced URLs are compatible with Espressif-IDE v4.0.0 and remain actively maintained by Espressif. No updates or corrections needed.
| 核心转储调试 | ||
| ============ |
There was a problem hiding this comment.
Fix heading underline length.
The RST heading underline should match the character count of the heading text. "核心转储调试" is 6 characters, but the underline has 12 equals signs. This may cause rendering issues in some RST processors.
Apply this diff to fix the underline length:
核心转储调试
-============
+======📝 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.
| 核心转储调试 | |
| ============ | |
| 核心转储调试 | |
| ====== |
🤖 Prompt for AI Agents
In docs/zh_CN/additionalfeatures/coredumpdebugging.rst around lines 3 to 4, the
RST heading underline is too long: the heading "核心转储调试" is 6 characters but the
underline has 12 '=' characters; replace the underline with exactly six '='
characters so the underline length matches the heading text length to ensure
correct RST rendering.
This is just a basic PR for reviewing the Rebase of master on v4.0 branch to avoid any missing parts.
Further once I close a few more tickets for release v4.0 we can use this as a release testing build as well
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.