brtnshrdr / angular2-hotkeys

Keyboard shortcuts for Angular 2 apps
MIT License
203 stars 95 forks source link

pause() / unpause() skips Hotkeys #101

Closed SublimePotato closed 1 year ago

SublimePotato commented 5 years ago

https://github.com/brtnshrdr/angular2-hotkeys/blob/29fb4938158c30c9a17d0c5b281978d6a15892c7/src/hotkeys.service.ts#L132

Problem is that you modify the array while looping over it which results in skipping items since the index changes. To fix this you could simply create a shallow copy of the array.

Here's my local fix for that:

import { HotkeysService } from 'angular2-hotkeys';

export class HotkeyUtil {
  static pauseAll(hs: HotkeysService) {
    // shallow copy because the native pause() is bugged
    const copy = hs.hotkeys.slice(0);
    for (const hk of copy) {
      // exclude cheat-sheet
      if (hk.combo !== '?') {
        hs.pause(hk);
      }
    }
  }
  static unpauseAll(hs: HotkeysService) {
    // shallow copy because the native unpause() is bugged
    const copy = hs.pausedHotkeys.slice(0);
    for (const hk of copy) {
      hs.unpause(hk);
    }
  }
}
wittlock commented 5 years ago

Thank you for the bug report and possible solution!

motabass commented 4 years ago

@SublimePotato Thanks for the workaround.

Are There any Plans for fixing this?

SamWolfs commented 4 years ago

@wittlock I've had the somewhat ingenious idea of editing the code as follows:

if (Array.isArray(hotkey)) {
    const temp: Hotkey[] = [];
    for (const key of hotkey.slice()) {
        temp.push(this.unpause(key) as Hotkey);
   }
   return temp;
}

Note the hotkey.slice() in the for-loop, which should ensure that the splice happening on pausedHotkeys won't be modifying the looping array.

I've tested this in my current project and it doesn't seem to impact current functionality, while fixing the skipping issue; if you deem the solution appropriate, I can make a pull request.

SamWolfs commented 4 years ago

@brtnshrdr Could I have your input on the comment above? Thanks!

Coffee-Tea commented 1 year ago

Should be fixed in v13.4.0. Would be great if anyone could confirm that it's working for you.

cc @SamWolfs @SublimePotato