Rework key-sequence parsing.

This reworks the parsing of key sequences like `<c-a-Z>x`:

Advantages over the status quo:

- Parses key sequences in one only place (previously two),
- Removes two hideous regular expression.
- Admits multi-modifier bindings (like `<c-a-Z>`).
- Adds more (and better) tests.
- (And it should be easier to maintain.)

Replaces #2210 (this also simplifies key parsing substantially).

3 files changed, 69 insertions, 24 deletions

diff --git a/background_scripts/commands.coffee b/background_scripts/commands.coffee
index e905c410..f43bd02b 100644
--- a/background_scripts/commands.coffee
+++ b/background_scripts/commands.coffee
@@ -23,13 +23,13 @@ Commands =
 
     @availableCommands[command] = extend options, description: description
 
-  mapKeyToCommand: ({ key, command, options }) ->
+  mapKeyToCommand: ({ key, keySequence, command, options }) ->
     unless @availableCommands[command]
       BgUtils.log "#{command} doesn't exist!"
       return
 
     options ?= {}
-    @keyToCommandRegistry[key] = extend { command, options }, @availableCommands[command]
+    @keyToCommandRegistry[key] = extend { keySequence, command, options }, @availableCommands[command]
 
   # Lower-case the appropriate portions of named keys.
   #
@@ -39,10 +39,18 @@ Commands =
   # humans may prefer other forms <Left> or <C-a>.
   # On the other hand, <c-a> and <c-A> are different named keys - for one of
   # them you have to press "shift" as well.
+  # We sort modifiers here to match the order used in keyboard_utils.coffee.
   normalizeKey: (key) ->
-    key.replace(/<[acm]-/ig, (match) -> match.toLowerCase())
-       .replace(/<([acm]-)?([a-zA-Z0-9]{2,})>/g, (match, optionalPrefix, keyName) ->
-          "<" + (if optionalPrefix then optionalPrefix else "") + keyName.toLowerCase() + ">")
+    if key.length == 0
+      []
+    else if 0 == key.search /^<([^<>]+)>(.*)/ # Parse "<c-a>bcd" as "<c-a>" and "bcd".
+      [modifiers..., keyChar] = RegExp.$1.split "-"
+      keyChar = keyChar.toLowerCase() unless keyChar.length == 1
+      modifiers = (modifier.toLowerCase() for modifier in modifiers)
+      modifiers.sort()
+      ["<#{[modifiers..., keyChar].join "-"}>", @normalizeKey(RegExp.$2)...]
+    else
+      [key[0], @normalizeKey(key[1..])...]
 
   parseCustomKeyMappings: (customKeyMappings) ->
     for line in customKeyMappings.split "\n"
@@ -52,13 +60,15 @@ Commands =
           when "map"
             [ _, key, command, optionList... ] = tokens
             if command? and @availableCommands[command]
-              key = @normalizeKey key
+              keySequence = @normalizeKey key
+              key = keySequence.join ""
               BgUtils.log "Mapping #{key} to #{command}"
-              @mapKeyToCommand { key, command, options: @parseCommandOptions command, optionList }
+              @mapKeyToCommand { key, command, keySequence, options: @parseCommandOptions command, optionList }
 
           when "unmap"
             if tokens.length == 2
-              key = @normalizeKey tokens[1]
+              keySequence = @normalizeKey tokens[1]
+              key = keySequence.join ""
               BgUtils.log "Unmapping #{key}"
               delete @keyToCommandRegistry[key]
 
@@ -90,24 +100,25 @@ Commands =
 
   clearKeyMappingsAndSetDefaults: ->
     @keyToCommandRegistry = {}
-    @mapKeyToCommand { key, command } for own key, command of defaultKeyMappings
+    for own key, command of defaultKeyMappings
+      keySequence = @normalizeKey key
+      key = keySequence.join ""
+      @mapKeyToCommand { key, command, keySequence }
 
   # This generates a nested key-to-command mapping structure. There is an example in mode_key_handler.coffee.
   generateKeyStateMapping: ->
-    # Keys are either literal characters, or "named" - for example <a-b> (alt+b), <left> (left arrow) or <f12>
-    # This regular expression captures two groups: the first is a named key, the second is the remainder of
-    # the string.
-    namedKeyRegex = /^(<(?:[amc]-.|(?:[amc]-)?[a-z0-9]{2,})>)(.*)$/
     keyStateMapping = {}
     for own keys, registryEntry of @keyToCommandRegistry
       currentMapping = keyStateMapping
-      while 0 < keys.length
-        [key, keys] = if 0 == keys.search namedKeyRegex then [RegExp.$1, RegExp.$2] else [keys[0], keys[1..]]
+      for key, index in registryEntry.keySequence
         if currentMapping[key]?.command
-          break # Do not overwrite existing command bindings, they take priority.
-        else if 0 < keys.length
+          # Do not overwrite existing command bindings, they take priority.  NOTE(smblott) This is the legacy
+          # behaviour.
+          break
+        else if index < registryEntry.keySequence.length - 1
           currentMapping = currentMapping[key] ?= {}
         else
+          delete registryEntry.keySequence # We don't need this any more.
           currentMapping[key] = registryEntry
     chrome.storage.local.set normalModeKeyStateMapping: keyStateMapping
 
diff --git a/lib/keyboard_utils.coffee b/lib/keyboard_utils.coffee
index dabf864d..f0e791d4 100644
--- a/lib/keyboard_utils.coffee
+++ b/lib/keyboard_utils.coffee
@@ -111,9 +111,10 @@ KeyboardUtils =
           modifiers = []
 
           keyChar = keyChar.toUpperCase() if event.shiftKey
-          modifiers.push "m" if event.metaKey
-          modifiers.push "c" if event.ctrlKey
+          # These must be in alphabetical order (to match the sorted modifier order in Commands.normalizeKey).
           modifiers.push "a" if event.altKey
+          modifiers.push "c" if event.ctrlKey
+          modifiers.push "m" if event.metaKey
 
           keyChar = [modifiers..., keyChar].join "-"
           if 1 < keyChar.length then "<#{keyChar}>" else keyChar
diff --git a/tests/unit_tests/commands_test.coffee b/tests/unit_tests/commands_test.coffee
index f501a960..1915e062 100644
--- a/tests/unit_tests/commands_test.coffee
+++ b/tests/unit_tests/commands_test.coffee
@@ -4,12 +4,45 @@ global.Settings = {postUpdateHooks: {}, get: (-> ""), set: ->}
 {Commands} = require "../../background_scripts/commands.js"
 
 context "Key mappings",
+  setup ->
+    @testKeySequence = (key, expectedKeyText, expectedKeyLength) ->
+      keySequence = Commands.normalizeKey key
+      assert.equal expectedKeyText, keySequence.join "/"
+      assert.equal expectedKeyLength, keySequence.length
+
   should "lowercase keys correctly", ->
-    assert.equal (Commands.normalizeKey '<c-a>'), '<c-a>'
-    assert.equal (Commands.normalizeKey '<C-a>'), '<c-a>'
-    assert.equal (Commands.normalizeKey '<C-A>'), '<c-A>'
-    assert.equal (Commands.normalizeKey '<F12>'), '<f12>'
-    assert.equal (Commands.normalizeKey '<C-F12>'), '<c-f12>'
+    @testKeySequence "a", "a", 1
+    @testKeySequence "A", "A", 1
+    @testKeySequence "ab", "a/b", 2
+
+  should "parse keys with modifiers", ->
+    @testKeySequence "<c-a>", "<c-a>", 1
+    @testKeySequence "<c-A>", "<c-A>", 1
+    @testKeySequence "<c-a><a-b>", "<c-a>/<a-b>", 2
+    @testKeySequence "<m-a>", "<m-a>", 1
+
+  should "normalize with modifiers", ->
+    # Modifiers should be in alphabetical order.
+    @testKeySequence "<m-c-a-A>", "<a-c-m-A>", 1
+
+  should "parse and normalize named keys", ->
+    @testKeySequence "<space>", "<space>", 1
+    @testKeySequence "<Space>", "<space>", 1
+    @testKeySequence "<C-Space>", "<c-space>", 1
+    @testKeySequence "<f12>", "<f12>", 1
+    @testKeySequence "<F12>", "<f12>", 1
+
+  should "handle angle brackets", ->
+    @testKeySequence "<", "<", 1
+    @testKeySequence ">", ">", 1
+
+    @testKeySequence "<<", "</<", 2
+    @testKeySequence ">>", ">/>", 2
+
+    @testKeySequence "<>", "</>", 2
+    @testKeySequence "<>", "</>", 2
+
+    @testKeySequence "<<space>", "</<space>", 2
 
 context "Validate commands and options",
   should "have either noRepeat or repeatLimit, but not both", ->