Skip to content

Commit

Permalink
don't create and dispose submenus (#165206)
Browse files Browse the repository at this point in the history 
fixes #161413
  • Loading branch information
@sbatten
sbatten committed Nov 2, 2022
1 parent c6559cc commit bb4e4c4
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 61 deletions.
68 changes: 28 additions & 40 deletions src/vs/workbench/browser/parts/titlebar/menubarControl.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import { ICommandService } from 'vs/platform/commands/common/commands';
import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry';
import { OpenRecentAction } from 'vs/workbench/browser/actions/windowActions';
import { isICommandActionToggleInfo } from 'vs/platform/action/common/action';
import { createAndFillInContextMenuActions } from 'vs/platform/actions/browser/menuEntryActionViewItem';

export type IOpenRecentAction = IAction & { uri: URI; remoteAuthority?: string };

Expand Down Expand Up @@ -646,6 +647,12 @@ export class CustomMenubarControl extends MenubarControl {
this._onVisibilityChange.fire(visible);
}

private toActionsArray(menu: IMenu): IAction[] {
const result: IAction[] = [];
createAndFillInContextMenuActions(menu, { shouldForwardArgs: true }, result);
return result;
}

private reinstallDisposables = this._register(new DisposableStore());
private setupCustomMenubar(firstTime: boolean): void {
// If there is no container, we cannot setup the menubar
Expand Down Expand Up @@ -704,67 +711,48 @@ export class CustomMenubarControl extends MenubarControl {
}

// Update the menu actions
const updateActions = (menu: IMenu, target: IAction[], topLevelTitle: string) => {
const updateActions = (menuActions: readonly IAction[], target: IAction[], topLevelTitle: string) => {
target.splice(0);
const groups = menu.getActions();

for (const group of groups) {
const [, actions] = group;

for (const action of actions) {
this.insertActionsBefore(action, target);
for (const menuItem of menuActions) {
this.insertActionsBefore(menuItem, target);

if (menuItem instanceof Separator) {
target.push(menuItem);
} else if (menuItem instanceof SubmenuItemAction || menuItem instanceof MenuItemAction) {
// use mnemonicTitle whenever possible
let title = typeof action.item.title === 'string'
? action.item.title
: action.item.title.mnemonicTitle ?? action.item.title.value;

if (action instanceof SubmenuItemAction) {
let submenu = this.menus[action.item.submenu.id];
if (!submenu) {
submenu = this.mainMenuDisposables.add(this.menus[action.item.submenu.id] = this.menuService.createMenu(action.item.submenu, this.contextKeyService));
this.mainMenuDisposables.add(submenu.onDidChange(() => {
if (!this.focusInsideMenubar) {
const actions: IAction[] = [];
updateActions(menu, actions, topLevelTitle);
if (this.menubar && this.topLevelTitles[topLevelTitle]) {
this.menubar.updateMenu({ actions: actions, label: mnemonicMenuLabel(this.topLevelTitles[topLevelTitle]) });
}
}
}, this));
}
let title = typeof menuItem.item.title === 'string'
? menuItem.item.title
: menuItem.item.title.mnemonicTitle ?? menuItem.item.title.value;

if (menuItem instanceof SubmenuItemAction) {
const submenuActions: SubmenuAction[] = [];
updateActions(submenu, submenuActions, topLevelTitle);
updateActions(menuItem.actions, submenuActions, topLevelTitle);

if (submenuActions.length > 0) {
target.push(new SubmenuAction(action.id, mnemonicMenuLabel(title), submenuActions));
target.push(new SubmenuAction(menuItem.id, mnemonicMenuLabel(title), submenuActions));
}
} else {
if (isICommandActionToggleInfo(action.item.toggled)) {
title = action.item.toggled.mnemonicTitle ?? action.item.toggled.title ?? title;
if (isICommandActionToggleInfo(menuItem.item.toggled)) {
title = menuItem.item.toggled.mnemonicTitle ?? menuItem.item.toggled.title ?? title;
}

const newAction = new Action(action.id, mnemonicMenuLabel(title), action.class, action.enabled, () => this.commandService.executeCommand(action.id));
newAction.tooltip = action.tooltip;
newAction.checked = action.checked;
const newAction = new Action(menuItem.id, mnemonicMenuLabel(title), menuItem.class, menuItem.enabled, () => this.commandService.executeCommand(menuItem.id));
newAction.tooltip = menuItem.tooltip;
newAction.checked = menuItem.checked;
target.push(newAction);
}
}

target.push(new Separator());
}

// Append web navigation menu items to the file menu when not compact
if (menu === this.menus.File && this.currentCompactMenuMode === undefined) {
if (topLevelTitle === 'File' && this.currentCompactMenuMode === undefined) {
const webActions = this.getWebNavigationActions();
if (webActions.length) {
target.push(...webActions);
target.push(new Separator()); // to account for pop below
}
}

target.pop();
};

for (const title of Object.keys(this.topLevelTitles)) {
Expand All @@ -773,7 +761,7 @@ export class CustomMenubarControl extends MenubarControl {
this.reinstallDisposables.add(menu.onDidChange(() => {
if (!this.focusInsideMenubar) {
const actions: IAction[] = [];
updateActions(menu, actions, title);
updateActions(this.toActionsArray(menu), actions, title);
this.menubar?.updateMenu({ actions: actions, label: mnemonicMenuLabel(this.topLevelTitles[title]) });
}
}));
Expand All @@ -783,7 +771,7 @@ export class CustomMenubarControl extends MenubarControl {
this.reinstallDisposables.add(this.webNavigationMenu.onDidChange(() => {
if (!this.focusInsideMenubar) {
const actions: IAction[] = [];
updateActions(menu, actions, title);
updateActions(this.toActionsArray(menu), actions, title);
this.menubar?.updateMenu({ actions: actions, label: mnemonicMenuLabel(this.topLevelTitles[title]) });
}
}));
Expand All @@ -792,7 +780,7 @@ export class CustomMenubarControl extends MenubarControl {

const actions: IAction[] = [];
if (menu) {
updateActions(menu, actions, title);
updateActions(this.toActionsArray(menu), actions, title);
}

if (this.menubar) {
Expand Down
34 changes: 13 additions & 21 deletions src/vs/workbench/electron-sandbox/parts/titlebar/menubarControl.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import { Separator } from 'vs/base/common/actions';
import { IMenuService, IMenu, SubmenuItemAction } from 'vs/platform/actions/common/actions';
import { IAction, Separator } from 'vs/base/common/actions';
import { IMenuService, SubmenuItemAction, MenuItemAction } from 'vs/platform/actions/common/actions';
import { IContextKeyService } from 'vs/platform/contextkey/common/contextkey';
import { IWorkspacesService } from 'vs/platform/workspaces/common/workspaces';
import { isMacintosh } from 'vs/base/common/platform';
Expand All @@ -26,6 +26,7 @@ import { IPreferencesService } from 'vs/workbench/services/preferences/common/pr
import { ICommandService } from 'vs/platform/commands/common/commands';
import { OpenRecentAction } from 'vs/workbench/browser/actions/windowActions';
import { isICommandActionToggleInfo } from 'vs/platform/action/common/action';
import { createAndFillInContextMenuActions } from 'vs/platform/actions/browser/menuEntryActionViewItem';

export class NativeMenubarControl extends MenubarControl {

Expand Down Expand Up @@ -93,7 +94,9 @@ export class NativeMenubarControl extends MenubarControl {
const menu = this.menus[topLevelMenuName];
if (menu) {
const menubarMenu: IMenubarMenu = { items: [] };
this.populateMenuItems(menu, menubarMenu, menubarData.keybindings);
const menuActions: IAction[] = [];
createAndFillInContextMenuActions(menu, { shouldForwardArgs: true }, menuActions);
this.populateMenuItems(menuActions, menubarMenu, menubarData.keybindings);
if (menubarMenu.items.length === 0) {
return false; // Menus are incomplete
}
Expand All @@ -104,13 +107,11 @@ export class NativeMenubarControl extends MenubarControl {
return true;
}

private populateMenuItems(menu: IMenu, menuToPopulate: IMenubarMenu, keybindings: { [id: string]: IMenubarKeybinding | undefined }) {
const groups = menu.getActions();

for (const group of groups) {
const [, actions] = group;

actions.forEach(menuItem => {
private populateMenuItems(menuActions: readonly IAction[], menuToPopulate: IMenubarMenu, keybindings: { [id: string]: IMenubarKeybinding | undefined }) {
for (const menuItem of menuActions) {
if (menuItem instanceof Separator) {
menuToPopulate.items.push({ id: 'vscode.menubar.separator' });
} else if (menuItem instanceof MenuItemAction || menuItem instanceof SubmenuItemAction) {

// use mnemonicTitle whenever possible
const title = typeof menuItem.item.title === 'string'
Expand All @@ -120,8 +121,7 @@ export class NativeMenubarControl extends MenubarControl {
if (menuItem instanceof SubmenuItemAction) {
const submenu = { items: [] };

const menuToDispose = this.menuService.createMenu(menuItem.item.submenu, this.contextKeyService);
this.populateMenuItems(menuToDispose, submenu, keybindings);
this.populateMenuItems(menuItem.actions, submenu, keybindings);

if (submenu.items.length > 0) {
const menubarSubmenuItem: IMenubarMenuItemSubmenu = {
Expand All @@ -132,8 +132,6 @@ export class NativeMenubarControl extends MenubarControl {

menuToPopulate.items.push(menubarSubmenuItem);
}

menuToDispose.dispose();
} else {
if (menuItem.id === OpenRecentAction.ID) {
const actions = this.getOpenRecentActions().map(this.transformOpenRecentAction);
Expand All @@ -160,13 +158,7 @@ export class NativeMenubarControl extends MenubarControl {
keybindings[menuItem.id] = this.getMenubarKeybinding(menuItem.id);
menuToPopulate.items.push(menubarMenuItem);
}
});

menuToPopulate.items.push({ id: 'vscode.menubar.separator' });
}

if (menuToPopulate.items.length > 0) {
menuToPopulate.items.pop();
}
}
}

Expand Down

0 comments on commit bb4e4c4

Please sign in to comment.