IEP-1650 Convert NVS Title Area dialog to the editor #1339
Conversation
WalkthroughReplaces the dialog-based NVS CSV editor with an EditorPart-based editor and a dedicated NvsCsvEditorPage; registers a new editor id in plugin.xml; adds column enum, label-provider refactor, editing-support factory, preference/settings types and service; updates handler to open the editor via the workbench; minor dialog-file adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Handler as NvsEditorHandler
participant Service as NvsTableDataService
participant WB as Workbench
participant Editor as NvsEditor
participant Page as NvsCsvEditorPage
User->>Handler: request open NVS editor
Handler->>Service: ensure `nvs.csv` exists (save if missing)
Handler->>WB: openEditor(FileEditorInput, NvsEditor.ID)
WB->>Editor: init(site, input)
Editor->>Editor: resolve IFile, set title
WB->>Editor: createPartControl(parent)
Editor->>Page: instantiate NvsCsvEditorPage(parent, dirtyCallback)
Page->>Service: load CSV → beans
Page->>Page: populate TableViewer
User->>Page: edit cells / controls
Page-->>Editor: dirtyCallback(true)
Editor->>WB: firePropertyChange(IS_DIRTY)
User->>Page: Click Save
Page->>Service: saveCsv(updated beans)
Page-->>Editor: dirtyCallback(false)
Editor->>WB: firePropertyChange(IS_DIRTY)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditor.java (1)
96-103: Add dispose to release page resources.Add a dispose hook so the page can detach listeners/refs when the editor closes.
@Override public void setFocus() { if (editorPage != null) { editorPage.setFocus(); } } + +@Override +public void dispose() { + try { + if (editorPage != null) { + editorPage.dispose(); // implement no-op safely in the page + editorPage = null; + } + } finally { + super.dispose(); + } +}bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (3)
739-743: Enable/disable Generate button using instance-scoped errors.Update to the instance map after refactor.
- generateButton.setEnabled(ErrorListenerMap.INSTANCE.validationErrors.isEmpty()); + generateButton.setEnabled(validationErrors.isEmpty());
356-458: Typos in method/variable names (readability).createEnctyptionLable, encyptionKeyLbl, encriptionColumn, etc. Consider renaming for clarity.
Example:
- private void createEnctyptionLable(Composite parent) + private void createEncryptionSection(Composite parent) @@ - Label encyptionKeyLbl + Label encryptionKeyLbl @@ - TableColumn encriptionColumn + TableColumn encodingColumn
761-767: Minor: use uppercase long literal.Style/readability.
- Long decodedSize = 0l; + Long decodedSize = 0L;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
bundles/com.espressif.idf.ui/plugin.xml(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/handlers/NvsEditorHandler.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
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-137)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.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (7)
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-137)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/EclipseUtil.java (1)
EclipseUtil(36-186)
⏰ 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
- GitHub Check: build_macos
🔇 Additional comments (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditor.java (1)
23-40: Init flow looks correct.Good: resolve IFile via ResourceUtil, set part name/tooltip.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (1)
233-257: Save flow looks good; consider early return on empty/invalid with status update already in place.Current logic validates, logs, refreshes, and returns true only on success. LGTM.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java
Outdated
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java
Outdated
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/handlers/NvsEditorHandler.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/handlers/NvsEditorHandler.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (3)
470-474: Use ArrayContentProvider instead of casting lambda to IStructuredContentProvider.This is the same issue previously flagged: IStructuredContentProvider is not a functional interface (it has multiple abstract methods: getElements, inputChanged, dispose). Casting a lambda to it is invalid.
Apply this diff:
+import org.eclipse.jface.viewers.ArrayContentProvider; @@ tableViewer = new TableViewer(csvTable); -tableViewer.setContentProvider((IStructuredContentProvider) input -> { - @SuppressWarnings("unchecked") - List<NvsTableBean> list = (List<NvsTableBean>) input; - return list.toArray(); -}); +tableViewer.setContentProvider(ArrayContentProvider.getInstance());
646-679: Mouse hover adds a new MouseMoveListener each time → listener leak.This is the same issue previously flagged: every mouseHover event adds another MouseMoveListener (line 671) that's never removed, causing listener accumulation and performance degradation.
Prefer JFace's built-in tooltip support:
+import org.eclipse.jface.viewers.ColumnViewerToolTipSupport; @@ csvTable.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true)); ((GridData) csvTable.getLayoutData()).widthHint = 1000; - -csvTable.addMouseTrackListener(new MouseTrackAdapter() -{ - Shell errorToolTip; - - @Override - public void mouseHover(MouseEvent event) - { - ViewerCell item = tableViewer.getCell(new Point(event.x, event.y)); - if (item != null) - { - if (errorToolTip != null && !errorToolTip.isDisposed()) - errorToolTip.dispose(); - errorToolTip = new Shell(mainControl.getShell(), SWT.ON_TOP | SWT.TOOL); - errorToolTip.setLayout(new FillLayout()); - Label label = new Label(errorToolTip, SWT.NONE); - label.setForeground(mainControl.getShell().getDisplay().getSystemColor(SWT.COLOR_INFO_FOREGROUND)); - label.setBackground(mainControl.getShell().getDisplay().getSystemColor(SWT.COLOR_INFO_BACKGROUND)); - label.setText(new NvsBeanValidator().validateBean((NvsTableBean) item.getElement(), - item.getColumnIndex())); - - Point size = errorToolTip.computeSize(SWT.DEFAULT, SWT.DEFAULT); - Rectangle rect = item.getBounds(); - Point pt = csvTable.toDisplay(rect.x, rect.y); - errorToolTip.setBounds(pt.x, pt.y, size.x, size.y); - errorToolTip.setVisible(true); - csvTable.addMouseMoveListener(e -> { - if (errorToolTip != null && !errorToolTip.isDisposed()) - { - errorToolTip.dispose(); - } - }); - } - } -});Then implement tooltip support in your NvsTableEditorLabelProvider by overriding
getToolTipText(Object element)to return validation messages. Enable tooltips after creating the viewer:// After tableViewer is created in createTableViewer(): ColumnViewerToolTipSupport.enableFor(tableViewer);
682-709: Refresh the editor's project, not the current selection.This is the same issue previously flagged: using
EclipseUtil.getSelectedProjectInExplorer()at lines 702-703 can be null and is unrelated to the file being edited. The editor should refresh its own project.Apply this diff:
try { - EclipseUtil.getSelectedProjectInExplorer().refreshLocal(IResource.DEPTH_INFINITE, - new NullProgressMonitor()); + csvFile.getProject().refreshLocal(IResource.DEPTH_INFINITE, new NullProgressMonitor()); } catch (CoreException e) { Logger.log(e); }
🧹 Nitpick comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (1)
735-735: Use uppercase 'L' for long literals.Line 735 uses
0l(lowercase 'l'), which can be confused with the digit1or uppercaseI. Use0Lfor clarity.Apply this diff:
-Long decodedSize = 0l; +Long decodedSize = 0L;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (7)
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-137)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/EclipseUtil.java (1)
EclipseUtil(36-186)
⏰ 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
🔇 Additional comments (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (2)
101-102: Good refactor: instance-scoped validation state.The validationErrors map is now an instance field rather than a static singleton, which correctly supports multiple open editors without cross-talk or leaks.
206-230: LGTM: Save action correctly refreshes the editor's project.The save action properly uses
csvFile.getProject()for the refresh, ensuring the correct project is updated regardless of the current selection state.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (4)
641-675: Mouse hover adds a MouseMoveListener per hover → listener leak; switch to JFace viewer tooltips.
- Remove the ad-hoc MouseTrack/MouseMove tooltip logic.
- Enable ColumnViewerToolTipSupport for the TableViewer and return tooltips from your label provider.
@@ - csvTable.addMouseTrackListener(new MouseTrackAdapter() - { - Shell errorToolTip; - @Override - public void mouseHover(MouseEvent event) - { - ViewerCell item = tableViewer.getCell(new Point(event.x, event.y)); - if (item != null) - { - if (errorToolTip != null && !errorToolTip.isDisposed()) - errorToolTip.dispose(); - errorToolTip = new Shell(mainControl.getShell(), SWT.ON_TOP | SWT.TOOL); - errorToolTip.setLayout(new FillLayout()); - Label label = new Label(errorToolTip, SWT.NONE); - label.setForeground(mainControl.getShell().getDisplay().getSystemColor(SWT.COLOR_INFO_FOREGROUND)); - label.setBackground(mainControl.getShell().getDisplay().getSystemColor(SWT.COLOR_INFO_BACKGROUND)); - label.setText(new NvsBeanValidator().validateBean((NvsTableBean) item.getElement(), - item.getColumnIndex())); - Point size = errorToolTip.computeSize(SWT.DEFAULT, SWT.DEFAULT); - Rectangle rect = item.getBounds(); - Point pt = csvTable.toDisplay(rect.x, rect.y); - errorToolTip.setBounds(pt.x, pt.y, size.x, size.y); - errorToolTip.setVisible(true); - csvTable.addMouseMoveListener(e -> { - if (errorToolTip != null && !errorToolTip.isDisposed()) - { - errorToolTip.dispose(); - } - }); - } - } - }); + // Tooltips are handled by ColumnViewerToolTipSupport in createTableViewer()@@ -import org.eclipse.jface.viewers.ArrayContentProvider; +import org.eclipse.jface.viewers.ArrayContentProvider; +import org.eclipse.jface.viewers.ColumnViewerToolTipSupport; @@ tableViewer = new TableViewer(csvTable); tableViewer.setContentProvider(ArrayContentProvider.getInstance()); @@ - tableViewer.setLabelProvider(new NvsTableEditorLabelProvider()); + tableViewer.setLabelProvider(new NvsTableEditorLabelProvider()); + ColumnViewerToolTipSupport.enableFor(tableViewer);Ensure NvsTableEditorLabelProvider returns per-cell tooltips (override getToolTipText/shift to ColumnLabelProviders per column if needed).
Also applies to: 467-483, 28-35
695-699: Refresh the editor’s project, not the selection.- EclipseUtil.getSelectedProjectInExplorer().refreshLocal(IResource.DEPTH_INFINITE, - new NullProgressMonitor()); + csvFile.getProject().refreshLocal(IResource.DEPTH_INFINITE, new NullProgressMonitor());
256-266: Fix redundant/incorrect ternary when appending validation errors.- if (!validationErrors.isEmpty()) - { - newErrorMessage = newErrorMessage.isBlank() ? newErrorMessage - : newErrorMessage.concat(StringUtil.LINE_SEPARATOR).concat(" "); //$NON-NLS-1$ + if (!validationErrors.isEmpty()) + { + if (!newErrorMessage.isBlank()) + { + newErrorMessage = newErrorMessage.concat(StringUtil.LINE_SEPARATOR).concat(" "); //$NON-NLS-1$ + }
317-321: Save button bypasses the editor save lifecycle; invoke workbench save.@@ - saveButton.addSelectionListener(SelectionListener.widgetSelectedAdapter(t -> getSaveAction())); + saveButton.addSelectionListener(SelectionListener.widgetSelectedAdapter(t -> { + PlatformUI.getWorkbench().getActiveWorkbenchWindow().getActivePage() + .saveEditor(PlatformUI.getWorkbench().getActiveWorkbenchWindow().getActivePage().getActiveEditor(), false); + }));@@ -import org.eclipse.swt.widgets.Text; +import org.eclipse.swt.widgets.Text; +import org.eclipse.ui.PlatformUI;If you intentionally keep direct save, ensure clearing dirty after successful save (e.g., dirtyStateListener.accept(false)).
Also applies to: 48-61
🧹 Nitpick comments (3)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (3)
126-128: Spellings: Encryption/Label typos (method and variables).These reduce readability and make search harder.
@@ - createEnctyptionLable(mainControl); - createSizeOfPartitionLable(mainControl); + createEncryptionLabel(mainControl); + createSizeOfPartitionLabel(mainControl); @@ - private void createEnctyptionLable(Composite parent) + private void createEncryptionLabel(Composite parent) @@ - private void createSizeOfPartitionLable(Composite parent) + private void createSizeOfPartitionLabel(Composite parent) @@ - Label encyptionKeyLbl = new Label(encryptionComposite, SWT.NONE); - encyptionKeyLbl.setText(Messages.NvsEditorDialog_PathToEncrKeyLbl); + Label encryptionKeyLbl = new Label(encryptionComposite, SWT.NONE); + encryptionKeyLbl.setText(Messages.NvsEditorDialog_PathToEncrKeyLbl); @@ - encyptionKeyLbl.setLayoutData(new GridData(SWT.BEGINNING, SWT.CENTER, false, false)); + encryptionKeyLbl.setLayoutData(new GridData(SWT.BEGINNING, SWT.CENTER, false, false)); @@ - Button encyptionKeyBrowseButton = new Button(encryptionComposite, SWT.PUSH); - encyptionKeyBrowseButton.setText(Messages.NvsEditorDialog_EncKeyBrowseTxt); - encyptionKeyBrowseButton.setLayoutData(new GridData(SWT.BEGINNING, SWT.CENTER, false, false)); + Button encryptionKeyBrowseButton = new Button(encryptionComposite, SWT.PUSH); + encryptionKeyBrowseButton.setText(Messages.NvsEditorDialog_EncKeyBrowseTxt); + encryptionKeyBrowseButton.setLayoutData(new GridData(SWT.BEGINNING, SWT.CENTER, false, false)); @@ - TableColumn encriptionColumn = new TableColumn(csvTable, SWT.NONE); - encriptionColumn.setText(columnNames[2]); + TableColumn encryptionColumn = new TableColumn(csvTable, SWT.NONE); + encryptionColumn.setText(columnNames[2]); @@ - encriptionColumn.setWidth(100); + encryptionColumn.setWidth(100); @@ - canBeDisposedList.add(encyptionKeyLbl); + canBeDisposedList.add(encryptionKeyLbl); @@ - canBeDisposedList.add(encyptionKeyBrowseButton); + canBeDisposedList.add(encryptionKeyBrowseButton);Also applies to: 328-330, 432-434, 345-349, 366-369, 621-630, 404-408
730-734: Use uppercase L in long literal.- Long decodedSize = 0l; + Long decodedSize = 0L;
475-476: Redundant viewer.update after setInput.setInput drives refresh; extra updates are unnecessary.
- list.forEach(bean -> tableViewer.update(bean, columnNames));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (5)
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.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.ui/src/com/espressif/idf/ui/EclipseUtil.java (1)
EclipseUtil(36-186)
⏰ 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 (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (1)
467-471: Good fix: use ArrayContentProvider instead of a lambda cast.The content provider choice is correct and avoids the invalid IStructuredContentProvider lambda cast flagged earlier. Nice.
Also applies to: 28-35
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (6)
385-386: Use OS path instead of URI string for FileDialog.
getLocationURI().toString()returns a URI format (e.g., "file:/..."), not an OS-native path. FileDialog expects a filesystem path.- dlg.setFilterPath(csvFile.getProject().getLocationURI().toString()); + dlg.setFilterPath(csvFile.getProject().getLocation().toOSString());
250-262: Fix redundant ternary logic.Lines 252-253: The ternary returns
newErrorMessageunchanged when blank, then concatenates validation errors to it. This is confusing logic.if (!validationErrors.isEmpty()) { - newErrorMessage = newErrorMessage.isBlank() ? newErrorMessage - : newErrorMessage.concat(StringUtil.LINE_SEPARATOR).concat(" "); //$NON-NLS-1$ + if (!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$ - }
311-314: Save button bypasses editor save lifecycle.The save button directly calls
getSaveAction(), which doesn't integrate with the editor's save lifecycle or clear the dirty state properly.Consider removing the save button from the page UI entirely, as users should use the standard editor save action (Ctrl+S or File > Save). If the button must remain for UX reasons, it should trigger the editor's save command:
Button saveButton = new Button(userButtonComp, SWT.PUSH); saveButton.setText(Messages.NvsTableEditorDialog_SaveAction); saveButton.setLayoutData(new GridData(SWT.FILL, SWT.BEGINNING, false, false)); -saveButton.addSelectionListener(SelectionListener.widgetSelectedAdapter(t -> getSaveAction())); +saveButton.addSelectionListener(SelectionListener.widgetSelectedAdapter(t -> { + // Trigger editor save through workbench + IWorkbenchPage page = PlatformUI.getWorkbench().getActiveWorkbenchWindow().getActivePage(); + if (page != null) { + page.saveEditor(page.getActiveEditor(), false); + } +}));Add import:
import org.eclipse.ui.IWorkbenchPage;
655-659: Refresh the editor's project, not the selected project.Using
EclipseUtil.getSelectedProjectInExplorer()can return null and is unrelated to the CSV file being edited.try { - EclipseUtil.getSelectedProjectInExplorer().refreshLocal(IResource.DEPTH_INFINITE, - new NullProgressMonitor()); + csvFile.getProject().refreshLocal(IResource.DEPTH_INFINITE, new NullProgressMonitor()); } catch (CoreException e) { Logger.log(e); }
134-134: Fix null Event passed to notifyListeners (can cause NPE).SWT can throw on null Event. This same issue occurs at lines 290 and 418.
Apply this fix:
+import org.eclipse.swt.widgets.Event;- encryptAction.notifyListeners(SWT.Selection, null); + Event event = new Event(); + event.widget = encryptAction; + encryptAction.notifyListeners(SWT.Selection, event);Similarly update lines 290 and 418:
- generateEncryptionKeyCheckBox.notifyListeners(SWT.Selection, null); + Event event = new Event(); + event.widget = generateEncryptionKeyCheckBox; + generateEncryptionKeyCheckBox.notifyListeners(SWT.Selection, event);- encryptionKeyText.notifyListeners(SWT.Modify, null); + Event event = new Event(); + event.widget = encryptionKeyText; + encryptionKeyText.notifyListeners(SWT.Modify, event);
365-377: Generate button and error banner aren't updated after validation changes.After validation, the UI state (generate button enablement and error banner) should be refreshed. Same issue at lines 444-456, 281-297, and 403-422.
encryptionKeyText.addModifyListener(e -> { String errMsg = validateEncKeyPath(); if (errMsg.isBlank()) { errorIconLabel.setImage(null); errorIconLabel.setToolTipText(null); } else { errorIconLabel.setImage(image); errorIconLabel.setToolTipText(errMsg); } markDirty(); + setGenerationButtonStatus(); + updateErrorMessage(); });Apply similar changes at lines 444-456, 281-297, and 403-422.
🧹 Nitpick comments (5)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsTableEditorLabelProvider.java (1)
59-75: Consider caching tooltip validation results to reduce redundant calls.Each tooltip hover runs 4 validations (one per column) on the same bean. If the table is large or validation becomes heavier, this could cause UI lag.
Optional optimization:
+import java.util.Map; +import java.util.WeakHashMap; + public class NvsTableEditorLabelProvider extends CellLabelProvider { + private final Map<NvsTableBean, String> tooltipCache = new WeakHashMap<>(); private final NvsBeanValidator validator = new NvsBeanValidator(); + @Override + public void update(ViewerCell cell) + { + // ... existing code ... + // Invalidate cache on update + tooltipCache.remove(bean); + } + @Override public String getToolTipText(Object element) { + NvsTableBean bean = (NvsTableBean) element; + return tooltipCache.computeIfAbsent(bean, b -> { - StringBuilder tooltip = new StringBuilder(); - NvsTableBean bean = (NvsTableBean) element; - - for (int col = 0; col < 4; col++) - { - String status = validator.validateBean(bean, col); - if (!status.isBlank()) + StringBuilder tooltip = new StringBuilder(); + for (int col = 0; col < 4; col++) { - tooltip.append("Column ").append(col).append(": ").append(status).append("\n"); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ + String status = validator.validateBean(b, col); + if (!status.isBlank()) + { + tooltip.append("Column ").append(col).append(": ").append(status).append("\n"); + } } - } - - return tooltip.isEmpty() ? null : tooltip.toString().trim(); + return tooltip.isEmpty() ? null : tooltip.toString().trim(); + }); } }bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (4)
734-745: Reuse validator instance instead of creating new ones.A new
NvsBeanValidatoris created for each bean in the loop. Use a field validator or create once before the loop.+ private final NvsBeanValidator validator = new NvsBeanValidator(); + private boolean validateBeansBeforeSaving(List<NvsTableBean> beansToSave) { String errorMsg; if (beansToSave.isEmpty()) { saveErrorMsg = Messages.NvsEditorDialog_EmptyTableErrorMsg; return false; } if (!validateFirstBean(beansToSave.get(0))) { return false; } for (NvsTableBean bean : beansToSave) { for (int i = 0; i < columnNames.length; i++) { - errorMsg = new NvsBeanValidator().validateBean(bean, i); + errorMsg = validator.validateBean(bean, i); if (!errorMsg.isBlank()) { saveErrorMsg = errorMsg; return false; } } } saveErrorMsg = StringUtil.EMPTY; return true; }Also update line 752 similarly:
private boolean validateFirstBean(NvsTableBean nvsTableBean) { - String errorMsg = new NvsBeanValidator().validateFirstBean(nvsTableBean); + String errorMsg = validator.validateFirstBean(nvsTableBean); if (!errorMsg.isBlank()) { saveErrorMsg = errorMsg; return false; } return true; }
518-527: Consider directly setting encoding instead of recursive modify call.Line 526 recursively calls
modify()to reset the encoding when type changes. While functional, directly setting the encoding value is clearer and avoids recursion.case 1: String newType = NvsTableDataService.getTypes()[(int) value]; if (newType.contentEquals(bean.getType())) { break; } bean.setType(newType); ((ComboBoxCellEditor) cellEditors[2]).setItems(NvsTableDataService.getEncodings(bean.getType())); - tableViewer.getCellModifier().modify(element, columnNames[2], 0); + // Reset encoding to first option when type changes + bean.setEncoding(NvsTableDataService.getEncodings(bean.getType())[0]); break;
690-690: Use uppercase 'L' for long literal.Lowercase 'l' can be confused with '1'. Use '0L' instead of '0l'.
- Long decodedSize = 0l; + Long decodedSize = 0L;
592-603: Avoid setting status messages in canModify().
canModify()is called frequently during table interaction. Setting information messages here can be noisy and may overwrite important messages. Consider showing these messages only when edit attempts are blocked (e.g., in a selection listener).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
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/NvsTableEditorLabelProvider.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsTableEditorLabelProvider.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/NvsCsvEditorPage.java (7)
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-137)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/EclipseUtil.java (1)
EclipseUtil(36-186)
⏰ 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
- GitHub Check: build_macos
🔇 Additional comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsTableEditorLabelProvider.java (1)
14-57: LGTM! Clean refactor to CellLabelProvider.The shift from ITableLabelProvider/ITableColorProvider to CellLabelProvider with centralized update-based rendering is well done. Per-cell validation styling provides immediate visual feedback.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/handlers/NvsEditorHandler.java (2)
30-34: Consider verifying file creation succeeded before opening editor.If
saveCsvfails, the editor will attempt to open a non-existent file, potentially causing a confusing error. WhileNvsTableDataServicelogs errors internally, the handler has no way to detect and handle the failure.You could add a post-creation check:
if (!csvFile.exists()) { // Ensure the file exists before opening the editor new NvsTableDataService().saveCsv(csvFile, new ArrayList<>()); + if (!csvFile.exists()) + { + Logger.logError("Failed to create NVS CSV file: " + csvFile.getFullPath()); + return null; + } }
36-52: Good null checking; consider adding logging for null window/page.The null checks properly address the previous NPE concern. However, if the window or page is null, the method silently returns without feedback, making debugging difficult.
Consider logging unexpected null cases:
var window = PlatformUI.getWorkbench().getActiveWorkbenchWindow(); if (window != null) { IWorkbenchPage page = window.getActivePage(); if (page != null) { try { FileEditorInput input = new FileEditorInput(csvFile); page.openEditor(input, NvsEditor.ID); } catch (PartInitException e) { Logger.log(e); } } + else + { + Logger.logError("No active workbench page available to open NVS editor"); + } } +else +{ + Logger.logError("No active workbench window available to open NVS editor"); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
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.435Z
Learnt from: sigmaaa
PR: espressif/idf-eclipse-plugin#1339
File: bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/handlers/NvsEditorHandler.java:27-34
Timestamp: 2025-10-27T18:07:42.435Z
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/src/com/espressif/idf/ui/nvs/handlers/NvsEditorHandler.java
🧬 Code graph analysis (1)
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-137)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)
⏰ 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 (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/handlers/NvsEditorHandler.java (1)
10-18: LGTM! Imports align with the editor-based approach.The new imports properly support the workbench-based editor opening flow, replacing the previous dialog approach.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (2)
412-412: Use OS-native path for FileDialog filter path.The FileDialog expects an OS-native filesystem path, but
getLocationURI().toString()returns a URI string (e.g.,file:/C:/pathon Windows), which won't work correctly as a filter path.Apply this diff:
- dlg.setFilterPath(csvFile.getProject().getLocationURI().toString()); + dlg.setFilterPath(csvFile.getProject().getLocation().toOSString());
219-252: Clear dirty state after successful save.After a successful save at line 251, the editor's dirty state should be cleared so the UI reflects that the file is now clean. Currently,
dirtyStateListener.accept(false)is never called, leaving the editor marked as dirty even after save succeeds.Apply this diff to clear the dirty state:
catch (CoreException e) { Logger.log(e); } + dirtyStateListener.accept(false); // Clear dirty state return true; // Save was successful }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
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-137)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)
⏰ 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
- GitHub Check: build_macos
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (3)
412-412: Use OS path instead of URI string for FileDialog.Line 412 passes a URI string to
setFilterPath, but FileDialog expects a native filesystem path. This was flagged in a previous review but not yet addressed.Apply this diff:
- dlg.setFilterPath(csvFile.getProject().getLocationURI().toString()); + dlg.setFilterPath(csvFile.getProject().getLocation().toOSString());
219-252: Clear dirty state after successful save.After a successful save at line 251, the dirty state is not cleared. The editor will remain marked as modified even though the save succeeded. This was flagged in a previous review but not yet addressed.
Apply this diff to clear the dirty state:
catch (CoreException e) { Logger.log(e); } + dirtyStateListener.accept(false); // Clear dirty state after successful save return true; // Save was successful }
129-129: Critical: null Event passed to notifyListeners can cause NPE.Line 129 passes
nulltonotifyListeners, which can throw aNullPointerExceptionin SWT. This issue was flagged in a previous review and marked as addressed, but the null argument is still present. The same issue exists at lines 318 and 443.Apply this diff to fix all three occurrences:
+import org.eclipse.swt.widgets.Event;- encryptAction.notifyListeners(SWT.Selection, null); + encryptAction.notifyListeners(SWT.Selection, new Event());- generateEncryptionKeyCheckBox.notifyListeners(SWT.Selection, null); + generateEncryptionKeyCheckBox.notifyListeners(SWT.Selection, new Event());- encryptionKeyText.notifyListeners(SWT.Modify, null); + encryptionKeyText.notifyListeners(SWT.Modify, new Event());
🧹 Nitpick comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (1)
269-281: Simplify redundant null checks.After line 271 ensures
saveErrorMsgis never null, the null check at line 274 is redundant. Similarly, the null check at line 280 could be simplified.Consider this simplification:
- if (saveErrorMsg == null) - { - saveErrorMsg = StringUtil.EMPTY; - } - - if (saveErrorMsg != null && !saveErrorMsg.isBlank()) + if (saveErrorMsg == null) + { + saveErrorMsg = StringUtil.EMPTY; + } + else if (!saveErrorMsg.isBlank()) { newErrorMessage = String.format(Messages.NvsEditorDialog_ComplexSaveErrorMsg, saveErrorMsg); } if (!validationErrors.isEmpty()) { - if (newErrorMessage != null && !newErrorMessage.isBlank()) + if (!newErrorMessage.isBlank()) newErrorMessage = newErrorMessage.concat(StringUtil.LINE_SEPARATOR).concat(" ");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
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-137)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)
⏰ 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)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (2)
137-158: Good consolidation of validation logic.The
validateGenerationState()method effectively centralizes validation checks and UI updates, addressing previous concerns about button status and error messages not being refreshed after input changes.
489-493: LGTM: Proper use of ArrayContentProvider and tooltip support.Lines 492-493 correctly use
ArrayContentProvider.getInstance()andColumnViewerToolTipSupport.enableFor(), addressing previous review concerns about lambda casting and manual mouse hover listeners.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (2)
129-129: Pass a non-null Event to notifyListeners to prevent potential NPE.This issue was previously flagged: calling
notifyListenerswith anullEvent parameter can cause aNullPointerExceptionin some SWT implementations. The same issue exists at lines 319 and 444.Apply this diff to fix all three occurrences:
+import org.eclipse.swt.widgets.Event;- encryptAction.notifyListeners(SWT.Selection, null); + encryptAction.notifyListeners(SWT.Selection, new Event());- generateEncryptionKeyCheckBox.notifyListeners(SWT.Selection, null); + generateEncryptionKeyCheckBox.notifyListeners(SWT.Selection, new Event());- encryptionKeyText.notifyListeners(SWT.Modify, null); + encryptionKeyText.notifyListeners(SWT.Modify, new Event());
413-413: Use OS path instead of URI string for FileDialog filter path.FileDialog expects a platform filesystem path, not a URI string.
Apply this diff:
- dlg.setFilterPath(csvFile.getProject().getLocationURI().toString()); + dlg.setFilterPath(csvFile.getProject().getLocation().toOSString());
🧹 Nitpick comments (3)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (3)
270-283: Simplify redundant null handling.After ensuring
saveErrorMsgis non-null on line 272, the null check on line 275 is redundant.Apply this diff:
- if (saveErrorMsg == null) - { - saveErrorMsg = StringUtil.EMPTY; - } - - if (saveErrorMsg != null && !saveErrorMsg.isBlank()) + if (saveErrorMsg != null && !saveErrorMsg.isBlank()) { newErrorMessage = String.format(Messages.NvsEditorDialog_ComplexSaveErrorMsg, saveErrorMsg); } + else + { + saveErrorMsg = StringUtil.EMPTY; + }
571-579: Consider extracting first-row protection logic.The check for preventing edits to the first row is duplicated in three places (Type, Encoding, Value columns). Extracting this to a helper method would improve maintainability.
Add a helper method:
private boolean isFirstRowEditable(Object element, String messageKey) { if (tableViewer.getElementAt(0).equals(element)) { setMessage(Messages.NvsEditorDialog_FirstRowIsFixedInfoMsg, IMessageProvider.INFORMATION); return false; } return true; }Then use it in each
canEditmethod:protected boolean canEdit(Object element) { if (!isFirstRowEditable(element, Messages.NvsEditorDialog_FirstRowIsFixedInfoMsg)) { return false; } // ... rest of the logic }Also applies to: 642-656, 709-723
838-865: Instantiate NvsBeanValidator once instead of per-bean.Creating a new
NvsBeanValidatorinstance for each bean (line 855) is inefficient. Since the validator has no instance state, create it once before the loop.Apply this diff:
private boolean validateBeansBeforeSaving(List<NvsTableBean> beansToSave) { + NvsBeanValidator validator = new NvsBeanValidator(); String errorMsg; if (beansToSave.isEmpty()) { saveErrorMsg = Messages.NvsEditorDialog_EmptyTableErrorMsg; return false; } if (!validateFirstBean(beansToSave.get(0))) { return false; } for (NvsTableBean bean : beansToSave) { for (int i = 0; i < columnNames.length; i++) { - errorMsg = new NvsBeanValidator().validateBean(bean, i); + errorMsg = validator.validateBean(bean, i); if (!errorMsg.isBlank()) { saveErrorMsg = errorMsg; return false; } } } saveErrorMsg = StringUtil.EMPTY; return true; }Apply the same pattern in
validateFirstBeanat line 869:private boolean validateFirstBean(NvsTableBean nvsTableBean) { - String errorMsg = new NvsBeanValidator().validateFirstBean(nvsTableBean); + NvsBeanValidator validator = new NvsBeanValidator(); + String errorMsg = validator.validateFirstBean(nvsTableBean); if (!errorMsg.isBlank()) { saveErrorMsg = errorMsg; return false; } return true; }Or better, create a single validator instance as a field if validation is called frequently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
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-137)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)
⏰ 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)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (2)
219-253: LGTM - Save action properly clears dirty state.The save action correctly validates beans, saves the CSV, refreshes the project, and clears the dirty state via
dirtyStateListener.accept(false)at line 251. This addresses previous concerns about dirty state management.
137-158: Well-structured validation orchestration.The
validateGenerationState()method provides a single point to run all validation checks and update dependent UI state (button enablement and error messages). This addresses previous concerns about inconsistent validation state updates.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (1)
426-442: Use OS-native path for FileDialog filter path.Line 432 uses
toString()which may not produce a valid OS filesystem path on all platforms. FileDialog expects OS-native paths.Apply this diff:
{ FileDialog dlg = new FileDialog(mainControl.getShell(), SWT.OPEN); - dlg.setFilterPath(csvFile.getProject().getLocation().toString()); + dlg.setFilterPath(csvFile.getProject().getLocation().toOSString()); dlg.setFilterExtensions(new String[] { "*.bin", "*" }); //$NON-NLS-1$ //$NON-NLS-2$
🧹 Nitpick comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (1)
282-314: Consider simplifying redundant null checks.After initializing
newErrorMessagetoStringUtil.EMPTYat line 287, the null check at line 300 is redundant. Similarly, after the null guard at lines 289-292, the null check at line 294 is unnecessary.Apply this diff to simplify:
public void updateErrorMessage() { String newErrorMessage = StringUtil.EMPTY; if (saveErrorMsg == null) { saveErrorMsg = StringUtil.EMPTY; } - if (saveErrorMsg != null && !saveErrorMsg.isBlank()) + if (!saveErrorMsg.isBlank()) { newErrorMessage = String.format(Messages.NvsEditorDialog_ComplexSaveErrorMsg, saveErrorMsg); } if (!validationErrors.isEmpty()) { - if (newErrorMessage != null && !newErrorMessage.isBlank()) + if (!newErrorMessage.isBlank()) newErrorMessage = newErrorMessage.concat(StringUtil.LINE_SEPARATOR).concat(" "); //$NON-NLS-1$
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
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-137)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)
🔇 Additional comments (7)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (7)
109-149: LGTM!The constructor and initialization logic are well-structured. The use of
new Event()at line 147 correctly avoids potential NPE, preferences are loaded after UI creation, and initial validation state is properly established.
151-176: Excellent centralized validation pattern.The
validateGenerationState()method provides a single point of truth for validation state, clearing errors, running all checks, and updating the UI. This prevents inconsistencies and makes the validation flow easy to follow.
435-441: Verify whether pack() is necessary after setText().Line 439 calls
pack()on the parent composite after setting the encryption key path. This recalculates the parent's size and might cause unexpected layout changes. Text fields typically handle content updates without requiring parent repacking.Consider whether this line can be removed without affecting the intended behavior.
507-764: Well-structured table viewer implementation.The table viewer setup correctly uses
ArrayContentProvider.getInstance(), enables tooltip support viaColumnViewerToolTipSupport, and implements proper editing support for all columns. The dirty state is consistently marked when values change, and CSV parsing errors are properly caught.
237-272: Save implementation handles lifecycle correctly.The save action properly validates beans before saving, updates the status message, refreshes the project, clears the dirty state via
dirtyStateListener.accept(false), and persists preferences. This addresses the editor lifecycle concern from previous reviews.
775-806: LGTM!The partition generation action correctly validates state before proceeding, generates the partition, updates the status message, and refreshes the project to show the new binary file.
938-984: Proper preference handling implementation.The preference persistence correctly uses project-scoped preferences via
ProjectScope, provides sensible defaults, and handlesBackingStoreExceptionduring flush. Loading on initialization and saving after successful save ensures preferences stay synchronized.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (1)
430-446: Use OS-native path for FileDialog.Line 436 uses
toString()which may not produce a correct OS path on all platforms. UsetoOSString()instead.Apply this diff:
{ FileDialog dlg = new FileDialog(mainControl.getShell(), SWT.OPEN); - dlg.setFilterPath(csvFile.getProject().getLocation().toString()); + dlg.setFilterPath(csvFile.getProject().getLocation().toOSString()); dlg.setFilterExtensions(new String[] { "*.bin", "*" }); //$NON-NLS-1$ //$NON-NLS-2$ dlg.setText(Messages.NvsEditorDialog_EncrPartitionKeyDlgTxt); String dir = dlg.open();
🧹 Nitpick comments (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (1)
289-318: Simplify redundant null checks.Lines 293-296 ensure
saveErrorMsgis never null, making the null check at line 298 redundant. Similarly,newErrorMessageis initialized toStringUtil.EMPTYat line 291, making the null check at line 304 redundant.Apply this diff to simplify:
public void updateErrorMessage() { String newErrorMessage = StringUtil.EMPTY; if (saveErrorMsg == null) { saveErrorMsg = StringUtil.EMPTY; } - if (saveErrorMsg != null && !saveErrorMsg.isBlank()) + if (!saveErrorMsg.isBlank()) { newErrorMessage = String.format(Messages.NvsEditorDialog_ComplexSaveErrorMsg, saveErrorMsg); } if (!validationErrors.isEmpty()) { - if (newErrorMessage != null && !newErrorMessage.isBlank()) + if (!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); }bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsTableEditorLabelProvider.java (1)
16-16: Consider using a static validator instance.Each column creates its own
NvsBeanValidatorinstance (4 total for the table). SinceNvsBeanValidatoris stateless, consider making this field static to reduce object creation.Apply this diff:
public abstract class NvsTableEditorLabelProvider extends ColumnLabelProvider { - protected NvsBeanValidator validator = new NvsBeanValidator(); + protected static final NvsBeanValidator VALIDATOR = new NvsBeanValidator();Then update references from
validatortoVALIDATORin lines 37 and 62.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
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/NvsTableEditorLabelProvider.java(1 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/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java
🧬 Code graph analysis (2)
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-137)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/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)
⏰ 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). (3)
- GitHub Check: build_macos
- GitHub Check: build
- GitHub Check: precommit
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (1)
430-446: Use toOSString() for native file system path.The FileDialog filter path should use a native OS path format rather than a URI string representation.
Apply this diff:
FileDialog dlg = new FileDialog(mainControl.getShell(), SWT.OPEN); - dlg.setFilterPath(csvFile.getProject().getLocation().toString()); + dlg.setFilterPath(csvFile.getProject().getLocation().toOSString()); dlg.setFilterExtensions(new String[] { "*.bin", "*" }); //$NON-NLS-1$ //$NON-NLS-2$
🧹 Nitpick comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (1)
986-996: Remove redundant tableViewer.refresh() call.The refresh at line 994 is redundant since refresh was already called at line 992 immediately after setInput().
Apply this diff:
private void getAddRowAction() { @SuppressWarnings("unchecked") List<NvsTableBean> beansToSave = (List<NvsTableBean>) tableViewer.getInput(); beansToSave.add(new NvsTableBean()); tableViewer.setInput(beansToSave); tableViewer.refresh(); csvTable.select(csvTable.getItemCount() - 1); - tableViewer.refresh(); markDirty(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java(1 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/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java
🧬 Code graph analysis (1)
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-137)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)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (1)
433-433: UsetoOSString()for FileDialog filter path.Line 433 uses
getLocation().toString()which may not produce a valid platform-specific path forFileDialog.setFilterPath(). UsetoOSString()to ensure the path is formatted correctly for the OS.- dlg.setFilterPath(csvFile.getProject().getLocation().toString()); + dlg.setFilterPath(csvFile.getProject().getLocation().toOSString());
🧹 Nitpick comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsColumn.java (1)
67-75: Misleading comment about exception reachability.The comment at line 73 suggests the
IndexOutOfBoundsExceptionis a fallback that "should ideally not be reachable," but throwing an exception for invalid indices is the correct and expected behavior for this method—it's defensive programming, not an error condition.Consider clarifying the comment:
- // This should ideally not be reachable if validation is correct - throw new IndexOutOfBoundsException("Invalid column index: " + index); //$NON-NLS-1$ + throw new IndexOutOfBoundsException("Invalid column index: " + index); //$NON-NLS-1$
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
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/NvsEditorSupportFactory.java(1 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/src/com/espressif/idf/ui/nvs/dialog/NvsEditorSupportFactory.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java
🧬 Code graph analysis (2)
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)
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-137)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)
⏰ 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
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java
Outdated
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorSupportFactory.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorSupportFactory.java (1)
74-80: Tooltip still incorrectly implies the entire first row is fixed.The tooltip displays "FirstRowIsFixed" for all columns, but
NvsKeyEditingSupport.canEdit()(line 145) returnstrue, allowing KEY to be edited in the first row. This was flagged in a previous review but remains unaddressed.As suggested previously, update the tooltip to reflect that only some fields are fixed, or return
nullfor the KEY column.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (1)
421-437: FileDialog filter path should use OS-native path format.At line 427,
csvFile.getProject().getLocation().toString()returns a workspace-relative path string, butFileDialog.setFilterPath()expects an OS-native filesystem path. Use.toOSString()instead.Apply this diff:
- dlg.setFilterPath(csvFile.getProject().getLocation().toString()); + dlg.setFilterPath(csvFile.getProject().getLocation().toOSString());
🧹 Nitpick comments (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorPreferenceService.java (1)
22-22: Consider referencing the plugin ID constant instead of hardcoding.The PLUGIN_ID is hardcoded here, which creates a maintenance burden if the plugin ID changes. Consider referencing
IDFCorePlugin.PLUGIN_IDor a similar constant.Apply this diff:
- private static final String PLUGIN_ID = "com.espressif.idf.core"; //$NON-NLS-1$ + private static final String PLUGIN_ID = com.espressif.idf.core.IDFCorePlugin.PLUGIN_ID;bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (1)
280-309: Redundant null check in error message concatenation.At line 295,
newErrorMessageis checked for!= null, but it's initialized toStringUtil.EMPTYat line 282 and never assignednull, making the null check unnecessary. You can simplify to just!newErrorMessage.isBlank().Apply this diff:
if (!validationErrors.isEmpty()) { - if (newErrorMessage != null && !newErrorMessage.isBlank()) + if (!newErrorMessage.isBlank()) newErrorMessage = newErrorMessage.concat(StringUtil.LINE_SEPARATOR).concat(" "); //$NON-NLS-1$
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
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/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)
🧰 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/src/com/espressif/idf/ui/nvs/dialog/NvsEditorPreferenceService.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 (4)
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-137)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/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-137)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)
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)
⏰ 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
🔇 Additional comments (7)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorSettings.java (1)
14-26: LGTM! Clean record design for settings DTO.The immutable record implementation is well-structured with a clear factory method for defaults. The design appropriately leaves validation to the UI and service layers.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorPreferenceService.java (1)
61-68: Verify that silent failure is acceptable for preference save errors.If
preferences.flush()throws aBackingStoreException, the error is only logged without user notification. For a "Chill" review this is acceptable, but consider whether users should be informed of preference save failures in production.If user notification is desired, you could propagate the exception or set an error message via a callback.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditorSupportFactory.java (1)
129-161: Verify that KEY editing on the first row is intentional.
NvsKeyEditingSupportoverridescanEdit()to returntrue(line 145), allowing the KEY column in the first row to be edited. This differs from theBaseNvsEditingSupportbehavior (line 125) where the first row is non-editable. Confirm this is the intended design—if the first row's KEY should also be fixed, useBaseNvsEditingSupportas the base class forNvsKeyEditingSupport.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (4)
95-138: Well-structured initialization and UI creation flow.The constructor properly initializes the preference service, and
createControl()sets up the UI components in a logical order. The integration withNvsEditorPreferenceServiceand the use ofvalidateGenerationState()for initial button state are good practices.
226-263: Save action properly clears dirty state and persists preferences.The
getSaveAction()method now correctly clears the dirty state (line 258) and saves preferences (line 261), addressing previous review feedback.
502-544: Table viewer initialization follows JFace best practices.The use of
ArrayContentProvider.getInstance()(line 506) andColumnViewerToolTipSupport.enableFor()(line 507) are correct. The factory-based approach for label providers and editing support is clean and maintainable.
718-743: Settings load/save integration with preference service is well-designed.The separation between UI state (
loadSettingsToUi,saveSettingsFromUi) and persistence (NvsEditorPreferenceService) provides clean separation of concerns. Settings are loaded on initialization and saved after successful save operations.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (1)
426-428: Use OS-native path format for FileDialog.
IPath.toString()returns a portable path string, butFileDialog.setFilterPath()expects an OS-native file system path. UsetoOSString()for platform compatibility.- dlg.setFilterPath(csvFile.getProject().getLocation().toString()); + dlg.setFilterPath(csvFile.getProject().getLocation().toOSString());
🧹 Nitpick comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (1)
535-543: Consider showing an error message if CSV parsing fails.Currently, if CSV parsing throws an IOException, the editor opens with an empty table and no user feedback. Consider setting an error message via
setErrorMessage()in the catch block to inform users of the parse failure.catch (IOException e) { Logger.log(e); + setErrorMessage(String.format("Failed to load CSV file: %s", e.getMessage())); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java(1 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/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java
🧬 Code graph analysis (1)
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-137)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)
⏰ 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
🔇 Additional comments (15)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsCsvEditorPage.java (15)
95-105: LGTM! Clean constructor with proper initialization.The constructor properly initializes all necessary components and scopes the preference service to the project level, which is appropriate for per-project settings.
110-138: LGTM! Proper initialization flow.The control creation flow is well-structured: UI components are created, settings are loaded, and initial validation is performed to ensure a consistent state on startup.
144-165: LGTM! Centralized validation approach prevents state inconsistencies.The
validateGenerationState()method follows a good pattern: clear errors, run all validations, then update UI once. This prevents partial updates and ensures the UI always reflects the complete validation state.
226-263: LGTM! Complete save workflow with proper state management.The save action correctly validates, persists data, refreshes the project, clears the dirty state, and saves user preferences. The workflow is complete and handles errors appropriately.
268-309: LGTM! Proper error aggregation and dirty state management.The methods correctly aggregate errors from multiple sources (save validation and generation validation) and present them clearly to the user. The dirty state is updated after validation runs.
315-360: LGTM! Button bar properly wired with validation updates.All action buttons are correctly wired with selection listeners, and the encrypt toggle properly calls
validateGenerationState()to update dependent UI elements.
405-419: LGTM! Proper validation flow with UI feedback.The encryption key path text field correctly updates error decoration and triggers validation on modification, ensuring users get immediate feedback.
484-498: LGTM! Inline validation feedback for partition size.The size text field uses
ControlDecorationfor inline error feedback and properly triggers validation on modification, providing a good user experience.
505-508: LGTM! Proper use of standard JFace content provider and tooltip support.The table viewer correctly uses
ArrayContentProvider.getInstance()and enables tooltip support, both of which are best practices for JFace table viewers.
546-553: LGTM! Standard table creation with appropriate styles.The table is created with suitable styles for a data editor: full selection, scrollbars, visible headers and lines.
555-586: LGTM! Generation action with proper validation and refresh.The partition generation action validates state first, executes the generation, and refreshes the project to ensure the generated binary appears in the workspace.
588-633: LGTM! Comprehensive validation for generation prerequisites.The validation methods properly check all prerequisites for partition generation: button enablement, encryption key readability, and partition size constraints (minimum 4096 bytes, multiple of 4096).
635-674: LGTM! Thorough bean validation with type-safe column indexing.The validation methods properly check all beans before saving, using the
NvsColumnenum for type-safe column indexing and ensuring the first bean is a namespace entry as required by the NVS format.
676-713: LGTM! Row manipulation with proper guards and dirty state tracking.The add and delete row actions properly protect the first row from deletion, confirm destructive operations, and mark the editor dirty after modifications.
718-743: LGTM! Clean settings persistence with proper separation.The settings load/save methods properly separate UI state from persistence logic, using a type-safe settings object for data transfer. Settings are loaded at initialization and saved on each successful save operation.
sigmaaa
changed the title
kolipakakondal
left a comment
kolipakakondal
left a comment
There was a problem hiding this comment.
Thanks for the PR @sigmaaa
LGTM
|
@sigmaaa hi! Full CSV-based NVS editor page: inline 4‑column table editing, per-cell editors, add/delete rows, toolbar (save, generate partition), encryption key controls, live validation, status messages, and partition generation. ✅ LGTM 👍 |
* IEP-1549: Updating java version for release and builds (#1241) Co-authored-by: Kondal Kolipaka <kondal.kolipaka@espressif.com> * 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 <85216275+alirana01@users.noreply.github.com> Co-authored-by: Denys Almazov <almazovdenys@gmail.com>
* IEP-1549: Updating java version for release and builds (#1241) Co-authored-by: Kondal Kolipaka <kondal.kolipaka@espressif.com> * 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 <85216275+alirana01@users.noreply.github.com> Co-authored-by: Denys Almazov <almazovdenys@gmail.com>
Description
Converted NVS title area dialog to the editor
Fixes # (IEP-1650)
Type of change
Please delete options that are not relevant.
How has this been tested?
Test as an old nvs editor
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
New Features
Refactor
Chores