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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 25, 2019
Merged

[keybinding] Align 'Open Preferences' and 'Save As' keybindings with VS Code on Mac OS #6620

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9591f75
[keybinding] Make '⌘,' the default keybinding to 'Open Preferences' o…
jankeromnes Nov 25, 2019
925d3b3
[keybinding] Align 'Save As' keybinding with VS Code on Mac OS
jankeromnes Nov 25, 2019
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

- [core] Switched the frontend application's shutdown hook from `window.unload` to `window.beforeunload`. [#6530](https://github.com/eclipse-theia/theia/issues/6530)
- [core] label providers can now notify that element labels and icons may have changed and should be refreshed [#5884](https://github.com/theia-ide/theia/pull/5884)
- [core] aligned 'Open Preferences' and 'Save As' keybindings with VS Code on Mac OS [#6620](https://github.com/eclipse-theia/theia/pull/6620)
- [scm] added handling when opening diff-editors to respect preference `workbench.list.openMode` [#6481](https://github.com/eclipse-theia/theia/pull/6481)

Breaking changes:
Expand Down
8 changes: 8 additions & 0 deletions packages/preferences/src/browser/preferences-contribution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import { FileSystem } from '@theia/filesystem/lib/common';
import { UserStorageService } from '@theia/userstorage/lib/browser';
import { PreferencesContainer } from './preferences-tree-widget';
import { EditorManager } from '@theia/editor/lib/browser';
import { isFirefox } from '@theia/core/lib/browser';
import { isOSX } from '@theia/core/lib/common/os';

@injectable()
export class PreferencesContribution extends AbstractViewContribution<PreferencesContainer> {
Expand Down Expand Up @@ -61,6 +63,12 @@ export class PreferencesContribution extends AbstractViewContribution<Preference
}

registerKeybindings(keybindings: KeybindingRegistry): void {
if (isOSX && !isFirefox) {
keybindings.registerKeybinding({
command: CommonCommands.OPEN_PREFERENCES.id,
keybinding: 'cmd+,'
});
}
Copy link
Member

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?

Copy link
Member Author

@jankeromnes jankeromnes Nov 25, 2019 •
edited
Loading

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, while
  • ctrl+, is there for people who already got used to typing ctrl+, 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.)

Copy link
Member

Choose a reason for hiding this comment

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

ok

Copy link
Contributor

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?

Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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

Copy link
Contributor

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.

keybindings.registerKeybinding({
command: CommonCommands.OPEN_PREFERENCES.id,
keybinding: 'ctrl+,'
Expand Down
2 changes: 1 addition & 1 deletion packages/workspace/src/browser/workspace-frontend-contribution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ export class WorkspaceFrontendContribution implements CommandContribution, Keybi
});
keybindings.registerKeybinding({
command: WorkspaceCommands.SAVE_AS.id,
keybinding: 'ctrl+shift+s',
keybinding: 'ctrlcmd+shift+s',
jankeromnes marked this conversation as resolved.
Show resolved Hide resolved
});
}

Expand Down