Refactor menu nodes #14676

tsmaeder · 2024-12-27T15:42:43Z

What it does

Makes menu nodes active object that can decide on visibility, enablement, etc. themselves. This should simplify handling of menus and toolbar items because a lot of special-case code can be moved to polymorphous implementations instead of being handled at each client site. This also allows the special handling of parameter transformations for plugin-contributed actions (toolbar/menu) to be confined to the plugin engine implementation.

How to test

Test that menu functionality still works, in particular actions contributed from plugins. That includes menus and toolbars everywhere for browser and electron cases.

Follow-ups

One strange thing is with the menu/toolbar support is that the MenuRegistry and associated classes are located in the "common" part of the core packages. If we decide that this is no longer that case, we could move the code to "browser". However, this would introduce another huge batch of changes and therefore has been left out of this PR.
There are also a couple of renamings that would make the naming more consistent and logical that have been left out of the PR to keep the size of the PR manageable.

Breaking changes

This PR introduces breaking changes to be documented after a first round of reviews.

This PR introduces breaking changes and requires careful review. If yes, the breaking changes section in the changelog has been updated.

Attribution

Contributed on behalf of STMicroelectronics

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

Fixes eclipse-theia#14217 Makes menu nodes active object that can decide on visibility, enablement, etc. themselves. Contributed on behalf of STMicroelectronics Signed-off-by: Thomas Mäder <[email protected]>

dhuebner · 2025-01-03T10:59:08Z

@tsmaeder

Linting and Playwright-tests are failing. I will fix the tests and add a commit.
Notebook cell actions are broken (I can have a look)
Also CompoundMenuNodeRole.flat menu functionality is not working (see screenshot below). What is the alternative?
It would be nice to update the in-code documentation and also comments because of changed signature or behavior. Especially public API like changes in menu-model-registry.ts should be updated.

tsmaeder · 2025-01-03T13:41:13Z

@dhuebner What are you trying to achieve through CompoundMenuNodeRole.flat?

dhuebner · 2025-01-03T14:12:35Z

packages/toolbar/src/browser/toolbar.tsx

-        if (!ReactTabBarToolbarItem.is(item)) {
-            toolbarItemClassNames = TabBarToolbar.Styles.TAB_BAR_TOOLBAR_ITEM;
-            if (this.evaluateWhenClause(item.when)) {
-                toolbarItemClassNames += ' enabled';


Without adding 'enabled', toolbar items are rendered as disabled

That is the responsibility of the toolbar item.

Then we are missing some implementation, cause the test is failing because workbench.action.showCommands toolbar item is not enabled.

tsmaeder · 2025-01-03T14:19:13Z

@dhuebner rigth now, I'm mostly looking for functional and architectural feedback. I can fix the tests once the dust settles.

dhuebner · 2025-01-03T14:31:07Z

@tsmaeder

What are you trying to achieve through CompoundMenuNodeRole.flat?

The previous effect was that if you click on three dots toolbar item in notebook cell the children of the more sub-menu were rendered directly (see code before). Now an empty composite menu is rendered and the children only if you click on it.

dhuebner · 2025-01-03T14:40:05Z

@tsmaeder
I've pushed fixes for Notebook and Preferences tests. The toolbar test is still failing because of missing enabled css class, see comments above.
Overall it seems to be a huge impact change that might break adopters.

msujew

Looks generally really good. I believe we can improve on the API. Feel free to request my review again once the tests are fixed and I'll do a more in-depth code review.

msujew · 2025-01-06T11:34:47Z

packages/core/src/common/menu/menu-types.ts


+export type CompoundMenuNode = MenuNode & {


Question: Is there a specific reason to prefer type intersections to interfaces? I find interfaces a bit better to read, so I'd prefer them here.

msujew · 2025-01-06T12:32:28Z

packages/core/src/electron-browser/menu/electron-context-menu-renderer.ts

+    protected override doRender(menuPath: MenuPath, menu: CompoundMenuNode,
+        anchor: Anchor,
+        contextMatcher: ContextMatcher,
+        args?: any,
+        context?: HTMLElement,
+        onHide?: () => void
+    ): ContextMenuAccess {


Question: Why not continue using a Options object in here? Using a list of arguments make this more difficult to override and more prone to breakage.

msujew · 2025-01-06T13:03:19Z

packages/core/src/common/menu/menu-model-registry.ts

-        if (!groupNode) {
-            groupNode = new CompositeMenuNode(menuId, label, options, parent);
-            disposable = this.changeEventOnDispose(parent.addNode(groupNode));
+    registerSubmenu(menuPath: MenuPath, label: string, sortString?: string, icon?: string, when?: string, contextKeyOverlay?: Record<string, string>): Disposable {


Suggestion: When refactoring the arguments of the method anyway, why not make it into one single object?

I feel like most JS/TS APIs are moving/have moved to objects, especially if there are optional parameters to a function.

It feels more readable to me, since you no longer need to rely on explicit ordering of arguments.

It preserves better backwards compatibility, as adding a new field to an object is easier than adding a new parameter to a function. At least in my opinion, JS/TS itself doesn't care too much about that during the runtime/compiletime.

msujew · 2025-01-06T13:03:39Z

packages/core/src/common/menu/menu-model-registry.ts

-    registerIndependentSubmenu(id: string, label: string, options?: SubMenuOptions): Disposable {
-        if (this.independentSubmenus.has(id)) {
-            console.debug(`Independent submenu with path ${id} registered, but given ID already exists.`);
+    linkCompoundMenuNode(newParentPath: MenuPath, submenuPath: MenuPath, order?: string, when?: string): Disposable {


Suggestion: We should use objects, see reasoning above.

msujew · 2025-01-06T13:47:17Z

packages/core/src/common/menu/menu-model-registry.ts

-        let nonEmptyNode = undefined;
+    static removeSingleRootNode(fullMenuModel: CompoundMenuNode): CompoundMenuNode {
+
+        let singleChild = undefined;


Minor: Implicitly typed as any

Suggested change

let singleChild = undefined;

let singleChild: CompoundMenuNode | undefined = undefined;

msujew · 2025-01-06T13:50:44Z

packages/core/src/common/menu/menu-types.ts

-     */
-    iconClass?: string;
-}
+export interface CommandMenu extends MenuNode, RenderedMenuNode, Action {


Suggestion: On the other hand, if an interface is completely empty, I'd prefer an intersection type, but that's really only personal preference.

msujew · 2025-01-06T13:51:54Z

packages/core/src/electron-browser/menu/electron-context-menu-renderer.ts

@@ -86,7 +92,7 @@ export class ElectronContextMenuRenderer extends BrowserContextMenuRenderer {
    protected useNativeStyle: boolean = true;

    constructor(@inject(ElectronMainMenuFactory) private electronMenuFactory: ElectronMainMenuFactory) {


Suggestion: If we no longer need the factory in the constructor, I'd rather have it as a protected field injector. That makes it much easier to override from an adopter perspective.

msujew · 2025-01-06T13:53:08Z

packages/core/src/electron-browser/menu/electron-main-menu-factory.ts

@@ -71,11 +66,28 @@ export type ElectronMenuItemRole = ('undo' | 'redo' | 'cut' | 'copy' | 'paste' |
    'selectNextTab' | 'selectPreviousTab' | 'mergeAllWindows' | 'clearRecentDocuments' |
    'moveTabToNewWindow' | 'windowMenu');

+function traverseMenuDto(items: MenuDto[], callback: (item: MenuDto) => void): void {


Suggestion: WDYT about making these protected methods of the class that uses these functions? Having those as local functions, makes overriding for adopters difficult.

msujew · 2025-01-06T14:07:31Z

packages/test/src/browser/view/test-view-contribution.ts

+        menus.registerSubmenu([...PLUGIN_TEST_VIEW_TITLE_MENU, TestViewCommands.RUN_ALL_TESTS.id], '', undefined, undefined, undefined, {
+            'testing.profile.context.group': 'run'
+        });
+
+        menus.registerSubmenu([...PLUGIN_TEST_VIEW_TITLE_MENU, TestViewCommands.DEBUG_ALL_TESTS.id], '', undefined, undefined, undefined, {
+            'testing.profile.context.group': 'debug'
+        });


Having undefined, undefined, undefined here is a good point to support my argument in favor of objects :)

Signed-off-by: Thomas Mäder <[email protected]>

Refactor menu nodes

3bda287

Fixes eclipse-theia#14217 Makes menu nodes active object that can decide on visibility, enablement, etc. themselves. Contributed on behalf of STMicroelectronics Signed-off-by: Thomas Mäder <[email protected]>

tsmaeder requested review from dhuebner and msujew December 27, 2024 15:42

Fixed failing notebook tests and lint errors

077028e

dhuebner reviewed Jan 3, 2025

View reviewed changes

Fix preferences tests

f56fd00

msujew reviewed Jan 6, 2025

View reviewed changes

Fix linter issues

761cc49

Signed-off-by: Thomas Mäder <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor menu nodes #14676

Refactor menu nodes #14676

tsmaeder commented Dec 27, 2024 •

edited

Loading

dhuebner commented Jan 3, 2025 •

edited

Loading

tsmaeder commented Jan 3, 2025

dhuebner Jan 3, 2025

tsmaeder Jan 3, 2025

dhuebner Jan 3, 2025

tsmaeder commented Jan 3, 2025

dhuebner commented Jan 3, 2025

dhuebner commented Jan 3, 2025

msujew left a comment

msujew Jan 6, 2025

msujew Jan 6, 2025

msujew Jan 6, 2025

msujew Jan 6, 2025

msujew Jan 6, 2025

msujew Jan 6, 2025

msujew Jan 6, 2025

msujew Jan 6, 2025

msujew Jan 6, 2025

	let singleChild = undefined;
	let singleChild: CompoundMenuNode \| undefined = undefined;

		@@ -86,7 +92,7 @@ export class ElectronContextMenuRenderer extends BrowserContextMenuRenderer {
		protected useNativeStyle: boolean = true;

		constructor(@inject(ElectronMainMenuFactory) private electronMenuFactory: ElectronMainMenuFactory) {

Refactor menu nodes #14676

Are you sure you want to change the base?

Refactor menu nodes #14676

Conversation

tsmaeder commented Dec 27, 2024 • edited Loading

What it does

How to test

Follow-ups

Breaking changes

Attribution

Review checklist

Reminder for reviewers

dhuebner commented Jan 3, 2025 • edited Loading

tsmaeder commented Jan 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder commented Jan 3, 2025

dhuebner commented Jan 3, 2025

dhuebner commented Jan 3, 2025

msujew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder commented Dec 27, 2024 •

edited

Loading

dhuebner commented Jan 3, 2025 •

edited

Loading