Skip to content

Commit a0b7054

Browse files
alexdimaalexdima
committed
Fixes microsoft#35225: Detect menu items invoked via keybinding and dispatch keybinding on renderer
1 parent 57c1e82 commit a0b7054

8 files changed

Lines changed: 87 additions & 18 deletions

File tree

‎src/vs/platform/keybinding/common/abstractKeybindingService.ts‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,10 +152,20 @@ export abstract class AbstractKeybindingService extends Disposable implements IK
152152
this._currentChord = null;
153153
}
154154

155+
public dispatchByUserSettingsLabel(userSettingsLabel: string, target: IContextKeyServiceTarget): void {
156+
const keybindings = this.resolveUserBinding(userSettingsLabel);
157+
if (keybindings.length === 1) {
158+
this._doDispatch(keybindings[0], target);
159+
}
160+
}
161+
155162
protected _dispatch(e: IKeyboardEvent, target: IContextKeyServiceTarget): boolean {
163+
return this._doDispatch(this.resolveKeyboardEvent(e), target);
164+
}
165+
166+
private _doDispatch(keybinding: ResolvedKeybinding, target: IContextKeyServiceTarget): boolean {
156167
let shouldPreventDefault = false;
157168

158-
const keybinding = this.resolveKeyboardEvent(e);
159169
if (keybinding.isChord()) {
160170
console.warn('Unexpected keyboard event mapped to a chord');
161171
return false;

‎src/vs/platform/keybinding/common/keybinding.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,8 @@ export interface IKeybindingService {
5757
*/
5858
softDispatch(keyboardEvent: IKeyboardEvent, target: IContextKeyServiceTarget): IResolveResult | null;
5959

60+
dispatchByUserSettingsLabel(userSettingsLabel: string, target: IContextKeyServiceTarget): void;
61+
6062
/**
6163
* Look up keybindings for a command.
6264
* Use `lookupKeybinding` if you are interested in the preferred keybinding.

‎src/vs/platform/keybinding/test/common/mockKeybindingService.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -120,8 +120,7 @@ export class MockKeybindingService implements IKeybindingService {
120120
return null;
121121
}
122122

123-
dispatchEvent(e: IKeyboardEvent, target: IContextKeyServiceTarget): boolean {
124-
return false;
123+
dispatchByUserSettingsLabel(userSettingsLabel: string, target: IContextKeyServiceTarget): void {
125124
}
126125

127126
mightProducePrintableCharacter(e: IKeyboardEvent): boolean {

‎src/vs/platform/menubar/common/menubar.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ export interface IMenubarMenu {
2525

2626
export interface IMenubarKeybinding {
2727
label: string;
28+
userSettingsLabel: string;
2829
isNative?: boolean; // Assumed true if missing
2930
}
3031

‎src/vs/platform/menubar/electron-main/menubar.ts‎

Lines changed: 58 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import * as nls from 'vs/nls';
77
import { isMacintosh, language } from 'vs/base/common/platform';
88
import { IEnvironmentService } from 'vs/platform/environment/common/environment';
99
import { app, shell, Menu, MenuItem, BrowserWindow } from 'electron';
10-
import { OpenContext, IRunActionInWindowRequest, getTitleBarStyle } from 'vs/platform/windows/common/windows';
10+
import { OpenContext, IRunActionInWindowRequest, getTitleBarStyle, IRunKeybindingInWindowRequest } from 'vs/platform/windows/common/windows';
1111
import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
1212
import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry';
1313
import { IUpdateService, StateType } from 'vs/platform/update/common/update';
@@ -31,6 +31,15 @@ interface IMenuItemClickHandler {
3131
inNoWindow: () => void;
3232
}
3333

34+
type IMenuItemInvocation = (
35+
{ type: 'commandId'; commandId: string; }
36+
| { type: 'keybinding'; userSettingsLabel: string; }
37+
);
38+
39+
interface IMenuItemWithKeybinding {
40+
userSettingsLabel?: string;
41+
}
42+
3443
export class Menubar {
3544

3645
private static readonly MAX_MENU_RECENT_ENTRIES = 10;
@@ -621,17 +630,49 @@ export class Menubar {
621630
}
622631
}
623632

633+
private static _menuItemIsTriggeredViaKeybinding(event: Electron.Event, userSettingsLabel: string): boolean {
634+
// The event coming in from Electron will inform us only about the modifier keys pressed.
635+
// The strategy here is to check if the modifier keys match those of the keybinding,
636+
// since it is highly unlikely to use modifier keys when clicking with the mouse
637+
if (!userSettingsLabel) {
638+
// There is no keybinding
639+
return false;
640+
}
641+
642+
let ctrlRequired = /ctrl/.test(userSettingsLabel);
643+
let shiftRequired = /shift/.test(userSettingsLabel);
644+
let altRequired = /alt/.test(userSettingsLabel);
645+
let metaRequired = /cmd/.test(userSettingsLabel) || /super/.test(userSettingsLabel);
646+
647+
if (!ctrlRequired && !shiftRequired && !altRequired && !metaRequired) {
648+
// This keybinding does not use any modifier keys, so we cannot use this heuristic
649+
return false;
650+
}
651+
652+
return (
653+
ctrlRequired === event.ctrlKey
654+
&& shiftRequired === event.shiftKey
655+
&& altRequired === event.altKey
656+
&& metaRequired === event.metaKey
657+
);
658+
}
659+
624660
private createMenuItem(label: string, commandId: string | string[], enabled?: boolean, checked?: boolean): Electron.MenuItem;
625661
private createMenuItem(label: string, click: () => void, enabled?: boolean, checked?: boolean): Electron.MenuItem;
626662
private createMenuItem(arg1: string, arg2: any, arg3?: boolean, arg4?: boolean): Electron.MenuItem {
627663
const label = this.mnemonicLabel(arg1);
628-
const click: () => void = (typeof arg2 === 'function') ? arg2 : (menuItem: Electron.MenuItem, win: Electron.BrowserWindow, event: Electron.Event) => {
664+
const click: () => void = (typeof arg2 === 'function') ? arg2 : (menuItem: Electron.MenuItem & IMenuItemWithKeybinding, win: Electron.BrowserWindow, event: Electron.Event) => {
665+
const userSettingsLabel = menuItem.userSettingsLabel;
629666
let commandId = arg2;
630667
if (Array.isArray(arg2)) {
631668
commandId = this.isOptionClick(event) ? arg2[1] : arg2[0]; // support alternative action if we got multiple action Ids and the option key was pressed while invoking
632669
}
633670

634-
this.runActionInRenderer(commandId);
671+
if (isMacintosh && userSettingsLabel && Menubar._menuItemIsTriggeredViaKeybinding(event, userSettingsLabel)) {
672+
this.runActionInRenderer({ type: 'keybinding', userSettingsLabel });
673+
} else {
674+
this.runActionInRenderer({ type: 'commandId', commandId });
675+
}
635676
};
636677
const enabled = typeof arg3 === 'boolean' ? arg3 : this.windowsMainService.getWindowCount() > 0;
637678
const checked = typeof arg4 === 'boolean' ? arg4 : false;
@@ -704,7 +745,7 @@ export class Menubar {
704745
};
705746
}
706747

707-
private runActionInRenderer(id: string): void {
748+
private runActionInRenderer(invocation: IMenuItemInvocation): void {
708749
// We make sure to not run actions when the window has no focus, this helps
709750
// for https://github.com/Microsoft/vscode/issues/25907 and specifically for
710751
// https://github.com/Microsoft/vscode/issues/11928
@@ -719,18 +760,21 @@ export class Menubar {
719760
}
720761

721762
if (activeWindow) {
722-
if (!activeWindow.isReady && isMacintosh && id === 'workbench.action.toggleDevTools' && !this.environmentService.isBuilt) {
723-
// prevent this action from running twice on macOS (https://github.com/Microsoft/vscode/issues/62719)
724-
// we already register a keybinding in bootstrap-window.js for opening developer tools in case something
725-
// goes wrong and that keybinding is only removed when the application has loaded (= window ready).
726-
return;
763+
if (invocation.type === 'commandId') {
764+
if (!activeWindow.isReady && isMacintosh && invocation.commandId === 'workbench.action.toggleDevTools' && !this.environmentService.isBuilt) {
765+
// prevent this action from running twice on macOS (https://github.com/Microsoft/vscode/issues/62719)
766+
// we already register a keybinding in bootstrap-window.js for opening developer tools in case something
767+
// goes wrong and that keybinding is only removed when the application has loaded (= window ready).
768+
return;
769+
}
770+
this.windowsMainService.sendToFocused('vscode:runAction', { id: invocation.commandId, from: 'menu' } as IRunActionInWindowRequest);
771+
} else {
772+
this.windowsMainService.sendToFocused('vscode:runKeybinding', { userSettingsLabel: invocation.userSettingsLabel } as IRunKeybindingInWindowRequest);
727773
}
728-
729-
this.windowsMainService.sendToFocused('vscode:runAction', { id, from: 'menu' } as IRunActionInWindowRequest);
730774
}
731775
}
732776

733-
private withKeybinding(commandId: string | undefined, options: Electron.MenuItemConstructorOptions): Electron.MenuItemConstructorOptions {
777+
private withKeybinding(commandId: string | undefined, options: Electron.MenuItemConstructorOptions & IMenuItemWithKeybinding): Electron.MenuItemConstructorOptions {
734778
const binding = typeof commandId === 'string' ? this.keybindings[commandId] : undefined;
735779

736780
// Apply binding if there is one
@@ -739,6 +783,7 @@ export class Menubar {
739783
// if the binding is native, we can just apply it
740784
if (binding.isNative !== false) {
741785
options.accelerator = binding.label;
786+
options.userSettingsLabel = binding.userSettingsLabel;
742787
}
743788

744789
// the keybinding is not native so we cannot show it as part of the accelerator of
@@ -768,6 +813,7 @@ export class Menubar {
768813

769814
const originalClick = options.click;
770815
options.click = (item, window, event) => {
816+
console.log(`444444`);
771817
this.reportMenuActionTelemetry(commandId);
772818
if (originalClick) {
773819
originalClick(item, window, event);

‎src/vs/platform/windows/common/windows.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -408,6 +408,10 @@ export interface IRunActionInWindowRequest {
408408
args?: any[];
409409
}
410410

411+
export interface IRunKeybindingInWindowRequest {
412+
userSettingsLabel: string;
413+
}
414+
411415
export class ActiveWindowManager implements IDisposable {
412416

413417
private disposables: IDisposable[] = [];

‎src/vs/workbench/browser/parts/titlebar/menubarControl.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -980,13 +980,13 @@ export class MenubarControl extends Disposable {
980980
// first try to resolve a native accelerator
981981
const electronAccelerator = binding.getElectronAccelerator();
982982
if (electronAccelerator) {
983-
return { label: electronAccelerator };
983+
return { label: electronAccelerator, userSettingsLabel: binding.getUserSettingsLabel() };
984984
}
985985

986986
// we need this fallback to support keybindings that cannot show in electron menus (e.g. chords)
987987
const acceleratorLabel = binding.getLabel();
988988
if (acceleratorLabel) {
989-
return { label: acceleratorLabel, isNative: false };
989+
return { label: acceleratorLabel, isNative: false, userSettingsLabel: binding.getUserSettingsLabel() };
990990
}
991991

992992
return null;

‎src/vs/workbench/electron-browser/window.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import { toResource, IUntitledResourceInput } from 'vs/workbench/common/editor';
1515
import { IEditorService, IResourceEditor } from 'vs/workbench/services/editor/common/editorService';
1616
import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry';
1717
import { IWorkspaceConfigurationService } from 'vs/workbench/services/configuration/common/configuration';
18-
import { IWindowsService, IWindowService, IWindowSettings, IOpenFileRequest, IWindowsConfiguration, IAddFoldersRequest, IRunActionInWindowRequest, IPathData } from 'vs/platform/windows/common/windows';
18+
import { IWindowsService, IWindowService, IWindowSettings, IOpenFileRequest, IWindowsConfiguration, IAddFoldersRequest, IRunActionInWindowRequest, IPathData, IRunKeybindingInWindowRequest } from 'vs/platform/windows/common/windows';
1919
import { IContextMenuService } from 'vs/platform/contextview/browser/contextView';
2020
import { ITitleService } from 'vs/workbench/services/title/common/titleService';
2121
import { IWorkbenchThemeService, VS_HC_THEME } from 'vs/workbench/services/themes/common/workbenchThemeService';
@@ -38,6 +38,7 @@ import { AccessibilitySupport, isRootUser, isWindows, isMacintosh } from 'vs/bas
3838
import product from 'vs/platform/node/product';
3939
import { INotificationService } from 'vs/platform/notification/common/notification';
4040
import { EditorServiceImpl } from 'vs/workbench/browser/parts/editor/editor';
41+
import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding';
4142

4243
const TextInputActions: IAction[] = [
4344
new Action('undo', nls.localize('undo', "Undo"), null, true, () => document.execCommand('undo') && Promise.resolve(true)),
@@ -71,6 +72,7 @@ export class ElectronWindow extends Themable {
7172
@IWorkbenchThemeService protected themeService: IWorkbenchThemeService,
7273
@INotificationService private notificationService: INotificationService,
7374
@ICommandService private commandService: ICommandService,
75+
@IKeybindingService private keybindingService: IKeybindingService,
7476
@IContextMenuService private contextMenuService: IContextMenuService,
7577
@ITelemetryService private telemetryService: ITelemetryService,
7678
@IWorkspaceEditingService private workspaceEditingService: IWorkspaceEditingService,
@@ -133,6 +135,11 @@ export class ElectronWindow extends Themable {
133135
});
134136
});
135137

138+
// Support runKeybinding event
139+
ipc.on('vscode:runKeybinding', (event: any, request: IRunKeybindingInWindowRequest) => {
140+
this.keybindingService.dispatchByUserSettingsLabel(request.userSettingsLabel, document.activeElement);
141+
});
142+
136143
// Error reporting from main
137144
ipc.on('vscode:reportError', (event: any, error: string) => {
138145
if (error) {

0 commit comments

Comments
 (0)