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

Sequences are broken if part of them are unbound #273

Open
philfreo opened this issue May 29, 2015 · 7 comments
Open

Sequences are broken if part of them are unbound #273

philfreo opened this issue May 29, 2015 · 7 comments

Comments

@philfreo
Copy link

Take a look at this and follow the instructions on screen to see what I think is a bug
https://jsfiddle.net/9kov1p60/

Basically, if you have two different shortcuts, g r and r, I would expect r to only fire if it wasn't immediately preceded by a g... but even more important is that unbinding r alone should not break the sequence g r, but apparently it is.

@ccampbell
Copy link
Owner

Thanks for this... It is definitely strange behavior, but I think part of it has to do with dynamically adding and removing bindings. If you take a look at this fiddle

https://jsfiddle.net/9kov1p60/1/

You will see the case of g r and r works as expected. What I think is happening is the sequence for g r also binds a callback to the r key that I think adding the later r event is overwriting and preventing the sequence from firing at all (not sure why though).

It's strange though cause it works even if you do this:

https://jsfiddle.net/9kov1p60/2/

I will look into it a little further... but I suspect what is actually happening is as soon as you press g i and bind the new r binding, it breaks the g r sequence and it never fires at all.

What you can do in the mean time is always bind the r key up front and just set a flag in the g i binding like var inbox = true; then in the callback for r you can check the flag before doing what you want to do.

@philfreo
Copy link
Author

philfreo commented Jun 1, 2015

What you can do in the mean time is always bind the r key up front

Hey, thanks for the workaround but as you can imagine this is only a simplified part of a much bigger application, where a workaround like this (having all possible keyboard shortcuts bound at all times) could become pretty unmanageable

but I suspect what is actually happening is as soon as you press g i and bind the new r binding, it breaks the g r sequence and it never fires at all.

yeah, that does seem to be the case

hopefully this can be fixed :)

@nivekmai
Copy link

I've experienced this as well, and have abstracted mousetrap into a "scope" system. I scope sets of kb commands in to sub objects:
{ views: [ {combo: 'g v', func: doThings()}, {combo: 'g a', func: startStuff()} ], actions: [ {combo: 'k l', func: runCode()} ] }

then when I call my "mount" or "unmount", I simply mousetrap.reset() and run through the array and mousetrap.bind() one at a time. This allows the workaround suggested to be implemented in a pretty scalable way (since you can define your kb commands in a specific order, but also have them scoped to different parts of your application). It's definitely not ideal, I wish that unbind() would just work, but this works until it gets fixed.

--edit:
Just took a peak at the code, this would totally be happening as unbind() simply bind()s to an undefined function:

  1. bind 'g e'
  2. bind 'e'

things go wonky when hitting 'e'

  1. bind 'e'
  2. unbind 'e'
  3. bind 'g e'

basically the same situation as the first, even though 'e' and 'g e' are never 'bound' at the same time.

@smsloggett
Copy link

Hi there, we're using Mousetrap latest on a project and are experiencing this exact issue. Has there been any progress on resolving the issue? Any help we can give? We've tried some debugging of the sequence matching but haven't found the root cause yet.

@kafeltz
Copy link

kafeltz commented Apr 5, 2016

The issue is strange because adding new binding inside a binder is so weird that the solution should be reviewed.

@smsloggett
Copy link

We note there is a pull request for a fix: #319

@sebastienbarre
Copy link

The PR works and it would be great if it was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants