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.

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

keybindings: add context menu to keyboard shortcuts #12791

Merged
merged 7 commits into from
Oct 21, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions packages/core/src/common/keybinding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,32 @@ export namespace Keybinding {
export function is(arg: unknown): arg is Keybinding {
return isObject(arg) && 'command' in arg && 'keybinding' in arg;
}

export function replaceKeybinding(keybindings: Keybinding[], oldKeybinding: Keybinding, newKeybinding: Keybinding): boolean {
const indexOld = keybindings.findIndex(keybinding => Keybinding.equals(keybinding, oldKeybinding, false, true));
if (indexOld >= 0) {
const indexNew = keybindings.findIndex(keybinding => Keybinding.equals(keybinding, newKeybinding, false, true));
if (indexNew >= 0 && indexNew !== indexOld) {
// if keybindings already contain the new keybinding, remove the old keybinding and update the new one
keybindings.splice(indexOld, 1);
keybindings[indexNew] = newKeybinding;
} else {
keybindings[indexOld] = newKeybinding;
}
return true;
}
return false;
}

export function addKeybinding(keybindings: Keybinding[], newKeybinding: Keybinding): void {
const index = keybindings.findIndex(keybinding => Keybinding.equals(keybinding, newKeybinding, false, true));
if (index >= 0) {
// if keybindings already contain the new keybinding, update it
keybindings[index] = newKeybinding;
} else {
keybindings.push(newKeybinding);
}
}
}

/**
Expand Down
191 changes: 172 additions & 19 deletions packages/keymaps/src/browser/keybindings-widget.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,15 @@ import * as fuzzy from '@theia/core/shared/fuzzy';
import { injectable, inject, postConstruct, unmanaged } from '@theia/core/shared/inversify';
import { Emitter, Event } from '@theia/core/lib/common/event';
import { CommandRegistry, Command } from '@theia/core/lib/common/command';
import { Keybinding } from '@theia/core/lib/common/keybinding';
import { ReactWidget } from '@theia/core/lib/browser/widgets/react-widget';
import {
KeybindingRegistry, SingleTextInputDialog, KeySequence, ConfirmDialog, Message, KeybindingScope,
SingleTextInputDialogProps, Key, ScopedKeybinding, codicon, StatefulWidget, Widget
SingleTextInputDialogProps, Key, ScopedKeybinding, codicon, StatefulWidget, Widget, ContextMenuRenderer, SELECTED_CLASS
} from '@theia/core/lib/browser';
import { KeymapsService } from './keymaps-service';
import { AlertMessage } from '@theia/core/lib/browser/widgets/alert-message';
import { DisposableCollection, isOSX } from '@theia/core';
import { DisposableCollection, isOSX, isObject } from '@theia/core';
import { nls } from '@theia/core/lib/common/nls';

/**
Expand All @@ -47,6 +48,19 @@ export interface KeybindingItem {
visible?: boolean;
}

export namespace KeybindingItem {
export function is(arg: unknown): arg is KeybindingItem {
return isObject(arg) && 'command' in arg && 'labels' in arg;
}

export function keybinding(item: KeybindingItem): Keybinding {
return item.keybinding ?? {
command: item.command.id,
keybinding: ''
};
}
}

export interface RenderableLabel {
readonly value: string;
segments?: RenderableStringSegment[];
Expand Down Expand Up @@ -84,8 +98,17 @@ export class KeybindingWidget extends ReactWidget implements StatefulWidget {
@inject(KeymapsService)
protected readonly keymapsService: KeymapsService;

@inject(ContextMenuRenderer)
protected readonly contextMenuRenderer: ContextMenuRenderer;

static readonly ID = 'keybindings.view.widget';
static readonly LABEL = nls.localizeByDefault('Keyboard Shortcuts');
static readonly CONTEXT_MENU = ['keybinding-context-menu'];
static readonly COPY_MENU = [...KeybindingWidget.CONTEXT_MENU, 'a_copy'];
static readonly EDIT_MENU = [...KeybindingWidget.CONTEXT_MENU, 'b_edit'];
static readonly ADD_MENU = [...KeybindingWidget.CONTEXT_MENU, 'c_add'];
static readonly REMOVE_MENU = [...KeybindingWidget.CONTEXT_MENU, 'd_remove'];
static readonly SHOW_MENU = [...KeybindingWidget.CONTEXT_MENU, 'e_show'];

/**
* The list of all available keybindings.
Expand Down Expand Up @@ -180,6 +203,23 @@ export class KeybindingWidget extends ReactWidget implements StatefulWidget {
}
}

/**
* Show keybinding items with the same key sequence as the given item.
* @param item the keybinding item
*/
public showSameKeybindings(item: KeybindingItem): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

public is not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review and comments, Thomas. Let me try to address your concerns.

The public modifiers can be omitted in TypeScript, of course.

However, I aimed at stylistic consistency with the surrounding code here, where both of the directly preceding methods hasSearch() and clearSearch() have already had the public modifier. Also, I didn't find it mentioned in the style guide at all.

However, if you do dislike them, I can remove all the public modifiers in my code, no problem. In that case, may I humbly suggest the style guide probably needs to be updated accordingly?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, of course, that there is no guidance in the style guide. However, most code in Theia does not use public. I usually remove it when I touch a piece of code. But it's not a reason to reject the PR, of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I'll remove all the public modifiers in the KeybindingWidget then, including the old code, for the sake of stylistic consistency, if you don't mind. It seems that I haven't introduced any public modifiers elsewhere.

const keybinding = item.keybinding;
if (keybinding) {
const search = this.findSearchField();
if (search) {
const query = `"${this.keybindingRegistry.acceleratorFor(keybinding, '+', true).join(' ')}"`;
search.value = query;
this.query = query;
this.doSearchKeybindings();
}
}
}

protected override onActivateRequest(msg: Message): void {
super.onActivateRequest(msg);
this.focusInputField();
Expand All @@ -192,13 +232,27 @@ export class KeybindingWidget extends ReactWidget implements StatefulWidget {
this.onDidUpdateEmitter.fire(undefined);
const searchField = this.findSearchField();
this.query = searchField ? searchField.value.trim().toLocaleLowerCase() : '';
const queryItems = this.query.split(/[+\s]/);
let query = this.query;
const startsWithQuote = query.startsWith('"');
const endsWithQuote = query.endsWith('"');
const matchKeybindingOnly = startsWithQuote && endsWithQuote;
tsmaeder marked this conversation as resolved.
Show resolved Hide resolved
if (startsWithQuote) {
query = query.slice(1);
}
if (endsWithQuote) {
query = query.slice(0, -1);
}
const queryItems = query.split(/[+\s]/);
this.items.forEach(item => {
let matched = !this.query;
matched = this.formatAndMatchCommand(item) || matched;
matched = this.formatAndMatchKeybinding(item, queryItems) || matched;
matched = this.formatAndMatchContext(item) || matched;
matched = this.formatAndMatchSource(item) || matched;
if (!matchKeybindingOnly) {
matched = this.formatAndMatchCommand(item) || matched;
}
matched = this.formatAndMatchKeybinding(item, queryItems, matchKeybindingOnly) || matched;
if (!matchKeybindingOnly) {
matched = this.formatAndMatchContext(item) || matched;
matched = this.formatAndMatchSource(item) || matched;
}
item.visible = matched;
});
this.update();
Expand All @@ -209,7 +263,7 @@ export class KeybindingWidget extends ReactWidget implements StatefulWidget {
return Boolean(item.labels.command.segments);
}

protected formatAndMatchKeybinding(item: KeybindingItem, queryItems: string[]): boolean {
protected formatAndMatchKeybinding(item: KeybindingItem, queryItems: string[], exactMatch?: boolean): boolean {
if (item.keybinding) {
const unmatchedTerms = queryItems.filter(Boolean);
const segments = this.keybindingRegistry.resolveKeybinding(item.keybinding).reduce<RenderableStringSegment[]>((collection, code, codeIndex) => {
Expand All @@ -232,7 +286,13 @@ export class KeybindingWidget extends ReactWidget implements StatefulWidget {
return collection;
}, []);
item.labels.keybinding = { value: item.labels.keybinding.value, segments };
return !unmatchedTerms.length;
if (unmatchedTerms.length) {
return false;
}
if (exactMatch) {
return !segments.some(segment => segment.key && !segment.match);
}
return true;
}
item.labels.keybinding = { value: '' };
return false;
Expand Down Expand Up @@ -364,7 +424,9 @@ export class KeybindingWidget extends ReactWidget implements StatefulWidget {
protected renderRow(item: KeybindingItem, index: number): React.ReactNode {
const { command, keybinding } = item;
// TODO get rid of array functions in event handlers
return <tr className='kb-item-row' key={index} onDoubleClick={() => this.editKeybinding(item)}>
return <tr className='kb-item-row' key={index} onDoubleClick={event => this.handleItemDoubleClick(item, index, event)}
onClick={event => this.handleItemClick(item, index, event)}
onContextMenu={event => this.handleItemContextMenu(item, index, event)}>
<td className='kb-actions'>
{this.renderActions(item)}
</td>
Expand All @@ -383,6 +445,37 @@ export class KeybindingWidget extends ReactWidget implements StatefulWidget {
</tr>;
}

protected handleItemClick(item: KeybindingItem, index: number, event: React.MouseEvent<HTMLElement>): void {
event.preventDefault();
this.selectItem(item, index, event.currentTarget);
}

protected handleItemDoubleClick(item: KeybindingItem, index: number, event: React.MouseEvent<HTMLElement>): void {
event.preventDefault();
this.selectItem(item, index, event.currentTarget);
this.editKeybinding(item);
}

protected handleItemContextMenu(item: KeybindingItem, index: number, event: React.MouseEvent<HTMLElement>): void {
event.preventDefault();
this.selectItem(item, index, event.currentTarget);
setTimeout(() => this.contextMenuRenderer.render({
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the setTimout()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it should be necessary, the context menu renderer is used in other places without it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I'll remove it then.

menuPath: KeybindingWidget.CONTEXT_MENU,
anchor: event.nativeEvent,
args: [item, this]
}));
}

protected selectItem(item: KeybindingItem, index: number, element: HTMLElement): void {
if (!element.classList.contains(SELECTED_CLASS)) {
const selected = element.parentElement?.getElementsByClassName(SELECTED_CLASS)[0];
if (selected) {
selected.classList.remove(SELECTED_CLASS);
}
element.classList.add(SELECTED_CLASS);
}
}

/**
* Render the actions container with action icons.
* @param item the keybinding item for the row.
Expand All @@ -408,7 +501,7 @@ export class KeybindingWidget extends ReactWidget implements StatefulWidget {
* @param item the keybinding item for the row.
*/
protected renderReset(item: KeybindingItem): React.ReactNode {
return (item.keybinding && item.keybinding.scope === KeybindingScope.USER)
return this.canResetKeybinding(item)
? <a title='Reset Keybinding' href='#' onClick={e => {
e.preventDefault();
this.resetKeybinding(item);
Expand Down Expand Up @@ -572,22 +665,74 @@ export class KeybindingWidget extends ReactWidget implements StatefulWidget {
* Prompt users to update the keybinding for the given command.
* @param item the keybinding item.
*/
protected editKeybinding(item: KeybindingItem): void {
public editKeybinding(item: KeybindingItem): void {
pisv marked this conversation as resolved.
Show resolved Hide resolved
const command = item.command.id;
const oldKeybinding = item.keybinding;
const dialog = new EditKeybindingDialog({
title: nls.localize('theia/keymaps/editKeybindingTitle', 'Edit Keybinding for {0}', command),
title: nls.localize('theia/keymaps/editKeybindingTitle', 'Edit Keybinding for {0}', item.labels.command.value),
tsmaeder marked this conversation as resolved.
Show resolved Hide resolved
maxWidth: 400,
initialValue: oldKeybinding?.keybinding,
validate: newKeybinding => this.validateKeybinding(command, oldKeybinding?.keybinding, newKeybinding),
}, this.keymapsService, item);
}, this.keymapsService, item, this.canResetKeybinding(item));
dialog.open().then(async keybinding => {
if (keybinding && keybinding !== oldKeybinding?.keybinding) {
await this.keymapsService.setKeybinding({
...oldKeybinding,
command,
keybinding
}, oldKeybinding);
}
});
}

/**
* Prompt users to update when expression for the given keybinding.
* @param item the keybinding item
*/
public editWhenExpression(item: KeybindingItem): void {
const keybinding = item.keybinding;
if (!keybinding) {
return;
}
const dialog = new SingleTextInputDialog({
title: nls.localize('theia/keymaps/editWhenExpressionTitle', 'Edit When Expression for {0}', item.labels.command.value),
maxWidth: 400,
initialValue: keybinding.when
});
dialog.open().then(async when => {
if (when === undefined) {
return; // cancelled by the user
}
if (when !== (keybinding.when ?? '')) {
if (when === '') {
when = undefined;
}
await this.keymapsService.setKeybinding({
...keybinding,
when
}, keybinding);
}
});
}

/**
* Prompt users to add a keybinding for the given command.
* @param item the keybinding item
*/
public addKeybinding(item: KeybindingItem): void {
const command = item.command.id;
const dialog = new SingleTextInputDialog({
title: nls.localize('theia/keymaps/addKeybindingTitle', 'Add Keybinding for {0}', item.labels.command.value),
maxWidth: 400,
validate: newKeybinding => this.validateKeybinding(command, undefined, newKeybinding),
});
dialog.open().then(async keybinding => {
if (keybinding) {
await this.keymapsService.setKeybinding({
...item.keybinding,
command,
keybinding
}, oldKeybinding);
}, undefined);
}
});
}
Expand Down Expand Up @@ -618,13 +763,21 @@ export class KeybindingWidget extends ReactWidget implements StatefulWidget {
* Reset the keybinding to its default value.
* @param item the keybinding item.
*/
protected async resetKeybinding(item: KeybindingItem): Promise<void> {
public async resetKeybinding(item: KeybindingItem): Promise<void> {
const confirmed = await this.confirmResetKeybinding(item);
if (confirmed) {
this.keymapsService.removeKeybinding(item.command.id);
}
}

/**
* Whether the keybinding can be reset to its default value.
* @param item the keybinding item
*/
public canResetKeybinding(item: KeybindingItem): boolean {
return item.keybinding?.scope === KeybindingScope.USER || this.keymapsService.hasKeybinding('-' + item.command.id);
tsmaeder marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Validate the provided keybinding value against its previous value.
* @param command the command label.
Expand Down Expand Up @@ -745,12 +898,13 @@ class EditKeybindingDialog extends SingleTextInputDialog {
constructor(
@inject(SingleTextInputDialogProps) props: SingleTextInputDialogProps,
@inject(KeymapsService) protected readonly keymapsService: KeymapsService,
item: KeybindingItem
item: KeybindingItem,
canReset: boolean
) {
super(props);
this.item = item;
// Add the `Reset` button if the command currently has a custom keybinding.
if (this.item.keybinding && this.item.keybinding.scope === KeybindingScope.USER) {
if (canReset) {
this.appendResetButton();
}
}
Expand Down Expand Up @@ -796,5 +950,4 @@ class EditKeybindingDialog extends SingleTextInputDialog {
protected reset(): void {
this.keymapsService.removeKeybinding(this.item.command.id);
}

}
Loading
Loading