From 4600d99b2b9e1be32a7dfd46f111c8a04357d3e9 Mon Sep 17 00:00:00 2001 From: Benjamin Christopher Simmonds <44439583+benibenj@users.noreply.github.com> Date: Sat, 13 Apr 2024 18:18:02 +0200 Subject: [PATCH] Improve Configure Keybindings Support (#210251) * Improve Configure Keybindings Support * fix test --- .../browser/inlineCompletionsHintsWidget.ts | 3 +- .../browser/inlineEditHintsWidget.ts | 4 +- src/vs/platform/actions/browser/toolbar.ts | 94 +++++++++++-------- src/vs/platform/actions/common/menuService.ts | 23 ++--- .../quickinput/browser/commandsQuickAccess.ts | 2 +- .../browser/parts/paneCompositePart.ts | 9 +- .../notebook/browser/diff/diffComponents.ts | 4 +- .../browser/commandsQuickAccess.ts | 25 ++--- .../scm/browser/scmRepositoryRenderer.ts | 2 +- .../contrib/scm/browser/scmViewPane.ts | 8 +- 10 files changed, 98 insertions(+), 76 deletions(-) diff --git a/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsHintsWidget.ts b/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsHintsWidget.ts index 0bc511b4f71..00c79719484 100644 --- 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 index 59553805863..ee0b537a88c 100644 --- 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 index 444fe76700d..4857d4bc07b 100644 --- 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; - } - primaryActions.push(action.hideActions.hide); + // -- Hide Actions -- + if (toggleActions.length > 0) { + let noHide = false; - } else { - primaryActions.push(toAction({ - id: 'label', - label: localize('hide', "Hide"), - enabled: false, - run() { } - })); + // 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() { } + })); + } } 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 index f35cd70ac2c..eb87cda4c5b 100644 --- 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 index f74b013ddc2..feb47bd2f62 100644 --- 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 index efb1d75565c..e7f574aca24 100644 --- 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 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 index de90f2796c5..59a4d27a8e2 100644 --- 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 index 59ec31d81d6..869a10910fe 100644 --- 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 index 7e544893911..e33a86c854a 100644 --- 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 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 index 21a67b91962..52befadc91e 100644 --- a/src/vs/workbench/contrib/scm/browser/scmViewPane.ts +++ b/src/vs/workbench/contrib/scm/browser/scmViewPane.ts @@ -803,6 +803,7 @@ class HistoryItemGroupRenderer implements ICompressibleTreeRenderer