Improve Configure Keybindings Support (microsoft#210251)

benibenj · web-flow · commit 4600d99b2b9e · 2024-04-13T09:18:02.000-07:00
* Improve Configure Keybindings Support

* fix test
diff --git a/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsHintsWidget.ts b/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsHintsWidget.ts
@@ -319,9 +319,10 @@ export class CustomizedMenuWorkbenchToolBar extends WorkbenchToolBar {
 		@IContextKeyService private readonly contextKeyService: IContextKeyService,
 		@IContextMenuService contextMenuService: IContextMenuService,
 		@IKeybindingService keybindingService: IKeybindingService,
+		@ICommandService commandService: ICommandService,
 		@ITelemetryService telemetryService: ITelemetryService,
 	) {
-		super(container, { resetMenu: menuId, ...options2 }, menuService, contextKeyService, contextMenuService, keybindingService, telemetryService);
+		super(container, { resetMenu: menuId, ...options2 }, menuService, contextKeyService, contextMenuService, keybindingService, commandService, telemetryService);
 
 		this._store.add(this.menu.onDidChange(() => this.updateToolbar()));
 		this.updateToolbar();
diff --git a/src/vs/editor/contrib/inlineEdit/browser/inlineEditHintsWidget.ts b/src/vs/editor/contrib/inlineEdit/browser/inlineEditHintsWidget.ts
@@ -19,6 +19,7 @@ import { InlineEditWidget } from 'vs/editor/contrib/inlineEdit/browser/inlineEdi
 import { MenuEntryActionViewItem, createAndFillInActionBarActions } from 'vs/platform/actions/browser/menuEntryActionViewItem';
 import { IMenuWorkbenchToolBarOptions, WorkbenchToolBar } from 'vs/platform/actions/browser/toolbar';
 import { IMenuService, MenuId, MenuItemAction } from 'vs/platform/actions/common/actions';
+import { ICommandService } from 'vs/platform/commands/common/commands';
 import { IContextKeyService } from 'vs/platform/contextkey/common/contextkey';
 import { IContextMenuService } from 'vs/platform/contextview/browser/contextView';
 import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
@@ -202,9 +203,10 @@ export class CustomizedMenuWorkbenchToolBar extends WorkbenchToolBar {
 		@IContextKeyService private readonly contextKeyService: IContextKeyService,
 		@IContextMenuService contextMenuService: IContextMenuService,
 		@IKeybindingService keybindingService: IKeybindingService,
+		@ICommandService commandService: ICommandService,
 		@ITelemetryService telemetryService: ITelemetryService,
 	) {
-		super(container, { resetMenu: menuId, ...options2 }, menuService, contextKeyService, contextMenuService, keybindingService, telemetryService);
+		super(container, { resetMenu: menuId, ...options2 }, menuService, contextKeyService, contextMenuService, keybindingService, commandService, telemetryService);
 
 		this._store.add(this.menu.onDidChange(() => this.updateToolbar()));
 		this._store.add(this.editor.onDidChangeCursorPosition(() => this.updateToolbar()));
diff --git a/src/vs/platform/actions/browser/toolbar.ts b/src/vs/platform/actions/browser/toolbar.ts
@@ -5,7 +5,7 @@
 
 import { addDisposableListener, getWindow } from 'vs/base/browser/dom';
 import { StandardMouseEvent } from 'vs/base/browser/mouseEvent';
-import { IToolBarOptions, ToolBar } from 'vs/base/browser/ui/toolbar/toolbar';
+import { IToolBarOptions, ToggleMenuAction, ToolBar } from 'vs/base/browser/ui/toolbar/toolbar';
 import { IAction, Separator, SubmenuAction, toAction, WorkbenchActionExecutedClassification, WorkbenchActionExecutedEvent } from 'vs/base/common/actions';
 import { coalesceInPlace } from 'vs/base/common/arrays';
 import { intersection } from 'vs/base/common/collections';
@@ -16,6 +16,8 @@ import { DisposableStore } from 'vs/base/common/lifecycle';
 import { localize } from 'vs/nls';
 import { createAndFillInActionBarActions } from 'vs/platform/actions/browser/menuEntryActionViewItem';
 import { IMenuActionOptions, IMenuService, MenuId, MenuItemAction, SubmenuItemAction } from 'vs/platform/actions/common/actions';
+import { createConfigureKeybindingAction } from 'vs/platform/actions/common/menuService';
+import { ICommandService } from 'vs/platform/commands/common/commands';
 import { IContextKeyService } from 'vs/platform/contextkey/common/contextkey';
 import { IContextMenuService } from 'vs/platform/contextview/browser/contextView';
 import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding';
@@ -90,12 +92,13 @@ export class WorkbenchToolBar extends ToolBar {
 		@IMenuService private readonly _menuService: IMenuService,
 		@IContextKeyService private readonly _contextKeyService: IContextKeyService,
 		@IContextMenuService private readonly _contextMenuService: IContextMenuService,
-		@IKeybindingService keybindingService: IKeybindingService,
+		@IKeybindingService private readonly _keybindingService: IKeybindingService,
+		@ICommandService private readonly _commandService: ICommandService,
 		@ITelemetryService telemetryService: ITelemetryService,
 	) {
 		super(container, _contextMenuService, {
 			// defaults
-			getKeyBinding: (action) => keybindingService.lookupKeybinding(action.id) ?? undefined,
+			getKeyBinding: (action) => _keybindingService.lookupKeybinding(action.id) ?? undefined,
 			// options (override defaults)
 			..._options,
 			// mandatory (overide options)
@@ -181,8 +184,8 @@ export class WorkbenchToolBar extends ToolBar {
 		coalesceInPlace(extraSecondary);
 		super.setActions(primary, Separator.join(extraSecondary, secondary));
 
-		// add context menu for toggle actions
-		if (toggleActions.length > 0) {
+		// add context menu for toggle and configure keybinding actions
+		if (toggleActions.length > 0 || primary.length > 0) {
 			this._sessionDisposables.add(addDisposableListener(this.getElement(), 'contextmenu', e => {
 				const event = new StandardMouseEvent(getWindow(this.getElement()), e);
 
@@ -193,47 +196,53 @@ export class WorkbenchToolBar extends ToolBar {
 				event.preventDefault();
 				event.stopPropagation();
 
-				let noHide = false;
-
-				// last item cannot be hidden when using ignore strategy
-				if (toggleActionsCheckedCount === 1 && this._options?.hiddenItemStrategy === HiddenItemStrategy.Ignore) {
-					noHide = true;
-					for (let i = 0; i < toggleActions.length; i++) {
-						if (toggleActions[i].checked) {
-							toggleActions[i] = toAction({
-								id: action.id,
-								label: action.label,
-								checked: true,
-								enabled: false,
-								run() { }
-							});
-							break; // there is only one
-						}
-					}
-				}
-
 				const primaryActions = [];
 
+				// -- Configure Keybinding Action --
 				if (action instanceof MenuItemAction && action.menuKeybinding) {
 					primaryActions.push(action.menuKeybinding);
+				} else if (!(action instanceof SubmenuItemAction || action instanceof ToggleMenuAction)) {
+					primaryActions.push(createConfigureKeybindingAction(action.id, undefined, this._commandService, this._keybindingService));
 				}
 
-				// add "hide foo" actions
-				if (!noHide && (action instanceof MenuItemAction || action instanceof SubmenuItemAction)) {
-					if (!action.hideActions) {
-						// no context menu for MenuItemAction instances that support no hiding
-						// those are fake actions and need to be cleaned up
-						return;
+				// -- Hide Actions --
+				if (toggleActions.length > 0) {
+					let noHide = false;
+
+					// last item cannot be hidden when using ignore strategy
+					if (toggleActionsCheckedCount === 1 && this._options?.hiddenItemStrategy === HiddenItemStrategy.Ignore) {
+						noHide = true;
+						for (let i = 0; i < toggleActions.length; i++) {
+							if (toggleActions[i].checked) {
+								toggleActions[i] = toAction({
+									id: action.id,
+									label: action.label,
+									checked: true,
+									enabled: false,
+									run() { }
+								});
+								break; // there is only one
+							}
+						}
+					}
+
+					// add "hide foo" actions
+					if (!noHide && (action instanceof MenuItemAction || action instanceof SubmenuItemAction)) {
+						if (!action.hideActions) {
+							// no context menu for MenuItemAction instances that support no hiding
+							// those are fake actions and need to be cleaned up
+							return;
+						}
+						primaryActions.push(action.hideActions.hide);
+
+					} else {
+						primaryActions.push(toAction({
+							id: 'label',
+							label: localize('hide', "Hide"),
+							enabled: false,
+							run() { }
+						}));
 					}
-					primaryActions.push(action.hideActions.hide);
-
-				} else {
-					primaryActions.push(toAction({
-						id: 'label',
-						label: localize('hide', "Hide"),
-						enabled: false,
-						run() { }
-					}));
 				}
 
 				const actions = Separator.join(primaryActions, toggleActions);
@@ -251,6 +260,10 @@ export class WorkbenchToolBar extends ToolBar {
 					}));
 				}
 
+				if (actions.length === 0) {
+					return;
+				}
+
 				this._contextMenuService.showContextMenu({
 					getAnchor: () => event,
 					getActions: () => actions,
@@ -318,9 +331,10 @@ export class MenuWorkbenchToolBar extends WorkbenchToolBar {
 		@IContextKeyService contextKeyService: IContextKeyService,
 		@IContextMenuService contextMenuService: IContextMenuService,
 		@IKeybindingService keybindingService: IKeybindingService,
+		@ICommandService commandService: ICommandService,
 		@ITelemetryService telemetryService: ITelemetryService,
 	) {
-		super(container, { resetMenu: menuId, ...options }, menuService, contextKeyService, contextMenuService, keybindingService, telemetryService);
+		super(container, { resetMenu: menuId, ...options }, menuService, contextKeyService, contextMenuService, keybindingService, commandService, telemetryService);
 
 		// update logic
 		const menu = this._store.add(menuService.createMenu(menuId, contextKeyService, { emitEventsForSubmenuChanges: true }));
diff --git a/src/vs/platform/actions/common/menuService.ts b/src/vs/platform/actions/common/menuService.ts
@@ -248,7 +248,7 @@ class MenuInfo {
 					const menuHide = createMenuHide(this._id, isMenuItem ? item.command : item, this._hiddenStates);
 					if (isMenuItem) {
 						// MenuItemAction
-						const menuKeybinding = createMenuKeybindingAction(this._id, item.command, item.when, this._commandService, this._keybindingService);
+						const menuKeybinding = createConfigureKeybindingAction(item.command.id, item.when, this._commandService, this._keybindingService);
 						activeActions.push(new MenuItemAction(item.command, item.alt, options, menuHide, menuKeybinding, this._contextKeyService, this._commandService));
 					} else {
 						// SubmenuItemAction
@@ -442,19 +442,16 @@ function createMenuHide(menu: MenuId, command: ICommandAction | ISubmenuItem, st
 	};
 }
 
-function createMenuKeybindingAction(menu: MenuId, command: ICommandAction | ISubmenuItem, when: ContextKeyExpression | undefined = undefined, commandService: ICommandService, keybindingService: IKeybindingService): IAction | undefined {
-	if (isISubmenuItem(command)) {
-		return undefined;
-	}
-
-	const configureKeybindingAction = toAction({
-		id: `configureKeybinding/${menu.id}/${command.id}`,
-		label: keybindingService.lookupKeybinding(command.id) ? localize('change keybinding', "Change Keybinding") : localize('configure keybinding', "Configure Keybinding"),
+export function createConfigureKeybindingAction(commandId: string, when: ContextKeyExpression | undefined = undefined, commandService: ICommandService, keybindingService: IKeybindingService): IAction {
+	const hasKeybinding = !!keybindingService.lookupKeybinding(commandId);
+	return toAction({
+		id: `configureKeybinding/${commandId}`,
+		label: hasKeybinding ? localize('change keybinding', "Change Keybinding") : localize('configure keybinding', "Configure Keybinding"),
 		run() {
-			const whenValue = when ? when.serialize() : undefined;
-			commandService.executeCommand('workbench.action.openGlobalKeybindings', `@command:${command.id}` + (whenValue ? ` +when:${whenValue}` : ''));
+			// Only set the when clause when there is no keybinding
+			// It is possible that the action and the keybinding have different when clauses
+			const whenValue = !hasKeybinding && when ? when.serialize() : undefined;
+			commandService.executeCommand('workbench.action.openGlobalKeybindings', `@command:${commandId}` + (whenValue ? ` +when:${whenValue}` : ''));
 		}
 	});
-
-	return configureKeybindingAction;
 }
diff --git a/src/vs/platform/quickinput/browser/commandsQuickAccess.ts b/src/vs/platform/quickinput/browser/commandsQuickAccess.ts
@@ -56,7 +56,7 @@ export abstract class AbstractCommandsQuickAccessProvider extends PickerQuickAcc
 	constructor(
 		options: ICommandsQuickAccessOptions,
 		@IInstantiationService private readonly instantiationService: IInstantiationService,
-		@IKeybindingService private readonly keybindingService: IKeybindingService,
+		@IKeybindingService protected readonly keybindingService: IKeybindingService,
 		@ICommandService private readonly commandService: ICommandService,
 		@ITelemetryService private readonly telemetryService: ITelemetryService,
 		@IDialogService private readonly dialogService: IDialogService
diff --git a/src/vs/workbench/browser/parts/paneCompositePart.ts b/src/vs/workbench/browser/parts/paneCompositePart.ts
@@ -29,7 +29,6 @@ import { localize } from 'vs/nls';
 import { CompositeDragAndDropObserver, toggleDropEffect } from 'vs/workbench/browser/dnd';
 import { EDITOR_DRAG_AND_DROP_BACKGROUND } from 'vs/workbench/common/theme';
 import { IPartOptions } from 'vs/workbench/browser/part';
-import { ToolBar } from 'vs/base/browser/ui/toolbar/toolbar';
 import { CompositeMenuActions } from 'vs/workbench/browser/actions';
 import { IMenuService, MenuId } from 'vs/platform/actions/common/actions';
 import { ActionsOrientation, prepareActions } from 'vs/base/browser/ui/actionbar/actionbar';
@@ -40,6 +39,7 @@ import { Composite } from 'vs/workbench/browser/composite';
 import { ViewsSubMenu } from 'vs/workbench/browser/parts/views/viewPaneContainer';
 import { createAndFillInActionBarActions } from 'vs/platform/actions/browser/menuEntryActionViewItem';
 import { IHoverService } from 'vs/platform/hover/browser/hover';
+import { HiddenItemStrategy, WorkbenchToolBar } from 'vs/platform/actions/browser/toolbar';
 
 export enum CompositeBarPosition {
 	TOP,
@@ -122,7 +122,7 @@ export abstract class AbstractPaneCompositePart extends CompositePart<PaneCompos
 	private compositeBarPosition: CompositeBarPosition | undefined = undefined;
 	private emptyPaneMessageElement: HTMLElement | undefined;
 
-	private globalToolBar: ToolBar | undefined;
+	private globalToolBar: WorkbenchToolBar | undefined;
 	private readonly globalActions: CompositeMenuActions;
 
 	private blockOpening = false;
@@ -315,13 +315,14 @@ export abstract class AbstractPaneCompositePart extends CompositePart<PaneCompos
 		const globalTitleActionsContainer = titleArea.appendChild($('.global-actions'));
 
 		// Global Actions Toolbar
-		this.globalToolBar = this._register(new ToolBar(globalTitleActionsContainer, this.contextMenuService, {
+		this.globalToolBar = this._register(this.instantiationService.createInstance(WorkbenchToolBar, globalTitleActionsContainer, {
 			actionViewItemProvider: (action, options) => this.actionViewItemProvider(action, options),
 			orientation: ActionsOrientation.HORIZONTAL,
 			getKeyBinding: action => this.keybindingService.lookupKeybinding(action.id),
 			anchorAlignmentProvider: () => this.getTitleAreaDropDownAnchorAlignment(),
 			toggleMenuTitle: localize('moreActions', "More Actions..."),
-			hoverDelegate: this.toolbarHoverDelegate
+			hoverDelegate: this.toolbarHoverDelegate,
+			hiddenItemStrategy: HiddenItemStrategy.NoHide
 		}));
 
 		this.updateGlobalToolbarActions();
diff --git a/src/vs/workbench/contrib/notebook/browser/diff/diffComponents.ts b/src/vs/workbench/contrib/notebook/browser/diff/diffComponents.ts
@@ -42,6 +42,7 @@ import { fixedDiffEditorOptions, fixedEditorOptions, fixedEditorPadding } from '
 import { AccessibilityVerbositySettingId } from 'vs/workbench/contrib/accessibility/browser/accessibilityConfiguration';
 import { IAccessibilityService } from 'vs/platform/accessibility/common/accessibility';
 import { DiffEditorWidget } from 'vs/editor/browser/widget/diffEditor/diffEditorWidget';
+import { ICommandService } from 'vs/platform/commands/common/commands';
 
 export function getOptimizedNestedCodeEditorWidgetOptions(): ICodeEditorWidgetOptions {
 	return {
@@ -82,6 +83,7 @@ class PropertyHeader extends Disposable {
 		},
 		@IContextMenuService private readonly contextMenuService: IContextMenuService,
 		@IKeybindingService private readonly keybindingService: IKeybindingService,
+		@ICommandService private readonly commandService: ICommandService,
 		@INotificationService private readonly notificationService: INotificationService,
 		@IMenuService private readonly menuService: IMenuService,
 		@IContextKeyService private readonly contextKeyService: IContextKeyService,
@@ -125,7 +127,7 @@ class PropertyHeader extends Disposable {
 
 				return undefined;
 			}
-		}, this.menuService, this.contextKeyService, this.contextMenuService, this.keybindingService, this.telemetryService);
+		}, this.menuService, this.contextKeyService, this.contextMenuService, this.keybindingService, this.commandService, this.telemetryService);
 		this._register(this._toolbar);
 		this._toolbar.context = {
 			cell: this.cell
diff --git a/src/vs/workbench/contrib/quickaccess/browser/commandsQuickAccess.ts b/src/vs/workbench/contrib/quickaccess/browser/commandsQuickAccess.ts
@@ -126,17 +126,20 @@ export class CommandsQuickAccessProvider extends AbstractEditorCommandsQuickAcce
 		return [
 			...this.getCodeEditorCommandPicks(),
 			...this.getGlobalCommandPicks()
-		].map(picks => ({
-			...picks,
-			buttons: [{
-				iconClass: ThemeIcon.asClassName(Codicon.gear),
-				tooltip: localize('configure keybinding', "Configure Keybinding"),
-			}],
-			trigger: (): TriggerAction => {
-				this.preferencesService.openGlobalKeybindingSettings(false, { query: createKeybindingCommandQuery(picks.commandId, picks.commandWhen) });
-				return TriggerAction.CLOSE_PICKER;
-			},
-		}));
+		].map(picks => {
+			const hasKeybinding = !!this.keybindingService.lookupKeybindings(picks.commandId);
+			return {
+				...picks,
+				buttons: [{
+					iconClass: ThemeIcon.asClassName(Codicon.gear),
+					tooltip: hasKeybinding ? localize('change keybinding', "Change Keybinding") : localize('configure keybinding', "Configure Keybinding"),
+				}],
+				trigger: (): TriggerAction => {
+					this.preferencesService.openGlobalKeybindingSettings(false, { query: createKeybindingCommandQuery(picks.commandId, picks.commandWhen) });
+					return TriggerAction.CLOSE_PICKER;
+				},
+			};
+		});
 	}
 
 	protected hasAdditionalCommandPicks(filter: string, token: CancellationToken): boolean {
diff --git a/src/vs/workbench/contrib/scm/browser/scmRepositoryRenderer.ts b/src/vs/workbench/contrib/scm/browser/scmRepositoryRenderer.ts
@@ -80,7 +80,7 @@ export class RepositoryRenderer implements ICompressibleTreeRenderer<ISCMReposit
 		const name = append(label, $('span.name'));
 		const description = append(label, $('span.description'));
 		const actions = append(provider, $('.actions'));
-		const toolBar = new WorkbenchToolBar(actions, { actionViewItemProvider: this.actionViewItemProvider, resetMenu: this.toolbarMenuId }, this.menuService, this.contextKeyService, this.contextMenuService, this.keybindingService, this.telemetryService);
+		const toolBar = new WorkbenchToolBar(actions, { actionViewItemProvider: this.actionViewItemProvider, resetMenu: this.toolbarMenuId }, this.menuService, this.contextKeyService, this.contextMenuService, this.keybindingService, this.commandService, this.telemetryService);
 		const countContainer = append(provider, $('.count'));
 		const count = new CountBadge(countContainer, {}, defaultCountBadgeStyles);
 		const visibilityDisposable = toolBar.onDidChangeDropdownVisibility(e => provider.classList.toggle('active', e));
diff --git a/src/vs/workbench/contrib/scm/browser/scmViewPane.ts b/src/vs/workbench/contrib/scm/browser/scmViewPane.ts
