enable dynamic menu on linux/windows (#59936)

* enable dynamic menu on linux/windows workaround to fix #33457 * Address feedback * other comment
microsoft · Oct 4, 2018 · e4172c2 · e4172c2
1 parent e428654
commit e4172c2
Show file tree

Hide file tree

Showing 4 changed files with 20 additions and 34 deletions.
diff --git a/src/vs/code/electron-main/app.ts b/src/vs/code/electron-main/app.ts
@@ -61,7 +61,6 @@ import { IMenubarService } from 'vs/platform/menubar/common/menubar';
 import { MenubarService } from 'vs/platform/menubar/electron-main/menubarService';
 import { MenubarChannel } from 'vs/platform/menubar/node/menubarIpc';
 import { ILabelService, RegisterFormatterEvent } from 'vs/platform/label/common/label';
-import { CodeMenu } from 'vs/code/electron-main/menus';
 import { hasArgs } from 'vs/platform/environment/node/argv';
 import { RunOnceScheduler } from 'vs/base/common/async';
 import { registerContextMenuListener } from 'vs/base/parts/contextmenu/electron-main/contextmenu';
@@ -594,23 +593,6 @@ export class CodeApplication {
 			}
 		}
 
-		// Install Menu
-		const instantiationService = accessor.get(IInstantiationService);
-		const configurationService = accessor.get(IConfigurationService);
-
-		// Initial value sets the value for macOS. false indicates using Dynamic Menus
-		// Eventually this will not be necessary as the dynamic menu will takeover
-		let createNativeMenu = false;
-		if (platform.isLinux) {
-			createNativeMenu = configurationService.getValue<string>('window.titleBarStyle') !== 'custom';
-		} else if (platform.isWindows) {
-			createNativeMenu = configurationService.getValue<string>('window.titleBarStyle') === 'native';
-		}
-
-		if (createNativeMenu) {
-			instantiationService.createInstance(CodeMenu);
-		}
-
 		// Jump List
 		this.historyMainService.updateWindowsJumpList();
 		this.historyMainService.onRecentlyOpenedChange(() => this.historyMainService.updateWindowsJumpList());

diff --git a/src/vs/platform/menubar/electron-main/menubar.ts b/src/vs/platform/menubar/electron-main/menubar.ts
@@ -36,6 +36,11 @@ export class Menubar {
 	private closedLastWindow: boolean;
 
 	private menuUpdater: RunOnceScheduler;
+	private menuGC: RunOnceScheduler;
+
+	// Array to keep menus around so that GC doesn't cause crash as explained in #55347
+	// TODO@sbatten Remove this when fixed upstream by Electron
+	private oldMenus: Menu[];
 
 	private menubarMenus: IMenubarData;
 
@@ -56,6 +61,8 @@ export class Menubar {
 	) {
 		this.menuUpdater = new RunOnceScheduler(() => this.doUpdateMenu(), 0);
 
+		this.menuGC = new RunOnceScheduler(() => { this.oldMenus = []; }, 10000);
+
 		this.menubarMenus = this.stateService.getItem<IMenubarData>(Menubar.lastKnownMenubarStorageKey) || Object.create(null);
 
 		this.keybindings = this.stateService.getItem<IMenubarData>(Menubar.lastKnownAdditionalKeybindings) || Object.create(null);
@@ -64,6 +71,8 @@ export class Menubar {
 
 		this.closedLastWindow = false;
 
+		this.oldMenus = [];
+
 		this.install();
 
 		this.registerListeners();
@@ -202,6 +211,12 @@ export class Menubar {
 	}
 
 	private install(): void {
+		// Store old menu in our array to avoid GC to collect the menu and crash. See #55347
+		// TODO@sbatten Remove this when fixed upstream by Electron
+		const oldMenu = Menu.getApplicationMenu();
+		if (oldMenu) {
+			this.oldMenus.push(oldMenu);
+		}
 
 		// If we don't have a menu yet, set it to null to avoid the electron menu.
 		// This should only happen on the first launch ever
@@ -293,15 +308,6 @@ export class Menubar {
 			menubar.append(macWindowMenuItem);
 		}
 
-		// Preferences
-		if (!isMacintosh) {
-			const preferencesMenu = new Menu();
-			const preferencesMenuItem = new MenuItem({ label: this.mnemonicLabel(nls.localize({ key: 'mPreferences', comment: ['&& denotes a mnemonic'] }, "&&Preferences")), submenu: preferencesMenu });
-
-			this.setMenuById(preferencesMenu, 'Preferences');
-			menubar.append(preferencesMenuItem);
-		}
-
 		// Help
 		const helpMenu = new Menu();
 		const helpMenuItem = new MenuItem({ label: this.mnemonicLabel(nls.localize({ key: 'mHelp', comment: ['&& denotes a mnemonic'] }, "&&Help")), submenu: helpMenu, role: 'help' });
@@ -314,6 +320,9 @@ export class Menubar {
 		} else {
 			Menu.setApplicationMenu(null);
 		}
+
+		// Dispose of older menus after some time
+		this.menuGC.schedule();
 	}
 
 	private setMacApplicationMenu(macApplicationMenu: Electron.Menu): void {

diff --git a/src/vs/platform/menubar/electron-main/menubarService.ts b/src/vs/platform/menubar/electron-main/menubarService.ts
@@ -8,7 +8,6 @@ import { Menubar } from 'vs/platform/menubar/electron-main/menubar';
 import { ILogService } from 'vs/platform/log/common/log';
 import { TPromise } from 'vs/base/common/winjs.base';
 import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
-import { isMacintosh } from 'vs/base/common/platform';
 
 export class MenubarService implements IMenubarService {
 	_serviceBrand: any;
@@ -20,10 +19,7 @@ export class MenubarService implements IMenubarService {
 		@ILogService private logService: ILogService
 	) {
 		// Install Menu
-		// TODO@sbatten Only do this for macOS as the dynamic menus are currently stable. #55347
-		if (isMacintosh) {
-			this._menubar = this.instantiationService.createInstance(Menubar);
-		}
+		this._menubar = this.instantiationService.createInstance(Menubar);
 	}
 
 	updateMenubar(windowId: number, menus: IMenubarData, additionalKeybindings?: Array<IMenubarKeybinding>): TPromise<void> {

diff --git a/src/vs/workbench/browser/parts/titlebar/menubarControl.ts b/src/vs/workbench/browser/parts/titlebar/menubarControl.ts
@@ -468,8 +468,7 @@ export class MenubarControl extends Disposable {
 	private doSetupMenubar(): void {
 		if (!isMacintosh && this.currentTitlebarStyleSetting === 'custom') {
 			this.setupCustomMenubar();
-		} else if (isMacintosh) {
-			// TODO@sbatten currently limiting dynamic native menus to macOS #55347
+		} else {
 			// Send menus to main process to be rendered by Electron
 			const menubarData = {};
 			if (this.getMenubarMenus(menubarData)) {