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.

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

Add keybindings with numpad keys for Zoom commands and handle multiple keybindings per command #11363

Merged
merged 1 commit into from
Jul 13, 2022

Conversation

AlexandraBuzila
Copy link
Contributor

What it does

How to test

Zoom In and Out should work with the + and - keys from the numpad.

To test the handling of multiple keybindings for one command:

  1. Open the Keyboard Shortcuts editor and keymaps.json
  2. Search for Zoom In command in the editor -> two entries should show
  3. Modify one of the entries and change the keybinding in the editor -> two new entries should be visible in keymaps.json, one for the new keybinding and one that disables the default one
  4. Save keymaps.json to update the UI
  5. Modify the second (default) entry for the Zoom In command in the editor -> two new entries should be visible in keymaps.json, one for the new keybinding and one that disables the default one
  6. Save keymaps.json to update the UI
  7. Modify the second (default) entry again in the editor -> one entry should update in the keymaps.json (the one that was added at step 5)
  8. Save keymaps.json to update the UI
  9. Reset one of the Zoom In commands in the editor -> both commands should return to their default values and the command should be removed completely from keymaps.json

Review checklist

Reminder for reviewers

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@AlexandraBuzila do you mind taking a look at the failing tests?

@vince-fugnitto vince-fugnitto added the keybindings issues related to keybindings label Jun 30, 2022
- display all available keybindings for a command in the Keyboard
Shortcuts dialog and handle multiple keybindings in the keymaps.json
(eclipse-theia#11240)
- add keybindings with the + and - keys from the numpad for the Zoom In
and Zoom Out commands, in the electron app (Fixes eclipse-theia#11239)

Signed-off-by: Alexandra Buzila <[email protected]>
@AlexandraBuzila
Copy link
Contributor Author

I rebased the branch and updated the failing test.

@msujew msujew self-requested a review July 8, 2022 13:49
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I confirmed that the changes work well for me 👍

There is a difference between our behavior and that of vscode but I prefer ours. Basically I noticed in VS Code that if you modify each entry for a command that has multiple keybindings vscode will only have one active at a time.

I did notice some issues when performing updates, and doing a revert (where the whole file is deleted) so I opened #11399 which should fix it.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

The changes look good to me as well 👍

  • The view displays all keybindings when multiple keybindings exist
  • Changing these keybindings is correctly applied (after saving the keymaps.json)
  • Reseting a keybinding resets all of the keybindings for the same command

I originally though I've noticed an issue when swapping two keybindings for a command (one disappears), but vscode behaves in the same way, so at least it is consistent. I also believe there's no better way dealing with this.

@JonasHelming
Copy link
Contributor

@vince-fugnitto : Are you fin with merging this?

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@msujew msujew merged commit 32acfda into eclipse-theia:master Jul 13, 2022
@AlexandraBuzila AlexandraBuzila deleted the keybindings branch July 14, 2022 07:11
@vince-fugnitto vince-fugnitto added this to the 1.28.0 milestone Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keybindings issues related to keybindings
Projects
None yet
4 participants