Add support for customizable keyboard shortcuts #1938
Conversation
Added logic for checking whether user is typing something. This unblocks shortcut possibilities that earlier clashed with text entry, thus a lot of keytype-specific restrictions have been removed.
MUI's ListItem has slightly different HTML for normal items and items with secondary actions, so commands with and without shortcuts are rendered slightly different. Having both in story is better for testing.
newIDE/app/src/CommandPalette/CommandPalette/AutocompletePicker.js
Outdated
Show resolved
Hide resolved
| {areaWiseCommands[areaName].map(commandName => { | ||
| // Get default and user-set shortcuts | ||
| const userShortcut = props.userShortcutMap[commandName]; | ||
| const defaultShortcut = defaultShortcuts[commandName] || ''; | ||
| const shortcutString = userShortcut || defaultShortcut; | ||
| const shortcutDisplayName = getShortcutDisplayName( | ||
| shortcutString | ||
| ); | ||
| // Check if shortcut clashes with another command | ||
| const clashingCommandName = Object.keys(commandsList) | ||
| .filter(commandName => !commandsList[commandName].noShortcut) | ||
| .find(otherCommandName => { | ||
| if (otherCommandName === commandName) return false; | ||
| const otherShortcut = | ||
| props.userShortcutMap[otherCommandName] || | ||
| defaultShortcuts[otherCommandName] || | ||
| ''; | ||
| if (shortcutString !== otherShortcut) return false; | ||
| if (shortcutString === '') return false; |
There was a problem hiding this comment.
In this logic, it seems that the complexity is more important than what it could be.
You iterate on all commands (well, on all areas, then on each commands of this area, so at the end it's on all commands). For each command, you get its shortcutString (so far so good).
Then inside this loop, you're iterating again on the commandsList, computing for each one its shortcutString.
At the end, this means that you're roughly doing n^2 the work of computing the shortcut strings and comparing them. When we'll have 200 commands, this means 40000 operations, which start to make a lot for some low end or mobile devices :)
Instead, consider the approach of using an object as a map of used shortcutString. In pseudo code:
const shortcutStringToCommands = {};
for each command:
// Get userShortcut and defaultShortcut as before
const shortcutString = userShortcut || defaultShortcut
shortcutStringToCommands[shortcutString] = (shortcutStringToCommands[shortcutString] || []).concat(command);
This is doing a single loop on commands, so complexity is n. Better, you can put this in a separate function outside of the render.
Then in the render, you iterate on commands again, and for each you look into shortcutStringToCommands, with the shortcutString of the command. If the array contains just your command, you're fine. If it contains something else, you have one (or more!) clashing commands :)
At the end, this is done in roughly 2*n operations (2 separate loops), so even with 200 commands, we'll have 400 operations to do - it should be more than fast enough even for the future and low end devices :)
|
Added a few comments but seems we're on a good path! |
4ian
added
the
🏁PR almost ready: final fixes
nilaymaj
force-pushed
the
kbd-shortcuts
branch
from
f927389
to
92856d9
Compare
|
I've added a
Is this what you had in mind? |
|
Yeah this looks good to me! 👍
…On Tue, Aug 25, 2020 at 3:01 PM Nilay Majorwar ***@***.***> wrote:
I've added a DismissableAlertMessage for the command palette hint in the
shortcuts list (which came along with a flag in "Hints and Explanations"
tab in preferences). Looks like this:
[image: Screenshot_20200825_192647]
<https://user-images.githubusercontent.com/18032938/91183400-0d2d0e80-e709-11ea-95c6-06b59ea750c0.png>
Is this what you had in mind?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#1938 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJYRAW77OJVB62KG24OHVLSCO7ZZANCNFSM4QJLLOTQ>
.
|
|
This looks good to me, merging now!🎉 Very nice work, the code is of great quality! As the last time, I appreciate your attention to details when working on this 👏 |
This PR adds support for customizable keyboard shortcuts to both the web and desktop app. Every shortcut is linked to a corresponding command, and the default shortcuts can be overridden by the user in a new "Keyboard Shortcuts" tab in preferences dialog.