Skip to content
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

enable dynamic menu on linux/windows #59936

Merged
merged 3 commits into from
Oct 4, 2018
Merged

enable dynamic menu on linux/windows #59936

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0f3a481
enable dynamic menu on linux/windows
sbatten Oct 3, 2018
3c34bc2
Address feedback
sbatten Oct 4, 2018
d7b022a
other comment
sbatten Oct 4, 2018
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
18 changes: 0 additions & 18 deletions src/vs/code/electron-main/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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());
Expand Down
24 changes: 15 additions & 9 deletions src/vs/platform/menubar/electron-main/menubar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ export class Menubar {
private closedLastWindow: boolean;

private menuUpdater: RunOnceScheduler;
private menuGC: RunOnceScheduler;

private oldMenus: Menu[];
Copy link
Member

Choose a reason for hiding this comment

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

@sbatten this needs a comment at least why we are doing it with a reference to the electron issue and a TODO to remove this when Electron fixed it!


private menubarMenus: IMenubarData;

Expand All @@ -56,6 +59,8 @@ export class Menubar {
) {
this.menuUpdater = new RunOnceScheduler(() => this.doUpdateMenu(), 0);

this.menuGC = new RunOnceScheduler(() => { this.oldMenus = this.oldMenus.slice(this.oldMenus.length - 1); }, 10000);
Copy link
Member

Choose a reason for hiding this comment

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

@sbatten I am not sure it helps to always keep the last menu around, I would probably empty the entire array after 10 seconds. Reason is that the user might be lucky to open the menu very early, then we got 2-3 updates and so the last menu in this array will not be the one the user had opened anyways.


this.menubarMenus = this.stateService.getItem<IMenubarData>(Menubar.lastKnownMenubarStorageKey) || Object.create(null);

this.keybindings = this.stateService.getItem<IMenubarData>(Menubar.lastKnownAdditionalKeybindings) || Object.create(null);
Expand All @@ -64,6 +69,8 @@ export class Menubar {

this.closedLastWindow = false;

this.oldMenus = [];

this.install();

this.registerListeners();
Expand Down Expand Up @@ -203,6 +210,11 @@ export class Menubar {

private install(): void {

const oldMenu = Menu.getApplicationMenu();
Copy link
Member

Choose a reason for hiding this comment

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

@sbatten again, comment to explain why

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
if (Object.keys(this.menubarMenus).length === 0) {
Expand Down Expand Up @@ -293,15 +305,6 @@ export class Menubar {
menubar.append(macWindowMenuItem);
}

// Preferences
sbatten marked this conversation as resolved.
Show resolved Hide resolved
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' });
Expand All @@ -314,6 +317,9 @@ export class Menubar {
} else {
Menu.setApplicationMenu(null);
}

// Dispose of older menus after some time
this.menuGC.schedule();
}

private setMacApplicationMenu(macApplicationMenu: Electron.Menu): void {
Expand Down
6 changes: 1 addition & 5 deletions src/vs/platform/menubar/electron-main/menubarService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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> {
Expand Down
3 changes: 1 addition & 2 deletions src/vs/workbench/browser/parts/titlebar/menubarControl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down