-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[keybinding] Align 'Open Preferences' and 'Save As' keybindings with VS Code on Mac OS #6620
Conversation
950473d
to
f25c081
Compare
Signed off and added |
035b20b
to
58d3789
Compare
…n Mac OS (except in Firefox) Signed-off-by: Jan Keromnes <jan.keromnes@typefox.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Jan Keromnes <jan.keromnes@typefox.io>
58d3789
to
925d3b3
Compare
Thanks a lot @svenefftinge! I also threw in |
All green! @svenefftinge or @akosyakov, please merge this PR for me, as I'm not yet a committer. |
command: CommonCommands.OPEN_PREFERENCES.id, | ||
keybinding: 'cmd+,' | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be else
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intended: E.g. on Mac OS, in Chrome or Electron, we'll register both cmd+,
and ctrl+,
:
cmd+,
is the new default keybinding, whilectrl+,
is there for people who already got used to typingctrl+,
for Preferences on Mac OS
Side note: In the File
> Settings
> Open Preferences
menu entry, only the first keybinding shows up. (I double-checked that both keybindings work as expected, but that only the first is shown in the menu.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: this change will disable the default Chrome binding for opening Chrome preferences on macOS. Was it on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in order to follow VS Code's convention, we'd like to replace the behavior of cmd+,
in browsers. (This only works in Chrome though -- Firefox will continue to open browser preferences, which is why we set a different default keybinding there).
Would you be surprised that hitting cmd+,
in Theia in Chrome now opens the Theia preferences and no longer the Chrome preferences?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we follow VS Code's convention in electron only and not disable/override the browser bindings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. My understanding was that we try to follow VS Code's convention everywhere, but that in some browsers, it's sometimes impossible to override browser bindings (e.g. it's impossible to override cmd+,
in Firefox) in which case we fall back on alternative bindings, but I may be wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we try to follow VS Code's convention everywhere
I thought, no. My understanding was the following; we mimic the VS Code keymap in electron and leave the rest as is. For instance, why would we ever distinguish between macOS and other platforms when using Gitpod in Chrome.
CC: @svenefftinge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keybindings are part of a user's muscle memory. So we want to meet expectations as much as possible. I.e. the only reason to diverge from the default keybindings from VS Code (resp. keymaps like emacs or vim) is that browsers hinder us from handling certain keybindings.
Since CMD+, works in Chrome it should be used.
Fixes #6606.
What it does
In VS Code, the default keybinding to open Settings is:
In Theia, it is currently Ctrl + , everywhere. While this works everywhere, it's perceived as surprising on Mac OS.
Thus, on Mac OS we want the default keybinding to be Cmd + ,. However:
How to test
File
>Settings
>Open Preferences
should show⌘,
as the default keybinding^,
as the default keybindingReview checklist
Reminder for reviewers