Discussion thread for shortcuts

[nilaymaj]

Created a new PR and branch for work on keyboard shortcuts, because the commit history on the original PR got a bit bloated up (and because a new PR is cleaner anyway). Haven't done any significant work on shortcuts yet though - something came up that I never knew would take two full days to finish 😅. I've just pulled the dialog open logic into a separate file (UI/OpenedDialogChecker.js).

So what I'm planning to do is to:

• Alter the command palette UI implementation to use ref and useImperativeHandle.
• Write a function that converts a keyboard event object into a string like Ctrl+Shift+P (or null, if the keypress does not correspond to a proper keyboard shortcut).
• Set up the keyboard listener. It listens for key presses, and if no dialog is open, uses above function to convert event object into shortcut string, gets the command associated with this string from the mapping and hands it over to the command palette.

... and then finally check how well it works out!

[nilaymaj]

@4ian I've added a very minimal implementation of the shortcut system - it's basically intended as a proof of concept or a prototype instead of actual code, so there are a few Flow errors too that I've ignored for now.

• Converted the command palette implementation to a ref-based one, and it works perfectly.
• For a prototype, I've added code in src/CommandPalette/Shortcuts.js. It's just
  • a function for converting event to shortcut string,
  • a mapping between shortcut string and command name,
  • and a custom hook that listens for shortcuts and passes corresponding command name to the palette via callback

... and it's working. Ctrl+Shift+O properly opens the command palette with "Edit object..." options open, and "F5" directly runs the preview as expected. Commands only run when they're registered.
So at least we know that this Mainframe-in-the-middle system will work! 😀

Something you might want to review: While shifting the palette implementation to ref-based approach, instead of adding another state for handling the whole dialog's open state (which would have made a total of 3 open states in the component), I instead went for a more enum-based state. So I've added something like this:

type PaletteMode = 'closed' | 'command' | 'option';

const CommandPalette = React.forwardRef<{||}, CommandPaletteInterface>(
  (props, ref) => {
    ... commandManager and useStyles ...

    const [mode, setMode] = React.useState<PaletteMode>('closed');
    const [
      selectedCommand,
      selectCommand,
    ] = React.useState<null | NamedCommandWithOptions>(null);

    ... rest of command palette ...
  }
)

...and then
to close palette, setMode('closed');
to normally open palette, setMode('command');
to open palette at options level, setMode('option') and selectCommand(command).

It looks simpler to use and less prone to bugs (to me, at least). But I haven't seen this pattern in the codebase yet, so I wonder if this is okay?

[nilaymaj]

@4ian We have a basic working version of keyboard shortcuts now! I've added a shortcut map field in the preferences, and added two supporting functions - one to reset this map to default value, and one to edit the shortcut of a particular command.

Then, I've added a basic UI version of the shortcut customization tab in preferences dialog (my screen-recording tool ran into some issues, so please make do with screenshots 😅 ):

All the functionality is working. The UI is not complete yet - I've yet to add the following:

• Confirmation prompt for "Reset all shortcuts to default" button
• Button for resetting a single shortcut to default
• Warning for overlapping shortcuts
• ... and cosmetic improvements

Another thing to be done is decide exactly what kinds of shortcuts we are going to allow. For example,

• We don't want single letter shortcuts like A or 2, since that clashes with text entry.
• But we do want function keys like F5 (preview, maybe?) to be valid shortcuts.
• We also don't want Shift+A since again that would clash with entering capitals in a text entry.
• We also need to prevent single +, - etc as shortcuts - that would clash with entering expressions.

To me, these points hint to something like keeping Ctrl as a compulsory key in shortcuts, unless the action key is a function row key. There are probably some more quirks about the current implementation that I haven't taken care of.

[nilaymaj]

@4ian I'm adding the shortcut-specific reset to default button. I tried using RaisedButton for the edit shortcut button, and it looks like this:

In my opinion, RaisedButton is taking a bit more space than I needed it to take - and combined with the filled variant, it seems too dominating. But then I guess a user would open this tab almost only when the user wants to change a shortcut so maybe this does makes sense. What do you think?

[4ian]

Well this will probably generate "too much" raised button, so forget about it and let's go back to what you had previously. Thanks anyway for testing :)

[nilaymaj]

Sorry to be out of touch for the last few days. My system got into some serious issues due to a bad kernel or driver update, and after trying to fix it (in vain), I had to uninstall the OS and hop over to another one - took some time to get things running. Also lost a bit of work that I kept in a git stash but forgot to carry over. This was a week almost wasted, basically :(

Some progress:

• I've added an optional noShortcut property for each command in the central commands list. Commands with noShortcut: true are not shown in the shortcuts dialog. This was necessary to keep already existing shortcuts separate from the new shortcut system. There are still other problems in this aspect, but I've added this at least for now.
• The shortcuts list now loops through the list of commands in the central commands list, filters out commands with shortcuts disabled (noShortcut: true) and lists the rest. Also, I've added logic for no shortcut defined for a command (the "None"s in gif below - I should probably change it to "No shortcut" or something, though). I'll add a button for removing the shortcut too.
• We have so many commands/shortcuts that the shortcuts list is so long it's a bit daunting 😅. Command categories (or "areas") would do well here to break the list of shortcuts in the UI into categories with headings like "General", "IDE", "Scene Editor", etc. We can also show these areas in the command palette: Scene: Edit object... - something like VSCode does for most commands.

I've added default shortcuts for some main commands from each "area". They're mostly random - I'll try exploring shortcuts in other apps to get some sensible defaults, then I'll put it up on the forum for suggestions as you mentioned.

[4ian]

Also lost a bit of work that I kept in a git stash but forgot to carry over. This was a week almost wasted, basically :(

Sorry to hear! That's always frustrating. A good reason for pushing regularly your changes to some branch your work in progress (you can do this on a private repo or a branch not assigned to a PR, just to keep these to you) - you never know when your computer will suddenly die 😬

I've added an optional noShortcut property for each command in the central commands list. Commands with noShortcut: true are not shown in the shortcuts dialog. This was necessary to keep already existing shortcuts separate from the new shortcut system.

Ok sounds good!

The shortcuts list now loops through the list of commands in the central commands list, filters out commands with shortcuts disabled (noShortcut: true) and lists the rest.

Sounds good too.

Command categories (or "areas") would do well here to break the list of shortcuts in the UI into categories with headings like "General", "IDE", "Scene Editor", etc. We can also show these areas in the command palette: Scene: Edit object... - something like VSCode does for most commands.

Yes I agree. I was originally thinking that the "Scene:" could be added by the ScopedCommandProvider, but actually this can "simply" be a area (or some other property name) in the list of commands. Then you group by area before iterating on commands to display shortcuts :)

I should probably change it to "No shortcut" or something, though

No shortcut is surely a bit more explicit :)

[nilaymaj]

I've added the shortcut clash logic and UI (will push a commit in a while). Here's a screenshot, which also shows the area-based categorisation of commands and shortcuts.

Something to note: the Chip I've used for displaying the shortcut doesn't seem to be very compatible with the Nord theme. For the default theme and dark theme, the Chip gets a suiting background color, but in the Nord theme it just shows up with the same background color as in the dark theme, which makes it look quite non-Nord. I'll try to fix it once the functionality-related stuff is done.

EDIT:
Just a short note on the clash detection: For each shortcut displayed, I loop through the user's shortcut map to find a clashing shortcut. On the profiler, the shortcut list takes 2.5 - 3.5ms to render. Not a problem, I hope? I can change it to a faster implementation if needed.

[nilaymaj]

@4ian I tried implementing the "handled by Electron" logic, and it works fine... except for one issue - apparently Electron's shortcut system does not care if the user is typing something while a shortcut is pressed. Fair on their side I think, but causes a bit of trouble for us. Consider the user sets shortcut for creating a new project as single key N. Since this shortcut would be handled by Electron, whenever the user presses "N" while typing something, the "n" is typed properly in the text box, but the create new project dialog also opens :(

I can see two options:

• Step back and remove support for single-key shortcuts: will work fine, but then we'll lose single-key shortcuts
• Pass palette onLaunch function to ElectronMainMenu, and add the "is user typing?" check somewhere in between: Right now, Electron menu items are currently connected directly to the handlers. We can instead pass onLaunchCommand from the palette to ElectronMainMenu, and use something like below to get handlers for Electron (only the overlapping parts):

const getElectronHandler = (onLaunchCommand, commandName) => {
  return () => {
    if (isUserTyping()) return;
    onLaunchCommand(commandName);
  }
}

The second option looks a bit hacky to me, though.

What should I do? Is there another option?

[4ian]

Nilay, as discussed let's take a look at:

• Having electron to handle commands that are in the main menu - we don't really have the choice here so even if a one letter shortcut is used, we should let it happen (though we should verify if it's actually supported by OSes! If not, we could force a modifier for these).
• For the default shortcut handling, let's have a "userShortcutMap" and a "defaultShortcutMap" (so that we can add default shortcuts without erasing the ones set by the user).
• Improve the shortcut dialog ("bonus", not a critical feature)

[nilaymaj]

Having electron to handle commands that are in the main menu

That's the current behaviour - the previous commit adds this functionality so that on the desktop app, handledByElectron-marked commands/shortcuts are handled by Electron instead. So no more changes required for this, I guess.

though we should verify if it's actually supported by OSes

Single-key shortcuts on both desktop and web app seem to be working pretty fine for me on Linux (except for the Electron issue, obviously). Will need some more testing though, yes.

For the default shortcut handling, let's have a "userShortcutMap" and a "defaultShortcutMap"

Yup, will do that today. Shouldn't take much time.

Improve the shortcut dialog ("bonus", not a critical feature)

I'll go for that once the above is done.

[nilaymaj]

Just pushed a commit that improves the UX of the detect shortcut dialog. Now it dynamically shows "Ctrl + ", "Ctrl + Shift +", etc as the user presses and releases keys. I haven't added "Not a valid shortcut" message - right now if the shortcut is invalid, only the "Set Shortcut" button gets disabled. I'll try adding the help message tomorrow.

[nilaymaj]

I just realised - is the "This shortcut is reserved by your browser" message in detect shortcut dialog really necessary? If we try to keep the default shortcuts avoiding browser-reserved shortcuts, users are anyway going to realise their custom shortcut is browser-reserved when they try pressing it in the detect shortcut dialog. This happened to me - I tried to set Ctrl+N (which is Chrome-reserved) as a shortcut for a command, and a new tab opened up from the detect shortcut dialog itself, reminding me that it's browser-reserved.

Adding a message is good for UX since some users may wonder why GDevelop "opened" a new tab, but not showing a message apparently does not add any big gaps either. Can we leave it as an improvement for later on?

[4ian]

Can we leave it as an improvement for later on?

Yes, it's indeed likely that the user will understand when pressing the shortcut that this is somehow superseded by the browser.

I think we're in a good state for going ahead with a PR to master branch? This will allow me to do a new review pass and have this part of the next release :)

[nilaymaj]

Okay, I've pulled upstream changes and resolved all conflicts. There are a couple of little issues to take care of before the PR:

[nilaymaj]

A final point before I make the PR - since opening the command palette is now governed by the shortcuts system, I think the "Enable command palette" preference flag is obsolete now. Is it fine to remove that flag from preferences now? If a user does want to "disable" the command palette, the user can simply remove the palette keyboard shortcut.

[4ian]

This preference flag is indeed obsolete! You can remove it from the preferences and from the dialog :)