IEP-1789: Add support for eim_idf.json 3.0#1485
Open
sigmaaa wants to merge 10 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR replaces ChangesEIM JSON model abstraction and UI migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java (1)
384-400: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWait for old-config export before reloading and reopening the manager.
If the old config branch runs here,
handleOldConfigExport()only queuesstartExportOldConfig()on the UI thread, so Line 391 can reload before conversion finishes and Line 400 can open the manager with stale or missingeim_idf.json.Suggested fix
- if (toolInitializer.isOldEspIdfConfigPresent() && !toolInitializer.isOldConfigExported()) - { - Logger.log("Old configuration found and not converted"); - handleOldConfigExport(); - } - try - { - eimConfig = toolInitializer.loadEimConfig(); - } - catch (EimVersionMismatchException e) - { - Logger.log(e); - MessageDialog.openError(Display.getDefault().getActiveShell(), e.msgTitle(), - e.getMessage()); - return; - } - openEspIdfManager(); + Runnable reloadAndOpen = () -> { + try + { + eimConfig = toolInitializer.loadEimConfig(); + openEspIdfManager(); + } + catch (EimVersionMismatchException e) + { + Logger.log(e); + MessageDialog.openError(Display.getDefault().getActiveShell(), e.msgTitle(), + e.getMessage()); + } + }; + if (toolInitializer.isOldEspIdfConfigPresent() && !toolInitializer.isOldConfigExported()) + { + Logger.log("Old configuration found and not converted"); + handleOldConfigExport(reloadAndOpen); + return; + } + reloadAndOpen.run();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java` around lines 384 - 400, The startup flow in EspressifToolStartup should not continue past handleOldConfigExport() until the old-config conversion has finished, because it only schedules startExportOldConfig() asynchronously on the UI thread. Update the branch around isOldEspIdfConfigPresent()/isOldConfigExported() so loadEimConfig() and openEspIdfManager() wait for the export to complete, ensuring eim_idf.json is present before reloading and reopening the manager.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/EimIdfJsonLoader.java`:
- Around line 49-56: `EimIdfJsonLoader.load` currently uses `FileReader` and
directly calls `JsonParser.parseReader(...).getAsJsonObject()` and
`root.get("version").getAsString()`, which can throw unchecked exceptions and
escape `ToolInitializer`’s checked error handling. Switch the loader to a UTF-8
reader, and wrap JSON-shape/parse failures in an `IOException` from `load` so
malformed, non-object, or null-version payloads fail cleanly. Also remove the
now-unused `FileReader` import.
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/EimJsonVersion.java`:
- Around line 32-55: The EimJsonVersion.parse flow currently lets malformed
version strings escape as NumberFormatException from parseVersionNumber instead
of the declared EimVersionMismatchException. Update parse and/or
parseVersionNumber so inputs like "v3" or "3.x" are caught and normalized into
EimVersionMismatchException, using the existing EimJsonVersion.parse and
parseVersionNumber methods to preserve the schema-version mismatch contract.
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/InstallationStatus.java`:
- Around line 41-54: The InstallationStatus.fromJson parser currently falls back
to FINISHED for any unknown status, which can incorrectly mark bad or future
schema-3 values as activatable. Update fromJson to fail closed by mapping
unrecognized values to a non-activatable state such as BROKEN, or by rejecting
the document before it reaches EimConfigAdapterV3 and
DefaultEimInstallationModel. Keep the null/blank handling, but ensure only known
status strings return a healthy state.
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/presentation/DefaultEimInstallationPresentationRenderer.java`:
- Around line 29-32: The FINISHED branch in
DefaultEimInstallationPresentationRenderer currently returns an empty status
text for inactive installations, which causes ESPIDFMainTablePage to render a
blank cell. Update the inactive path in the renderer to supply an explicit
visible status string via EimInstallationPresentation.StatusKind.INACTIVE and
the status text field instead of "", keeping the active path unchanged.
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java`:
- Around line 188-189: Validate the Python path once at the start of
SetupToolsInIde before any setup-side effects, using installation.getPython() in
the same place you already resolve it for the activation-script branch, and fail
fast there if it is missing. Then pass that resolved pythonPath through the
later consumers in SetupToolsInIde instead of calling
installation.getPython().orElseThrow() again, so setupEnvVarsInEclipse() and the
other setup steps never begin mutating IDE state before the required path is
confirmed.
- Around line 99-108: The activation-script launch is still using the unmodified
system environment even though SetupToolsInIde prepares an augmented env with
config.getGitPath(). Update the process start path in the same flow that uses
getActivationScript() and ToolsUtility.getExportScriptCommand() to pass the env
map you built, so the activation script sees the added PATH/Git entry. Keep the
existing missing-script handling unchanged and ensure the environment passed to
the script is the one modified by addGitToEnvironment().
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java`:
- Around line 170-175: Guard the configured eimPath in
ToolInitializer.getEimPath() before passing it to Paths.get(...), since
config.getEimPath() can contain an invalid path from eim_idf.json and currently
causes InvalidPathException. Update the logic around the eimPath optional to
validate or safely catch path parsing failures before Files.exists(...) so
invalid config values fall through to PATH/default resolution instead of
breaking EspressifToolStartup.earlyStartup(). Use the existing getEimPath() flow
and the eimPath variable as the main place to apply the fix.
In
`@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/ESPIDFManagerEditor.java`:
- Around line 88-92: The first-startup setup is being triggered too early in
ESPIDFManagerEditor, before the refresh job has populated the table. Move the
call to setupInitialEspIdf() out of the createPage() flow and into the refresh
completion/UI update path used by refreshEditorUI(), so it runs only after rows
are loaded and first-startup activation is not skipped.
In
`@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java`:
- Around line 81-91: The EIM JSON state is being marked as seen even when
accepted handling fails, so update the flow in EimJsonUiChangeHandler to persist
state only after launchEspIdfManager reports success. Make launchEspIdfManager
return a boolean and have it return true only after IDE.openEditor(...) and the
table refresh dispatch complete successfully, then call
EimJsonStateChecker.updateLastSeenState() only when that success path is
reached. Keep the IOException handling in the validation/launch path from
swallowing failures that should prevent state persistence.
In
`@tests/com.espressif.idf.core.test/src/com/espressif/idf/core/tools/test/EimIdfJsonLoaderTest.java`:
- Around line 67-73: The V3 fixture in EimIdfJsonLoaderTest is not testing the
BROKEN-status path because DefaultEimInstallationModel.isActivatable() also
depends on activationScript being present. Update the fixture used by the
idfInstalled entry in this test to include an activationScript so activation is
possible except for status, and keep the assertions focused on
isActivatable()/presentation.isActivateEnabled() reflecting the broken status
mapping.
---
Outside diff comments:
In
`@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java`:
- Around line 384-400: The startup flow in EspressifToolStartup should not
continue past handleOldConfigExport() until the old-config conversion has
finished, because it only schedules startExportOldConfig() asynchronously on the
UI thread. Update the branch around
isOldEspIdfConfigPresent()/isOldConfigExported() so loadEimConfig() and
openEspIdfManager() wait for the export to complete, ensuring eim_idf.json is
present before reloading and reopening the manager.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee81527f-7e81-4db6-b8ec-7450c075e85b
📒 Files selected for processing (35)
bundles/com.espressif.idf.core/META-INF/MANIFEST.MFbundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/Messages.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/EimIdfJsonLoader.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/EimJsonVersion.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/InstallationStatus.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/adapter/EimConfigAdapter.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/adapter/EimConfigAdapterFactory.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/adapter/EimConfigAdapterV2.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/adapter/EimConfigAdapterV3.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/model/DefaultEimConfigModel.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/model/DefaultEimInstallationModel.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/model/EimConfigModel.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/model/EimInstallationModel.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/presentation/DefaultEimInstallationPresentationRenderer.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/presentation/EimInstallationPresentation.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/presentation/EimInstallationPresentationRenderer.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/presentation/EimInstallationPresentationRendererFactory.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/schema/v3/EimJsonV3.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/schema/v3/IdfInstalledV3.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/messages.propertiesbundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EimButtonLaunchListener.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/SetupToolsJobListener.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/ESPIDFManagerEditor.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/EimEditorInput.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/render/EimInstallationStatusSwtMapper.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.javatests/com.espressif.idf.core.test/src/com/espressif/idf/core/tools/test/EimIdfJsonLoaderTest.java
💤 Files with no reviewable changes (1)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/model/EimInstallationModel.java`:
- Around line 72-75: `EimInstallationModel.isActivatable()` currently enables
activation whenever the status and activation script are present, but it also
needs to require Python so incomplete V2/V3 entries stay disabled. Update the
`isActivatable()` check to include the Python field/presence alongside
`getStatus().isActivatable()` and `getActivationScript().isPresent()`, so
`ESPIDFMainTablePage` won’t expose activation for installations that later fail
in the setup flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be8e492d-ab36-4dfe-94da-a18771abf327
📒 Files selected for processing (12)
bundles/com.espressif.idf.core/META-INF/MANIFEST.MFbundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/EimIdfJsonLoader.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/model/EimConfigModel.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/model/EimInstallationModel.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/presentation/EimInstallationPresentation.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/presentation/EimInstallationPresentationRenderer.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IdfInstalled.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/SetupToolsJobListener.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.javatests/com.espressif.idf.core.test/src/com/espressif/idf/core/tools/test/EimIdfJsonLoaderTest.java
💤 Files with no reviewable changes (3)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IdfInstalled.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java
- bundles/com.espressif.idf.core/META-INF/MANIFEST.MF
🚧 Files skipped from review as they are similar to previous changes (5)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/SetupToolsJobListener.java
- tests/com.espressif.idf.core.test/src/com/espressif/idf/core/tools/test/EimIdfJsonLoaderTest.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/eimjson/presentation/EimInstallationPresentation.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java
sigmaaa
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Support EIM eim_idf.json schema 3.0 by reading the new per-installation status field (in_progress, failed, finished, being_repaired, broken) and showing it in the ESP-IDF Manager table with color-coded labels. Activation is allowed only for healthy finished installs with valid Python and activation metadata. Older schemas (2.0 and 1.0) remain supported via tolerant parsing — missing status defaults to finished, so users can update the IDE without upgrading EIM.
Fixes # (IEP-1789)
Type of change
Please delete options that are not relevant.
How has this been tested?
Manual — schema and ESP-IDF Manager
Tested with both 2.0 and 3.0
eim_idf.jsonfiles (~/.espressif/tools/eim_idf.jsonon macOS/Linux,C:\Espressif\tools\eim_idf.jsonon Windows).status). Installs load; status shows Active/inactive; Activate works for valid installsin_progress,failed,being_repaired,broken,finished— correct label, color, and Activate enabled only forfinishedstatusvalue treated asbroken; Activate disabledfinishedinstall → env vars/toolchains update; sample project builds"version": "4.0"→ version mismatch errorManual — offline changes (IDE was closed)
Uses
EimJsonStateCheckeron startup.eim_idf.json→ restart → offline-change prompt appearsManual — online changes (IDE running)
Uses
EimJsonWatchServicewhile IDE is open.eim_idf.json→ change prompt; version cache invalidatedin_progress→finished) while IDE open → status updates; Activate enabled only whenfinishedManual — fresh install and upgrade paths
Regression (refactor touchpoints)
SetupToolsInIdecompletes;IDF_PATH, toolchains, and LSP paths updatepythonpath is not activatable even whenstatus: finishedTest Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
eim_idf.jsonhandling with schema/version compatibility parsing and more robust model loading.