Improve reliability of catching modifier key-up events

spec/helpers-spec.coffee

@@ -1,4 +1,4 @@
-{normalizeKeystrokes, keystrokesMatch} = require '../src/helpers'
+{normalizeKeystrokes, keystrokesMatch, isModifierKeyup} = require '../src/helpers'
 
 describe ".normalizeKeystrokes(keystrokes)", ->
   it "parses and normalizes the keystrokes", ->
@@ -61,3 +61,19 @@ describe ".keystrokesMatch(bindingKeystrokes, userKeystrokes)", ->
 
   it "returns 'keydownExact' for bindings that match and contain a remainder of only keyup events", ->
     assert.equal(keystrokesMatch(['a', 'b', '^b'], ['a', 'b']), 'keydownExact')
+
+describe ".isModifierKeyup(keystroke)", ->
+  it "returns true for single modifier keyups", ->
+    assert(isModifierKeyup('^ctrl'))
+    assert(isModifierKeyup('^shift'))
+    assert(isModifierKeyup('^alt'))
+    assert(isModifierKeyup('^cmd'))
+    assert(isModifierKeyup('^ctrl-shift'))
+    assert(isModifierKeyup('^alt-cmd'))
+  it "returns false for modifier keydowns", ->
+    assert(!isModifierKeyup('ctrl-x'))
+    assert(!isModifierKeyup('shift-x'))
+    assert(!isModifierKeyup('alt-x'))
+    assert(!isModifierKeyup('cmd-x'))
+    assert(!isModifierKeyup('ctrl-shift-x'))
+    assert(!isModifierKeyup('alt-cmd-x'))

spec/key-binding-spec.coffee

@@ -0,0 +1,54 @@
+KeyBinding = require '../src/key-binding'
+
+describe "KeyBinding", ->
+
+  describe "is_matched_modifer_keydown_keyup", ->
+
+    describe "returns false when the binding...", ->
+      it "has no modifier keys", ->
+        kb = new KeyBinding('test', 'whatever', 'a', 'body', 0)
+        assert(not kb.is_matched_modifer_keydown_keyup())
+      it "has no keyups", ->
+        kb = new KeyBinding('test', 'whatever', 'ctrl-a', 'body', 0)
+        assert(not kb.is_matched_modifer_keydown_keyup())
+      it "is a bare modifier", ->
+        kb = new KeyBinding('test', 'whatever', 'ctrl', 'body', 0)
+        assert(not kb.is_matched_modifer_keydown_keyup())
+      it "is a bare modifier keyup", ->
+        kb = new KeyBinding('test', 'whatever', '^ctrl', 'body', 0)
+        assert(not kb.is_matched_modifer_keydown_keyup())
+      it "has mismatched last_keystroke: ctrl ^shift", ->
+        kb = new KeyBinding('test', 'whatever', 'ctrl-a ^shift', 'body', 0)
+        assert(not kb.is_matched_modifer_keydown_keyup())
+      it "has mismatched last_keystroke: cmd ^alt", ->
+        kb = new KeyBinding('test', 'whatever', 'cmd-a ^alt', 'body', 0)
+        assert(not kb.is_matched_modifer_keydown_keyup())
+      it "has partially mismatched last_keystroke: ctrl-cmd ^ctrl", ->
+        kb = new KeyBinding('test', 'whatever', 'ctrl-cmd-a ^ctrl', 'body', 0)
+        assert(not kb.is_matched_modifer_keydown_keyup())
+      it "has partially mismatched last_keystroke: ctrl-cmd ^cmd", ->
+        kb = new KeyBinding('test', 'whatever', 'ctrl-cmd-a ^cmd', 'body', 0)
+        assert(not kb.is_matched_modifer_keydown_keyup())
+      it "has partially mismatched last_keystroke: ctrl ^ctrl-shift", ->
+        kb = new KeyBinding('test', 'whatever', 'ctrl ^ctrl-shift', 'body', 0)
+        assert(not kb.is_matched_modifer_keydown_keyup())
+
+    describe "returns true when the binding...", ->
+      it "has a matched ctrl", ->
+        kb = new KeyBinding('test', 'whatever', 'ctrl-a ^ctrl', 'body', 0)
+        assert(kb.is_matched_modifer_keydown_keyup())
+      it "has a matched shift", ->
+        kb = new KeyBinding('test', 'whatever', 'shift-a ^shift', 'body', 0)
+        assert(kb.is_matched_modifer_keydown_keyup())
+      it "has a matched alt", ->
+        kb = new KeyBinding('test', 'whatever', 'alt-a ^alt', 'body', 0)
+        assert(kb.is_matched_modifer_keydown_keyup())
+      it "has a matched cmd", ->
+        kb = new KeyBinding('test', 'whatever', 'cmd-a ^cmd', 'body', 0)
+        assert(kb.is_matched_modifer_keydown_keyup())
+      it "has a matched ctrl-shift", ->
+        kb = new KeyBinding('test', 'whatever', 'ctrl-shift-a ^ctrl-shift', 'body', 0)
+        assert(kb.is_matched_modifer_keydown_keyup())
+      it "has matched bare last_keystroke", ->
+        kb = new KeyBinding('test', 'whatever', 'ctrl ^ctrl', 'body', 0)
+        assert(kb.is_matched_modifer_keydown_keyup())

src/helpers.coffee

@@ -187,19 +187,31 @@ nonAltModifiedKeyForKeyboardEvent = (event) ->
     else
       characters.unmodified
 
+exports.MODIFIERS = MODIFIERS
+
 exports.characterForKeyboardEvent = (event) ->
   event.key unless event.ctrlKey or event.metaKey
 
 exports.calculateSpecificity = calculateSpecificity
 
 exports.isBareModifier = (keystroke) -> ENDS_IN_MODIFIER_REGEX.test(keystroke)
 
+exports.isModifierKeyup = (keystroke) -> keystroke.startsWith('^') and ENDS_IN_MODIFIER_REGEX.test(keystroke)
+
 exports.keydownEvent = (key, options) ->
   return buildKeyboardEvent(key, 'keydown', options)
 
 exports.keyupEvent = (key, options) ->
   return buildKeyboardEvent(key, 'keyup', options)
 
+exports.getModKeys = (keystroke) ->
+  keys = keystroke.split('-')
+  mod_keys = []
+  for key in keys when MODIFIERS.has(key)
+    mod_keys.push(key)
+  mod_keys
+
+
 buildKeyboardEvent = (key, eventType, {ctrl, shift, alt, cmd, keyCode, target, location}={}) ->
   ctrlKey = ctrl ? false
   altKey = alt ? false

src/key-binding.coffee

@@ -1,4 +1,4 @@
-{calculateSpecificity} = require './helpers'
+{calculateSpecificity, MODIFIERS, getModKeys} = require './helpers'
 
 module.exports =
 class KeyBinding
@@ -12,6 +12,7 @@ class KeyBinding
     @selector = selector.replace(/!important/g, '')
     @specificity = calculateSpecificity(selector)
     @index = @constructor.currentIndex++
+    @isMatchedModifierKeydownKeyup = null
 
   matches: (keystroke) ->
     multiKeystroke = /\s/.test keystroke
@@ -28,3 +29,30 @@ class KeyBinding
         keyBinding.specificity - @specificity
     else
       keyBinding.priority - @priority
+
+  # Returns true iff the binding starts with one or more modifier keydowns and ends
+  # with the matching set of modifier keyups.
+  #
+  # The modifier key order in each must match. Bare modifier keydown
+  # combinations are not handled specially, e.g. "ctrl ^ctrl" also returns true.
+  # The keymap manager ignores them, there's no reason to do the additional work
+  # to identify them again here.
+  is_matched_modifer_keydown_keyup: ->
+    # this is likely to get checked repeatedly so we calc it once and cache it
+    return @isMatchedModifierKeydownKeyup if @isMatchedModifierKeydownKeyup?
+
+    if not @keystrokeArray?.length > 1
+      return @isMatchedModifierKeydownKeyup = false
+
+    last_keystroke = @keystrokeArray[@keystrokeArray.length-1]
+    if not last_keystroke.startsWith('^')
+      return @isMatchedModifierKeydownKeyup = false
+
+    mod_keys_down = getModKeys(@keystrokeArray[0])
+    mod_keys_up = getModKeys(last_keystroke.substring(1))
+    if mod_keys_down.length != mod_keys_up.length
+      return @isMatchedModifierKeydownKeyup = false
+    for i in [0..mod_keys_down.length-1]
+      if mod_keys_down[i] != mod_keys_up[i]
+        return @isMatchedModifierKeydownKeyup = false
+    return @isMatchedModifierKeydownKeyup = true

src/keymap-manager.coffee

@@ -7,7 +7,7 @@ path = require 'path'
 {Emitter, Disposable, CompositeDisposable} = require 'event-kit'
 KeyBinding = require './key-binding'
 CommandEvent = require './command-event'
-{normalizeKeystrokes, keystrokeForKeyboardEvent, isBareModifier, keydownEvent, keyupEvent, characterForKeyboardEvent, keystrokesMatch} = require './helpers'
+{normalizeKeystrokes, keystrokeForKeyboardEvent, isBareModifier, keydownEvent, keyupEvent, characterForKeyboardEvent, keystrokesMatch, isModifierKeyup} = require './helpers'
 
 Platforms = ['darwin', 'freebsd', 'linux', 'sunos', 'win32']
 OtherPlatforms = Platforms.filter (platform) -> platform isnt process.platform
@@ -95,6 +95,10 @@ class KeymapManager
   pendingStateTimeoutHandle: null
   dvorakQwertyWorkaroundEnabled: false
 
+  # Pending matches to bindings that begin with a modifier keydown and and with
+  # the matching modifier keyup
+  pendingPartialMatchedModifierKeystrokes: null
+
   ###
   Section: Construction and Destruction
   ###
@@ -490,7 +494,9 @@ class KeymapManager
     #
     # Godspeed.
 
+    # keystroke is the atom keybind syntax, e.g. 'ctrl-a'
     keystroke = @keystrokeForKeyboardEvent(event)
+    console.log(keystroke)
 
     # We dont care about bare modifier keys in the bindings. e.g. `ctrl y` isnt going to work.
     if event.type is 'keydown' and @queuedKeystrokes.length > 0 and isBareModifier(keystroke)
@@ -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])
+        keystroke_mod_keyups = getModKeys(keystroke)
+        if keystroke_mod_keyups.length == 1 and binding_mod_keyups.has(keystroke_mod_keyups[0])
+          exactMatchCandidates.push(binding)
+          # Ian TODO remove from @pendingPartialMatchedModifierKeystrokes
+        # Ian TODO deal with all the other partial match possibilities
+
     # If any partial match *was* pending but has now failed to match, add it to
     # the list of bindings to disable so we don't attempt to match it again
     # during a subsequent event replay by `terminatePendingState`.
@@ -525,7 +540,7 @@ class KeymapManager
     shouldUsePartialMatches = hasPartialMatches
 
     # Determine if the current keystrokes match any bindings *exactly*. If we
-    # do find and exact match, the next step depends on whether we have any
+    # do find an exact match, the next step depends on whether we have any
     # partial matches. If we have no partial matches, we dispatch the command
     # immediately. Otherwise we break and allow ourselves to enter the pending
     # state with a timeout.
@@ -552,7 +567,7 @@ class KeymapManager
           if hasPartialMatches
             # When there is a set of bindings like `'ctrl-y', 'ctrl-y ^ctrl'`,
             # and a `ctrl-y` comes in, this will allow the `ctrl-y` command to be
-            # dispatched without waiting for any other keystrokes
+            # dispatched without waiting for any other keystrokes.
             allPartialMatchesContainKeyupRemainder = true
             for partialMatch in partialMatches
               if keydownExactMatchCandidates.indexOf(partialMatch) < 0
@@ -562,6 +577,7 @@ class KeymapManager
             shouldUsePartialMatches = false
 
           if @dispatchCommandEvent(exactMatchCandidate.command, target, event)
+            console.log('dispatched: ' + exactMatchCandidate.keystrokes)
             dispatchedExactMatch = exactMatchCandidate
             eventHandled = true
             break
@@ -700,14 +716,29 @@ class KeymapManager
     @bindingsToDisable = []
 
   enterPendingState: (pendingPartialMatches, enableTimeout) ->
-    @cancelPendingState() if @pendingStateTimeoutHandle?
+    if @pendingStateTimeoutHandle?
+      @cancelPendingState()
+    else
+      @buildPendingPartialMatchedModiferKeystrokes()
+
     @pendingPartialMatches = pendingPartialMatches
     if enableTimeout
       @pendingStateTimeoutHandle = setTimeout(@terminatePendingState.bind(this, true), @partialMatchTimeout)
 
-  cancelPendingState: ->
+  buildPendingPartialMatchedModiferKeystrokes: ->
+    @pendingPartialMatchedModifierKeystrokes = null
+    for match in @pendingPartialMatches?
+      if match.is_matched_modifer_keydown_keyup()
+        @pendingPartialMatchedModifierKeystrokes.push(match)
+
+  cancelPendingState: (modifierKeyupMatched = false) ->
     clearTimeout(@pendingStateTimeoutHandle)
     @pendingStateTimeoutHandle = null
+
+    # Ian TODO perf?
+    # Preserve pending modifier keydown-keyup matches beyond pending state
+    @buildPendingPartialMatchedModiferKeystrokes()
+
     @pendingPartialMatches = null
 
   # This is called by {::handleKeyboardEvent} when no matching bindings are
@@ -718,7 +749,7 @@ class KeymapManager
   #
   # Note that replaying events has a recursive behavior. Replaying will set
   # member state (e.g. @queuedKeyboardEvents) just like real events, and will
-  # likely result in another call this function. The replay process will
+  # likely result in another call to this function. The replay process will
   # potentially replay the events (or a subset of events) several times, while
   # disabling bindings here and there. See any spec that handles multiple
   # keystrokes failures to match a binding.