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

Jump to bottom

Refactor: softDispatch doesn't return null #181891

Merged
merged 4 commits into from
May 16, 2023
Merged

Refactor: softDispatch doesn't return null #181891

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6160404
fix `softDispatch` returning `NoMatchingKb` vs null
ulugbekna May 9, 2023
a4a3e2c
refactor: `softDispatch` doesn't return null
ulugbekna May 9, 2023
c9fa463
Adopt softDispatch changes
alexdima May 12, 2023
dee667b
Merge remote-tracking branch 'origin/main' into ulugbekna/particular-…
alexdima May 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/vs/editor/contrib/hover/browser/hover.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ export class ModesHoverController implements IEditorContribution {

const resolvedKeyboardEvent = this._keybindingService.softDispatch(e, this._editor.getDomNode());
// If the beginning of a multi-chord keybinding is pressed, or the command aims to focus the hover, set the variable to true, otherwise false
const mightTriggerFocus = (resolvedKeyboardEvent?.kind === ResultKind.MoreChordsNeeded || (resolvedKeyboardEvent && resolvedKeyboardEvent.kind === ResultKind.KbFound && resolvedKeyboardEvent.commandId === 'editor.action.showHover' && this._contentWidget?.isVisible()));
const mightTriggerFocus = (resolvedKeyboardEvent?.kind === ResultKind.MoreChordsNeeded || (resolvedKeyboardEvent.kind === ResultKind.KbFound && resolvedKeyboardEvent.commandId === 'editor.action.showHover' && this._contentWidget?.isVisible()));

if (e.keyCode !== KeyCode.Ctrl && e.keyCode !== KeyCode.Alt && e.keyCode !== KeyCode.Meta && e.keyCode !== KeyCode.Shift
&& !mightTriggerFocus) {
Expand Down
10 changes: 5 additions & 5 deletions src/vs/platform/keybinding/common/abstractKeybindingService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import * as nls from 'vs/nls';
import { ICommandService } from 'vs/platform/commands/common/commands';
import { IContextKeyService, IContextKeyServiceTarget } from 'vs/platform/contextkey/common/contextkey';
import { IKeybindingService, IKeyboardEvent, KeybindingsSchemaContribution } from 'vs/platform/keybinding/common/keybinding';
import { ResolutionResult, KeybindingResolver, ResultKind } from 'vs/platform/keybinding/common/keybindingResolver';
import { ResolutionResult, KeybindingResolver, ResultKind, NoMatchingKb } from 'vs/platform/keybinding/common/keybindingResolver';
import { ResolvedKeybindingItem } from 'vs/platform/keybinding/common/resolvedKeybindingItem';
import { ILogService } from 'vs/platform/log/common/log';
import { INotificationService } from 'vs/platform/notification/common/notification';
Expand Down Expand Up @@ -138,18 +138,18 @@ export abstract class AbstractKeybindingService extends Disposable implements IK

// TODO@ulugbekna: update namings to align with `_doDispatch`
// TODO@ulugbekna: this fn doesn't seem to take into account single-modifier keybindings, eg `shift shift`
public softDispatch(e: IKeyboardEvent, target: IContextKeyServiceTarget): ResolutionResult | null {
public softDispatch(e: IKeyboardEvent, target: IContextKeyServiceTarget): ResolutionResult {
this._log(`/ Soft dispatching keyboard event`);
const keybinding = this.resolveKeyboardEvent(e);
if (keybinding.hasMultipleChords()) {
console.warn('Unexpected keyboard event mapped to multiple chords');
return null;
console.warn('keyboard event should not be mapped to multiple chords');
return NoMatchingKb;
}
const [firstChord,] = keybinding.getDispatchChords();
if (firstChord === null) {
// cannot be dispatched, probably only modifier keys
this._log(`\\ Keyboard event cannot be dispatched`);
return null;
return NoMatchingKb;
}

const contextValue = this._contextKeyService.getContext(target);
Expand Down
2 changes: 1 addition & 1 deletion src/vs/platform/keybinding/common/keybinding.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ export interface IKeybindingService {
/**
* Resolve and dispatch `keyboardEvent`, but do not invoke the command or change inner state.
*/
softDispatch(keyboardEvent: IKeyboardEvent, target: IContextKeyServiceTarget): ResolutionResult | null;
softDispatch(keyboardEvent: IKeyboardEvent, target: IContextKeyServiceTarget): ResolutionResult;

dispatchByUserSettingsLabel(userSettingsLabel: string, target: IContextKeyServiceTarget): void;

Expand Down
2 changes: 1 addition & 1 deletion src/vs/platform/keybinding/common/keybindingResolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export type ResolutionResult =

// util definitions to make working with the above types easier within this module:

const NoMatchingKb: ResolutionResult = { kind: ResultKind.NoMatchingKb };
export const NoMatchingKb: ResolutionResult = { kind: ResultKind.NoMatchingKb };
const MoreChordsNeeded: ResolutionResult = { kind: ResultKind.MoreChordsNeeded };
function KbFound(commandId: string | null, commandArgs: any, isBubble: boolean): ResolutionResult {
return { kind: ResultKind.KbFound, commandId, commandArgs, isBubble };
Expand Down
6 changes: 3 additions & 3 deletions src/vs/platform/keybinding/test/common/mockKeybindingService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { ResolvedKeybinding, KeyCodeChord, Keybinding } from 'vs/base/common/key
import { OS } from 'vs/base/common/platform';
import { ContextKeyExpression, ContextKeyValue, IContextKey, IContextKeyChangeEvent, IContextKeyService, IContextKeyServiceTarget, IScopedContextKeyService } from 'vs/platform/contextkey/common/contextkey';
import { IKeybindingService, IKeyboardEvent } from 'vs/platform/keybinding/common/keybinding';
import { ResolutionResult } from 'vs/platform/keybinding/common/keybindingResolver';
import { NoMatchingKb, ResolutionResult } from 'vs/platform/keybinding/common/keybindingResolver';
import { ResolvedKeybindingItem } from 'vs/platform/keybinding/common/resolvedKeybindingItem';
import { USLayoutResolvedKeybinding } from 'vs/platform/keybinding/common/usLayoutResolvedKeybinding';

Expand Down Expand Up @@ -135,8 +135,8 @@ export class MockKeybindingService implements IKeybindingService {
return 0;
}

public softDispatch(keybinding: IKeyboardEvent, target: IContextKeyServiceTarget): ResolutionResult | null {
return null;
public softDispatch(keybinding: IKeyboardEvent, target: IContextKeyServiceTarget): ResolutionResult {
return NoMatchingKb;
}

public dispatchByUserSettingsLabel(userSettingsLabel: string, target: IContextKeyServiceTarget): void {
Expand Down
2 changes: 1 addition & 1 deletion src/vs/platform/list/browser/listService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -810,7 +810,7 @@ function createKeyboardNavigationEventFilter(keybindingService: IKeybindingServi

const result = keybindingService.softDispatch(event, event.target);

if (result?.kind === ResultKind.MoreChordsNeeded) {
if (result.kind === ResultKind.MoreChordsNeeded) {
inMultiChord = true;
return false;
}
Expand Down
6 changes: 3 additions & 3 deletions src/vs/workbench/browser/actions/developerActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ class ToggleScreencastModeAction extends Action2 {
const shortcut = keybindingService.softDispatch(event, event.target);

// Hide the single arrow key pressed
if (shortcut && shortcut.kind === ResultKind.KbFound && shortcut.commandId && configurationService.getValue('screencastMode.hideSingleEditorCursorMoves') && (
if (shortcut.kind === ResultKind.KbFound && shortcut.commandId && configurationService.getValue('screencastMode.hideSingleEditorCursorMoves') && (
['cursorLeft', 'cursorRight', 'cursorUp', 'cursorDown'].includes(shortcut.commandId))
) {
return;
Expand Down Expand Up @@ -324,8 +324,8 @@ class ToggleScreencastModeAction extends Action2 {
ToggleScreencastModeAction.disposable = disposables;
}

private _isKbFound(resolutionResult: ResolutionResult | null): resolutionResult is { kind: ResultKind.KbFound; commandId: string | null; commandArgs: any; isBubble: boolean } {
return resolutionResult !== null && resolutionResult.kind === ResultKind.KbFound;
private _isKbFound(resolutionResult: ResolutionResult): resolutionResult is { kind: ResultKind.KbFound; commandId: string | null; commandArgs: any; isBubble: boolean } {
return resolutionResult.kind === ResultKind.KbFound;
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/vs/workbench/browser/parts/dialogs/dialogHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ export class BrowserDialogHandler extends AbstractDialogHandler {
type: this.getDialogType(type),
keyEventProcessor: (event: StandardKeyboardEvent) => {
const resolved = this.keybindingService.softDispatch(event, this.layoutService.container);
if (resolved && resolved.kind === ResultKind.KbFound && resolved.commandId) {
if (resolved.kind === ResultKind.KbFound && resolved.commandId) {
if (BrowserDialogHandler.ALLOWABLE_COMMANDS.indexOf(resolved.commandId) === -1) {
EventHelper.stop(event, true);
}
Expand Down
4 changes: 2 additions & 2 deletions src/vs/workbench/contrib/terminal/browser/terminalInstance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -953,7 +953,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
// Respect chords if the allowChords setting is set and it's not Escape. Escape is
// handled specially for Zen Mode's Escape, Escape chord, plus it's important in
// terminals generally
const isValidChord = resolveResult?.kind === ResultKind.MoreChordsNeeded && this._configHelper.config.allowChords && event.key !== 'Escape';
const isValidChord = resolveResult.kind === ResultKind.MoreChordsNeeded && this._configHelper.config.allowChords && event.key !== 'Escape';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @Tyriar

I refactored the keybindings resolver and, consequently, AbstractKeybindingService#softDispatch a bit in last iteration and propagated the changes to various parts of vscode that use the softDispatch. I introduced a regression in one such place and now trying to make sure my changes in other places make sense.

Would you mind confirming that what this line does matches the desired result? This particular PR doesn't do much, there's a more meaningful diff here - https://github.com/microsoft/vscode/pull/178699/files#

if (this._keybindingService.inChordMode || isValidChord) {
event.preventDefault();
return false;
Expand All @@ -973,7 +973,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {

// for keyboard events that resolve to commands described
// within commandsToSkipShell, either alert or skip processing by xterm.js
if (resolveResult && resolveResult.kind === ResultKind.KbFound && resolveResult.commandId && this._skipTerminalCommands.some(k => k === resolveResult.commandId) && !this._configHelper.config.sendKeybindingsToShell) {
if (resolveResult.kind === ResultKind.KbFound && resolveResult.commandId && this._skipTerminalCommands.some(k => k === resolveResult.commandId) && !this._configHelper.config.sendKeybindingsToShell) {
// don't alert when terminal is opened or closed
if (this._storageService.getBoolean(SHOW_TERMINAL_CONFIG_PROMPT_KEY, StorageScope.APPLICATION, true) &&
this._hasHadInput &&
Expand Down
3 changes: 2 additions & 1 deletion src/vs/workbench/services/hover/browser/hoverService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { DisposableStore, IDisposable, toDisposable } from 'vs/base/common/lifec
import { addDisposableListener, EventType } from 'vs/base/browser/dom';
import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding';
import { StandardKeyboardEvent } from 'vs/base/browser/keyboardEvent';
import { ResultKind } from 'vs/platform/keybinding/common/keybindingResolver';

export class HoverService implements IHoverService {
declare readonly _serviceBrand: undefined;
Expand Down Expand Up @@ -114,7 +115,7 @@ export class HoverService implements IHoverService {
}
const event = new StandardKeyboardEvent(e);
const keybinding = this._keybindingService.resolveKeyboardEvent(event);
if (keybinding.getSingleModifierDispatchChords().some(value => !!value) || this._keybindingService.softDispatch(event, event.target)) {
if (keybinding.getSingleModifierDispatchChords().some(value => !!value) || this._keybindingService.softDispatch(event, event.target).kind !== ResultKind.NoMatchingKb) {
return;
}
if (hideOnKeyDown && (!this._currentHoverOptions?.trapFocus || e.key !== 'Tab')) {
Expand Down
2 changes: 1 addition & 1 deletion src/vs/workbench/services/progress/browser/progressService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,7 +557,7 @@ export class ProgressService extends Disposable implements IProgressService {
disableDefaultAction: options.sticky,
keyEventProcessor: (event: StandardKeyboardEvent) => {
const resolved = this.keybindingService.softDispatch(event, this.layoutService.container);
if (resolved && resolved.kind === ResultKind.KbFound && resolved.commandId) {
if (resolved.kind === ResultKind.KbFound && resolved.commandId) {
if (!allowableCommands.includes(resolved.commandId)) {
EventHelper.stop(event, true);
}
Expand Down