Added support for custom menu node registration. #8404
Conversation
vince-fugnitto
added
enhancement
There was a problem hiding this comment.
I verified the functionality of the pull-request and it works well:
- verified using the
browsertarget - verified using the
electrontarget - verified if there were any regressions with existing menus (sub-menus, action menus, context menus)
I also reviewed the code and do not have any comments, I'll however give @akosyakov a chance to review as well.
|
I will review API changes tomorrow morning. |
Sure. No hurry at all. It is not urgent for me. It can even wait until next week if you're busy. |
packages/core/src/common/menu.ts
Outdated
| const actionNode = new ActionMenuNode(item, this.commands); | ||
| return parent.addNode(actionNode); | ||
| } | ||
|
|
||
| registerMenuPlaceholder(menuPath: MenuPath, label: string, options?: SubMenuOptions): Disposable { |
There was a problem hiding this comment.
Maybe we could add registerMenuNode(MenuPath, string, MenuNode) instead, it would allow downstreams do whatever they want and won't require us to add new APIs for different cases. I was trying to find other APIs having something like registerMenuPlaceholder, but could not. Chrome for instance creates a UI menu element assigns it a title and pushes in the parent menu first.
There was a problem hiding this comment.
Rendering is abstract from MenuNode so such API is not feasible. Ok, let's go with registerMenuPlaceholder. Although meaning feels unclear to me. Maybe just a naming thing.
There was a problem hiding this comment.
I wonder also how hard is to the client to achieve such effect with registerMenuAction api. It seems to be one command which is always visible, but disabled and then many menu actions are reusing it?
There was a problem hiding this comment.
I wonder also how hard is to the client
It's hard, as the menu items are always enabled in electron:
Maybe we could add registerMenuNode(MenuPath, string, MenuNode) instead,
Let's give it a try. I did not want to introduce new APIs at all but add a groupLabel?: stirng to the SubMenuOptions. But there were cases it did not work. Let me check this once more.
There was a problem hiding this comment.
But sub menu and menu group are not the same? Maybe registerMenuGroup(subMenuPath, 'Recently Closed')? It can be then applied to any menu path, not only for submenus, to get effects like in Chrome for Recently Closed, for instance.
I think such naming also aligned with what we already have and provides explicit API to control menu groups' options.
packages/core/src/common/menu.ts
Outdated
| @@ -78,15 +78,21 @@ export class MenuModelRegistry { | |||
| } | |||
| } | |||
|
|
|||
| registerMenuAction(menuPath: MenuPath, item: MenuAction): Disposable { | |||
| const parent = this.findGroup(menuPath); | |||
| registerMenuAction(menuPath: MenuPath, item: MenuAction, options?: SubMenuOptions): Disposable { | |||
There was a problem hiding this comment.
Why do we need to pass SubMenuOptions here? Clients should call registerSubmenu to configure the sub menu properly.
There was a problem hiding this comment.
Good catch! That's a leftover and has to be removed.
kittaakos
force-pushed
the
menu-group-label
branch
from
366972b to
b55b223
Compare
kittaakos
changed the title
I implemented this and added an example. Please review. |
| return { | ||
| context: this.context, | ||
| contextKeyService: this.contextKeyService, | ||
| commandRegistry: this.commandRegistry, | ||
| keybindingRegistry: this.keybindingRegistry | ||
| keybindingRegistry: this.keybindingRegistry, | ||
| meneWidgetFactory: this |
Signed-off-by: Akos Kitta <kittaakos@typefox.io>
`MenuModelRegistry#registerMenuNode` Signed-off-by: Akos Kitta <kittaakos@typefox.io>
kittaakos
force-pushed
the
menu-group-label
branch
from
b55b223 to
b92e222
Compare
Signed-off-by: Akos Kitta kittaakos@typefox.io
What it does
From now on, it's possible to register a custom menu node into the menu registry.
Added a placeholder example:
How to test
Check the sample apps.
Review checklist
Reminder for reviewers