Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IEP-1336 Convert partitions.csv or nvs.csv model windows to editors #1070

Merged
merged 2 commits into from
Nov 6, 2024
Merged

IEP-1336 Convert partitions.csv or nvs.csv model windows to editors #1070

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3b470ba
feat: File associations for nvs and partition tables
sigmaaa Oct 25, 2024
835a677
fix: creating empty page to close editor without exception
sigmaaa Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions bundles/com.espressif.idf.ui/plugin.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,6 +713,26 @@
id="com.espressif.idf.ui.manageespidf"
name="%editor.espidf.manager.name">
</editor>
<editor
class="com.espressif.idf.ui.nvs.dialog.NvsEditor"
default="false"
icon="icons/ESP-IDF_NVS_Table_Editor.png"
id="com.espressif.idf.ui.nvs_table_editor"
name="NVS Table Editor">
<contentTypeBinding
contentTypeId="com.espressif.idf.ui.nvs_editor_content_type">
</contentTypeBinding>
</editor>
<editor
class="com.espressif.idf.ui.partitiontable.dialog.PartitionTableEditor"
default="false"
icon="icons/ESP-IDF_Partition_Table_Editor.png"
id="com.espressif.idf.ui.partition_table_editor"
name="Partition Table Editor">
<contentTypeBinding
contentTypeId="com.espressif.idf.ui.partition_editor_content_type">
</contentTypeBinding>
</editor>
</extension>
<extension
point="org.eclipse.ui.views">
Expand Down Expand Up @@ -818,6 +838,18 @@
file-extensions="md"
file-names="README.md">
</file-association>
<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>
Comment on lines +841 to +852
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance content type declarations.

Consider the following improvements:

  1. Remove redundant priority="normal" as it's the default value
  2. 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.

Suggested change
<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>

</extension>
<extension
point="org.eclipse.ltk.core.refactoring.renameParticipants">
Expand Down
55 changes: 55 additions & 0 deletions bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/NvsEditor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package com.espressif.idf.ui.nvs.dialog;

import org.eclipse.core.commands.ExecutionException;
import org.eclipse.core.runtime.IProgressMonitor;
import org.eclipse.swt.SWT;
import org.eclipse.swt.widgets.Composite;
import org.eclipse.ui.PlatformUI;
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
{
Comment on lines +13 to +14
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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 IReusableEditor

Committable suggestion was skipped due to low confidence.

public NvsEditor()
{
PlatformUI.getWorkbench().getDisplay().asyncExec(() -> {
try
{
new NvsEditorHandler().execute(null);
}
catch (ExecutionException e)
{
Logger.log(e);
}
});

}
Comment on lines +15 to +28
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical: Constructor implementation has severe issues.

The current constructor implementation has several critical problems:

  1. Executing async code in constructor violates object lifecycle management
  2. Missing required super() call
  3. Silent error handling masks issues from users
  4. 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.

Suggested change
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));

}
Comment on lines +30 to +38
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical: Page creation implementation contradicts PR objectives.

The current implementation creates an empty page and immediately closes the editor, which:

  1. Contradicts the PR objective of using TitleAreaDialog for NVS table editing
  2. Creates poor UX by showing and immediately closing the editor
  3. 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.


public void doSave(IProgressMonitor monitor)
{
// Nothing to do
}

public void doSaveAs()
{
// Nothing to do
}

public boolean isSaveAsAllowed()
{
return false;
}

}
56 changes: 56 additions & 0 deletions ...espressif.idf.ui/src/com/espressif/idf/ui/partitiontable/dialog/PartitionTableEditor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package com.espressif.idf.ui.partitiontable.dialog;

import org.eclipse.core.commands.ExecutionException;
import org.eclipse.core.runtime.IProgressMonitor;
import org.eclipse.swt.SWT;
import org.eclipse.swt.widgets.Composite;
import org.eclipse.ui.PlatformUI;
import org.eclipse.ui.part.MultiPageEditorPart;

import com.espressif.idf.core.logging.Logger;
import com.espressif.idf.ui.partitiontable.handlers.PartitionTableEditorHandler;

public class PartitionTableEditor extends MultiPageEditorPart
{

public PartitionTableEditor()
{
PlatformUI.getWorkbench().getDisplay().asyncExec(() -> {
try
{
new PartitionTableEditorHandler().execute(null);
}
catch (ExecutionException e)
{
Logger.log(e);
}
});

}

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));
}

public void doSave(IProgressMonitor monitor)
{
// Nothing to do

}

public void doSaveAs()
{
// Nothing to do
}

public boolean isSaveAsAllowed()
{
return false;
}

}
Loading