Open
Conversation
📝 WalkthroughWalkthroughThe PR converts ESPIDFMainTablePage from a singleton pattern to direct instantiation via constructor, introduces a multi-column TableViewer with IdfRow data model replacing the previous single-column editor, adds async processing with CompletableFuture, and updates all call sites accordingly. UI messages for guidance text and button labels are updated to reflect the new table-based interaction model. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…se UI guidelines" This reverts commit 987e64f.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (4)
224-250:Display.getDefault()called repeatedly inside label provider methods.
getForeground()callsDisplay.getDefault().getSystemColor(...)up to two times per cell render. While not a correctness bug, it's more idiomatic and slightly more efficient to obtain the display from the viewer's control once.Sketch
`@Override` public Color getForeground(Object element) { var row = (IdfRow) element; + var display = Display.getCurrent(); if (row.original().equals(currentInstallingNode)) - return Display.getDefault().getSystemColor(SWT.COLOR_DARK_YELLOW); - return row.isActive() ? Display.getDefault().getSystemColor(SWT.COLOR_DARK_GREEN) - : Display.getDefault().getSystemColor(SWT.COLOR_GRAY); + return display.getSystemColor(SWT.COLOR_DARK_YELLOW); + return row.isActive() ? display.getSystemColor(SWT.COLOR_DARK_GREEN) + : display.getSystemColor(SWT.COLOR_GRAY); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java` around lines 224 - 250, The label provider is repeatedly calling Display.getDefault() in getForeground; instead capture the Display once from the viewer (e.g. final Display display = viewer.getControl().getDisplay()) before calling createCol and use display.getSystemColor(...) inside the anonymous ColumnLabelProvider methods; update the references in getForeground (and anywhere else in this provider) to use that cached display and avoid repeated Display.getDefault() calls, keeping the rest of the logic (IdfRow, currentInstallingNode, isActive) unchanged.
341-356: Dead code:else if (first instanceof IdfInstalled)branch is unreachable.The table input is always
List<IdfRow>(set at line 406), soselection.getFirstElement()will always be anIdfRow, never a rawIdfInstalled. This branch adds confusion with no benefit.Proposed fix
private IdfInstalled getSelectedIdf() { var selection = (IStructuredSelection) tableViewer.getSelection(); if (selection.isEmpty()) return null; Object first = selection.getFirstElement(); if (first instanceof IdfRow firstRow) { return firstRow.original(); } - else if (first instanceof IdfInstalled rawInstalled) - { - return rawInstalled; - } return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java` around lines 341 - 356, The getSelectedIdf method contains an unreachable else-if branch checking "first instanceof IdfInstalled"; since the table input is always List<IdfRow>, remove the unreachable branch and simplify the method to only handle IdfRow cases: cast/unwrap the first selection to IdfRow (using IdfRow firstRow) and return firstRow.original(), keeping the existing empty-selection/null handling and preserving use of tableViewer and IStructuredSelection.
85-101: UnusedLocalResourceManager— created but never stored or used.Line 87 creates a
LocalResourceManagerwhose reference is immediately discarded. If no managed resources (images, colors, fonts) are allocated through it, the allocation is pointless. If managed resources are planned, store the reference in a field so it can be used.Proposed fix: remove unused allocation
public Composite createPage(Composite parent) { - new LocalResourceManager(JFaceResources.getResources(), parent); - container = new Composite(parent, SWT.NONE);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java` around lines 85 - 101, The LocalResourceManager instance created in createPage (new LocalResourceManager(JFaceResources.getResources(), parent)) is never stored or used; either remove that unused allocation or assign it to a field (e.g., a private LocalResourceManager resourceManager) and use it for any managed resources and dispose it in the page's lifecycle (e.g., on dispose) to avoid leaks; update createPage to set resourceManager = new LocalResourceManager(...) and ensure disposeResourceManager is called when the page/container is disposed.
374-391: UncheckedRuntimeExceptionwrapping loses the original exception's context in logs.Line 390 wraps the caught exception in a
RuntimeException, and theexceptionallyhandler at line 410 logs onlyex.getCause().toString(). The stack trace is lost. Consider logging withLogger.log(Logger.ERROR, ...)or similar that preserves the stack, or at minimum logex.getCause()with its stack trace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java` around lines 374 - 391, The supplyAsync block in ESPIDFMainTablePage catches exceptions and throws new RuntimeException(e), which loses stacktrace clarity and pairs poorly with the existing exceptionally handler that logs ex.getCause().toString(); replace the throw with a CompletionException(e) (or simply let the exception propagate) so CompletableFuture preserves the original cause, and update the exceptionally handler that currently logs ex.getCause().toString() to log the full throwable (e.g., Logger.log(Logger.ERROR, "Failed to load IDF rows", ex.getCause()) or logger.error("...", ex.getCause())) so the stacktrace is preserved; look for CompletableFuture.supplyAsync(...) and the matching exceptionally(...) block to apply these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java`:
- Around line 130-198: Replace all hardcoded user-visible strings in
createMainContent and associated UI code with entries from the Messages resource
bundle: change group.setText("Installed IDF Versions") to
group.setText(Messages.EspIdfManagerInstalledVersions), change the
createActionButton label/tooltips ("Activate", "Set this version as the active
ESP-IDF", "Update Environment", the long tooltip) to Messages keys (e.g.,
Messages.EspIdfManagerActivate, Messages.EspIdfManagerActivateTooltip,
Messages.EspIdfManagerUpdateEnvironment,
Messages.EspIdfManagerUpdateEnvironmentTooltip), and replace the
column/header/status strings ("Status", "Setting up...", "\u2713 Active") used
in createColumns or wherever status text is emitted with
Messages.EspIdfManagerStatus, Messages.EspIdfManagerSettingUp,
Messages.EspIdfManagerActive (or similar keys). Add the corresponding keys to
the Messages properties and Java Messages accessor class following the existing
pattern used for Messages.EspIdfManagerVersionCol so that all UI text is
externalized for i18n.
---
Duplicate comments:
In
`@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java`:
- Around line 469-483: The comparator in compare(Viewer, Object, Object) casts
to IdfRow and calls compareToIgnoreCase on r1.version()/name()/path(), which can
NPE if those fields are null; modify the switch in compare to perform null-safe
comparisons (e.g., normalize to empty string or use explicit null checks and
decide null ordering) for cases 1, 2, 3 so compareToIgnoreCase is never invoked
on null, and keep the existing Boolean.compare for case 0 and the direction flip
using (direction == SWT.UP) ? -rc : rc unchanged; locate this logic in the
compare method of ESPIDFMainTablePage.java where propertyIndex is switched and
update those branches to handle nulls consistently.
- Around line 7-8: The code currently imports and uses AWT's java.awt.Desktop
(and java.net.URI) in ESPIDFMainTablePage; replace any
Desktop.getDesktop().browse(...) or Desktop.open(...) calls with the SWT-native
Program.launch(...) API: import org.eclipse.swt.program.Program, convert the URI
to a String (uri.toString() or the original URL string) and call
Program.launch(urlString); remove the java.awt.Desktop import and the direct
java.net.URI use if not needed (or keep URI only to build the string), and
adjust exception handling accordingly since Program.launch returns boolean
rather than throwing IOExceptions.
- Around line 374-412: The code calls configParser.getEimJson(true) on the
background thread but then re-calls configParser.getEimJson(false) on the UI
thread (in ESPIDFMainTablePage’s CompletableFuture.thenAcceptAsync block),
risking inconsistent state and UI blocking; fix by computing and capturing the
parsed EIM JSON on the background thread (the CompletableFuture.supplyAsync that
creates rows) and pass or store that result so the UI thread reuses it instead
of calling getEimJson(false). Concretely, modify the supplyAsync work (the
lambda that calls configParser.getEimJson(true) and builds IdfRow list) to also
return or set the EIM JSON (e.g., return a small holder object or set
this.eimJson inside the background thread safely) and remove the call to
configParser.getEimJson(false) from the thenAcceptAsync block so
updateLaunchButtonState(), tableViewer.setInput(...), and updateButtonState()
use the already-parsed EIM JSON and the computed rows.
- Around line 59-61: IdfRow's components (version, name, path) can be null
causing NPEs in label providers and the comparator; add a compact constructor to
the record IdfRow that normalizes null Strings to "" (empty string) for the
version, name and path fields so callers (where IdfRow is constructed from
ToolsUtility.getIdfVersion(...), idf.getName(), idf.getPath()) never get null
values; update the compact constructor inside IdfRow to coerce null -> "" for
each String parameter and leave original and isActive unchanged so existing
label provider code and the comparator continue to work without NPEs.
---
Nitpick comments:
In
`@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java`:
- Around line 224-250: The label provider is repeatedly calling
Display.getDefault() in getForeground; instead capture the Display once from the
viewer (e.g. final Display display = viewer.getControl().getDisplay()) before
calling createCol and use display.getSystemColor(...) inside the anonymous
ColumnLabelProvider methods; update the references in getForeground (and
anywhere else in this provider) to use that cached display and avoid repeated
Display.getDefault() calls, keeping the rest of the logic (IdfRow,
currentInstallingNode, isActive) unchanged.
- Around line 341-356: The getSelectedIdf method contains an unreachable else-if
branch checking "first instanceof IdfInstalled"; since the table input is always
List<IdfRow>, remove the unreachable branch and simplify the method to only
handle IdfRow cases: cast/unwrap the first selection to IdfRow (using IdfRow
firstRow) and return firstRow.original(), keeping the existing
empty-selection/null handling and preserving use of tableViewer and
IStructuredSelection.
- Around line 85-101: The LocalResourceManager instance created in createPage
(new LocalResourceManager(JFaceResources.getResources(), parent)) is never
stored or used; either remove that unused allocation or assign it to a field
(e.g., a private LocalResourceManager resourceManager) and use it for any
managed resources and dispose it in the page's lifecycle (e.g., on dispose) to
avoid leaks; update createPage to set resourceManager = new
LocalResourceManager(...) and ensure disposeResourceManager is called when the
page/container is disposed.
- Around line 374-391: The supplyAsync block in ESPIDFMainTablePage catches
exceptions and throws new RuntimeException(e), which loses stacktrace clarity
and pairs poorly with the existing exceptionally handler that logs
ex.getCause().toString(); replace the throw with a CompletionException(e) (or
simply let the exception propagate) so CompletableFuture preserves the original
cause, and update the exceptionally handler that currently logs
ex.getCause().toString() to log the full throwable (e.g.,
Logger.log(Logger.ERROR, "Failed to load IDF rows", ex.getCause()) or
logger.error("...", ex.getCause())) so the stacktrace is preserved; look for
CompletableFuture.supplyAsync(...) and the matching exceptionally(...) block to
apply these changes.
Comment on lines
+130
to
+198
| private void createMainContent(Composite parent) | ||
| { | ||
| if (idfInstalledList != null && idfInstalledList.size() == 1) | ||
| var group = new Group(parent, SWT.SHADOW_ETCHED_IN); | ||
| group.setText("Installed IDF Versions"); | ||
| group.setLayout(new GridLayout(2, false)); | ||
| group.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true)); | ||
|
|
||
| // --- Table --- | ||
| var tableComp = new Composite(group, SWT.NONE); | ||
| var tableLayout = new TableColumnLayout(); | ||
| tableComp.setLayout(tableLayout); | ||
| tableComp.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true)); | ||
|
|
||
| tableViewer = new TableViewer(tableComp, SWT.BORDER | SWT.FULL_SELECTION | SWT.V_SCROLL | SWT.H_SCROLL); | ||
| var table = tableViewer.getTable(); | ||
| table.setHeaderVisible(true); | ||
| table.setLinesVisible(true); | ||
| table.addListener(SWT.MeasureItem, e -> e.height = 28); | ||
|
|
||
| tableViewer.setContentProvider(ArrayContentProvider.getInstance()); | ||
| tableViewer.setComparator(comparator); | ||
|
|
||
| createColumns(tableViewer, tableLayout); | ||
|
|
||
| // --- Buttons --- | ||
| var buttonComp = new Composite(group, SWT.NONE); | ||
| buttonComp.setLayout(new GridLayout(1, true)); | ||
| buttonComp.setLayoutData(new GridData(SWT.LEFT, SWT.TOP, false, false)); | ||
|
|
||
| btnActivate = createActionButton(buttonComp, "Activate", "Set this version as the active ESP-IDF"); | ||
| btnReinstall = createActionButton(buttonComp, "Update Environment", | ||
| "Refresh toolchains, Python virtual environment, and IDE settings for the CURRENTLY ACTIVE version"); | ||
|
|
||
| // --- Listeners --- | ||
| tableViewer.addSelectionChangedListener(event -> updateButtonState()); | ||
|
|
||
| tableViewer.addDoubleClickListener(event -> { | ||
| var idf = getSelectedIdf(); | ||
| if (idf != null && !idf.equals(currentInstallingNode) && !ToolsUtility.isIdfInstalledActive(idf)) | ||
| { | ||
| performToolsSetup(idf); | ||
| } | ||
|
|
||
| }); | ||
|
|
||
| SelectionAdapter btnListener = new SelectionAdapter() | ||
| { | ||
| // activate the only available esp-idf first check if its not already active | ||
| Preferences scopedPreferenceStore = InstanceScope.INSTANCE.getNode(UIPlugin.PLUGIN_ID); | ||
| if (!scopedPreferenceStore.getBoolean(EimConstants.INSTALL_TOOLS_FLAG, false)) | ||
| @Override | ||
| public void widgetSelected(SelectionEvent e) | ||
| { | ||
| SetupToolsInIde setupToolsInIde = new SetupToolsInIde(idfInstalledList.get(0), eimJson, | ||
| getConsoleStream(true), getConsoleStream(false)); | ||
| SetupToolsJobListener toolsActivationJobListener = new SetupToolsJobListener(ESPIDFMainTablePage.this, | ||
| setupToolsInIde); | ||
| setupToolsInIde.addJobChangeListener(toolsActivationJobListener); | ||
| setupToolsInIde.schedule(); | ||
| if (e.widget == btnActivate) | ||
| { | ||
| // Activate depends on SELECTION | ||
| var idf = getSelectedIdf(); | ||
| if (idf != null) | ||
| performToolsSetup(idf); | ||
| } | ||
| else if (e.widget == btnReinstall) | ||
| { | ||
| // Update Environment depends on ACTIVE STATUS (ignores selection) | ||
| performUpdateOnActiveIdf(); | ||
| } | ||
| } | ||
| }; | ||
| btnActivate.addSelectionListener(btnListener); | ||
| btnReinstall.addSelectionListener(btnListener); | ||
|
|
||
| updateButtonState(); | ||
| } |
There was a problem hiding this comment.
Hardcoded UI strings should be externalized to Messages.
Lines 133 ("Installed IDF Versions"), 159 ("Activate", "Set this version as the active ESP-IDF"), 160-161 ("Update Environment", tooltip text), 224 ("Status"), 231 ("Setting up..."), and 232 ("\u2713 Active") are all user-visible strings that bypass the project's Messages externalization pattern already used elsewhere in this file (e.g., Messages.EspIdfManagerVersionCol). This breaks i18n/l10n support.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java`
around lines 130 - 198, Replace all hardcoded user-visible strings in
createMainContent and associated UI code with entries from the Messages resource
bundle: change group.setText("Installed IDF Versions") to
group.setText(Messages.EspIdfManagerInstalledVersions), change the
createActionButton label/tooltips ("Activate", "Set this version as the active
ESP-IDF", "Update Environment", the long tooltip) to Messages keys (e.g.,
Messages.EspIdfManagerActivate, Messages.EspIdfManagerActivateTooltip,
Messages.EspIdfManagerUpdateEnvironment,
Messages.EspIdfManagerUpdateEnvironmentTooltip), and replace the
column/header/status strings ("Status", "Setting up...", "\u2713 Active") used
in createColumns or wherever status text is emitted with
Messages.EspIdfManagerStatus, Messages.EspIdfManagerSettingUp,
Messages.EspIdfManagerActive (or similar keys). Add the corresponding keys to
the Messages properties and Java Messages accessor class following the existing
pattern used for Messages.EspIdfManagerVersionCol so that all UI text is
externalized for i18n.
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
Please include a summary of the change and which issue is fixed.
Fixes # (IEP-XXX)
Type of change
Please delete options that are not relevant.
How has this been tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
New Features
UI/UX Improvements