IEP-1498 Clean-up extensions in the ui plugin #1209
Conversation
WalkthroughThe plugin.xml file for the Eclipse plugin was extensively reorganized and expanded. Menu, command, and handler definitions were consolidated and restructured, with new commands, handlers, and menu contributions added for various ESP-IDF-related actions. The changes introduce a dedicated Espressif main menu with dynamic submenus and refined visibility conditions. Command declarations and their handler mappings were updated, new command categories were introduced, and wizard and perspective extensions were moved and clarified. Some redundant or duplicate definitions were removed, and property tester extensions were repositioned for clarity. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EclipseUI
participant EspressifMenu
participant CommandHandler
User->>EclipseUI: Open Espressif menu
EclipseUI->>EspressifMenu: Display menu items (commands, submenus)
User->>EspressifMenu: Select a command (e.g., New Project)
EspressifMenu->>CommandHandler: Invoke associated handler
CommandHandler->>EclipseUI: Execute command action (e.g., open wizard, run tool)
Possibly related PRs
Suggested reviewers
Poem
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. 🪧 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: 2
🔭 Outside diff range comments (1)
bundles/com.espressif.idf.ui/plugin.xml (1)
405-409:⚠️ Potential issueFully qualify
defaultHandlerforchangeLanguage.
You wrote:<command defaultHandler="ChangeLanguageHandler" id="com.espressif.idf.ui.changeLanguage" …>Eclipse requires the fully qualified class name. Update to:
defaultHandler="com.espressif.idf.ui.handlers.ChangeLanguageHandler"
🧹 Nitpick comments (4)
bundles/com.espressif.idf.ui/plugin.xml (4)
23-30: Namespace menu ID to avoid collisions.
The<menu id="espressifMenu">is unprefixed and could clash with other plugins. Consider using a fully qualified ID, e.g.<menu id="com.espressif.idf.ui.menu.espressifMenu" …>
51-55: Deduplicate and correct separator IDs.
There are duplicate and misspelled separator IDs:
- At lines 51–55:
name="com.espressif.idf.ui.separator1"- At lines 207–210: same ID
- Separators at lines 83–85 and 143–146 use
seperatorinstead ofseparator.IDs must be unique and typo-free. Please rename to something like:
- name="com.espressif.idf.ui.separator1" + name="com.espressif.idf.ui.menu.separator.tools"Also applies to: 207-210
112-112: Remove trailing whitespace inlabel.
Line 112 haslabel="%command.label.4 "with an extra space before the closing quote. This can cause unexpected UI rendering.
123-132: Add missingtooltipfor consistency.
The “New Project” and “New Component” commands underpopup:common.new.menudon’t includetooltipattributes, but most other entries do. For a consistent UX, consider adding:tooltip="%wizard.tooltip"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.ui/plugin.xml(3 hunks)
🔇 Additional comments (3)
bundles/com.espressif.idf.ui/plugin.xml (3)
69-77: Validate dynamic menu contribution.
The dynamic submenu is defined as:<menu … id="changeLanguage"> <dynamic class="com.espressif.idf.ui.menuitem.LanguageDynamicMenuItem" id="com.espressif.idf.ui.LanguageDynamicMenu"> </dynamic> </menu>
- Verify that the
id="com.espressif.idf.ui.LanguageDynamicMenu"is unique.- Ensure
LanguageDynamicMenuItemimplementsIContributionItemcorrectly and is on the plugin’s classpath.
395-398: Category declaration looks correct.
Your new command categoryorg.plugin.espressif.commands.categoryis declared properly and matches the IDs used later.
426-466: 🛠️ Refactor suggestionInconsistent command IDs and missing handler.
- The ID
com.espressif.idf.ui.commands.installespidfuses a pluralcommandssegment, unlike the singular patterncom.espressif.idf.ui.command.*.- That entry also lacks a
defaultHandler, so it won’t invoke any logic.Please:
- Rename to
com.espressif.idf.ui.command.installEspIdf.- Add the correct handler, for example:
- <command categoryId="org.plugin.espressif.commands.category" - id="com.espressif.idf.ui.commands.installespidf" + <command categoryId="org.plugin.espressif.commands.category" + id="com.espressif.idf.ui.command.installEspIdf" name="%command.name.installEspIdf" + defaultHandler="com.espressif.idf.ui.update.InstallEspIdfHandler">Likely an incorrect or invalid review comment.
| commandId="com.espressif.idf.ui.appLvlTracing"> | ||
| </handler> | ||
| <handler |
There was a problem hiding this comment.
Handler for appLvlTracing has no implementation.
You declared:
<handler commandId="com.espressif.idf.ui.appLvlTracing">
</handler>- No
classattribute is provided, so the handler won’t run. - There is also no
<command id="com.espressif.idf.ui.appLvlTracing">in any commands extension, so this menu item is orphaned.
Please add both:
- A
<command id="com.espressif.idf.ui.appLvlTracing" …>entry. - A
class="…"on the<handler>(e.g.com.espressif.idf.ui.handlers.AppLevelTracingHandler).
| <visibleWhen | ||
| checkEnabled="false"> | ||
| <test | ||
| property="com.espressif.idf.ui.propertyTester.testProperty"> | ||
| </test> | ||
| </visibleWhen> |
There was a problem hiding this comment.
Fix the <test> element in <visibleWhen>.
You declared:
<visibleWhen checkEnabled="false">
<test property="com.espressif.idf.ui.propertyTester.testProperty">
</test>
</visibleWhen>But the <test> tag is missing a value (or forcePluginActivation, etc.) attribute, so the condition is never evaluated correctly. Add the appropriate value="…", for example:
<test property="com.espressif.idf.ui.propertyTester.testProperty" value="true"/>
There was a problem hiding this comment.
LGTM!
Just a precaution @sigmaaa kindly review all the subsequent PRs with care specifically where the plugin.xml from ui project is changed as it can cause a lot of conflicts and the xml is prone for missing changes and causing issues
There was a problem hiding this comment.
LGTM
@AndriiFilippov Could you check if the automated tests will verify these changes? Ideally, our automated scripts should capture any issues in the file menus and their command executions. If such tests are missing, please take this opportunity to add scripts that check whether all expected commands are available and that their actions work as intended.
Description
Group menus, commands, and handlers for better visibility
Was:
Now:
Fixes # (IEP-1498)
Type of change
Please delete options that are not relevant.
How has this been tested?
Verify that all commands in all menus work
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
New Features
Refactor