Added a way to unregister keybindings for a command #8269
Conversation
|
This is based on the comment: #8209 (comment) |
|
This is a duplicate of #8257 but with unit tests |
|
No clue whats wron with the build, but I have the same failure on latest master |
| keybindingRegistry.unregisterKeybinding(TEST_COMMAND2); | ||
|
|
||
| const bindings = keybindingRegistry.getKeybindingsForCommand(TEST_COMMAND2.id); | ||
| if (bindings) { |
There was a problem hiding this comment.
Why not expect? Same the other if.
There was a problem hiding this comment.
Not really the thing we test here, but I will adapt that.
|
|
||
| const bindings = keybindingRegistry.getKeybindingsForCommand(TEST_COMMAND2.id); | ||
| if (bindings) { | ||
| expect(bindings.length).to.be.equal(0); |
There was a problem hiding this comment.
Let's add an expect before unregister.
There was a problem hiding this comment.
Good point! I was expecting that there are test cases that validate the basic registration, but apparently there are none. I adapted that.
| if (bindings) { | ||
| bindings.forEach( | ||
| (value: Keybinding) => { | ||
| expect(value.command === keybinding.command && value.keybinding === keybinding.keybinding).to.be.false; |
There was a problem hiding this comment.
value.keybinding === keybinding.keybinding => #8170
There was a problem hiding this comment.
Let us fix that bug separatly, I can prepare a contribution
|
|
||
| keybindingRegistry.registerKeybinding(keybinding); | ||
| keybindingRegistry.registerKeybinding(keybinding2); | ||
|
|
There was a problem hiding this comment.
I think an expect is missing here. These tests also pass if register/unregister does not work at all.
|
fixes: eclipse-theia#8209 Signed-off-by: Jonas Helming <jhelming@eclipsesource.com>
|
I resolved the conflicts |
| @@ -184,28 +184,36 @@ export class KeybindingRegistry { | |||
| } | |||
|
|
|||
| /** | |||
| * Unregister keybinding from the registry | |||
| * Unregister keybindings from the registry using the key of the given keybinging | |||
There was a problem hiding this comment.
There is a typo here: keybinging.
There was a problem hiding this comment.
Thank you for taking care of this PR, @JonasHelming. Updated jsdocs, tests, convenient API 👍
fixes: #8209
Signed-off-by: Jonas Helming jhelming@eclipsesource.com
What it does
It adds the capability to remove all keybindings for a specific command. also adds docu to the three available functions.
How to test
see included unit tests or unregister the keybindings of any command via API
Review checklist
Reminder for reviewers