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

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?

packages/toolbar/src/browser/toolbar.tsx

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.

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

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

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.

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

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.

I don't think there is a use-case for the traverse functions to be polymorphic because they are simple algorithms. If anything should be overridable it should be what happens when a preference changes. But that is tightly coupled to the mechanisms in this class, so I don't think overriding parts of it makes much sense.

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

JonasHelming · 2025-01-29T09:25:52Z

@tsmaeder What is the current state of this PR?

tsmaeder · 2025-01-29T09:43:27Z

There are a couple of issues that need to be addressed before merging. Since it's a major refactoring, it's not a good idea to merge before the release anyway.

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]>

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

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

dhuebner reviewed Jan 3, 2025

View reviewed changes

packages/toolbar/src/browser/toolbar.tsx Show resolved Hide resolved

msujew reviewed Jan 6, 2025

View reviewed changes

tsmaeder force-pushed the 14217_refactor_menu_node branch 2 times, most recently from 2ecff6b to b161567 Compare January 9, 2025 15:50

tsmaeder and others added 7 commits February 3, 2025 15:51

Refactor menu nodes

57069cd

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]>

Fixed failing notebook tests and lint errors

d74fba4

Fix preferences tests

1c9e6ab

Fix linter issues

190a842

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

Applied some review suggestions

b4cd4ab

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

Refactorings to enable menu tests

fd1fdb2

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

Fix playwright enablement check.

4c1df46

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

tsmaeder force-pushed the 14217_refactor_menu_node branch from b161567 to 4c1df46 Compare February 3, 2025 16:43

tsmaeder added 3 commits February 4, 2025 06:33

Address review comment and add playwright test launch config

734bc21

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

Removed duplicate field

ad1436b

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

Changelog info

04352f2

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

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

tsmaeder Feb 4, 2025

JonasHelming commented Jan 29, 2025

tsmaeder commented Jan 29, 2025

		@@ -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

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

msujew Jan 6, 2025

Choose a reason for hiding this comment

msujew Jan 6, 2025

Choose a reason for hiding this comment

msujew Jan 6, 2025

Choose a reason for hiding this comment

msujew Jan 6, 2025

Choose a reason for hiding this comment

tsmaeder Feb 4, 2025

Choose a reason for hiding this comment

JonasHelming commented Jan 29, 2025

tsmaeder commented Jan 29, 2025

tsmaeder commented Dec 27, 2024 •

edited

Loading

dhuebner commented Jan 3, 2025 •

edited

Loading