"ESC" for closing cheat sheet is not working when there is "ESC" key specified for the app

[advorkina]

Configure hotkey module in a following way: HotkeyModule.forRoot({cheatSheetCloseEsc: true}) and cheat sheet can be closed with the "ESC" and the description is visible:

When "ESC" key is being added via hotkeysService.add or directive (to work in the web app) cheat sheet "ESC" is not working anymore even though the description is still visible. I suppose the same thing will be with the "SHIFT + ?" combo.

[advorkina]

After a little investigation I've discovered following problems:

1. this.mousetrap.bind always overwrites callbacks
2. hotkeysService.add removes old hotkey from the array (but anyway even without removing the solution won't work because of the moustrap)

I'd love to fix this problem and make a pull request but would be nice to discuss before I do it. Anyway I did a prototype which works for me but I haven't tested everything. Can you, please, tell me if this relatively big refactoring is fine or what do you think about it :) https://github.com/advorkina/angular2-hotkeys/blob/feature/esc-fix-for-cheatsheet/src/hotkeys.service.ts

The main difference in the refactoring is that there is hotkeysPerCombo dictionary in hotkeys.service.ts instead of hotkeys array.

Btw this refactoring may be useful here https://github.com/brtnshrdr/angular2-hotkeys/issues/68.

Thanks for your help!

[wittlock]

This is an interesting special case I haven't seen before or really considered. I've only glanced briefly at your code sample as I've just arrived back home from some travels. My initial thought is that it seems like a fairly specific special case to want to use the hotkeys for two different things that I'm not sure if it's worth building into the project.

Instead, in your case, I would probably suggest to disable ESC as a hotkey for closing the cheat sheet through the IHotkeyOptions and instead of having two different callbacks for the ESC key inside the hotkey.service just extend the parts you need with also checking if cheat sheet is being shown (the CheatSheet component has a helpVisible boolean, might need exposing through the service though to make this feasible) and then close it.

This is just a quick thought off the top of my head. Do you think that sounds like a reasonable approach though? I'd prefer not to introduce another layer in the service to solve, what I at least imagine to be, a somewhat small corner case, if there is a reasonable other approach that still solves the problem.

The bit where you mention the description is still shown despite not working anymore though seems like a legitimate bug worth looking into.

[advorkina]

Hi! Thanks for your detailed reply, hope your travels went well. I was thinking that my change was kinda a big refactoring for this tiny requirement. As you see I've made hotkey groups so that one hotkey could have multiple callbacks. Later I thought after seeing another bug (about hotkeys working while the cheat sheet was open) that inside of the callback you may specify if the callback is from the app or from the cheat sheet. But maybe its overcomplication. Let me see what I can do with your suggested solution and come back. :)

[advorkina]

Hey, so I think I've found solution for this problem without any changes in the library. Basically inside of our wrapper service I'm calling this.hotkeysService.cheatSheetToggle.next(false) when esc event is triggered inside of the wrapper before passing it to the app.

  public hotkeyCallback(ev: KeyboardEvent, combo: string, commands: CommandDetail[]): boolean {
    if (this._isCheatSheetVisible && combo === this.escKey) {
      this.hotkeysService.cheatSheetToggle.next(false);
    }
    // ...call app callbacks

But don't you think other people might have the same problem as me? Cause I don't really see that as corner case cause ESC is a pretty used key. What is your plan with options and visible ESC description? Maybe I can help implementing that. Thanks for you help!

[wittlock]

I do agree that ESC is a commonly used key. But considering it's disabled for closing the cheatsheet by default I also feel like it might not be too common to use it for both that and something else in your application. I might be wrong but I haven't really got the impression there's a huge demand for being able to bind one hotkey to multiple things.

Overall I'd like my best to keep complexity of the module down and adding multiple callbacks per hotkey feels like complexity that could be avoided and fairly easily worked around for the cases where it's needed while maintaining the simplicity of the module.

And I think the solution you've found is quite good looking and a less complex approach than extending the module.