Merge from master

src/vs/platform/actions/browser/menuItemActionItem.ts

@@ -3,21 +3,19 @@
  *  Licensed under the Source EULA. See License.txt in the project root for license information.
  *--------------------------------------------------------------------------------------------*/
 
-'use strict';
-
-import { localize } from 'vs/nls';
-import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding';
-import { IMenu, MenuItemAction, IMenuActionOptions, ICommandAction, SubmenuItemAction } from 'vs/platform/actions/common/actions';
-import { IAction } from 'vs/base/common/actions';
-import { IDisposable, dispose } from 'vs/base/common/lifecycle';
-import { ActionItem, Separator } from 'vs/base/browser/ui/actionbar/actionbar';
+import { addClasses, createCSSRule, removeClasses } from 'vs/base/browser/dom';
 import { domEvent } from 'vs/base/browser/event';
+import { ActionItem, Separator } from 'vs/base/browser/ui/actionbar/actionbar';
+import { IAction } from 'vs/base/common/actions';
 import { Emitter } from 'vs/base/common/event';
-import { IContextMenuService } from 'vs/platform/contextview/browser/contextView';
 import { IdGenerator } from 'vs/base/common/idGenerator';
-import { createCSSRule } from 'vs/base/browser/dom';
+import { dispose, IDisposable, toDisposable } from 'vs/base/common/lifecycle';
+import { isLinux, isWindows } from 'vs/base/common/platform';
+import { localize } from 'vs/nls';
+import { ICommandAction, IMenu, IMenuActionOptions, MenuItemAction, SubmenuItemAction } from 'vs/platform/actions/common/actions';
+import { IContextMenuService } from 'vs/platform/contextview/browser/contextView';
+import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding';
 import { INotificationService } from 'vs/platform/notification/common/notification';
-import { isWindows, isLinux } from 'vs/base/common/platform';
 
 // The alternative key on all platforms is alt. On windows we also support shift as an alternative key #44136
 class AlternativeKeyEmitter extends Emitter<boolean> {
@@ -100,32 +98,9 @@ export function fillInActions(groups: [string, (MenuItemAction | SubmenuItemActi
 		}
 
 		if (isPrimaryGroup(group)) {
+			const to = Array.isArray<IAction>(target) ? target : target.primary;
 
-			const head = Array.isArray<IAction>(target) ? target : target.primary;
-
-			// split contributed actions at the point where order
-			// changes form lt zero to gte
-			let pivot = 0;
-			for (; pivot < actions.length; pivot++) {
-				if ((<MenuItemAction>actions[pivot]).order >= 0) {
-					break;
-				}
-			}
-			// prepend contributed actions with order lte zero
-			head.unshift(...actions.slice(0, pivot));
-
-			// find the first separator which marks the end of the
-			// navigation group - might be the whole array length
-			let sep = 0;
-			while (sep < head.length) {
-				if (head[sep] instanceof Separator) {
-					break;
-				}
-				sep++;
-			}
-			// append contributed actions with order gt zero
-			head.splice(sep, 0, ...actions.slice(pivot));
-
+			to.unshift(...actions);
 		} else {
 			const to = Array.isArray<IAction>(target) ? target : target.secondary;
 
@@ -154,14 +129,16 @@ export class MenuItemActionItem extends ActionItem {
 
 	private _wantsAltCommand: boolean;
 	private _itemClassDispose: IDisposable;
+	private readonly _altKey: AlternativeKeyEmitter;
 
 	constructor(
-		public _action: MenuItemAction,
+		readonly _action: MenuItemAction,
 		@IKeybindingService private readonly _keybindingService: IKeybindingService,
 		@INotificationService protected _notificationService: INotificationService,
-		@IContextMenuService private readonly _contextMenuService: IContextMenuService
+		@IContextMenuService _contextMenuService: IContextMenuService
 	) {
 		super(undefined, _action, { icon: !!(_action.class || _action.item.iconLocation), label: !_action.class && !_action.item.iconLocation });
+		this._altKey = AlternativeKeyEmitter.getInstance(_contextMenuService);
 	}
 
 	protected get _commandAction(): IAction {
@@ -172,13 +149,12 @@ export class MenuItemActionItem extends ActionItem {
 		event.preventDefault();
 		event.stopPropagation();
 
-		const altKey = AlternativeKeyEmitter.getInstance(this._contextMenuService);
-		if (altKey.isPressed) {
-			altKey.suppressAltKeyUp();
+		if (this._altKey.isPressed) {
+			this._altKey.suppressAltKeyUp();
 		}
 
 		this.actionRunner.run(this._commandAction)
-			.done(undefined, err => this._notificationService.error(err));
+			.then(undefined, err => this._notificationService.error(err));
 	}
 
 	render(container: HTMLElement): void {
@@ -187,43 +163,45 @@ export class MenuItemActionItem extends ActionItem {
 		this._updateItemClass(this._action.item);
 
 		let mouseOver = false;
-		const alternativeKeyEmitter = AlternativeKeyEmitter.getInstance(this._contextMenuService);
-		let alternativeKeyDown = alternativeKeyEmitter.isPressed;
+
+		let alternativeKeyDown = this._altKey.isPressed;
 
 		const updateAltState = () => {
 			const wantsAltCommand = mouseOver && alternativeKeyDown;
 			if (wantsAltCommand !== this._wantsAltCommand) {
 				this._wantsAltCommand = wantsAltCommand;
-				this._updateLabel();
-				this._updateTooltip();
-				this._updateClass();
+				this.updateLabel();
+				this.updateTooltip();
+				this.updateClass();
 			}
 		};
 
-		this._callOnDispose.push(alternativeKeyEmitter.event(value => {
-			alternativeKeyDown = value;
-			updateAltState();
-		}));
+		if (this._action.alt) {
+			this._register(this._altKey.event(value => {
+				alternativeKeyDown = value;
+				updateAltState();
+			}));
+		}
 
-		this._callOnDispose.push(domEvent(container, 'mouseleave')(_ => {
+		this._register(domEvent(container, 'mouseleave')(_ => {
 			mouseOver = false;
 			updateAltState();
 		}));
 
-		this._callOnDispose.push(domEvent(container, 'mouseenter')(e => {
+		this._register(domEvent(container, 'mouseenter')(e => {
 			mouseOver = true;
 			updateAltState();
 		}));
 	}
 
-	_updateLabel(): void {
+	updateLabel(): void {
 		if (this.options.label) {
-			this.$e.text(this._commandAction.label);
+			this.label.textContent = this._commandAction.label;
 		}
 	}
 
-	_updateTooltip(): void {
-		const element = this.$e.getHTMLElement();
+	updateTooltip(): void {
+		const element = this.label;
 		const keybinding = this._keybindingService.lookupKeybinding(this._commandAction.id);
 		const keybindingLabel = keybinding && keybinding.getLabel();
 
@@ -232,7 +210,7 @@ export class MenuItemActionItem extends ActionItem {
 			: this._commandAction.label;
 	}
 
-	_updateClass(): void {
+	updateClass(): void {
 		if (this.options.icon) {
 			if (this._commandAction !== this._action) {
 				this._updateItemClass(this._action.alt.item);
@@ -260,8 +238,8 @@ export class MenuItemActionItem extends ActionItem {
 				MenuItemActionItem.ICON_PATH_TO_CSS_RULES.set(iconPathMapKey, iconClass);
 			}
 
-			this.$e.getHTMLElement().classList.add('icon', iconClass);
-			this._itemClassDispose = { dispose: () => this.$e.getHTMLElement().classList.remove('icon', iconClass) };
+			addClasses(this.label, 'icon', iconClass);
+			this._itemClassDispose = toDisposable(() => removeClasses(this.label, 'icon', iconClass));
 		}
 	}
 
@@ -285,10 +263,11 @@ export class ContextAwareMenuItemActionItem extends MenuItemActionItem {
 		event.stopPropagation();
 
 		this.actionRunner.run(this._commandAction, this._context)
-			.done(undefined, err => this._notificationService.error(err));
+			.then(undefined, err => this._notificationService.error(err));
 	}
 }
 
+// {{SQL CARBON EDIT}} - This is here to use the 'ids' generator above
 // Always show label for action items, instead of whether they don't have
 // an icon/CSS class. Useful for some toolbar scenarios in particular with
 // contributed actions from other extensions
@@ -305,7 +284,7 @@ export class LabeledMenuItemActionItem extends MenuItemActionItem {
 		super(_action, _labeledkeybindingService, _notificationService, _labeledcontextMenuService);
 	}
 	_updateLabel(): void {
-		this.$e.text(this._commandAction.label);
+		this.element.innerText = this._commandAction.label;
 	}
 
 	// Overwrite item class to ensure that we can pass in a CSS class that other items use
@@ -328,12 +307,12 @@ export class LabeledMenuItemActionItem extends MenuItemActionItem {
 				MenuItemActionItem.ICON_PATH_TO_CSS_RULES.set(iconPathMapKey, iconClass);
 			}
 
-			this.$e.getHTMLElement().classList.add('icon', iconClass);
-			this.$e.getHTMLElement().classList.add(this._defaultCSSClassToAdd);
+			this.element.classList.add('icon', iconClass);
+			this.element.classList.add(this._defaultCSSClassToAdd);
 			this._labeledItemClassDispose = {
 				dispose: () => {
-					this.$e.getHTMLElement().classList.remove('icon', iconClass);
-					this.$e.getHTMLElement().classList.remove(this._defaultCSSClassToAdd);
+					this.element.classList.remove('icon', iconClass);
+					this.element.classList.remove(this._defaultCSSClassToAdd);
 				}
 			};
 		}
@@ -348,3 +327,4 @@ export class LabeledMenuItemActionItem extends MenuItemActionItem {
 		super.dispose();
 	}
 }
+// {{SQL CARBON EDIT}} - End

src/vs/platform/actions/common/actions.ts

@@ -2,18 +2,16 @@
  *  Copyright (c) Microsoft Corporation. All rights reserved.
  *  Licensed under the Source EULA. See License.txt in the project root for license information.
  *--------------------------------------------------------------------------------------------*/
-'use strict';
 
 import { Action } from 'vs/base/common/actions';
-import { TPromise } from 'vs/base/common/winjs.base';
 import { SyncDescriptor0, createSyncDescriptor } from 'vs/platform/instantiation/common/descriptors';
 import { IConstructorSignature2, createDecorator } from 'vs/platform/instantiation/common/instantiation';
 import { IKeybindings } from 'vs/platform/keybinding/common/keybindingsRegistry';
 import { ContextKeyExpr, IContextKeyService } from 'vs/platform/contextkey/common/contextkey';
 import { ICommandService } from 'vs/platform/commands/common/commands';
 import { IDisposable } from 'vs/base/common/lifecycle';
-import { Event } from 'vs/base/common/event';
-import URI, { UriComponents } from 'vs/base/common/uri';
+import { Event, Emitter } from 'vs/base/common/event';
+import { URI, UriComponents } from 'vs/base/common/uri';
 
 export interface ILocalizedString {
 	value: string;
@@ -29,6 +27,7 @@ export interface IBaseCommandAction {
 export interface ICommandAction extends IBaseCommandAction {
 	iconLocation?: { dark: URI; light?: URI; };
 	precondition?: ContextKeyExpr;
+	toggled?: ContextKeyExpr;
 }
 
 export interface ISerializableCommandAction extends IBaseCommandAction {
@@ -59,53 +58,47 @@ export function isISubmenuItem(item: IMenuItem | ISubmenuItem): item is ISubmenu
 	return (item as ISubmenuItem).submenu !== undefined;
 }
 
-export class MenuId {
-
-	private static ID = 1;
-
-	static readonly EditorTitle = new MenuId();
-	static readonly EditorTitleContext = new MenuId();
-	static readonly EditorContext = new MenuId();
-	static readonly EmptyEditorGroupContext = new MenuId();
-	static readonly ExplorerContext = new MenuId();
-	static readonly OpenEditorsContext = new MenuId();
-	static readonly ProblemsPanelContext = new MenuId();
-	static readonly DebugVariablesContext = new MenuId();
-	static readonly DebugWatchContext = new MenuId();
-	static readonly DebugCallStackContext = new MenuId();
-	static readonly DebugBreakpointsContext = new MenuId();
-	static readonly DebugConsoleContext = new MenuId();
-	static readonly SCMTitle = new MenuId();
-	static readonly SCMSourceControl = new MenuId();
-	static readonly SCMResourceGroupContext = new MenuId();
-	static readonly SCMResourceContext = new MenuId();
-	static readonly SCMChangeContext = new MenuId();
-	static readonly CommandPalette = new MenuId();
-	static readonly ViewTitle = new MenuId();
-	static readonly ViewItemContext = new MenuId();
-	static readonly TouchBarContext = new MenuId();
-	static readonly SearchContext = new MenuId();
-	static readonly MenubarFileMenu = new MenuId();
-	static readonly MenubarEditMenu = new MenuId();
-	static readonly MenubarRecentMenu = new MenuId();
-	static readonly MenubarSelectionMenu = new MenuId();
-	static readonly MenubarViewMenu = new MenuId();
-	static readonly MenubarAppearanceMenu = new MenuId();
-	static readonly MenubarLayoutMenu = new MenuId();
-	static readonly MenubarGoMenu = new MenuId();
-	static readonly MenubarSwitchEditorMenu = new MenuId();
-	static readonly MenubarSwitchGroupMenu = new MenuId();
-	static readonly MenubarDebugMenu = new MenuId();
-	static readonly MenubarNewBreakpointMenu = new MenuId();
-	static readonly MenubarTasksMenu = new MenuId();
-	static readonly MenubarPreferencesMenu = new MenuId();
-	static readonly MenubarHelpMenu = new MenuId();
+export const enum MenuId {
+	CommandPalette,
+	DebugBreakpointsContext,
+	DebugCallStackContext,
+	DebugConsoleContext,
+	DebugVariablesContext,
+	DebugWatchContext,
+	EditorContext,
+	EditorTitle,
+	EditorTitleContext,
+	EmptyEditorGroupContext,
+	ExplorerContext,
+	MenubarAppearanceMenu,
+	MenubarDebugMenu,
+	MenubarEditMenu,
+	MenubarFileMenu,
+	MenubarGoMenu,
+	MenubarHelpMenu,
+	MenubarLayoutMenu,
+	MenubarNewBreakpointMenu,
+	MenubarPreferencesMenu,
+	MenubarRecentMenu,
+	MenubarSelectionMenu,
+	MenubarSwitchEditorMenu,
+	MenubarSwitchGroupMenu,
+	MenubarTerminalMenu,
+	MenubarViewMenu,
+	OpenEditorsContext,
+	ProblemsPanelContext,
+	SCMChangeContext,
+	SCMResourceContext,
+	SCMResourceGroupContext,
+	SCMSourceControl,
+	SCMTitle,
+	SearchContext,
+	TouchBarContext,
+	ViewItemContext,
+	ViewTitle,
 	// {{SQL CARBON EDIT}}
-	static readonly ObjectExplorerItemContext = new MenuId();
-	static readonly NotebookToolbar = new MenuId();
-
-
-	readonly id: string = String(MenuId.ID++);
+	ObjectExplorerItemContext,
+	NotebookToolbar,
 }
 
 export interface IMenuActionOptions {
@@ -130,15 +123,23 @@ export interface IMenuService {
 export interface IMenuRegistry {
 	addCommand(userCommand: ICommandAction): boolean;
 	getCommand(id: string): ICommandAction;
+	getCommands(): ICommandsMap;
 	appendMenuItem(menu: MenuId, item: IMenuItem | ISubmenuItem): IDisposable;
 	getMenuItems(loc: MenuId): (IMenuItem | ISubmenuItem)[];
+	onDidChangeMenu: Event<MenuId>;
+}
+
+export interface ICommandsMap {
+	[id: string]: ICommandAction;
 }
 
 export const MenuRegistry: IMenuRegistry = new class implements IMenuRegistry {
 
-	private _commands: { [id: string]: ICommandAction } = Object.create(null);
+	private readonly _commands: { [id: string]: ICommandAction } = Object.create(null);
+	private readonly _menuItems: { [loc: string]: (IMenuItem | ISubmenuItem)[] } = Object.create(null);
+	private readonly _onDidChangeMenu = new Emitter<MenuId>();
 
-	private _menuItems: { [loc: string]: (IMenuItem | ISubmenuItem)[] } = Object.create(null);
+	readonly onDidChangeMenu: Event<MenuId> = this._onDidChangeMenu.event;
 
 	addCommand(command: ICommandAction): boolean {
 		const old = this._commands[command.id];
@@ -150,27 +151,37 @@ export const MenuRegistry: IMenuRegistry = new class implements IMenuRegistry {
 		return this._commands[id];
 	}
 
-	appendMenuItem({ id }: MenuId, item: IMenuItem | ISubmenuItem): IDisposable {
+	getCommands(): ICommandsMap {
+		const result: ICommandsMap = Object.create(null);
+		for (const key in this._commands) {
+			result[key] = this.getCommand(key);
+		}
+		return result;
+	}
+
+	appendMenuItem(id: MenuId, item: IMenuItem | ISubmenuItem): IDisposable {
 		let array = this._menuItems[id];
 		if (!array) {
 			this._menuItems[id] = array = [item];
 		} else {
 			array.push(item);
 		}
+		this._onDidChangeMenu.fire(id);
 		return {
-			dispose() {
+			dispose: () => {
 				const idx = array.indexOf(item);
 				if (idx >= 0) {
 					array.splice(idx, 1);
+					this._onDidChangeMenu.fire(id);
 				}
 			}
 		};
 	}
 
-	getMenuItems({ id }: MenuId): (IMenuItem | ISubmenuItem)[] {
+	getMenuItems(id: MenuId): (IMenuItem | ISubmenuItem)[] {
 		const result = this._menuItems[id] || [];
 
-		if (id === MenuId.CommandPalette.id) {
+		if (id === MenuId.CommandPalette) {
 			// CommandPalette is special because it shows
 			// all commands by default
 			this._appendImplicitItems(result);
@@ -207,13 +218,12 @@ export class ExecuteCommandAction extends Action {
 		super(id, label);
 	}
 
-	run(...args: any[]): TPromise<any> {
+	run(...args: any[]): Promise<any> {
 		return this._commandService.executeCommand(this.id, ...args);
 	}
 }
 
 export class SubmenuItemAction extends Action {
-	// private _options: IMenuActionOptions;
 
 	readonly item: ISubmenuItem;
 	constructor(item: ISubmenuItem) {
@@ -224,14 +234,14 @@ export class SubmenuItemAction extends Action {
 
 export class MenuItemAction extends ExecuteCommandAction {
 
-	private _options: IMenuActionOptions;
-
 	readonly item: ICommandAction;
-	readonly alt: MenuItemAction;
+	readonly alt: MenuItemAction | undefined;
+
+	private _options: IMenuActionOptions;
 
 	constructor(
 		item: ICommandAction,
-		alt: ICommandAction,
+		alt: ICommandAction | undefined,
 		options: IMenuActionOptions,
 		@IContextKeyService contextKeyService: IContextKeyService,
 		@ICommandService commandService: ICommandService
@@ -239,13 +249,15 @@ export class MenuItemAction extends ExecuteCommandAction {
 		typeof item.title === 'string' ? super(item.id, item.title, commandService) : super(item.id, item.title.value, commandService);
 		this._cssClass = undefined;
 		this._enabled = !item.precondition || contextKeyService.contextMatchesRules(item.precondition);
+		this._checked = Boolean(item.toggled && contextKeyService.contextMatchesRules(item.toggled));
+
 		this._options = options || {};
 
 		this.item = item;
 		this.alt = alt ? new MenuItemAction(alt, undefined, this._options, contextKeyService, commandService) : undefined;
 	}
 
-	run(...args: any[]): TPromise<any> {
+	run(...args: any[]): Promise<any> {
 		let runArgs: any[] = [];
 
 		if (this._options.arg) {
@@ -266,9 +278,9 @@ export class SyncActionDescriptor {
 
 	private _id: string;
 	private _label: string;
-	private _keybindings: IKeybindings;
-	private _keybindingContext: ContextKeyExpr;
-	private _keybindingWeight: number;
+	private _keybindings: IKeybindings | undefined;
+	private _keybindingContext: ContextKeyExpr | undefined;
+	private _keybindingWeight: number | undefined;
 
 	constructor(ctor: IConstructorSignature2<string, string, Action>,
 		id: string, label: string, keybindings?: IKeybindings, keybindingContext?: ContextKeyExpr, keybindingWeight?: number
@@ -293,15 +305,15 @@ export class SyncActionDescriptor {
 		return this._label;
 	}
 
-	public get keybindings(): IKeybindings {
+	public get keybindings(): IKeybindings | undefined {
 		return this._keybindings;
 	}
 
-	public get keybindingContext(): ContextKeyExpr {
+	public get keybindingContext(): ContextKeyExpr | undefined {
 		return this._keybindingContext;
 	}
 
-	public get keybindingWeight(): number {
+	public get keybindingWeight(): number | undefined {
 		return this._keybindingWeight;
 	}
 }

src/vs/platform/actions/common/menu.ts

@@ -3,11 +3,8 @@
  *  Licensed under the Source EULA. See License.txt in the project root for license information.
  *--------------------------------------------------------------------------------------------*/
 
-'use strict';
-
-import { Event, Emitter } from 'vs/base/common/event';
+import { Event, Emitter, filterEvent, debounceEvent } from 'vs/base/common/event';
 import { IDisposable, dispose } from 'vs/base/common/lifecycle';
-import { TPromise } from 'vs/base/common/winjs.base';
 import { ContextKeyExpr, IContextKeyService } from 'vs/platform/contextkey/common/contextkey';
 import { MenuId, MenuRegistry, MenuItemAction, IMenu, IMenuItem, IMenuActionOptions, ISubmenuItem, SubmenuItemAction, isIMenuItem } from 'vs/platform/actions/common/actions';
 import { ICommandService } from 'vs/platform/commands/common/commands';
@@ -16,49 +13,74 @@ type MenuItemGroup = [string, (IMenuItem | ISubmenuItem)[]];
 
 export class Menu implements IMenu {
 
-	private _menuGroups: MenuItemGroup[] = [];
-	private _disposables: IDisposable[] = [];
-	private _onDidChange = new Emitter<IMenu>();
+	private readonly _onDidChange = new Emitter<IMenu>();
+	private readonly _disposables: IDisposable[] = [];
+
+	private _menuGroups: MenuItemGroup[];
+	private _contextKeys: Set<string>;
 
 	constructor(
-		id: MenuId,
-		startupSignal: TPromise<boolean>,
+		private readonly _id: MenuId,
 		@ICommandService private readonly _commandService: ICommandService,
 		@IContextKeyService private readonly _contextKeyService: IContextKeyService
 	) {
-		startupSignal.then(_ => {
-			const menuItems = MenuRegistry.getMenuItems(id);
-			const keysFilter = new Set<string>();
+		this._build();
 
-			let group: MenuItemGroup;
-			menuItems.sort(Menu._compareMenuItems);
+		// rebuild this menu whenever the menu registry reports an
+		// event for this MenuId
+		debounceEvent(
+			filterEvent(MenuRegistry.onDidChangeMenu, menuId => menuId === this._id),
+			() => { },
+			50
+		)(this._build, this, this._disposables);
 
-			for (let item of menuItems) {
-				// group by groupId
-				const groupName = item.group;
-				if (!group || group[0] !== groupName) {
-					group = [groupName, []];
-					this._menuGroups.push(group);
-				}
-				group[1].push(item);
+		// when context keys change we need to check if the menu also
+		// has changed
+		debounceEvent(
+			this._contextKeyService.onDidChangeContext,
+			(last, event) => last || event.affectsSome(this._contextKeys),
+			50
+		)(e => e && this._onDidChange.fire(), this, this._disposables);
+	}
 
-				// keep keys for eventing
-				Menu._fillInKbExprKeys(item.when, keysFilter);
+	private _build(): void {
+
+		// reset
+		this._menuGroups = [];
+		this._contextKeys = new Set();
+
+		const menuItems = MenuRegistry.getMenuItems(this._id);
+
+		let group: MenuItemGroup | undefined;
+		menuItems.sort(Menu._compareMenuItems);
+
+		for (let item of menuItems) {
+			// group by groupId
+			const groupName = item.group || '';
+			if (!group || group[0] !== groupName) {
+				group = [groupName, []];
+				this._menuGroups.push(group);
+			}
+			group![1].push(item);
+
+			// keep keys for eventing
+			Menu._fillInKbExprKeys(item.when, this._contextKeys);
+
+			// keep precondition keys for event if applicable
+			if (isIMenuItem(item) && item.command.precondition) {
+				Menu._fillInKbExprKeys(item.command.precondition, this._contextKeys);
 			}
 
-			// subscribe to context changes
-			this._disposables.push(this._contextKeyService.onDidChangeContext(event => {
-				if (event.affectsSome(keysFilter)) {
-					this._onDidChange.fire();
-				}
-			}));
-
-			this._onDidChange.fire(this);
-		});
+			// keep toggled keys for event if applicable
+			if (isIMenuItem(item) && item.command.toggled) {
+				Menu._fillInKbExprKeys(item.command.toggled, this._contextKeys);
+			}
+		}
+		this._onDidChange.fire(this);
 	}
 
 	dispose() {
-		this._disposables = dispose(this._disposables);
+		dispose(this._disposables);
 		this._onDidChange.dispose();
 	}
 
@@ -72,9 +94,8 @@ export class Menu implements IMenu {
 			const [id, items] = group;
 			const activeActions: (MenuItemAction | SubmenuItemAction)[] = [];
 			for (const item of items) {
-				if (this._contextKeyService.contextMatchesRules(item.when)) {
+				if (this._contextKeyService.contextMatchesRules(item.when || null)) {
 					const action = isIMenuItem(item) ? new MenuItemAction(item.command, item.alt, options, this._contextKeyService, this._commandService) : new SubmenuItemAction(item);
-					action.order = item.order; //TODO@Ben order is menu item property, not an action property
 					activeActions.push(action);
 				}
 			}
@@ -85,7 +106,7 @@ export class Menu implements IMenu {
 		return result;
 	}
 
-	private static _fillInKbExprKeys(exp: ContextKeyExpr, set: Set<string>): void {
+	private static _fillInKbExprKeys(exp: ContextKeyExpr | undefined, set: Set<string>): void {
 		if (exp) {
 			for (let key of exp.keys()) {
 				set.add(key);

src/vs/platform/actions/common/menuService.ts

@@ -0,0 +1,24 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the Source EULA. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+import { IContextKeyService } from 'vs/platform/contextkey/common/contextkey';
+import { MenuId, IMenu, IMenuService } from 'vs/platform/actions/common/actions';
+import { Menu } from 'vs/platform/actions/common/menu';
+import { ICommandService } from 'vs/platform/commands/common/commands';
+
+export class MenuService implements IMenuService {
+
+	_serviceBrand: any;
+
+	constructor(
+		@ICommandService private readonly _commandService: ICommandService
+	) {
+		//
+	}
+
+	createMenu(id: MenuId, contextKeyService: IContextKeyService): IMenu {
+		return new Menu(id, this._commandService, contextKeyService);
+	}
+}

src/vs/platform/actions/test/common/menuService.test.ts

@@ -0,0 +1,203 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the Source EULA. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+import * as assert from 'assert';
+import { MenuRegistry, MenuId, isIMenuItem } from 'vs/platform/actions/common/actions';
+import { MenuService } from 'vs/platform/actions/common/menuService';
+import { IDisposable, dispose } from 'vs/base/common/lifecycle';
+import { NullCommandService } from 'vs/platform/commands/common/commands';
+import { MockContextKeyService } from 'vs/platform/keybinding/test/common/mockKeybindingService';
+
+// --- service instances
+
+const contextKeyService = new class extends MockContextKeyService {
+	contextMatchesRules() {
+		return true;
+	}
+};
+
+// --- tests
+
+suite('MenuService', function () {
+
+	let menuService: MenuService;
+	let disposables: IDisposable[];
+	let testMenuId: MenuId;
+
+	setup(function () {
+		menuService = new MenuService(NullCommandService);
+		testMenuId = Math.PI;
+		disposables = [];
+	});
+
+	teardown(function () {
+		dispose(disposables);
+	});
+
+	test('group sorting', function () {
+
+		disposables.push(MenuRegistry.appendMenuItem(testMenuId, {
+			command: { id: 'one', title: 'FOO' },
+			group: '0_hello'
+		}));
+
+		disposables.push(MenuRegistry.appendMenuItem(testMenuId, {
+			command: { id: 'two', title: 'FOO' },
+			group: 'hello'
+		}));
+
+		disposables.push(MenuRegistry.appendMenuItem(testMenuId, {
+			command: { id: 'three', title: 'FOO' },
+			group: 'Hello'
+		}));
+
+		disposables.push(MenuRegistry.appendMenuItem(testMenuId, {
+			command: { id: 'four', title: 'FOO' },
+			group: ''
+		}));
+
+		disposables.push(MenuRegistry.appendMenuItem(testMenuId, {
+			command: { id: 'five', title: 'FOO' },
+			group: 'navigation'
+		}));
+
+		const groups = menuService.createMenu(testMenuId, contextKeyService).getActions();
+
+		assert.equal(groups.length, 5);
+		const [one, two, three, four, five] = groups;
+
+		assert.equal(one[0], 'navigation');
+		assert.equal(two[0], '0_hello');
+		assert.equal(three[0], 'hello');
+		assert.equal(four[0], 'Hello');
+		assert.equal(five[0], '');
+	});
+
+	test('in group sorting, by title', function () {
+
+		disposables.push(MenuRegistry.appendMenuItem(testMenuId, {
+			command: { id: 'a', title: 'aaa' },
+			group: 'Hello'
+		}));
+
+		disposables.push(MenuRegistry.appendMenuItem(testMenuId, {
+			command: { id: 'b', title: 'fff' },
+			group: 'Hello'
+		}));
+
+		disposables.push(MenuRegistry.appendMenuItem(testMenuId, {
+			command: { id: 'c', title: 'zzz' },
+			group: 'Hello'
+		}));
+
+		const groups = menuService.createMenu(testMenuId, contextKeyService).getActions();
+
+		assert.equal(groups.length, 1);
+		const [, actions] = groups[0];
+
+		assert.equal(actions.length, 3);
+		const [one, two, three] = actions;
+		assert.equal(one.id, 'a');
+		assert.equal(two.id, 'b');
+		assert.equal(three.id, 'c');
+	});
+
+	test('in group sorting, by title and order', function () {
+
+		disposables.push(MenuRegistry.appendMenuItem(testMenuId, {
+			command: { id: 'a', title: 'aaa' },
+			group: 'Hello',
+			order: 10
+		}));
+
+		disposables.push(MenuRegistry.appendMenuItem(testMenuId, {
+			command: { id: 'b', title: 'fff' },
+			group: 'Hello'
+		}));
+
+		disposables.push(MenuRegistry.appendMenuItem(testMenuId, {
+			command: { id: 'c', title: 'zzz' },
+			group: 'Hello',
+			order: -1
+		}));
+
+		disposables.push(MenuRegistry.appendMenuItem(testMenuId, {
+			command: { id: 'd', title: 'yyy' },
+			group: 'Hello',
+			order: -1
+		}));
+
+		const groups = menuService.createMenu(testMenuId, contextKeyService).getActions();
+
+		assert.equal(groups.length, 1);
+		const [, actions] = groups[0];
+
+		assert.equal(actions.length, 4);
+		const [one, two, three, four] = actions;
+		assert.equal(one.id, 'd');
+		assert.equal(two.id, 'c');
+		assert.equal(three.id, 'b');
+		assert.equal(four.id, 'a');
+	});
+
+
+	test('in group sorting, special: navigation', function () {
+
+		disposables.push(MenuRegistry.appendMenuItem(testMenuId, {
+			command: { id: 'a', title: 'aaa' },
+			group: 'navigation',
+			order: 1.3
+		}));
+
+		disposables.push(MenuRegistry.appendMenuItem(testMenuId, {
+			command: { id: 'b', title: 'fff' },
+			group: 'navigation',
+			order: 1.2
+		}));
+
+		disposables.push(MenuRegistry.appendMenuItem(testMenuId, {
+			command: { id: 'c', title: 'zzz' },
+			group: 'navigation',
+			order: 1.1
+		}));
+
+		const groups = menuService.createMenu(testMenuId, contextKeyService).getActions();
+
+		assert.equal(groups.length, 1);
+		const [[, actions]] = groups;
+
+		assert.equal(actions.length, 3);
+		const [one, two, three] = actions;
+		assert.equal(one.id, 'c');
+		assert.equal(two.id, 'b');
+		assert.equal(three.id, 'a');
+	});
+
+	test('special MenuId palette', function () {
+
+		disposables.push(MenuRegistry.appendMenuItem(MenuId.CommandPalette, {
+			command: { id: 'a', title: 'Explicit' }
+		}));
+
+		MenuRegistry.addCommand({ id: 'b', title: 'Implicit' });
+
+		let foundA = false;
+		let foundB = false;
+		for (const item of MenuRegistry.getMenuItems(MenuId.CommandPalette)) {
+			if (isIMenuItem(item)) {
+				if (item.command.id === 'a') {
+					assert.equal(item.command.title, 'Explicit');
+					foundA = true;
+				}
+				if (item.command.id === 'b') {
+					assert.equal(item.command.title, 'Implicit');
+					foundB = true;
+				}
+			}
+		}
+		assert.equal(foundA, true);
+		assert.equal(foundB, true);
+	});
+});