Skip to content

Pass correct "effectiveMenuPath" to menu actions. #16148

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

Merged
merged 3 commits into from
Sep 23, 2025
Merged

Pass correct "effectiveMenuPath" to menu actions. #16148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7f4faa1
Pass correct "effectiveMenuPath" to menu actions.
tsmaeder Aug 8, 2025
82491ea
Merge branch 'master' into 16132_fix_effective_menu_path
ndoschek Sep 22, 2025
af272ed
Remove commented code
colin-grant-work Sep 22, 2025
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
6 changes: 3 additions & 3 deletions packages/core/src/browser/menu/browser-menu-plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ export class MenuServices {
}

export interface MenuWidgetFactory {
createMenuWidget(effectiveMenuPath: MenuPath, menu: Submenu, contextMatcher: ContextMatcher, options: BrowserMenuOptions): MenuWidget;
createMenuWidget(effectiveMenuPath: MenuPath, menu: Submenu, contextMatcher: ContextMatcher, options: BrowserMenuOptions, args?: unknown[]): MenuWidget;
}

/**
Expand Down Expand Up @@ -321,11 +321,11 @@ export class DynamicMenuWidget extends MenuWidget {
const result: MenuWidget.IItemOptions[] = [];

for (const node of nodes) {
const nodePath = [...parentPath, node.id];
const nodePath = node.effectiveMenuPath || [...parentPath, node.id];
if (node.isVisible(nodePath, contextMatcher, context, ...(this.args || []))) {
if (CompoundMenuNode.is(node)) {
if (RenderedMenuNode.is(node)) {
const submenu = this.services.menuWidgetFactory.createMenuWidget(nodePath, node, this.contextMatcher, this.options);
const submenu = this.services.menuWidgetFactory.createMenuWidget(nodePath, node, this.contextMatcher, this.options, this.args);
if (submenu.items.length > 0) {
result.push({ type: 'submenu', submenu });
}
Expand Down
169 changes: 134 additions & 35 deletions packages/core/src/browser/shell/tab-bar-toolbar/tab-bar-toolbar-menu-adapters.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,47 +23,44 @@ import { ACTION_ITEM, codicon } from '../../widgets';
import { ContextMenuRenderer } from '../../context-menu-renderer';
import { TabBarToolbarItem } from './tab-toolbar-item';
import { ContextKeyService, ContextMatcher } from '../../context-key-service';
import { CommandMenu, CompoundMenuNode, MenuModelRegistry, MenuNode, MenuPath, RenderedMenuNode } from '../../../common/menu';
import { CommandMenu, CompoundMenuNode, ContextExpressionMatcher, Group, MenuModelRegistry, MenuNode, MenuPath, RenderedMenuNode, Submenu } from '../../../common/menu';

export const TOOLBAR_WRAPPER_ID_SUFFIX = '-as-tabbar-toolbar-item';

abstract class AbstractToolbarMenuWrapper {

constructor(
protected readonly effectiveMenuPath: MenuPath,
readonly effectiveMenuPath: MenuPath,
protected readonly commandRegistry: CommandRegistry,
protected readonly menuRegistry: MenuModelRegistry,
protected readonly contextKeyService: ContextKeyService,
protected readonly contextMenuRenderer: ContextMenuRenderer) {
}

protected abstract menuPath?: MenuPath;
protected abstract menuNode?: MenuNode;
protected abstract menuNode: MenuNode | undefined;
protected abstract id: string;
protected abstract icon: string | undefined;
protected abstract tooltip: string | undefined;
protected abstract text: string | undefined;
protected abstract executeCommand(e: React.MouseEvent<HTMLDivElement, MouseEvent>): void;
protected abstract executeCommand(widget: Widget, e: React.MouseEvent<HTMLDivElement, MouseEvent>): void;

isEnabled(): boolean {
isEnabled(widget: Widget): boolean {
if (CommandMenu.is(this.menuNode)) {
return this.menuNode.isEnabled(this.effectiveMenuPath);
return this.menuNode.isEnabled(this.effectiveMenuPath, widget);
}
return true;
}
isToggled(): boolean {
isToggled(widget: Widget): boolean {
if (CommandMenu.is(this.menuNode) && this.menuNode.isToggled) {
return !!this.menuNode.isToggled(this.effectiveMenuPath);
return !!this.menuNode.isToggled(this.effectiveMenuPath, widget);
}
return false;
}
render(widget: Widget): React.ReactNode {
return this.renderMenuItem(widget);
}

toMenuNode?(): MenuNode | undefined {
return this.menuNode;
}
abstract toMenuNode(): MenuNode | undefined;

/**
* Presents the menu to popup on the `event` that is the clicking of
Expand All @@ -78,7 +75,7 @@ abstract class AbstractToolbarMenuWrapper {
const anchor = toAnchor(event);

this.contextMenuRenderer.render({
menuPath: menuPath,
menuPath: this.effectiveMenuPath,
menu: this.menuNode as CompoundMenuNode,
args: [widget],
anchor,
Expand All @@ -102,51 +99,77 @@ abstract class AbstractToolbarMenuWrapper {
return <div key={this.id} className={TabBarToolbar.Styles.TAB_BAR_TOOLBAR_ITEM + ' enabled menu'}>
<div className={className}
title={this.tooltip || this.text}
onClick={e => this.executeCommand(e)}
onClick={e => this.executeCommand(widget, e)}
/>
<div className={ACTION_ITEM} onClick={event => this.showPopupMenu(widget, this.menuPath!, event, contextMatcher)} >
<div className={ACTION_ITEM} onClick={event => this.showPopupMenu(widget, this.effectiveMenuPath!, event, contextMatcher)} >
<div className={codicon('chevron-down') + ' chevron'} />
</div>
</div>;
} else {
return <div key={this.id} className={TabBarToolbar.Styles.TAB_BAR_TOOLBAR_ITEM + ' enabled menu'}>
<div className={className}
title={this.tooltip || this.text}
onClick={e => this.executeCommand(e)}
onClick={e => this.executeCommand(widget, e)}
/>
</div>;
}
}
}

export class ToolbarMenuNodeWrapper extends AbstractToolbarMenuWrapper implements TabBarToolbarItem {
export class SubmenuAsToolbarItemWrapper extends AbstractToolbarMenuWrapper implements TabBarToolbarItem {
constructor(
effectiveMenuPath: MenuPath,
commandRegistry: CommandRegistry,
menuRegistry: MenuModelRegistry,
contextKeyService: ContextKeyService,
contextMenuRenderer: ContextMenuRenderer,
protected readonly menuNode: MenuNode & RenderedMenuNode,
readonly group: string | undefined,
readonly menuPath?: MenuPath) {
protected readonly menuNode: Submenu,
readonly group: string | undefined) {
super(effectiveMenuPath, commandRegistry, menuRegistry, contextKeyService, contextMenuRenderer);
}
priority?: number | undefined;

executeCommand(e: React.MouseEvent<HTMLDivElement, MouseEvent>): void {
if (CommandMenu.is(this.menuNode)) {
this.menuNode.run(this.effectiveMenuPath);
}
executeCommand(widget: Widget, e: React.MouseEvent<HTMLDivElement, MouseEvent>): void {
}

isVisible(widget: Widget): boolean {
const menuNodeVisible = this.menuNode.isVisible(this.effectiveMenuPath, this.contextKeyService, widget.node);
if (CommandMenu.is(this.menuNode)) {
return menuNodeVisible;
} else if (CompoundMenuNode.is(this.menuNode)) {
return menuNodeVisible && !MenuModelRegistry.isEmpty(this.menuNode);
} else {
return menuNodeVisible;
}
const menuNodeVisible = this.menuNode.isVisible(this.effectiveMenuPath, this.contextKeyService, widget.node, widget);
return menuNodeVisible && !MenuModelRegistry.isEmpty(this.menuNode);
}

get id(): string { return this.menuNode.id + TOOLBAR_WRAPPER_ID_SUFFIX; }
get icon(): string | undefined { return this.menuNode.icon; }
get tooltip(): string | undefined { return this.menuNode.label; }
get text(): string | undefined {
return (this.group === NAVIGATION || this.group === undefined) ? undefined : this.menuNode.label;
}
get onDidChange(): Event<void> | undefined {
return this.menuNode.onDidChange;
}

override toMenuNode(): Group | undefined {
return new ToolbarItemAsSubmenuWrapper(this.menuNode!, this.effectiveMenuPath);
};
}

export class CommandMenuAsToolbarItemWrapper extends AbstractToolbarMenuWrapper implements TabBarToolbarItem {
constructor(
effectiveMenuPath: MenuPath,
commandRegistry: CommandRegistry,
menuRegistry: MenuModelRegistry,
contextKeyService: ContextKeyService,
contextMenuRenderer: ContextMenuRenderer,
protected readonly menuNode: CommandMenu,
readonly group: string | undefined) {
super(effectiveMenuPath, commandRegistry, menuRegistry, contextKeyService, contextMenuRenderer);
}

isVisible(widget: Widget): boolean {
return this.menuNode.isVisible(this.effectiveMenuPath, this.contextKeyService, widget.node, widget);
}

executeCommand(widget: Widget, e: React.MouseEvent<HTMLDivElement, MouseEvent>): void {
this.menuNode.run(this.effectiveMenuPath, widget);
}

get id(): string { return this.menuNode.id + TOOLBAR_WRAPPER_ID_SUFFIX; }
Expand All @@ -158,9 +181,13 @@ export class ToolbarMenuNodeWrapper extends AbstractToolbarMenuWrapper implement
get onDidChange(): Event<void> | undefined {
return this.menuNode.onDidChange;
}

override toMenuNode(): MenuNode | undefined {
return new ToolbarItemAsCommandMenuWrapper(this.menuNode, this.effectiveMenuPath);
}
}

export class ToolbarSubmenuWrapper extends AbstractToolbarMenuWrapper implements TabBarToolbarItem {
export class ToolbarActionWrapper extends AbstractToolbarMenuWrapper implements TabBarToolbarItem {
constructor(
effectiveMenuPath: MenuPath,
commandRegistry: CommandRegistry,
Expand All @@ -176,7 +203,7 @@ export class ToolbarSubmenuWrapper extends AbstractToolbarMenuWrapper implements
return this.toolbarItem.command ? this.commandRegistry.isEnabled(this.toolbarItem.command, widget) : !!this.toolbarItem.menuPath;
}

protected executeCommand(e: React.MouseEvent<HTMLElement>, widget?: Widget): void {
protected executeCommand(widget: Widget, e: React.MouseEvent<HTMLElement>): void {
e.preventDefault();
e.stopPropagation();

Expand Down Expand Up @@ -232,8 +259,80 @@ export class ToolbarSubmenuWrapper extends AbstractToolbarMenuWrapper implements
return this.toolbarItem.menuPath!;
}

get menuNode(): MenuNode | undefined {
get menuNode(): CompoundMenuNode | undefined {
return this.menuRegistry.getMenu(this.menuPath);
}

override toMenuNode(): MenuNode | undefined {
return new ToolbarItemAsSubmenuWrapper(this.menuNode!, this.effectiveMenuPath);
}
}

/**
* This class wraps a menu node, but replaces the effective menu path. Command parameters need to be mapped
* for commands contributed by extension and this mapping is keyed by the menu path
Comment on lines +272 to +273
Copy link
Contributor

@colin-grant-work colin-grant-work Aug 8, 2025

Choose a reason for hiding this comment

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

It probably would have been helpful if I'd made a similar comment back in #11290

*/
abstract class AbstractMenuNodeAsToolbarItemWrapper<T extends MenuNode> {
constructor(protected readonly menuNode: T, readonly effectiveMenuPath: MenuPath) { }

get label(): string | undefined {
if (RenderedMenuNode.is(this.menuNode)) {
return this.menuNode.label;
}
};
/**
* Icon classes for the menu node. If present, these will produce an icon to the left of the label in browser-style menus.
*/
get icon(): string | undefined {
if (RenderedMenuNode.is(this.menuNode)) {
return this.menuNode.label;
}
}
get id(): string {
return this.menuNode.id;
}
get sortString(): string {
return this.menuNode.sortString;
}

isVisible<K>(effectiveMenuPath: MenuPath, contextMatcher: ContextExpressionMatcher<K>, context: K | undefined, ...args: unknown[]): boolean {
return this.menuNode!.isVisible(this.effectiveMenuPath, contextMatcher, context, args);
}
}

/**
* Wrapper form submenu nodes
*/
class ToolbarItemAsSubmenuWrapper extends AbstractMenuNodeAsToolbarItemWrapper<CompoundMenuNode> implements Group {

get contextKeyOverlays(): Record<string, string> | undefined {
return this.menuNode.contextKeyOverlays;
}
isEmpty<T>(effectiveMenuPath: MenuPath, contextMatcher: ContextExpressionMatcher<T>, context: T | undefined, ...args: unknown[]): boolean {
return this.menuNode.isEmpty(this.effectiveMenuPath, contextMatcher, context, args);
}
get children(): MenuNode[] {
return this.menuNode.children;
}

}
/**
* Wrapper for command menus
*/
class ToolbarItemAsCommandMenuWrapper extends AbstractMenuNodeAsToolbarItemWrapper<CommandMenu> implements CommandMenu {

isEnabled(effectiveMenuPath: MenuPath, ...args: unknown[]): boolean {
return this.menuNode.isEnabled(this.effectiveMenuPath, ...args);
}
isToggled(effectiveMenuPath: MenuPath, ...args: unknown[]): boolean {
return this.menuNode.isToggled(this.effectiveMenuPath, ...args);
}
run(effectiveMenuPath: MenuPath, ...args: unknown[]): Promise<void> {
return this.menuNode.run(this.effectiveMenuPath, args);
}

override get label(): string {
return super.label!;
}

}
18 changes: 12 additions & 6 deletions packages/core/src/browser/shell/tab-bar-toolbar/tab-bar-toolbar-registry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { ContextKeyService } from '../../context-key-service';
import { FrontendApplicationContribution } from '../../frontend-application-contribution';
import { Widget } from '../../widgets';
import { ReactTabBarToolbarAction, RenderedToolbarAction } from './tab-bar-toolbar-types';
import { ToolbarMenuNodeWrapper, ToolbarSubmenuWrapper } from './tab-bar-toolbar-menu-adapters';
import { CommandMenuAsToolbarItemWrapper, SubmenuAsToolbarItemWrapper, ToolbarActionWrapper } from './tab-bar-toolbar-menu-adapters';
import { KeybindingRegistry } from '../../keybinding';
import { LabelParser } from '../../label-parser';
import { ContextMenuRenderer } from '../../context-menu-renderer';
Expand Down Expand Up @@ -90,7 +90,7 @@ export class TabBarToolbarRegistry implements FrontendApplicationContribution {
return this.doRegisterItem(new ReactToolbarItemImpl(this.commandRegistry, this.contextKeyService, item));
} else {
if (item.menuPath) {
return this.doRegisterItem(new ToolbarSubmenuWrapper(item.menuPath,
return this.doRegisterItem(new ToolbarActionWrapper(item.menuPath,
this.commandRegistry, this.menuRegistry, this.contextKeyService, this.contextMenuRenderer, item));
} else {
const wrapper = new RenderedToolbarItemImpl(this.commandRegistry, this.contextKeyService, this.keybindingRegistry, this.labelParser, item);
Expand Down Expand Up @@ -146,13 +146,19 @@ export class TabBarToolbarRegistry implements FrontendApplicationContribution {
for (const grandchild of child.children) {
if (grandchild.isVisible([...delegate.menuPath, child.id, grandchild.id],
this.contextKeyService, widget.node) && RenderedMenuNode.is(grandchild)) {
result.push(new ToolbarMenuNodeWrapper([...delegate.menuPath, child.id, grandchild.id], this.commandRegistry, this.menuRegistry,
this.contextKeyService, this.contextMenuRenderer, grandchild, child.id, delegate.menuPath));
if (CommandMenu.is(grandchild)) {
result.push(new CommandMenuAsToolbarItemWrapper([...delegate.menuPath, child.id, grandchild.id], this.commandRegistry,
this.menuRegistry, this.contextKeyService, this.contextMenuRenderer, grandchild, child.id));
} else if (CompoundMenuNode.is(grandchild)) {
result.push(new SubmenuAsToolbarItemWrapper([...delegate.menuPath, child.id, grandchild.id], this.commandRegistry, this.menuRegistry,
this.contextKeyService, this.contextMenuRenderer, grandchild, child.id));
}

}
}
} else if (CommandMenu.is(child)) {
result.push(new ToolbarMenuNodeWrapper([...delegate.menuPath, child.id], this.commandRegistry, this.menuRegistry,
this.contextKeyService, this.contextMenuRenderer, child, undefined, delegate.menuPath));
result.push(new CommandMenuAsToolbarItemWrapper([...delegate.menuPath, child.id], this.commandRegistry, this.menuRegistry,
this.contextKeyService, this.contextMenuRenderer, child, undefined));
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/browser/shell/tab-bar-toolbar/tab-bar-toolbar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { Anchor, ContextMenuAccess, ContextMenuRenderer } from '../../context-me
import { LabelParser } from '../../label-parser';
import { codicon, ReactWidget, Widget } from '../../widgets';
import { TabBarToolbarRegistry } from './tab-bar-toolbar-registry';
import { TabBarDelegator, TabBarToolbarAction } from './tab-bar-toolbar-types';
import { TAB_BAR_TOOLBAR_CONTEXT_MENU, TabBarDelegator, TabBarToolbarAction } from './tab-bar-toolbar-types';
import { KeybindingRegistry } from '../..//keybinding';
import { TabBarToolbarItem } from './tab-toolbar-item';
import { GroupImpl, MenuModelRegistry } from '../../../common/menu';
Expand Down Expand Up @@ -173,7 +173,7 @@ export class TabBarToolbar extends ReactWidget {
}
return this.contextMenuRenderer.render({
menu: MenuModelRegistry.removeSingleRootNodes(menu),
menuPath: ['contextMenu'],
menuPath: TAB_BAR_TOOLBAR_CONTEXT_MENU,
args: [this.current],
anchor,
context: this.current?.node || this.node,
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/browser/shell/tab-bar-toolbar/tab-toolbar-item.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ import { ActionMenuNode, GroupImpl, MenuNode } from '../../../common/menu';
export interface TabBarToolbarItem {
id: string;
isVisible(widget: Widget): boolean;
isEnabled(widget?: Widget): boolean;
isToggled(): boolean;
isEnabled(widget: Widget): boolean;
isToggled(widget: Widget): boolean;
render(widget?: Widget): React.ReactNode;
onDidChange?: Event<void>;
group?: string;
Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/browser/tree/tree-widget.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1442,9 +1442,10 @@ export class TreeWidget extends ReactWidget implements StatefulWidget {
if (contextMenuPath) {
const { x, y } = event.nativeEvent;
const args = this.toContextMenuArgs(node);
const target = event.currentTarget;
setTimeout(() => this.contextMenuRenderer.render({
menuPath: contextMenuPath,
context: event.currentTarget,
context: target,
anchor: { x, y },
args
}), 10);
Expand Down
Loading
Loading