Improve reliability of catching modifier key-up events #156

iolsen · 2016-10-17T02:04:58Z

For MRU tab switching, these key bindings were added:

'ctrl-tab ^ctrl': 'pane:move-active-item-to-top-of-stack'
'ctrl-shift-tab ^ctrl': 'pane:move-active-item-to-top-of-stack'

The ^ctrl is a key-up event on the ctrl key. Catching it is necessary to correctly push the current item to the top of the MRU stack. But we fail to catch the ctrl key-up in any of these cases:

The ctrl key is released beyond the pending partial match timeout of 1 second. If you release ctrl more than one second after last pressing tab, we don't catch it.
The ctrl key is released in concert with another modifier key, e.g. shift. Depending on the timing this could be treated like ^ctrl-shift, ^shift ^ctrl, or ^ctrl ^shift and only the last will successfully match the binding.
Typing any other keys between the key-down and key-up will result in the key-up not resolving to the bound command.

This PR addresses this bug as follows:

Persist pending keyup matches beyond the partial match timeout.
Fix a bug in the way modifier keyup events get translated to Atom keystroke strings.

Should fix atom/tabs#314 and is a prerequisite for adding the MRU list UI (atom/tabs#388) that fixes atom/atom#11035.

still very WIP, lots of pieces still on the floor

nathansobo

This is looking good.

Overall the approach of maintaining a separate collection of pending matches that you don't clear on the timeout seems sound. It's always hard for me to look at something this complex and say "yep, no regressions", but the approach makes sense.

Biggest style nitpick is you gotta convert all the snake_case to camelCase because that's how things are in JS/CoffeeScript land.

I made one suggestion that may or may not work out when explored, which is to store the pending keydown-keyup style bindings in a separate array always rather than puling them out of the partial matches. They really are a different class of match, and I think expressing that in the way the essential state is maintained might lead to more clarity. I could be wrong about this though and you'd have to dig in to find out.

nathansobo · 2016-10-17T16:48:11Z

spec/helpers-spec.coffee

+    assert(isModifierKeyup('^cmd'))
+    assert(isModifierKeyup('^ctrl-shift'))
+    assert(isModifierKeyup('^alt-cmd'))
+  it "returns false for modifier keydowns", ->


Nitpick: Should insert an empty line above this second it.

nathansobo · 2016-10-17T16:48:37Z

spec/key-binding-spec.coffee

+KeyBinding = require '../src/key-binding'
+
+describe "KeyBinding", ->
+


Nitpick: 🔥 this newline.

nathansobo · 2016-10-17T16:49:28Z

spec/key-binding-spec.coffee

+
+describe "KeyBinding", ->
+
+  describe "is_matched_modifer_keydown_keyup", ->


snake_case ▶️ camelCase

This method name is also quite a mouthful. I'm not sure I can come up with anything better though.

nathansobo · 2016-10-17T16:49:51Z

spec/key-binding-spec.coffee

+
+  describe "is_matched_modifer_keydown_keyup", ->
+
+    describe "returns false when the binding...", ->


I like this ... approach. Cute!

nathansobo · 2016-10-17T16:50:21Z

spec/key-binding-spec.coffee

@@ -0,0 +1,54 @@
+KeyBinding = require '../src/key-binding'


Great idea unit testing this class.

nathansobo · 2016-10-17T16:53:16Z

src/helpers.coffee

 exports.keydownEvent = (key, options) ->
  return buildKeyboardEvent(key, 'keydown', options)

 exports.keyupEvent = (key, options) ->
  return buildKeyboardEvent(key, 'keyup', options)

+exports.getModKeys = (keystroke) ->


We generally spell stuff out fully for maximal clarity throughout the Atom codebase. getModifierKeys or getModifiers would be better.

nathansobo · 2016-10-17T16:54:24Z

src/keymap-manager.coffee

@@ -95,6 +95,10 @@ class KeymapManager
  pendingStateTimeoutHandle: null
  dvorakQwertyWorkaroundEnabled: false

+  # Pending matches to bindings that begin with a modifier keydown and and with


and and ➡️ and end?

nathansobo · 2016-10-17T16:54:47Z

src/keymap-manager.coffee

    keystroke = @keystrokeForKeyboardEvent(event)
+    console.log(keystroke)


nathansobo · 2016-10-18T15:42:24Z

src/keymap-manager.coffee

@@ -513,6 +519,15 @@ class KeymapManager
    dispatchedExactMatch = null
    partialMatches = @findPartialMatches(partialMatchCandidates, target)

+    if @pendingPartialMatchedModifierKeystrokes? and isModifierKeyup(keystroke)
+      for binding in @pendingPartialMatchedModifierKeystrokes
+        binding_mod_keyups = getModKeys(binding.keystrokeArray[binding.keystrokeArray.length-1])


Could this be encapsulated in a method in the binding that takes the keystroke string?

nathansobo · 2016-10-18T15:44:44Z

src/keymap-manager.coffee

    @pendingPartialMatches = pendingPartialMatches
    if enableTimeout
      @pendingStateTimeoutHandle = setTimeout(@terminatePendingState.bind(this, true), @partialMatchTimeout)

-  cancelPendingState: ->
+  buildPendingPartialMatchedModiferKeystrokes: ->


I wonder if it would make more sense to store the matched-keydown-keyup style bindings in a separate array all the time rather than filtering them out of the partial matches array later. You could potentially do this in the findMatchCandidates method.

iolsen · 2016-10-28T23:05:45Z

This will get merged after the next release, meaning it should land in 1.14.

nathansobo · 2016-11-15T17:36:15Z

🎉

catching modifier keyups beyond pending timeout

94fd11b

still very WIP, lots of pieces still on the floor

iolsen added the work-in-progress label Oct 17, 2016

iolsen self-assigned this Oct 17, 2016

This was referenced Oct 17, 2016

add new pane switch events for MRU UI atom/atom#13008

Merged

Add config option for MRU tabs atom/atom#11035

Closed

nathansobo suggested changes Oct 18, 2016

View reviewed changes

iolsen added 13 commits October 18, 2016 15:32

fixed style issues from nathansobo's review

53d9c89

more 🐍 ▶️ 🐫

1888c70

require only one keyup from the keydown sequence

faca320

stop restricting new behavior to modifier keys

5695cc1

move keystroke matcher to KeyBinding

070c406

start unifying with keydownExact matches. use MATCH_TYPES everywhere.

c3bcde2

reset for simpler new plan. tests all pass.

f91f755

keyup matcher class in progress

c9b32cc

lint fixes

b5d6d15

PartialKeyupMatcher basically works, has tests

800dc79

new behavior works well. old tests need fixing.

8fde502

existing tests passing

1bd09a5

one more test. cleanup.

d77ccbd

iolsen changed the title ~~[WIP] Improve reliability of catching modifier key-up events~~ Improve reliability of catching modifier key-up events Oct 25, 2016

iolsen removed the work-in-progress label Oct 25, 2016

iolsen added 2 commits October 24, 2016 19:31

merged with master

13375ee

lint fixes

4fc8165

iolsen mentioned this pull request Oct 28, 2016

Rewrite coffee to JS #162

Open

iolsen added 2 commits November 1, 2016 09:32

Merge branch 'master' into io-modifier-keyups

2e38501

Merge branch 'master' into io-modifier-keyups

1ced703

iolsen merged commit eeaee76 into master Nov 15, 2016

iolsen deleted the io-modifier-keyups branch November 15, 2016 19:21

iolsen mentioned this pull request Nov 29, 2016

Buffer list not reliably sorted by MRU atom/fuzzy-finder#250

Closed

nathansobo mentioned this pull request Dec 22, 2016

Partial key mapping can get wrongly resolved #200

Closed

iolsen mentioned this pull request Jan 7, 2017

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

Closed

maxbrunsfeld mentioned this pull request Feb 15, 2017

Make etch perform updates synchronously in specs atom/keybinding-resolver#48

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve reliability of catching modifier key-up events #156

Improve reliability of catching modifier key-up events #156

iolsen commented Oct 17, 2016 •

edited

Loading

nathansobo left a comment

nathansobo Oct 17, 2016

nathansobo Oct 17, 2016

nathansobo Oct 17, 2016

nathansobo Oct 17, 2016

nathansobo Oct 17, 2016

nathansobo Oct 17, 2016

nathansobo Oct 17, 2016

nathansobo Oct 17, 2016

nathansobo Oct 17, 2016

nathansobo Oct 18, 2016

nathansobo Oct 18, 2016

iolsen commented Oct 28, 2016 •

edited

Loading

nathansobo commented Nov 15, 2016

		KeyBinding = require '../src/key-binding'

		describe "KeyBinding", ->


		describe "KeyBinding", ->

		describe "is_matched_modifer_keydown_keyup", ->


		describe "is_matched_modifer_keydown_keyup", ->

		describe "returns false when the binding...", ->

		keystroke = @keystrokeForKeyboardEvent(event)
		console.log(keystroke)

Improve reliability of catching modifier key-up events #156

Improve reliability of catching modifier key-up events #156

Conversation

iolsen commented Oct 17, 2016 • edited Loading

nathansobo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iolsen commented Oct 28, 2016 • edited Loading

nathansobo commented Nov 15, 2016

iolsen commented Oct 17, 2016 •

edited

Loading

iolsen commented Oct 28, 2016 •

edited

Loading