IEP-1336 Convert partitions.csv or nvs.csv model windows to editors #1070
Conversation
WalkthroughThe changes in this pull request focus on enhancing the Espressif IDF UI plugin by introducing two new editors, Changes
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (1)
bundles/com.espressif.idf.ui/plugin.xml (1)
716-735: Consider externalizing editor names for internationalization.The editor names "NVS Table Editor" and "Partition Table Editor" are hardcoded. Consider using externalized strings (e.g.,
%editor.name.nvsand%editor.name.partition) to support internationalization.- name="NVS Table Editor"> + name="%editor.name.nvs">- name="Partition Table Editor"> + name="%editor.name.partition">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- bundles/com.espressif.idf.ui/plugin.xml (2 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/partitiontable/dialog/PartitionTableEditor.java (1 hunks)
🔇 Additional comments (4)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditor.java (1)
1-50: Verify editor registration for nvs.csv files.Let's ensure the editor is properly registered in plugin.xml for handling nvs.csv files.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/partitiontable/dialog/PartitionTableEditor.java (3)
1-11: LGTM: Class structure aligns with requirementsThe class correctly extends MultiPageEditorPart and includes necessary imports.
30-46: Implement or document save behavior requirementsThe save-related methods are currently TODO stubs. If this editor is meant to be temporary (as suggested by the immediate closure), we should either:
- Document why save functionality isn't needed, or
- Implement proper save behavior if required
Would you like me to help implement the save functionality or create a GitHub issue to track this requirement?
Let's check if save functionality is implemented in related editors:
25-28:⚠️ Potential issueClarify the purpose of immediate editor closure
The current implementation of
createPages()immediately closes the editor, which seems counterintuitive. If this is intentional behavior (e.g., if the editor is meant to be temporary), it should be documented.Let's check if this pattern is used elsewhere in the codebase:
Consider these improvements:
- Add documentation explaining why the editor is closed immediately
- Ensure proper cleanup of resources
- Consider adding a user-friendly message explaining why the editor was closed
+ /** + * Creates the pages of the editor. + * Note: This implementation immediately closes the editor because [explain reason]. + */ protected void createPages() { + // Ensure cleanup of any resources + dispose(); getSite().getPage().closeEditor(this, false); }
| package com.espressif.idf.ui.nvs.dialog; | ||
|
|
||
| import org.eclipse.core.commands.ExecutionException; | ||
| import org.eclipse.core.runtime.IProgressMonitor; | ||
| import org.eclipse.ui.part.MultiPageEditorPart; | ||
|
|
||
| import com.espressif.idf.core.logging.Logger; | ||
| import com.espressif.idf.ui.nvs.handlers.NvsEditorHandler; | ||
|
|
||
| public class NvsEditor extends MultiPageEditorPart | ||
| { | ||
|
|
||
| public NvsEditor() | ||
| { | ||
| try | ||
| { | ||
| new NvsEditorHandler().execute(null); | ||
| } | ||
| catch (ExecutionException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| protected void createPages() | ||
| { | ||
| // TODO Auto-generated method stub | ||
| getSite().getPage().closeEditor(this, false); | ||
| } | ||
|
|
||
| public void doSave(IProgressMonitor monitor) | ||
| { | ||
| // TODO Auto-generated method stub | ||
|
|
||
| } | ||
|
|
||
| public void doSaveAs() | ||
| { | ||
| // TODO Auto-generated method stub | ||
|
|
||
| } | ||
|
|
||
| public boolean isSaveAsAllowed() | ||
| { | ||
| // TODO Auto-generated method stub | ||
| return false; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add essential editor lifecycle methods.
The editor is missing several essential methods for proper lifecycle management.
Consider adding these methods:
@Override
public void init(IEditorSite site, IEditorInput input) throws PartInitException {
if (!(input instanceof IFileEditorInput)) {
throw new PartInitException("Invalid input: Must be IFileEditorInput");
}
super.init(site, input);
setPartName(input.getName());
}
@Override
public boolean isDirty() {
// Implement dirty state tracking
return false;
}
@Override
public void setFocus() {
// Set focus to the active page
getContainer().setFocus();
}
@Override
public void dispose() {
// Clean up resources
super.dispose();
}| protected void createPages() | ||
| { | ||
| // TODO Auto-generated method stub | ||
| getSite().getPage().closeEditor(this, false); | ||
| } |
There was a problem hiding this comment.
Implement proper page creation instead of closing the editor.
The current implementation defeats the purpose of the editor by immediately closing it. According to the PR objectives, this should create a TitleAreaDialog for NVS table editing.
Consider this implementation:
protected void createPages()
{
- // TODO Auto-generated method stub
- getSite().getPage().closeEditor(this, false);
+ try {
+ // Create the dialog page
+ NvsTableDialog dialog = new NvsTableDialog(getSite().getShell());
+ int page = addPage(dialog.createDialogArea(getContainer()));
+ setPageText(page, "NVS Table Editor");
+ } catch (Exception e) {
+ Logger.log("Failed to create NVS editor pages", e);
+ MessageDialog.openError(
+ getSite().getShell(),
+ "Page Creation Error",
+ "Failed to create editor pages: " + e.getMessage());
+ }
}Committable suggestion was skipped due to low confidence.
| public class NvsEditor extends MultiPageEditorPart | ||
| { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing essential editor interfaces.
The editor should implement IReusableEditor if it's meant to be reused for different inputs, and ISaveablePart for proper integration with Eclipse's save lifecycle.
-public class NvsEditor extends MultiPageEditorPart
+public class NvsEditor extends MultiPageEditorPart implements IReusableEditorCommittable suggestion was skipped due to low confidence.
| public void doSave(IProgressMonitor monitor) | ||
| { | ||
| // TODO Auto-generated method stub | ||
|
|
||
| } | ||
|
|
||
| public void doSaveAs() | ||
| { | ||
| // TODO Auto-generated method stub | ||
|
|
||
| } | ||
|
|
||
| public boolean isSaveAsAllowed() | ||
| { | ||
| // TODO Auto-generated method stub | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Implement proper save functionality.
The editor is missing proper save functionality which is essential for editing nvs.csv files.
Consider implementing these methods:
public void doSave(IProgressMonitor monitor)
{
- // TODO Auto-generated method stub
+ try {
+ // Get the editor's input file
+ IFile file = ((IFileEditorInput) getEditorInput()).getFile();
+ // Save the NVS table data
+ saveNvsTableData(file, monitor);
+ // Mark editor as not dirty
+ firePropertyChange(PROP_DIRTY);
+ } catch (Exception e) {
+ Logger.log("Failed to save NVS table", e);
+ MessageDialog.openError(
+ getSite().getShell(),
+ "Save Error",
+ "Failed to save NVS table: " + e.getMessage());
+ }
}
public void doSaveAs()
{
- // TODO Auto-generated method stub
+ // Implement if needed in future
}
public boolean isSaveAsAllowed()
{
- // TODO Auto-generated method stub
- return false;
+ return false; // Save As not supported for NVS tables
}Committable suggestion was skipped due to low confidence.
| public NvsEditor() | ||
| { | ||
| try | ||
| { | ||
| new NvsEditorHandler().execute(null); | ||
| } | ||
| catch (ExecutionException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Refactor constructor implementation.
The current implementation has several issues:
- Executing the handler in the constructor violates separation of concerns and could cause issues during editor initialization
- Silent error handling could mask critical issues
- Missing call to super()
Consider this refactoring:
public NvsEditor()
{
+ super();
+ }
+
+ @Override
+ protected void init() {
try
{
new NvsEditorHandler().execute(null);
}
catch (ExecutionException e)
{
- Logger.log(e);
+ Logger.log("Failed to initialize NVS editor", e);
+ MessageDialog.openError(
+ getSite().getShell(),
+ "NVS Editor Error",
+ "Failed to initialize the NVS editor: " + e.getMessage());
}
}Committable suggestion was skipped due to low confidence.
| public PartitionTableEditor() | ||
| { | ||
| try | ||
| { | ||
| new PartitionTableEditorHandler().execute(null); | ||
| } | ||
| catch (ExecutionException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Refactor constructor to improve error handling and separation of concerns
The current implementation has several issues:
- Executing the handler in the constructor violates separation of concerns and could lead to issues during editor initialization
- Silent error handling could mask critical issues from users
- If execution fails, resources might not be properly cleaned up
Consider this alternative implementation:
- public PartitionTableEditor()
- {
- try
- {
- new PartitionTableEditorHandler().execute(null);
- }
- catch (ExecutionException e)
- {
- Logger.log(e);
- }
- }
+ public PartitionTableEditor() {
+ // Initialize editor resources here
+ }
+
+ @Override
+ public void init(IEditorSite site, IEditorInput input)
+ throws PartInitException {
+ super.init(site, input);
+ try {
+ new PartitionTableEditorHandler().execute(null);
+ } catch (ExecutionException e) {
+ Logger.log(e);
+ throw new PartInitException("Failed to initialize Partition Table Editor", e);
+ }
+ }Committable suggestion was skipped due to low confidence.
| <content-type | ||
| file-patterns="nvs.csv" | ||
| id="com.espressif.idf.ui.nvs_editor_content_type" | ||
| name="NVS Table" | ||
| priority="normal"> | ||
| </content-type> | ||
| <content-type | ||
| file-patterns="partitions.csv" | ||
| id="com.espressif.idf.ui.partition_editor_content_type" | ||
| name="Partition Table" | ||
| priority="normal"> | ||
| </content-type> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance content type declarations.
Consider the following improvements:
- Remove redundant
priority="normal"as it's the default value - Add
base-type="org.eclipse.core.runtime.text"to inherit text file behavior
<content-type
file-patterns="nvs.csv"
id="com.espressif.idf.ui.nvs_editor_content_type"
name="NVS Table"
- priority="normal">
+ base-type="org.eclipse.core.runtime.text">
</content-type>
<content-type
file-patterns="partitions.csv"
id="com.espressif.idf.ui.partition_editor_content_type"
name="Partition Table"
- priority="normal">
+ base-type="org.eclipse.core.runtime.text">
</content-type>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <content-type | |
| file-patterns="nvs.csv" | |
| id="com.espressif.idf.ui.nvs_editor_content_type" | |
| name="NVS Table" | |
| priority="normal"> | |
| </content-type> | |
| <content-type | |
| file-patterns="partitions.csv" | |
| id="com.espressif.idf.ui.partition_editor_content_type" | |
| name="Partition Table" | |
| priority="normal"> | |
| </content-type> | |
| <content-type | |
| file-patterns="nvs.csv" | |
| id="com.espressif.idf.ui.nvs_editor_content_type" | |
| name="NVS Table" | |
| base-type="org.eclipse.core.runtime.text"> | |
| </content-type> | |
| <content-type | |
| file-patterns="partitions.csv" | |
| id="com.espressif.idf.ui.partition_editor_content_type" | |
| name="Partition Table" | |
| base-type="org.eclipse.core.runtime.text"> | |
| </content-type> |
There was a problem hiding this comment.
Thanks for the PR. LGTM.
Can you check the errors reported in the error log while opening these files.
java.lang.NullPointerException: Cannot invoke "org.eclipse.e4.core.contexts.IEclipseContext.createChild(String)" because "parentContext" is null
at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.safeCreateGui(PartRenderingEngine.java:630)
at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.safeCreateGui(PartRenderingEngine.java:762)
at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$2.run(PartRenderingEngine.java:727)
at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:47)
at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.createGui(PartRenderingEngine.java:711)
at org.eclipse.e4.ui.internal.workbench.PartServiceImpl.createElement(PartServiceImpl.java:1314)
at org.eclipse.e4.ui.internal.workbench.PartServiceImpl.delegateBringToTop(PartServiceImpl.java:797)
at org.eclipse.e4.ui.internal.workbench.PartServiceImpl.bringToTop(PartServiceImpl.java:401)
at org.eclipse.e4.ui.internal.workbench.PartServiceImpl.showPart(PartServiceImpl.java:1271)
at org.eclipse.ui.internal.WorkbenchPage.busyOpenEditor(WorkbenchPage.java:3228)
at org.eclipse.ui.internal.WorkbenchPage.lambda$11(WorkbenchPage.java:3118)
at org.eclipse.swt.custom.BusyIndicator.showWhile(BusyIndicator.java:67)
at org.eclipse.ui.internal.WorkbenchPage.openEditor(WorkbenchPage.java:3116)
at org.eclipse.ui.internal.WorkbenchPage.openEditor(WorkbenchPage.java:3086)
at org.eclipse.ui.internal.WorkbenchPage.openEditor(WorkbenchPage.java:3077)
at org.eclipse.ui.ide.IDE.openEditor(IDE.java:546)
at org.eclipse.ui.ide.IDE.openEditor(IDE.java:506)
at org.eclipse.jdt.internal.ui.javaeditor.EditorUtility.openInEditor(EditorUtility.java:370)
at org.eclipse.jdt.internal.ui.javaeditor.EditorUtility.openInEditor(EditorUtility.java:172)
at org.eclipse.jdt.ui.actions.OpenAction.run(OpenAction.java:288)
at org.eclipse.jdt.ui.actions.OpenAction.run(OpenAction.java:254)
at org.eclipse.jdt.ui.actions.SelectionDispatchAction.dispatchRun(SelectionDispatchAction.java:274)
at org.eclipse.jdt.ui.actions.SelectionDispatchAction.run(SelectionDispatchAction.java:252)
at org.eclipse.jdt.internal.ui.navigator.OpenAndExpand.run(OpenAndExpand.java:53)
at org.eclipse.ui.actions.RetargetAction.run(RetargetAction.java:215)
at org.eclipse.ui.navigator.CommonNavigatorManager$1.open(CommonNavigatorManager.java:184)
at org.eclipse.ui.OpenAndLinkWithEditorHelper$InternalListener.open(OpenAndLinkWithEditorHelper.java:48)
at org.eclipse.jface.viewers.StructuredViewer$2.run(StructuredViewer.java:799)
at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:47)
at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:174)
at org.eclipse.jface.viewers.StructuredViewer.fireOpen(StructuredViewer.java:796)
at org.eclipse.jface.viewers.StructuredViewer.handleOpen(StructuredViewer.java:1110)
at org.eclipse.ui.navigator.CommonViewer.handleOpen(CommonViewer.java:451)
at org.eclipse.jface.util.OpenStrategy.fireOpenEvent(OpenStrategy.java:295)
at org.eclipse.jface.util.OpenStrategy$1.handleEvent(OpenStrategy.java:330)
at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:91)
at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4660)
at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1622)
at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1645)
at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1630)
at org.eclipse.swt.widgets.Widget.notifyListeners(Widget.java:1392)
at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4427)
at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:4003)
at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1151)
at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339)
at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1042)
at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:152)
at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:639)
at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339)
at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:546)
at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:173)
at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:152)
at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:208)
at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:143)
at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:109)
at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:439)
at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:271)
at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
at java.base/java.lang.reflect.Method.invoke(Method.java:580)
at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:668)
at org.eclipse.equinox.launcher.Main.basicRun(Main.java:605)
at org.eclipse.equinox.launcher.Main.run(Main.java:1481)
at org.eclipse.equinox.launcher.Main.main(Main.java:1454)
|
Hi @kolipakakondal, @AndriiFilippov I've added a workaround (adding empty page) to close the editor without exception in the last commit |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
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/partitiontable/dialog/PartitionTableEditor.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/partitiontable/dialog/PartitionTableEditor.java
🔇 Additional comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditor.java (1)
1-55: Verify editor registration and file association.
To ensure the editor works with double-clicking nvs.csv files as mentioned in PR objectives, we need to verify the plugin.xml configuration.
✅ Verification successful
Editor registration and file association are properly configured
The NVS editor is correctly registered in plugin.xml with:
- Editor class registration pointing to
com.espressif.idf.ui.nvs.dialog.NvsEditor - Content type binding for
nvs.csvfiles viacom.espressif.idf.ui.nvs_editor_content_type - Proper icon and name configuration
- Content type definition with file pattern
nvs.csv
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify editor registration and file association in plugin.xml
# Test 1: Check editor registration
echo "Checking editor registration..."
rg -A 5 '<editor.*class="com\.espressif\.idf\.ui\.nvs\.dialog\.NvsEditor"' plugin.xml
# Test 2: Check content type association
echo "Checking content type association..."
rg -A 5 '<content-type.*file-patterns="nvs\.csv"' plugin.xml
# Test 3: Check file extension association
echo "Checking file extension association..."
rg -A 5 '<fileTypes.*extension="csv"' plugin.xml
Length of output: 597
Script:
#!/bin/bash
# Search for plugin.xml files in the repository
echo "Searching for plugin.xml files..."
fd "plugin.xml"
# After finding the correct path, let's check editor registration
echo -e "\nChecking editor registration in all plugin.xml files..."
fd "plugin.xml" --exec rg -A 5 '<editor.*class="com\.espressif\.idf\.ui\.nvs\.dialog\.NvsEditor"' {}
# Check content type and file associations
echo -e "\nChecking content type and file associations..."
fd "plugin.xml" --exec rg -A 5 'nvs\.csv|\.csv' {}
Length of output: 1649
Script:
#!/bin/bash
# Let's specifically check the UI plugin.xml since that's where our editor is located
echo "Checking com.espressif.idf.ui/plugin.xml for editor and extension configurations..."
cat bundles/com.espressif.idf.ui/plugin.xml
# Also check for any editor associations in plugin.xml
echo -e "\nChecking for editor associations in plugin.xml..."
rg -A 10 '<editor' bundles/com.espressif.idf.ui/plugin.xml
Length of output: 26913
| public NvsEditor() | ||
| { | ||
| PlatformUI.getWorkbench().getDisplay().asyncExec(() -> { | ||
| try | ||
| { | ||
| new NvsEditorHandler().execute(null); | ||
| } | ||
| catch (ExecutionException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| }); | ||
|
|
||
| } |
There was a problem hiding this comment.
Critical: Constructor implementation has severe issues.
The current constructor implementation has several critical problems:
- Executing async code in constructor violates object lifecycle management
- Missing required super() call
- Silent error handling masks issues from users
- Handler execution belongs in editor initialization, not construction
Apply this refactoring to fix the issues:
public NvsEditor()
{
- PlatformUI.getWorkbench().getDisplay().asyncExec(() -> {
- try
- {
- new NvsEditorHandler().execute(null);
- }
- catch (ExecutionException e)
- {
- Logger.log(e);
- }
- });
+ super();
+}
+
+@Override
+protected void init() {
+ try {
+ new NvsEditorHandler().execute(null);
+ }
+ catch (ExecutionException e) {
+ Logger.log("Failed to initialize NVS editor", e);
+ MessageDialog.openError(
+ getSite().getShell(),
+ "Editor Initialization Error",
+ "Failed to initialize the NVS editor: " + e.getMessage());
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public NvsEditor() | |
| { | |
| PlatformUI.getWorkbench().getDisplay().asyncExec(() -> { | |
| try | |
| { | |
| new NvsEditorHandler().execute(null); | |
| } | |
| catch (ExecutionException e) | |
| { | |
| Logger.log(e); | |
| } | |
| }); | |
| } | |
| public NvsEditor() | |
| { | |
| super(); | |
| } | |
| @Override | |
| protected void init() { | |
| try { | |
| new NvsEditorHandler().execute(null); | |
| } | |
| catch (ExecutionException e) { | |
| Logger.log("Failed to initialize NVS editor", e); | |
| MessageDialog.openError( | |
| getSite().getShell(), | |
| "Editor Initialization Error", | |
| "Failed to initialize the NVS editor: " + e.getMessage()); | |
| } | |
| } |
| protected void createPages() | ||
| { | ||
| Composite emptyPage = new Composite(getContainer(), SWT.NONE); | ||
|
|
||
| int index = addPage(emptyPage); | ||
| setPageText(index, "Empty Page"); //$NON-NLS-1$ | ||
| getSite().getShell().getDisplay().asyncExec(() -> getSite().getPage().closeEditor(this, false)); | ||
|
|
||
| } |
There was a problem hiding this comment.
Critical: Page creation implementation contradicts PR objectives.
The current implementation creates an empty page and immediately closes the editor, which:
- Contradicts the PR objective of using TitleAreaDialog for NVS table editing
- Creates poor UX by showing and immediately closing the editor
- Makes the editor effectively non-functional
Implement proper dialog-based editing as per PR objectives:
protected void createPages()
{
- Composite emptyPage = new Composite(getContainer(), SWT.NONE);
-
- int index = addPage(emptyPage);
- setPageText(index, "Empty Page"); //$NON-NLS-1$
- getSite().getShell().getDisplay().asyncExec(() -> getSite().getPage().closeEditor(this, false));
+ try {
+ // Create dialog page using TitleAreaDialog as per PR objectives
+ NvsTableDialog dialog = new NvsTableDialog(getSite().getShell());
+ Composite dialogArea = dialog.createDialogArea(getContainer());
+
+ int index = addPage(dialogArea);
+ setPageText(index, "NVS Table Editor");
+
+ // Initialize the dialog with file content
+ IFile file = ((IFileEditorInput) getEditorInput()).getFile();
+ dialog.initializeFromFile(file);
+ } catch (Exception e) {
+ Logger.log("Failed to create NVS editor page", e);
+ MessageDialog.openError(
+ getSite().getShell(),
+ "Editor Error",
+ "Failed to create editor page: " + e.getMessage());
+ }
}Committable suggestion skipped: line range outside the PR's diff.
|
@sigmaaa hi ! Tested under: able to open with double click both NVS and Partition. LGTM 👍 |
Description
To avoid overriding and adjusting a lot of code, I'm using MultiPageEditor as a handler to call a TitleAreaDialog.
Fixes # (IEP-1336)
Type of change
Please delete options that are not relevant.
How has this been tested?
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
New Features
.nvs.csvand.partitions.csvfiles.Improvements