Skip to content

ATL-1118: Add keymaps customization support #231

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

Merged
merged 1 commit into from
Mar 26, 2021

Conversation

fstasi
Copy link
Contributor

@fstasi fstasi commented Mar 17, 2021

The default keybindings may be reasonable for beginners, but PRO users expect the keybindings to be customizable, as they are in other editors.

In this PR:

  • add the theia/keymap packages that let users change the default keybindings to their preference (see image below)

image

  • refactor the settings menu: from a single entry it's now submenu containing both the settings and the keymap actions to open the relevant functionalities (see images below)

image

(window version)
image

@fstasi fstasi requested a review from kittaakos March 17, 2021 09:50
@fstasi fstasi force-pushed the atl-1118--customize-keybindings branch 2 times, most recently from 0fd31ea to ecff615 Compare March 18, 2021 14:44
@fstasi fstasi marked this pull request as ready for review March 18, 2021 15:28
@fstasi fstasi changed the title Add keymaps customization support ATL-1118: Add keymaps customization support Mar 18, 2021
@fstasi fstasi force-pushed the atl-1118--customize-keybindings branch from ecff615 to 35ee1dd Compare March 19, 2021 09:44
@kittaakos
Copy link
Contributor

I wonder if the Settings name is appropriate here when the window title shows Preferences
Screen Shot 2021-03-19 at 15 52 22

and it is Preferences in the Java IDE:

Screen Shot 2021-03-19 at 15 53 22
Screen Shot 2021-03-19 at 15 53 31

What is the current policy? In the past, keeping the Java IDE UI/UX was a rule of thumb? Thanks!

Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

It works nicely and the code looks good 👍

Please wait for another verification before the merge. Thanks!

@fstasi fstasi force-pushed the atl-1118--customize-keybindings branch from 35ee1dd to 6de67b5 Compare March 23, 2021 11:24
@fstasi
Copy link
Contributor Author

fstasi commented Mar 23, 2021

It works nicely and the code looks good 👍

Please wait for another verification before the merge. Thanks!

adding @ubidefeo for a double check before merging

Copy link

@ubidefeo ubidefeo left a comment

Choose a reason for hiding this comment

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

I have only one problem with this, and it's the placement of the key-bindings as a sub-menu of "preferences".
Historically the IDE preferences menu item served as an item, not a container of sub-options.
When a new user reads "go to ... > preferences" they won't know wether settings or key-bindings is the place to go.
@giannicolab @per1234 what's your take on this?

@per1234
Copy link
Contributor

per1234 commented Mar 24, 2021

My concern with the new menu structure is that it breaks all the documentation/tutorials/etc. that tell people to open File > Preferences.

My preference would be to have the keyboard shortcut editor accessed via the File > Preferences dialog. But I'm guessing this is simply not technically feasible. So I'm thinking the best approach is to leave the File > Preferences menu path alone and add a new menu item for the keyboard shortcuts.

I think it's really nice to have configurable keyboard shortcuts. This is a feature I always look for in the applications I use.

@fstasi fstasi force-pushed the atl-1118--customize-keybindings branch from 6de67b5 to f366405 Compare March 24, 2021 14:20
@fstasi
Copy link
Contributor Author

fstasi commented Mar 24, 2021

@per1234 @giannicolab @ubidefeo updated as below
image

@fstasi fstasi force-pushed the atl-1118--customize-keybindings branch from f366405 to 033fad0 Compare March 24, 2021 15:09
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

Thanks Francesco!

@fstasi fstasi merged commit 562b77a into main Mar 26, 2021
@fstasi fstasi deleted the atl-1118--customize-keybindings branch March 26, 2021 14:35
@per1234 per1234 added topic: code Related to content of the project itself type: enhancement Proposed improvement labels Oct 28, 2021
@per1234 per1234 mentioned this pull request Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants