Skip to content

Commit bb4e4c4

Browse files
sbattensbatten
authored
don't create and dispose submenus (microsoft#165206)
fixes microsoft#161413
1 parent c6559cc commit bb4e4c4

File tree

2 files changed

+41
-61
lines changed

2 files changed

+41
-61
lines changed

‎src/vs/workbench/browser/parts/titlebar/menubarControl.ts

Lines changed: 28 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ import { ICommandService } from 'vs/platform/commands/common/commands';
4141
import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry';
4242
import { OpenRecentAction } from 'vs/workbench/browser/actions/windowActions';
4343
import { isICommandActionToggleInfo } from 'vs/platform/action/common/action';
44+
import { createAndFillInContextMenuActions } from 'vs/platform/actions/browser/menuEntryActionViewItem';
4445

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

@@ -646,6 +647,12 @@ export class CustomMenubarControl extends MenubarControl {
646647
this._onVisibilityChange.fire(visible);
647648
}
648649

650+
private toActionsArray(menu: IMenu): IAction[] {
651+
const result: IAction[] = [];
652+
createAndFillInContextMenuActions(menu, { shouldForwardArgs: true }, result);
653+
return result;
654+
}
655+
649656
private reinstallDisposables = this._register(new DisposableStore());
650657
private setupCustomMenubar(firstTime: boolean): void {
651658
// If there is no container, we cannot setup the menubar
@@ -704,67 +711,48 @@ export class CustomMenubarControl extends MenubarControl {
704711
}
705712

706713
// Update the menu actions
707-
const updateActions = (menu: IMenu, target: IAction[], topLevelTitle: string) => {
714+
const updateActions = (menuActions: readonly IAction[], target: IAction[], topLevelTitle: string) => {
708715
target.splice(0);
709-
const groups = menu.getActions();
710716

711-
for (const group of groups) {
712-
const [, actions] = group;
713-
714-
for (const action of actions) {
715-
this.insertActionsBefore(action, target);
717+
for (const menuItem of menuActions) {
718+
this.insertActionsBefore(menuItem, target);
716719

720+
if (menuItem instanceof Separator) {
721+
target.push(menuItem);
722+
} else if (menuItem instanceof SubmenuItemAction || menuItem instanceof MenuItemAction) {
717723
// use mnemonicTitle whenever possible
718-
let title = typeof action.item.title === 'string'
719-
? action.item.title
720-
: action.item.title.mnemonicTitle ?? action.item.title.value;
721-
722-
if (action instanceof SubmenuItemAction) {
723-
let submenu = this.menus[action.item.submenu.id];
724-
if (!submenu) {
725-
submenu = this.mainMenuDisposables.add(this.menus[action.item.submenu.id] = this.menuService.createMenu(action.item.submenu, this.contextKeyService));
726-
this.mainMenuDisposables.add(submenu.onDidChange(() => {
727-
if (!this.focusInsideMenubar) {
728-
const actions: IAction[] = [];
729-
updateActions(menu, actions, topLevelTitle);
730-
if (this.menubar && this.topLevelTitles[topLevelTitle]) {
731-
this.menubar.updateMenu({ actions: actions, label: mnemonicMenuLabel(this.topLevelTitles[topLevelTitle]) });
732-
}
733-
}
734-
}, this));
735-
}
724+
let title = typeof menuItem.item.title === 'string'
725+
? menuItem.item.title
726+
: menuItem.item.title.mnemonicTitle ?? menuItem.item.title.value;
736727

728+
if (menuItem instanceof SubmenuItemAction) {
737729
const submenuActions: SubmenuAction[] = [];
738-
updateActions(submenu, submenuActions, topLevelTitle);
730+
updateActions(menuItem.actions, submenuActions, topLevelTitle);
739731

740732
if (submenuActions.length > 0) {
741-
target.push(new SubmenuAction(action.id, mnemonicMenuLabel(title), submenuActions));
733+
target.push(new SubmenuAction(menuItem.id, mnemonicMenuLabel(title), submenuActions));
742734
}
743735
} else {
744-
if (isICommandActionToggleInfo(action.item.toggled)) {
745-
title = action.item.toggled.mnemonicTitle ?? action.item.toggled.title ?? title;
736+
if (isICommandActionToggleInfo(menuItem.item.toggled)) {
737+
title = menuItem.item.toggled.mnemonicTitle ?? menuItem.item.toggled.title ?? title;
746738
}
747739

748-
const newAction = new Action(action.id, mnemonicMenuLabel(title), action.class, action.enabled, () => this.commandService.executeCommand(action.id));
749-
newAction.tooltip = action.tooltip;
750-
newAction.checked = action.checked;
740+
const newAction = new Action(menuItem.id, mnemonicMenuLabel(title), menuItem.class, menuItem.enabled, () => this.commandService.executeCommand(menuItem.id));
741+
newAction.tooltip = menuItem.tooltip;
742+
newAction.checked = menuItem.checked;
751743
target.push(newAction);
752744
}
753745
}
754746

755-
target.push(new Separator());
756747
}
757748

758749
// Append web navigation menu items to the file menu when not compact
759-
if (menu === this.menus.File && this.currentCompactMenuMode === undefined) {
750+
if (topLevelTitle === 'File' && this.currentCompactMenuMode === undefined) {
760751
const webActions = this.getWebNavigationActions();
761752
if (webActions.length) {
762753
target.push(...webActions);
763-
target.push(new Separator()); // to account for pop below
764754
}
765755
}
766-
767-
target.pop();
768756
};
769757

770758
for (const title of Object.keys(this.topLevelTitles)) {
@@ -773,7 +761,7 @@ export class CustomMenubarControl extends MenubarControl {
773761
this.reinstallDisposables.add(menu.onDidChange(() => {
774762
if (!this.focusInsideMenubar) {
775763
const actions: IAction[] = [];
776-
updateActions(menu, actions, title);
764+
updateActions(this.toActionsArray(menu), actions, title);
777765
this.menubar?.updateMenu({ actions: actions, label: mnemonicMenuLabel(this.topLevelTitles[title]) });
778766
}
779767
}));
@@ -783,7 +771,7 @@ export class CustomMenubarControl extends MenubarControl {
783771
this.reinstallDisposables.add(this.webNavigationMenu.onDidChange(() => {
784772
if (!this.focusInsideMenubar) {
785773
const actions: IAction[] = [];
786-
updateActions(menu, actions, title);
774+
updateActions(this.toActionsArray(menu), actions, title);
787775
this.menubar?.updateMenu({ actions: actions, label: mnemonicMenuLabel(this.topLevelTitles[title]) });
788776
}
789777
}));
@@ -792,7 +780,7 @@ export class CustomMenubarControl extends MenubarControl {
792780

793781
const actions: IAction[] = [];
794782
if (menu) {
795-
updateActions(menu, actions, title);
783+
updateActions(this.toActionsArray(menu), actions, title);
796784
}
797785

798786
if (this.menubar) {

‎src/vs/workbench/electron-sandbox/parts/titlebar/menubarControl.ts

Lines changed: 13 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@
33
* Licensed under the MIT License. See License.txt in the project root for license information.
44
*--------------------------------------------------------------------------------------------*/
55

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

3031
export class NativeMenubarControl extends MenubarControl {
3132

@@ -93,7 +94,9 @@ export class NativeMenubarControl extends MenubarControl {
9394
const menu = this.menus[topLevelMenuName];
9495
if (menu) {
9596
const menubarMenu: IMenubarMenu = { items: [] };
96-
this.populateMenuItems(menu, menubarMenu, menubarData.keybindings);
97+
const menuActions: IAction[] = [];
98+
createAndFillInContextMenuActions(menu, { shouldForwardArgs: true }, menuActions);
99+
this.populateMenuItems(menuActions, menubarMenu, menubarData.keybindings);
97100
if (menubarMenu.items.length === 0) {
98101
return false; // Menus are incomplete
99102
}
@@ -104,13 +107,11 @@ export class NativeMenubarControl extends MenubarControl {
104107
return true;
105108
}
106109

107-
private populateMenuItems(menu: IMenu, menuToPopulate: IMenubarMenu, keybindings: { [id: string]: IMenubarKeybinding | undefined }) {
108-
const groups = menu.getActions();
109-
110-
for (const group of groups) {
111-
const [, actions] = group;
112-
113-
actions.forEach(menuItem => {
110+
private populateMenuItems(menuActions: readonly IAction[], menuToPopulate: IMenubarMenu, keybindings: { [id: string]: IMenubarKeybinding | undefined }) {
111+
for (const menuItem of menuActions) {
112+
if (menuItem instanceof Separator) {
113+
menuToPopulate.items.push({ id: 'vscode.menubar.separator' });
114+
} else if (menuItem instanceof MenuItemAction || menuItem instanceof SubmenuItemAction) {
114115

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

123-
const menuToDispose = this.menuService.createMenu(menuItem.item.submenu, this.contextKeyService);
124-
this.populateMenuItems(menuToDispose, submenu, keybindings);
124+
this.populateMenuItems(menuItem.actions, submenu, keybindings);
125125

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

133133
menuToPopulate.items.push(menubarSubmenuItem);
134134
}
135-
136-
menuToDispose.dispose();
137135
} else {
138136
if (menuItem.id === OpenRecentAction.ID) {
139137
const actions = this.getOpenRecentActions().map(this.transformOpenRecentAction);
@@ -160,13 +158,7 @@ export class NativeMenubarControl extends MenubarControl {
160158
keybindings[menuItem.id] = this.getMenubarKeybinding(menuItem.id);
161159
menuToPopulate.items.push(menubarMenuItem);
162160
}
163-
});
164-
165-
menuToPopulate.items.push({ id: 'vscode.menubar.separator' });
166-
}
167-
168-
if (menuToPopulate.items.length > 0) {
169-
menuToPopulate.items.pop();
161+
}
170162
}
171163
}
172164

0 commit comments

Comments
 (0)