From dcdea4aaf8d28c014a0aabb3e7c2d6cc2edba4e2 Mon Sep 17 00:00:00 2001 From: Vincent Fugnitto Date: Thu, 20 Jun 2019 13:09:13 -0400 Subject: [PATCH] Align keybindings-widget with vscode Fixes #5544 The purpose of the PR is to align the behavior and look of the keybindings-widget more closely with that of vscode. Features present in the PR: 1. removed the `command id` column. 2. renamed the `scope` column to `source`. 3. display the command id for commands without a label. 4. re-order the columns to that of vscode (actions, command, keybinding, source). 5. prioritized the display of commands with keybindings before those without. 6. updated the documentation. Signed-off-by: Vincent Fugnitto --- .../src/browser/keybindings-widget.tsx | 229 ++++++++++++++---- packages/keymaps/src/browser/style/index.css | 13 +- 2 files changed, 191 insertions(+), 51 deletions(-) diff --git a/packages/keymaps/src/browser/keybindings-widget.tsx b/packages/keymaps/src/browser/keybindings-widget.tsx index 1e7825f020a03..a03888a36332f 100644 --- a/packages/keymaps/src/browser/keybindings-widget.tsx +++ b/packages/keymaps/src/browser/keybindings-widget.tsx @@ -18,23 +18,50 @@ import React = require('react'); import debounce = require('lodash.debounce'); import * as fuzzy from 'fuzzy'; import { injectable, inject, postConstruct } from 'inversify'; -import { CommandRegistry, Command, Emitter, Event } from '@theia/core/lib/common'; +import { CommandRegistry, Emitter, Event } from '@theia/core/lib/common'; import { ReactWidget } from '@theia/core/lib/browser/widgets/react-widget'; import { KeybindingRegistry, SingleTextInputDialog, KeySequence, ConfirmDialog, Message, KeybindingScope } from '@theia/core/lib/browser'; import { KeymapsParser } from './keymaps-parser'; import { KeymapsService, KeybindingJson } from './keymaps-service'; import { AlertMessage } from '@theia/core/lib/browser/widgets/alert-message'; +/** + * Representation of a keybinding item for the view. + */ export interface KeybindingItem { + /** + * The id of the command. + */ id: string, + /** + * The human-readable label of the command. + */ command: string, + /** + * The keybinding of the command. + */ keybinding?: string, + /** + * The context / when closure of the command. + */ context?: string, - scope?: string, + /** + * The source of the command. + */ + source?: string, } +/** + * Representation of an individual table cell. + */ export interface CellData { + /** + * The cell value. + */ value: string, + /** + * Indicates if a cell's value is currently highlighted. + */ highlighted: boolean, } @@ -53,16 +80,26 @@ export class KeybindingWidget extends ReactWidget { @inject(KeymapsService) protected readonly keymapsService: KeymapsService; - protected items: KeybindingItem[]; + protected items: KeybindingItem[] = []; static readonly ID = 'keybindings.view.widget'; static readonly LABEL = 'Keyboard Shortcuts'; + /** + * The current user search query. + */ protected query: string = ''; + /** + * The regular expression used to extract values between fuzzy results. + */ protected readonly regexp = /(.*?)<\/match>/g; protected readonly keybindingSeparator = /\+<\/match>/g; + /** + * The fuzzy search options. + * The `pre` and `post` options are used to wrap fuzzy matches. + */ protected readonly fuzzyOptions = { pre: '', post: '', @@ -82,7 +119,10 @@ export class KeybindingWidget extends ReactWidget { this.title.closable = true; this.update(); + // Initialize the list of keybinding items. this.items = this.getItems(); + + // Listen to changes made in the `keymaps.json` and update the view accordingly. if (this.keymapsService.onDidChangeKeymaps) { this.toDispose.push(this.keymapsService.onDidChangeKeymaps(() => { this.doSearchKeybindings(); @@ -116,6 +156,9 @@ export class KeybindingWidget extends ReactWidget { this.focusInputField(); } + /** + * Perform a search based on the user's search query. + */ protected doSearchKeybindings(): void { this.onDidUpdateEmitter.fire(undefined); this.items = []; @@ -123,7 +166,7 @@ export class KeybindingWidget extends ReactWidget { this.query = searchField ? searchField.value.trim().toLocaleLowerCase() : ''; const items = this.getItems(); items.forEach(item => { - const keys: (keyof KeybindingItem)[] = ['id', 'command', 'keybinding', 'context', 'scope']; + const keys: (keyof KeybindingItem)[] = ['command', 'keybinding', 'context', 'source']; let matched = false; for (const key of keys) { const string = item[key]; @@ -133,13 +176,13 @@ export class KeybindingWidget extends ReactWidget { item[key] = fuzzyMatch.rendered; matched = true; } else { - // Match identical keybindings that have different orders + // Match identical keybindings that have different orders. if (key === 'keybinding') { const queryItems = this.query.split('+'); - // Handle key chords + // Handle key chords. const tempItems = string.split(' '); - // Store positions of `space` in the keybinding string + // Store positions of `space` in the keybinding string. const spaceIndexArr = [0]; let bindingItems: string[] = []; if (tempItems.length > 1) { @@ -161,12 +204,12 @@ export class KeybindingWidget extends ReactWidget { let keyIndex = -1; if (string) { bindingItems.forEach((bindingItem: string) => { - // Match every key in user query with every key in keybinding string + // Match every key in user query with every key in keybinding string. const tempFuzzyMatch = fuzzy.match(queryItem, bindingItem, this.fuzzyOptions); - // Select the match with the highest matching score + // Select the match with the highest matching score. if (tempFuzzyMatch && tempFuzzyMatch.score > keyFuzzyMatch.score) { keyFuzzyMatch = tempFuzzyMatch; - // Get index in the keybinding array + // Get index in the keybinding array. keyIndex = renderedResult.indexOf(bindingItem); } }); @@ -176,14 +219,14 @@ export class KeybindingWidget extends ReactWidget { if (keyIndex > -1) { renderedResult[keyIndex] = keyRendered; } - // Remove key from keybinding items if it is matched + // Remove key from keybinding items if it is matched. bindingItems.splice(keyIndex, 1, ''); matchCounter += 1; } } }); if (matchCounter === queryItems.length) { - // Handle rendering of key chords + // Handle rendering of key chords. if (spaceIndexArr.length > 0) { const chordRenderedResult = ''; renderedResult.forEach((resultKey, index) => { @@ -213,11 +256,18 @@ export class KeybindingWidget extends ReactWidget { this.update(); } + /** + * Get the search input if available. + * @returns the search input if available. + */ protected findSearchField(): HTMLInputElement | null { return document.getElementById('search-kb') as HTMLInputElement; } - protected focusInputField() { + /** + * Set the focus the search input field if available. + */ + protected focusInputField(): void { const input = document.getElementById('search-kb'); if (input) { (input as HTMLInputElement).focus(); @@ -225,6 +275,9 @@ export class KeybindingWidget extends ReactWidget { } } + /** + * Render the view. + */ protected render(): React.ReactNode { return
{this.renderSearch()} @@ -232,6 +285,9 @@ export class KeybindingWidget extends ReactWidget {
; } + /** + * Render the search container with the search input. + */ protected renderSearch(): React.ReactNode { return
@@ -242,6 +298,9 @@ export class KeybindingWidget extends ReactWidget {
; } + /** + * Render the warning message when no search results are found. + */ protected renderMessage(): React.ReactNode { return ; } + /** + * Render the keybindings table. + */ protected renderTable(): React.ReactNode { return
@@ -256,11 +318,10 @@ export class KeybindingWidget extends ReactWidget { - Label + Command Keybinding - Scope - Context - Command + Context / When + Source @@ -271,24 +332,28 @@ export class KeybindingWidget extends ReactWidget {
; } + /** + * Render the table rows. + */ protected renderRows(): React.ReactNode { return { this.items.map((item, index) => this.editKeybinding(item)}> - {this.renderActions(item)} - {this.renderMatchedData(item.command)} + + {this.renderActions(item)} + + + {this.renderMatchedData(item.command)} + {item.keybinding ? this.renderKeybinding(item.keybinding) : ''} - - {item.scope ? this.renderMatchedData(item.scope) : ''} - {(item.context) ? this.renderMatchedData(item.context) : ''} - - {this.renderMatchedData(item.id)} + + {item.source ? this.renderMatchedData(item.source) : ''} ) @@ -296,19 +361,36 @@ export class KeybindingWidget extends ReactWidget { ; } + /** + * Render the actions container with action icons. + * @param item {KeybindingItem} the keybinding item for the row. + */ protected renderActions(item: KeybindingItem): React.ReactNode { return {this.renderEdit(item)}{this.renderReset(item)}; } + /** + * Render the edit action used to update a keybinding. + * @param item {KeybindingItem} the keybinding item for the row. + */ protected renderEdit(item: KeybindingItem): React.ReactNode { return this.editKeybinding(item)}>; } + /** + * Render the reset action to reset the custom keybinding. + * Only visible if a keybinding has a `user` scope. + * @param item {KeybindingItem} the keybinding item for the row. + */ protected renderReset(item: KeybindingItem): React.ReactNode { - return (item.scope && item.scope === KeybindingScope[1].toLocaleLowerCase()) + return (item.source && item.source === KeybindingScope[1].toLocaleLowerCase()) ? this.resetKeybinding(item)}> : ''; } + /** + * Render the keybinding. + * @param keybinding {string} the keybinding value. + */ protected renderKeybinding(keybinding: string): React.ReactNode { const regex = new RegExp(this.keybindingSeparator); keybinding = keybinding.replace(regex, '+'); @@ -345,32 +427,61 @@ export class KeybindingWidget extends ReactWidget { ; } + /** + * Get the list of keybinding items. + * + * @returns the list of keybinding items. + */ protected getItems(): KeybindingItem[] { - const commands = this.commandRegistry.commands.sort((a, b) => this.compareCommands(a, b)); + // Sort the commands alphabetically. + const commands = this.commandRegistry.commands; const items: KeybindingItem[] = []; + // Build the keybinding items. for (let i = 0; i < commands.length; i++) { + // Obtain the keybinding for the given command. const keybindings = this.keybindingRegistry.getKeybindingsForCommand(commands[i].id); const item: KeybindingItem = { id: commands[i].id, - command: commands[i].label || '', + // Get the command label if available, else use the keybinding id. + command: commands[i].label || commands[i].id, keybinding: (keybindings && keybindings[0]) ? keybindings[0].keybinding : '', - context: (keybindings && keybindings[0]) ? keybindings[0].context : '', - scope: (keybindings && keybindings[0] && typeof keybindings[0].scope !== 'undefined') + context: (keybindings && keybindings[0]) + ? keybindings[0].context + ? keybindings[0].context : keybindings[0].when + : '', + source: (keybindings && keybindings[0] && typeof keybindings[0].scope !== 'undefined') ? KeybindingScope[keybindings[0].scope!].toLocaleLowerCase() : '', }; items.push(item); } - return items; + // Sort the keybinding item by label. + const sorted: KeybindingItem[] = items.sort((a: KeybindingItem, b: KeybindingItem) => this.compareItem(a.command, b.command)); + // Get the list of keybinding item with keybindings (visually put them at the top of the table). + const keyItems: KeybindingItem[] = sorted.filter((a: KeybindingItem) => !!a.keybinding); + // Get the remaining keybinding items (without keybindings). + const otherItems: KeybindingItem[] = sorted.filter((a: KeybindingItem) => !a.keybinding); + + // Return the list of keybinding items prioritizing those with a defined keybinding. + return [...keyItems, ...otherItems]; } - protected compareCommands(a: Command, b: Command): number { - if (a.label && b.label) { - return (a.label).localeCompare(b.label); - } else { - return 0; + /** + * Compare two strings. + * @param a {string | undefined} the first string. + * @param b {string | undefined} the second string. + */ + protected compareItem(a: string | undefined, b: string | undefined): number { + if (a && b) { + return (a.toLowerCase()).localeCompare(b.toLowerCase()); } + return 0; } + /** + * Determine if the keybinding currently exists in a user's `keymaps.json`. + * + * @returns `true` if the keybinding exists. + */ protected keybindingExistsInJson(keybindings: KeybindingJson[], command: string): boolean { for (let i = 0; i < keybindings.length; i++) { if (keybindings[i].command === command) { @@ -380,6 +491,10 @@ export class KeybindingWidget extends ReactWidget { return false; } + /** + * Prompt users to update the keybinding for the given command. + * @param item {KeybindingItem} the keybinding item. + */ protected editKeybinding(item: KeybindingItem): void { const command = this.getRawValue(item.command); const id = this.getRawValue(item.id); @@ -397,7 +512,13 @@ export class KeybindingWidget extends ReactWidget { }); } - protected async confirmResetKeybinding(command: string, commandId: string): Promise { + /** + * Prompt users for confirmation before resetting. + * @param command {string} the command label. + * + * @returns a Promise which resolves to `true` if a user accepts resetting. + */ + protected async confirmResetKeybinding(command: string): Promise { const dialog = new ConfirmDialog({ title: `Reset keybinding for '${command}'`, msg: 'Do you really want to reset this keybinding to its default value?' @@ -405,15 +526,27 @@ export class KeybindingWidget extends ReactWidget { return !!await dialog.open(); } + /** + * Reset the keybinding to its default value. + * @param item {KeybindingItem} the keybinding item. + */ protected async resetKeybinding(item: KeybindingItem): Promise { const rawCommandId = this.getRawValue(item.id); const rawCommand = this.getRawValue(item.command); - const confirmed = await this.confirmResetKeybinding(rawCommand, rawCommandId); + const confirmed = await this.confirmResetKeybinding(rawCommand); if (confirmed) { this.keymapsService.removeKeybinding(rawCommandId); } } + /** + * Validate the provided keybinding value against its previous value. + * @param command {string} the command label. + * @param oldKeybinding {string} the old keybinding value. + * @param keybinding {string} the new keybinding value. + * + * @returns the end user message to display. + */ protected validateKeybinding(command: string, oldKeybinding: string, keybinding: string): string { if (!keybinding) { return 'keybinding value is required'; @@ -433,6 +566,12 @@ export class KeybindingWidget extends ReactWidget { } } + /** + * Build the cell data with highlights if applicable. + * @param raw {string} the raw cell value. + * + * @returns the list of cell data. + */ protected buildCellData(raw: string): CellData[] { const data: CellData[] = []; @@ -466,23 +605,29 @@ export class KeybindingWidget extends ReactWidget { return data; } - protected renderMatchedData(item: string): React.ReactNode { + /** + * Render the fuzzy representation of a matched result. + * @param property {string} one of the `KeybindingItem` properties. + */ + protected renderMatchedData(property: string): React.ReactNode { if (this.query !== '') { - const cellData = this.buildCellData(item); + const cellData = this.buildCellData(property); return { cellData.map((data, index) => (data.highlighted) ? {data.value} : {data.value}) } ; } else { - return item; + return property; } } + /** + * Render the raw value of a item without fuzzy highlighting. + * @param property {string} one of the `KeybindingItem` properties. + */ protected getRawValue(property: string): string { return property.replace(new RegExp(this.regexp), '$1'); } - protected openKeybindings = () => this.keymapsService.open(this); - } diff --git a/packages/keymaps/src/browser/style/index.css b/packages/keymaps/src/browser/style/index.css index 59e57cc14b19d..76145db074a4f 100644 --- a/packages/keymaps/src/browser/style/index.css +++ b/packages/keymaps/src/browser/style/index.css @@ -70,8 +70,8 @@ padding: 2px 10px 5px 10px; } -.th-label, .th-scope, .th-context, .th-command, -.kb-label, .kb-scope, .kb-context, .kb-command { +.th-label, .th-source, .th-context, +.kb-label, .kb-source, .kb-context { padding: 2px 10px 5px 10px; min-height: 18px; overflow: hidden; @@ -92,7 +92,7 @@ font-size: calc(var(--theia-ui-font-size1) * 0.8); } -.td-scope { +.td-source { text-transform: lowercase; } @@ -134,7 +134,7 @@ width: 20%; } -.kb table .th-scope { +.kb table .th-source { width: 10%; } @@ -142,11 +142,6 @@ width: 15%; } -.kb table .th-command { - width: 30%; -} - - .message-container { align-items: center; display: flex;