Don't replay events when all matches have only keyup suffixes

oggy commented 7 years ago

I believe this fixes #190, in which I found that binding a command to alt-[ caused an errant "alt-character" to get inserted (MacOS + U.S keyboard layout) if alt is held down for a second.

The problem is that when alt-[ is pressed, the timeout is triggered, with the alt-[ left on the list of queued keys. When the timeout fires, the keys are replayed with the binding added to @bindingsToDisable, which is why the U+201C gets inserted. (Or if you have a 2nd command bound to alt-[ it'll fire that instead). This problem is not specific to the alt key, but is most noticeable due to all the built-in Mac OS alt- key combos serving as shadowed bindings.

cc @iolsen

Alternate Designs

I strove to keep this change as small as possible in the interest of fixing my immediate problem sooner.

However, I do find the complexity of the key handling algorithm uncomfortably complex, and I think it's largely due to the keyup semantics. I'm wondering: how would you feel about simplifying it so that instead of thinking of the keyup (^foo) as an event in the binding sequence, it's more like an annotation that just means "... and wait until foo is released." Which I'm hoping is sufficient for all real use cases.

More formally I'd propose:

keyups can only appear at the end of a binding (good: "alt-a ^alt", bad: "alt-a ^alt a")
keyups can only apply to modifier keys (bad: "alt-a ^alt-a")
there can only be one keyup (bad: "alt-shift-a ^alt ^shift") [alternatively we could just convert this to "alt-shift-a ^alt-shift"]
keyups can only specify the release of keys that were pressed earlier in the binding (bad: "alt-a ^ctrl")
such bindings will be queued for execution when the keydowns are performed when the keyup events occur

This would let us completely divorce the keyup handling from the keydown. i.e.:

on key down:

immediately fire the first exact match (with no keyups)
for each possible keyup suffix:
  Add the first such binding to a separate list of pendingKeyup bindings

on key up:

find bindings in pendingKeyup pending this key release and fire them.
(If there are multiple modifiers pending release, like `^alt-shift`, all need to be released.)

The last bullet above also removes some ugliness in the case where you want the keydown to give focus to another element until the keyup fires. I've run into this making the tab-switcher plugin, where one often wants to override the built-in ctrl-tab keys, but the keydown gives focus to separate "tab list" element. To achieve the right result you need:

"atom-workspace":
  "ctrl-tab": "tab-switcher:next"
  "ctrl-tab ^ctrl": "unset!"

"ol.tab-switcher-tab-list":
  "^ctrl": "tab-switcher:select"

This is a little confusing & ugly -- more intuitive to me would be:

"atom-workspace":
  "ctrl-tab": "tab-switcher:next"
  "ctrl-tab ^ctrl": "tab-switcher:select"

Thoughts about this idea? Happy to have a go at implementing it, but wanted to get your feedback first.

Benefits

Fixes #190.

Possible Drawbacks

None.

Applicable Issues

None.

iolsen commented 7 years ago

First, thanks for diving into this complex issue. Unfortunately I'm seeing the same behavior you describe in #190 even using this code. I will keep digging because I'd like to better understand what's going on here but I likely won't get to it until next week. It's almost certainly a weird interaction with the recent changes to better support international keyboards.

However, I do find the complexity of the key handling algorithm uncomfortably complex, and I think it's largely due to the keyup semantics.

No disagreement here! Unfortunately this same complexity makes it a significant effort and high risk to rewrite.

I'm wondering: how would you feel about simplifying it so that instead of thinking of the keyup (^foo) as an event in the binding sequence, it's more like an annotation that just means "... and wait until foo is released." Which I'm hoping is sufficient for all real use cases.

I think I don't understand this. The only keyup binding that ships in a default install of Atom now is for MRU tab traversal:

  'ctrl-tab': 'pane:show-next-recently-used-item'
  'ctrl-tab ^ctrl': 'pane:move-active-item-to-top-of-stack'
  'ctrl-shift-tab': 'pane:show-previous-recently-used-item'
  'ctrl-shift-tab ^ctrl': 'pane:move-active-item-to-top-of-stack'

So when a user presses ctrl-tab we need to immediately dispatch that command and wait until they release ctrl before dispatching the command that updates the MRU stack. What do you mean by "...and wait until foo is released?"

keyups can only appear at the end of a binding (good: "alt-a ^alt", bad: "alt-a ^alt a") keyups can only apply to modifier keys (bad: "alt-a ^alt-a") there can only be one keyup (bad: "alt-shift-a ^alt ^shift") [alternatively we could just convert this to "alt-shift-a ^alt-shift"] keyups can only specify the release of keys that were pressed earlier in the binding (bad: "alt-a ^ctrl")

I actually experimented with a set of validations very much like this one recently. In the end it was a ton of complexity for dubious gain: it's possible to add all kinds of keybindings that can lead to bizarre behavior within the app. There's not a clear benefit to attempting to prevent this particular subset.

on key down:

immediately fire the first exact match (with no keyups)
for each possible keyup suffix:
  Add the first such binding to a separate list of pendingKeyup bindings

on key up:

find bindings in pendingKeyup pending this key release and fire them.
(If there are multiple modifiers pending release, like `^alt-shift`, all need to be released.)

I believe this is actually an accurate description of the current behavior. Pending keyup matches get stashed in pendingKeyupMatcher. When there's a keyup event it updates its state and returns matches.

I'll stop here for now and see if we can come to a better common understanding via a couple more comment roundtrips. :) Again, I do really appreciate your taking the time to dive into some super gnarly code. More from me next week.

oggy commented 7 years ago

My mistake; the example in #190 failed for me too -- I was testing that the tab-switcher example worked (where the focus moves to another element on keydown). I've added another commit which fixes both.

What do you mean by "...and wait until foo is released?"

The distinction I was trying to make between the current & proposed behavior is we just wait until foo is released, as opposed to treating "^foo" as a full-fledged keystroke that could be surrounded by other keyup and keydown events in the binding. It sounds like you would agree that this is sufficient.

I believe this is actually an accurate description of the current behavior. Pending keyup matches get stashed in pendingKeyupMatcher. When there's a keyup event it updates its state and returns matches.

Thanks for the pointers; I had indeed overlooked the PartialKeyupMatcher bit, so it's a lot closer than I thought. However, I still find it unnecessarily complex that the keyup handling code includes all of the keydown handling, so e.g. the keyups can wind up on the queued keystrokes list, which means they're subject to replaying (which led to the bug(s) here).

What I'm thinking is that for a binding like "alt-a ^alt", everything could happen when alt-a is pressed except the actual command dispatch and emit() call, which is delayed until alt is released. And everything except those two things could be skipped for the keyup event. This would mean the keystrokes are removed from the queued keystrokes on keydown too.

I actually experimented with a set of validations very much like this one recently. In the end it was a ton of complexity for dubious gain: it's possible to add all kinds of keybindings that can lead to bizarre behavior within the app. There's not a clear benefit to attempting to prevent this particular subset.

I can see adding validations as adding complexity, but all I'm proposing is that scenarios outside these cases be considered undefined behavior -- that is, we shouldn't worry about changing the behavior for these unlikely cases in order to simplify things internally. (I actually would favor showing a warning if bindings include anything considered UB, but agree there is a complexity tradeoff.)

iolsen commented 7 years ago

There is a legitimate bug here but since we agree this PR doesn't resolve it, I'm going to close it.

oggy commented 7 years ago

@iolsen I believe this PR does solve the current problems. (First paragraph of my last comment: "I've added another commit which fixes both.")

Any thoughts on this fix?

iolsen commented 7 years ago

:man_facepalming: sorry I missed that. I'll take a look!

oggy commented 7 years ago

@iolsen did you get a chance to take a look at this?

iolsen commented 7 years ago

@oggy so sorry for the delay. I haven't looked at this yet but should have time in the next few weeks.

oggy commented 7 years ago

This problem is no longer happening as of 1.19 or 1.21-dev.

atom / atom-keymap