From 220f9387c973de4cc44c545642ad2c90170bd3ef Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Tue, 6 Dec 2022 16:34:19 -0800 Subject: [PATCH] Fix showing keybindings in code action widget (#168254) This also refactors how keybindings are passed in to remove the concept of a `IActionKeybindingResolver` --- .../browser/codeActionKeybindingResolver.ts | 5 +-- ...deActionMenuItems.ts => codeActionMenu.ts} | 19 ++++++-- .../codeAction/browser/codeActionUi.ts | 12 +++-- .../editor/contrib/codeAction/common/types.ts | 3 +- .../actionWidget/browser/actionList.ts | 44 +++++++++---------- .../actionWidget/browser/actionWidget.css | 3 -- .../actionWidget/browser/actionWidget.ts | 6 +-- .../actionWidget/common/actionWidget.ts | 5 --- 8 files changed, 52 insertions(+), 45 deletions(-) rename src/vs/editor/contrib/codeAction/browser/{codeActionMenuItems.ts => codeActionMenu.ts} (84%) diff --git a/src/vs/editor/contrib/codeAction/browser/codeActionKeybindingResolver.ts b/src/vs/editor/contrib/codeAction/browser/codeActionKeybindingResolver.ts index 67240672131..2ee4108224f 100644 --- a/src/vs/editor/contrib/codeAction/browser/codeActionKeybindingResolver.ts +++ b/src/vs/editor/contrib/codeAction/browser/codeActionKeybindingResolver.ts @@ -8,7 +8,6 @@ import { Lazy } from 'vs/base/common/lazy'; import { CodeAction } from 'vs/editor/common/languages'; import { codeActionCommandId, fixAllCommandId, organizeImportsCommandId, refactorCommandId, sourceActionCommandId } from 'vs/editor/contrib/codeAction/browser/codeAction'; import { CodeActionAutoApply, CodeActionCommandArgs, CodeActionKind } from 'vs/editor/contrib/codeAction/common/types'; -import { IActionKeybindingResolver } from 'vs/platform/actionWidget/common/actionWidget'; import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding'; interface ResolveCodeActionKeybinding { @@ -17,7 +16,7 @@ interface ResolveCodeActionKeybinding { readonly resolvedKeybinding: ResolvedKeybinding; } -export class CodeActionKeybindingResolver implements IActionKeybindingResolver { +export class CodeActionKeybindingResolver { private static readonly codeActionCommands: readonly string[] = [ refactorCommandId, codeActionCommandId, @@ -27,7 +26,7 @@ export class CodeActionKeybindingResolver implements IActionKeybindingResolver { ]; constructor( - private readonly keybindingService: IKeybindingService + @IKeybindingService private readonly keybindingService: IKeybindingService ) { } public getResolver(): (action: CodeAction) => ResolvedKeybinding | undefined { diff --git a/src/vs/editor/contrib/codeAction/browser/codeActionMenuItems.ts b/src/vs/editor/contrib/codeAction/browser/codeActionMenu.ts similarity index 84% rename from src/vs/editor/contrib/codeAction/browser/codeActionMenuItems.ts rename to src/vs/editor/contrib/codeAction/browser/codeActionMenu.ts index 18f495dc5d8..87d5e99356b 100644 --- a/src/vs/editor/contrib/codeAction/browser/codeActionMenuItems.ts +++ b/src/vs/editor/contrib/codeAction/browser/codeActionMenu.ts @@ -5,12 +5,14 @@ import 'vs/base/browser/ui/codicons/codiconStyles'; // The codicon symbol styles are defined here and must be loaded import { Codicon } from 'vs/base/common/codicons'; +import { ResolvedKeybinding } from 'vs/base/common/keybindings'; +import { CodeAction } from 'vs/editor/common/languages'; import { CodeActionItem, CodeActionKind } from 'vs/editor/contrib/codeAction/common/types'; import 'vs/editor/contrib/symbolIcons/browser/symbolIcons'; // The codicon symbol colors are defined here and must be loaded to get colors import { localize } from 'vs/nls'; import { ActionListItemKind, IListMenuItem } from 'vs/platform/actionWidget/browser/actionList'; -export interface ActionGroup { +interface ActionGroup { readonly kind: CodeActionKind; readonly title: string; readonly icon?: { readonly codicon: Codicon; readonly color?: string }; @@ -29,7 +31,11 @@ const codeActionGroups = Object.freeze([ uncategorizedCodeActionGroup, ]); -export function toMenuItems(inputCodeActions: readonly CodeActionItem[], showHeaders: boolean): IListMenuItem[] { +export function toMenuItems( + inputCodeActions: readonly CodeActionItem[], + showHeaders: boolean, + keybindingResolver: (action: CodeAction) => ResolvedKeybinding | undefined +): IListMenuItem[] { if (!showHeaders) { return inputCodeActions.map((action): IListMenuItem => { return { @@ -60,7 +66,14 @@ export function toMenuItems(inputCodeActions: readonly CodeActionItem[], showHea if (menuEntry.actions.length) { allMenuItems.push({ kind: ActionListItemKind.Header, group: menuEntry.group }); for (const action of menuEntry.actions) { - allMenuItems.push({ kind: ActionListItemKind.Action, item: action, group: menuEntry.group, label: action.action.title, disabled: !!action.action.disabled }); + allMenuItems.push({ + kind: ActionListItemKind.Action, + item: action, + group: menuEntry.group, + label: action.action.title, + disabled: !!action.action.disabled, + keybinding: keybindingResolver(action.action), + }); } } } diff --git a/src/vs/editor/contrib/codeAction/browser/codeActionUi.ts b/src/vs/editor/contrib/codeAction/browser/codeActionUi.ts index 4d876f1368d..6d57e990367 100644 --- a/src/vs/editor/contrib/codeAction/browser/codeActionUi.ts +++ b/src/vs/editor/contrib/codeAction/browser/codeActionUi.ts @@ -13,7 +13,8 @@ import { ICodeEditor } from 'vs/editor/browser/editorBrowser'; import { IPosition, Position } from 'vs/editor/common/core/position'; import { ScrollType } from 'vs/editor/common/editorCommon'; import { CodeActionTriggerType } from 'vs/editor/common/languages'; -import { toMenuItems } from 'vs/editor/contrib/codeAction/browser/codeActionMenuItems'; +import { CodeActionKeybindingResolver } from 'vs/editor/contrib/codeAction/browser/codeActionKeybindingResolver'; +import { toMenuItems } from 'vs/editor/contrib/codeAction/browser/codeActionMenu'; import { MessageController } from 'vs/editor/contrib/message/browser/messageController'; import { localize } from 'vs/nls'; import { IActionWidgetService, IRenderDelegate } from 'vs/platform/actionWidget/browser/actionWidget'; @@ -30,9 +31,12 @@ export interface IActionShowOptions { } export class CodeActionUi extends Disposable { + private readonly _lightBulbWidget: Lazy; private readonly _activeCodeActions = this._register(new MutableDisposable()); + private readonly _resolver: CodeActionKeybindingResolver; + #disposed = false; private _showDisabled = false; @@ -44,8 +48,8 @@ export class CodeActionUi extends Disposable { private readonly delegate: { applyCodeAction: (action: CodeActionItem, regtriggerAfterApply: boolean, preview: boolean) => Promise; }, + @IInstantiationService instantiationService: IInstantiationService, @IConfigurationService private readonly _configurationService: IConfigurationService, - @IInstantiationService readonly instantiationService: IInstantiationService, @IActionWidgetService private readonly _actionWidgetService: IActionWidgetService, @ICommandService private readonly _commandService: ICommandService, ) { @@ -57,6 +61,8 @@ export class CodeActionUi extends Disposable { return widget; }); + this._resolver = instantiationService.createInstance(CodeActionKeybindingResolver); + this._register(this._editor.onDidLayoutChange(() => this._actionWidgetService.hide())); } @@ -188,7 +194,7 @@ export class CodeActionUi extends Disposable { this._actionWidgetService.show( 'codeActionWidget', true, - toMenuItems(actionsToShow, this._shouldShowHeaders()), + toMenuItems(actionsToShow, this._shouldShowHeaders(), this._resolver.getResolver()), delegate, anchor, editorDom, diff --git a/src/vs/editor/contrib/codeAction/common/types.ts b/src/vs/editor/contrib/codeAction/common/types.ts index 25ac6511a6d..07f55364a3b 100644 --- a/src/vs/editor/contrib/codeAction/common/types.ts +++ b/src/vs/editor/contrib/codeAction/common/types.ts @@ -193,8 +193,7 @@ export class CodeActionItem implements IActionItem { constructor( public readonly action: languages.CodeAction, public readonly provider: languages.CodeActionProvider | undefined, - ) { - } + ) { } async resolve(token: CancellationToken): Promise { if (this.provider?.resolveCodeAction && !this.action.edit) { diff --git a/src/vs/platform/actionWidget/browser/actionList.ts b/src/vs/platform/actionWidget/browser/actionList.ts index e54d0a6d793..c5034cd137d 100644 --- a/src/vs/platform/actionWidget/browser/actionList.ts +++ b/src/vs/platform/actionWidget/browser/actionList.ts @@ -8,11 +8,12 @@ import { KeybindingLabel } from 'vs/base/browser/ui/keybindingLabel/keybindingLa import { IListEvent, IListMouseEvent, IListRenderer, IListVirtualDelegate } from 'vs/base/browser/ui/list/list'; import { List } from 'vs/base/browser/ui/list/listWidget'; import { Codicon } from 'vs/base/common/codicons'; +import { ResolvedKeybinding } from 'vs/base/common/keybindings'; import { Disposable } from 'vs/base/common/lifecycle'; import { OS } from 'vs/base/common/platform'; import 'vs/css!./actionWidget'; import { localize } from 'vs/nls'; -import { IActionItem, IActionKeybindingResolver } from 'vs/platform/actionWidget/common/actionWidget'; +import { IActionItem } from 'vs/platform/actionWidget/common/actionWidget'; import { IContextViewService } from 'vs/platform/contextview/browser/contextView'; import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding'; @@ -25,12 +26,14 @@ export interface IRenderDelegate { } export interface IListMenuItem { - item?: T; - kind: ActionListItemKind; - group?: { kind?: any; icon?: { codicon: Codicon; color?: string }; title: string }; - disabled?: boolean; - label?: string; - description?: string; + readonly item?: T; + readonly kind: ActionListItemKind; + readonly group?: { kind?: any; icon?: { codicon: Codicon; color?: string }; title: string }; + readonly disabled?: boolean; + readonly label?: string; + readonly description?: string; + + readonly keybinding?: ResolvedKeybinding; } interface IActionMenuTemplateData { @@ -64,10 +67,7 @@ class HeaderRenderer> implements IListRende } renderElement(element: IListMenuItem, _index: number, templateData: IHeaderTemplateData): void { - if (!element.group) { - return; - } - templateData.text.textContent = element.group?.title; + templateData.text.textContent = element.group?.title ?? ''; } disposeTemplate(_templateData: IHeaderTemplateData): void { @@ -77,11 +77,10 @@ class HeaderRenderer> implements IListRende class ActionItemRenderer> implements IListRenderer { - get templateId(): string { return 'action'; } + get templateId(): string { return ActionListItemKind.Action; } constructor( private readonly _supportsPreview: boolean, - private readonly _keybindingResolver: IActionKeybindingResolver | undefined, @IKeybindingService private readonly _keybindingService: IKeybindingService ) { } @@ -109,18 +108,17 @@ class ActionItemRenderer> implements IListR data.icon.className = Codicon.lightBulb.classNames; data.icon.style.color = 'var(--vscode-editorLightBulb-foreground)'; } + if (!element.item || !element.label) { return; } - data.text.textContent = stripNewlines(element.label); - const binding = this._keybindingResolver?.getResolver()(element.item); - if (binding) { - data.keybinding.set(binding); - } - if (!binding) { + data.text.textContent = stripNewlines(element.label); + + if (!element.keybinding) { dom.hide(data.keybinding.element); } else { + data.keybinding.set(element.keybinding); dom.show(data.keybinding.element); } @@ -138,6 +136,7 @@ class ActionItemRenderer> implements IListR } else { data.container.title = ''; } + if (element.description) { const label = new HighlightedLabel(dom.append(data.container, dom.$('span.label-description'))); label.element.classList.add('action-list-description'); @@ -159,14 +158,13 @@ export class ActionList extends Disposable { private readonly _actionLineHeight = 24; private readonly _headerLineHeight = 26; - private readonly _allMenuItems: IListMenuItem[]; + private readonly _allMenuItems: readonly IListMenuItem[]; constructor( user: string, preview: boolean, - items: IListMenuItem[], + items: readonly IListMenuItem[], private readonly _delegate: IRenderDelegate, - resolver: IActionKeybindingResolver | undefined, @IContextViewService private readonly _contextViewService: IContextViewService, @IKeybindingService private readonly _keybindingService: IKeybindingService ) { @@ -178,7 +176,7 @@ export class ActionList extends Disposable { getHeight: element => element.kind === ActionListItemKind.Header ? this._headerLineHeight : this._actionLineHeight, getTemplateId: element => element.kind }; - this._list = this._register(new List(user, this.domNode, virtualDelegate, [new ActionItemRenderer>(preview, resolver, this._keybindingService), new HeaderRenderer()], { + this._list = this._register(new List(user, this.domNode, virtualDelegate, [new ActionItemRenderer>(preview, this._keybindingService), new HeaderRenderer()], { keyboardSupport: false, accessibilityProvider: { getAriaLabel: element => { diff --git a/src/vs/platform/actionWidget/browser/actionWidget.css b/src/vs/platform/actionWidget/browser/actionWidget.css index b67ed0cbe8c..18cfaa1bc36 100644 --- a/src/vs/platform/actionWidget/browser/actionWidget.css +++ b/src/vs/platform/actionWidget/browser/actionWidget.css @@ -87,9 +87,6 @@ cursor: default !important; -webkit-touch-callout: none; -webkit-user-select: none; - -khtml-user-select: none; - -moz-user-select: none; - -ms-user-select: none; user-select: none; background-color: transparent !important; outline: 0 solid !important; diff --git a/src/vs/platform/actionWidget/browser/actionWidget.ts b/src/vs/platform/actionWidget/browser/actionWidget.ts index 360058d107e..cd993279a52 100644 --- a/src/vs/platform/actionWidget/browser/actionWidget.ts +++ b/src/vs/platform/actionWidget/browser/actionWidget.ts @@ -12,7 +12,7 @@ import 'vs/css!./actionWidget'; import { localize } from 'vs/nls'; import { Action2, registerAction2 } from 'vs/platform/actions/common/actions'; import { acceptSelectedActionCommand, ActionList, IListMenuItem, previewSelectedActionCommand } from 'vs/platform/actionWidget/browser/actionList'; -import { IActionItem, IActionKeybindingResolver } from 'vs/platform/actionWidget/common/actionWidget'; +import { IActionItem } from 'vs/platform/actionWidget/common/actionWidget'; import { IContextKeyService, RawContextKey } from 'vs/platform/contextkey/common/contextkey'; import { IContextViewService } from 'vs/platform/contextview/browser/contextView'; import { InstantiationType, registerSingleton } from 'vs/platform/instantiation/common/extensions'; @@ -58,10 +58,10 @@ class ActionWidgetService extends Disposable implements IActionWidgetService { super(); } - async show(user: string, supportsPreview: boolean, items: IListMenuItem[], delegate: IRenderDelegate, anchor: IAnchor, container: HTMLElement | undefined, actionBarActions?: readonly IAction[], resolver?: IActionKeybindingResolver): Promise { + async show(user: string, supportsPreview: boolean, items: IListMenuItem[], delegate: IRenderDelegate, anchor: IAnchor, container: HTMLElement | undefined, actionBarActions?: readonly IAction[]): Promise { const visibleContext = ActionWidgetContextKeys.Visible.bindTo(this._contextKeyService); - const list = this._instantiationService.createInstance(ActionList, user, supportsPreview, items, delegate, resolver); + const list = this._instantiationService.createInstance(ActionList, user, supportsPreview, items, delegate); this.contextViewService.showContextView({ getAnchor: () => anchor, render: (container: HTMLElement) => { diff --git a/src/vs/platform/actionWidget/common/actionWidget.ts b/src/vs/platform/actionWidget/common/actionWidget.ts index e4e11f2e96b..e0b76b41e29 100644 --- a/src/vs/platform/actionWidget/common/actionWidget.ts +++ b/src/vs/platform/actionWidget/common/actionWidget.ts @@ -3,7 +3,6 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { ResolvedKeybinding } from 'vs/base/common/keybindings'; import { IDisposable } from 'vs/base/common/lifecycle'; export interface ActionSet extends IDisposable { @@ -16,7 +15,3 @@ export interface IActionItem { // TODO: Use generics action: any; } - -export interface IActionKeybindingResolver { - getResolver(): (action: any) => ResolvedKeybinding | undefined; -}