Merge pull request #2368 from smblott-github/rework-key-mapping-parsing

Rework key-mapping parsing and help-dialog HTML

7 files changed, 159 insertions, 147 deletions

diff --git a/background_scripts/commands.coffee b/background_scripts/commands.coffee
index 6aa8ca0d..aece4bec 100644
--- a/background_scripts/commands.coffee
+++ b/background_scripts/commands.coffee
@@ -1,36 +1,55 @@
 Commands =
+  availableCommands: {}
+  keyToCommandRegistry: null
+  mapKeyRegistry: null
+
   init: ->
-    for own command, descriptor of commandDescriptions
-      @addCommand(command, descriptor[0], descriptor[1])
-    @loadKeyMappings Settings.get "keyMappings"
+    for own command, [description, options] of commandDescriptions
+      @availableCommands[command] = extend (options ? {}), description: description
+
     Settings.postUpdateHooks["keyMappings"] = @loadKeyMappings.bind this
+    @loadKeyMappings Settings.get "keyMappings"
+    @prepareHelpPageData()
 
   loadKeyMappings: (customKeyMappings) ->
-    @clearKeyMappingsAndSetDefaults()
-    @parseCustomKeyMappings customKeyMappings
-    @generateKeyStateMapping()
-    chrome.storage.local.set mapKeyRegistry: @mapKeyRegistry
-
-  availableCommands: {}
-  keyToCommandRegistry: {}
-
-  # Registers a command, making it available to be optionally bound to a key.
-  # options:
-  #  - background: whether this command needs to be run against the background page.
-  addCommand: (command, description, options = {}) ->
-    if command of @availableCommands
-      BgUtils.log "#{command} is already defined! Check commands.coffee for duplicates."
-      return
+    @keyToCommandRegistry = {}
+    @mapKeyRegistry = {}
 
-    @availableCommands[command] = extend options, description: description
+    configLines = ("map #{key} #{command}" for own key, command of defaultKeyMappings)
+    configLines.push BgUtils.parseLines(customKeyMappings)...
+    seen = {}
+    unmapAll = false
+    for line in configLines.reverse()
+      tokens = line.split /\s+/
+      switch tokens[0]
+        when "map"
+          if 3 <= tokens.length and not unmapAll
+            [_, key, command, optionsList...] = tokens
+            if not seen[key] and registryEntry = @availableCommands[command]
+              seen[key] = true
+              keySequence = @parseKeySequence key
+              options = @parseCommandOptions command, optionsList
+              @keyToCommandRegistry[key] = extend {keySequence, command, options}, @availableCommands[command]
+        when "unmap"
+          if tokens.length == 2
+            seen[tokens[1]] = true
+        when "unmapAll"
+          unmapAll = true
+        when "mapkey"
+          if tokens.length == 3
+            fromChar = @parseKeySequence tokens[1]
+            toChar = @parseKeySequence tokens[2]
+            @mapKeyRegistry[fromChar[0]] ?= toChar[0] if fromChar.length == toChar.length == 1
 
-  mapKeyToCommand: ({ key, keySequence, command, options }) ->
-    unless @availableCommands[command]
-      BgUtils.log "#{command} doesn't exist!"
-      return
+    chrome.storage.local.set mapKeyRegistry: @mapKeyRegistry
+    @installKeyStateMapping()
 
-    options ?= {}
-    @keyToCommandRegistry[key] = extend { keySequence, command, options }, @availableCommands[command]
+    # Push the key mapping for passNextKey into Settings so that it's available in the front end for insert
+    # mode.  We exclude single-key mappings (that is, printable keys) because when users press printable keys
+    # in insert mode they expect the character to be input, not to be droppped into some special Vimium
+    # mode.
+    Settings.set "passNextKeyKeys",
+      (key for own key of @keyToCommandRegistry when @keyToCommandRegistry[key].command == "passNextKey" and 1 < key.length)
 
   # Lower-case the appropriate portions of named keys.
   #
@@ -60,43 +79,6 @@ Commands =
       else
         [key[0], @parseKeySequence(key[1..])...]
 
-  parseCustomKeyMappings: (customKeyMappings) ->
-    for line in BgUtils.parseLines customKeyMappings
-      tokens = line.split /\s+/
-      switch tokens[0]
-        when "map"
-          [ _, key, command, optionList... ] = tokens
-          keySequence = @parseKeySequence key
-          if command? and @availableCommands[command]
-            key = keySequence.join ""
-            BgUtils.log "mapping [\"#{keySequence.join '", "'}\"] to #{command}"
-            @mapKeyToCommand { key, command, keySequence, options: @parseCommandOptions command, optionList }
-          else
-            BgUtils.log "skipping [\"#{keySequence.join '", "'}\"] for #{command} -- something is not right"
-
-        when "unmap"
-          if tokens.length == 2
-            keySequence = @parseKeySequence tokens[1]
-            key = keySequence.join ""
-            BgUtils.log "Unmapping #{key}"
-            delete @keyToCommandRegistry[key]
-
-        when "unmapAll"
-          @keyToCommandRegistry = {}
-
-        when "mapkey"
-          if tokens.length == 3
-            fromChar = @parseKeySequence tokens[1]
-            toChar = @parseKeySequence tokens[2]
-            @mapKeyRegistry[fromChar[0]] = toChar[0] if fromChar.length == toChar.length == 1
-
-    # Push the key mapping for passNextKey into Settings so that it's available in the front end for insert
-    # mode.  We exclude single-key mappings (that is, printable keys) because when users press printable keys
-    # in insert mode they expect the character to be input, not to be droppped into some special Vimium
-    # mode.
-    Settings.set "passNextKeyKeys",
-      (key for own key of @keyToCommandRegistry when @keyToCommandRegistry[key].command == "passNextKey" and 1 < key.length)
-
   # Command options follow command mappings, and are of one of two forms:
   #   key=value     - a value
   #   key           - a flag
@@ -113,16 +95,9 @@ Commands =
 
     options
 
-  clearKeyMappingsAndSetDefaults: ->
-    @keyToCommandRegistry = {}
-    @mapKeyRegistry = {}
-    for own key, command of defaultKeyMappings
-      keySequence = @parseKeySequence 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: ->
+  # This generates and installs a nested key-to-command mapping structure. There is an example in
+  # mode_key_handler.coffee.
+  installKeyStateMapping: ->
     keyStateMapping = {}
     for own keys, registryEntry of @keyToCommandRegistry
       currentMapping = keyStateMapping
@@ -136,9 +111,25 @@ Commands =
         else
           currentMapping[key] = extend {}, registryEntry
           # We don't need these properties in the content scripts.
-          delete registryEntry[prop] for prop in ["keySequence", "description"]
+          delete currentMapping[key][prop] for prop in ["keySequence", "description"]
     chrome.storage.local.set normalModeKeyStateMapping: keyStateMapping
 
+  # Build the "helpPageData" data structure which the help page needs and place it in Chrome storage.
+  prepareHelpPageData: ->
+    commandToKey = {}
+    for own key, registryEntry of @keyToCommandRegistry
+      (commandToKey[registryEntry.command] ?= []).push key
+    commandGroups = {}
+    for own group, commands of @commandGroups
+      commandGroups[group] = []
+      for command in commands
+        commandGroups[group].push
+          command: command
+          description: @availableCommands[command].description
+          keys: commandToKey[command] ? []
+          advanced: command in @advancedCommands
+    chrome.storage.local.set helpPageData: commandGroups
+
   # An ordered listing of all available commands, grouped by type. This is the order they will
   # be shown in the help page.
   commandGroups:
diff --git a/background_scripts/main.coffee b/background_scripts/main.coffee
index e57e061d..8f99cf3e 100644
--- a/background_scripts/main.coffee
+++ b/background_scripts/main.coffee
@@ -80,56 +80,6 @@ onURLChange = (details) ->
 chrome.webNavigation.onHistoryStateUpdated.addListener onURLChange # history.pushState.
 chrome.webNavigation.onReferenceFragmentUpdated.addListener onURLChange # Hash changed.
 
-# Retrieves the help dialog HTML template from a file, and populates it with the latest keybindings.
-getHelpDialogHtml = ({showUnboundCommands, showCommandNames, customTitle}) ->
-  commandsToKey = {}
-  for own key of Commands.keyToCommandRegistry
-    command = Commands.keyToCommandRegistry[key].command
-    commandsToKey[command] = (commandsToKey[command] || []).concat(key)
-
-  replacementStrings =
-    version: Utils.getCurrentVersion()
-    title: customTitle || "Help"
-    tip: if showCommandNames then "Tip: click command names to yank them to the clipboard." else "&nbsp;"
-
-  for own group of Commands.commandGroups
-    replacementStrings[group] =
-        helpDialogHtmlForCommandGroup(group, commandsToKey, Commands.availableCommands,
-                                      showUnboundCommands, showCommandNames)
-
-  replacementStrings
-
-#
-# Generates HTML for a given set of commands. commandGroups are defined in commands.js
-#
-helpDialogHtmlForCommandGroup = (group, commandsToKey, availableCommands,
-    showUnboundCommands, showCommandNames) ->
-  html = []
-  for command in Commands.commandGroups[group]
-    keys = commandsToKey[command] || []
-    bindings = ("<span class='vimiumHelpDialogKey'>#{Utils.escapeHtml key}</span>" for key in keys).join ", "
-    if (showUnboundCommands || commandsToKey[command])
-      isAdvanced = Commands.advancedCommands.indexOf(command) >= 0
-      description = availableCommands[command].description
-      if keys.join(", ").length < 12
-        helpDialogHtmlForCommand html, isAdvanced, bindings, description, showCommandNames, command
-      else
-        # If the length of the bindings is too long, then we display the bindings on a separate row from the
-        # description.  This prevents the column alignment from becoming out of whack.
-        helpDialogHtmlForCommand html, isAdvanced, bindings, "", false, ""
-        helpDialogHtmlForCommand html, isAdvanced, "", description, showCommandNames, command
-  html.join("\n")
-
-helpDialogHtmlForCommand = (html, isAdvanced, bindings, description, showCommandNames, command) ->
-  html.push "<tr class='vimiumReset #{"advanced" if isAdvanced}'>"
-  if description
-    html.push "<td class='vimiumReset'>#{bindings}</td>"
-    html.push "<td class='vimiumReset'></td><td class='vimiumReset vimiumHelpDescription'>", description
-    html.push("(<span class='vimiumReset commandName'>#{command}</span>)") if showCommandNames
-  else
-    html.push "<td class='vimiumReset' colspan='3' style='text-align: left;'>", bindings
-  html.push("</td></tr>")
-
 # Cache "content_scripts/vimium.css" in chrome.storage.local for UI components.
 do ->
   req = new XMLHttpRequest()
@@ -429,7 +379,6 @@ portHandlers =
 
 sendRequestHandlers =
   runBackgroundCommand: (request) -> BackgroundCommands[request.registryEntry.command] request
-  getHelpDialogHtml: getHelpDialogHtml
   # getCurrentTabUrl is used by the content scripts to get their full URL, because window.location cannot help
   # with Chrome-specific URLs like "view-source:http:..".
   getCurrentTabUrl: ({tab}) -> tab.url
diff --git a/content_scripts/vimium.css b/content_scripts/vimium.css
index edb1ab85..3e8f65d6 100644
--- a/content_scripts/vimium.css
+++ b/content_scripts/vimium.css
@@ -243,7 +243,7 @@ div#vimiumHelpDialog td.vimiumHelpDescription {
   font-family:"Helvetica Neue",Helvetica,Arial,sans-serif;
   font-size:14px;
 }
-div#vimiumHelpDialog span.commandName {
+div#vimiumHelpDialog span.vimiumCopyCommandNameName {
   font-style: italic;
   cursor: pointer;
   font-size: 12px;
@@ -275,6 +275,9 @@ div#vimiumHelpDialogFooter {
   position: relative;
   margin-bottom: 37px;
 }
+table.helpDialogBottom {
+  width:100%;
+}
 td.helpDialogBottomRight {
   width:100%;
   float:right;
diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee
index fcca98ab..21826944 100644
--- a/content_scripts/vimium_frontend.coffee
+++ b/content_scripts/vimium_frontend.coffee
@@ -646,8 +646,7 @@ enterFindMode = ->
   new FindMode()
 
 window.showHelp = (sourceFrameId) ->
-  chrome.runtime.sendMessage handler: "getHelpDialogHtml", (response) ->
-    HelpDialog.toggle {sourceFrameId, html: response}
+  HelpDialog.toggle {sourceFrameId, showAllCommandDetails: false}
 
 # If we are in the help dialog iframe, then HelpDialog is already defined with the necessary functions.
 window.HelpDialog ?=
diff --git a/pages/help_dialog.coffee b/pages/help_dialog.coffee
index 28aafb4a..4ac9116b 100644
--- a/pages/help_dialog.coffee
+++ b/pages/help_dialog.coffee
@@ -1,3 +1,12 @@
+$ = (id) -> document.getElementById id
+$$ = (element, selector) -> element.querySelector selector
+
+# The ordering we show key bindings is alphanumerical, except that special keys sort to the end.
+compareKeys = (a,b) ->
+  a = a.replace "<","~"
+  b = b.replace "<", "~"
+  if a < b then -1 else if b < a then 1 else 0
+
 # This overrides the HelpDialog implementation in vimium_frontend.coffee.  We provide aliases for the two
 # HelpDialog methods required by normalMode (isShowing() and toggle()).
 HelpDialog =
@@ -20,30 +29,67 @@ HelpDialog =
         chrome.runtime.sendMessage({handler: "openOptionsPageInNewTab"})
       false)
     document.getElementById("toggleAdvancedCommands").addEventListener("click",
-      HelpDialog.toggleAdvancedCommands, false)
+      HelpDialog.toggleAdvancedCommands.bind(HelpDialog), false)
 
     document.documentElement.addEventListener "click", (event) =>
       @hide() unless @dialogElement.contains event.target
     , false
 
-  show: ({html}) ->
-    for own placeholder, htmlString of html
-      @dialogElement.querySelector("#help-dialog-#{placeholder}").innerHTML = htmlString
+  instantiateHtmlTemplate: (parentNode, templateId, callback) ->
+    templateContent = document.querySelector(templateId).content
+    node = document.importNode templateContent, true
+    parentNode.appendChild node
+    callback parentNode.lastElementChild
+
+  show: ({showAllCommandDetails}) ->
+    $("help-dialog-title").textContent = if showAllCommandDetails then "Command Listing" else "Help"
+    $("help-dialog-version").textContent = Utils.getCurrentVersion()
+
+    chrome.storage.local.get "helpPageData", ({helpPageData}) =>
+      for own group, commands of helpPageData
+        container = @dialogElement.querySelector("#help-dialog-#{group}")
+        container.innerHTML = ""
+        for command in commands when showAllCommandDetails or 0 < command.keys.length
+          keysElement = null
+          descriptionElement = null
+
+          useTwoRows = 12 <= command.keys.join(", ").length
+          unless useTwoRows
+            @instantiateHtmlTemplate container, "#helpDialogEntry", (element) ->
+              element.classList.add "advanced" if command.advanced
+              keysElement = descriptionElement = element
+          else
+            @instantiateHtmlTemplate container, "#helpDialogEntryBindingsOnly", (element) ->
+              element.classList.add "advanced" if command.advanced
+              keysElement = element
+            @instantiateHtmlTemplate container, "#helpDialogEntry", (element) ->
+              element.classList.add "advanced" if command.advanced
+              descriptionElement = element
+
+          $$(descriptionElement, ".vimiumHelpDescription").textContent = command.description
+
+          keysElement = $$(keysElement, ".vimiumKeyBindings")
+          lastElement = null
+          for key in command.keys.sort compareKeys
+            @instantiateHtmlTemplate keysElement, "#keysTemplate", (element) ->
+              lastElement = element
+              $$(element, ".vimiumHelpDialogKey").innerHTML = Utils.escapeHtml key
+          # And strip off the trailing ", ", if necessary.
+          lastElement.removeChild $$ lastElement, ".commaSeparator" if lastElement
 
-    @showAdvancedCommands(@getShowAdvancedCommands())
+          if showAllCommandDetails
+            @instantiateHtmlTemplate $$(descriptionElement, ".vimiumHelpDescription"), "#commandNameTemplate", (element) ->
+              commandNameElement = $$ element, ".vimiumCopyCommandNameName"
+              commandNameElement.textContent = command.command
+              commandNameElement.title = "Click to copy \"#{command.command}\" to clipboard."
+              commandNameElement.addEventListener "click", ->
+                chrome.runtime.sendMessage handler: "copyToClipboard", data: commandNameElement.textContent
+                HUD.showForDuration("Yanked #{commandNameElement.textContent}.", 2000)
 
-    # When command names are shown, clicking on them copies their text to the clipboard (and they can be
-    # clicked with link hints).
-    for element in @dialogElement.getElementsByClassName "commandName"
-      do (element) ->
-        element.setAttribute "role", "link"
-        element.addEventListener "click", ->
-          commandName = element.textContent
-          chrome.runtime.sendMessage handler: "copyToClipboard", data: commandName
-          HUD.showForDuration("Yanked #{commandName}.", 2000)
+      @showAdvancedCommands(@getShowAdvancedCommands())
 
-    # "Click" the dialog element (so that it becomes scrollable).
-    DomUtils.simulateClick @dialogElement
+      # "Click" the dialog element (so that it becomes scrollable).
+      DomUtils.simulateClick @dialogElement
 
   hide: -> UIComponentServer.hide()
   toggle: -> @hide()
@@ -52,10 +98,15 @@ HelpDialog =
   # Advanced commands are hidden by default so they don't overwhelm new and casual users.
   #
   toggleAdvancedCommands: (event) ->
+    vimiumHelpDialogContainer = $ "vimiumHelpDialogContainer"
+    scrollHeightBefore = vimiumHelpDialogContainer.scrollHeight
     event.preventDefault()
     showAdvanced = HelpDialog.getShowAdvancedCommands()
     HelpDialog.showAdvancedCommands(!showAdvanced)
     Settings.set("helpDialog_showAdvancedCommands", !showAdvanced)
+    # Try to keep the "show advanced commands" button in the same scroll position.
+    scrollHeightDelta = vimiumHelpDialogContainer.scrollHeight - scrollHeightBefore
+    vimiumHelpDialogContainer.scrollTop += scrollHeightDelta if 0 < scrollHeightDelta
 
   showAdvancedCommands: (visible) ->
     document.getElementById("toggleAdvancedCommands").innerHTML =
diff --git a/pages/help_dialog.html b/pages/help_dialog.html
index c23b2ac1..7bc0d86c 100644
--- a/pages/help_dialog.html
+++ b/pages/help_dialog.html
@@ -66,7 +66,7 @@
         </div>
 
         <div>
-          <table>
+          <table class="helpDialogBottom">
             <tr>
               <td class="helpDialogBottomLeft">
                 <span id="help-dialog-tip"></span>
@@ -96,5 +96,26 @@
         </div>
       </div>
     </div>
+
+
+    <template id="helpDialogEntry">
+      <tr class="vimiumReset">
+        <td class="vimiumReset vimiumKeyBindings"></td>
+        <td class="vimiumReset"></td>
+        <td class="vimiumReset vimiumHelpDescription"></td>
+      </tr>
+    </template>
+
+    <template id="helpDialogEntryBindingsOnly">
+      <tr>
+        <td class="vimiumReset vimiumKeyBindings" colspan="3" style="text-align: left"></td>
+      </tr>
+    </template>
+
+    <template id="keysTemplate"><span><span class="vimiumHelpDialogKey"></span><span class="commaSeparator">, </span></span></template>
+
+    <template id="commandNameTemplate">
+      <span class="vimiumReset vimiumCopyCommandName">(<span class="vimiumCopyCommandNameName" role="link"></span>)</span>
+    </template>
   </body>
 </html>
diff --git a/pages/options.coffee b/pages/options.coffee
index 883405e6..7021322a 100644
--- a/pages/options.coffee
+++ b/pages/options.coffee
@@ -234,9 +234,7 @@ initOptionsPage = ->
     event.preventDefault()
 
   activateHelpDialog = ->
-    request = showUnboundCommands: true, showCommandNames: true, customTitle: "Command Listing"
-    chrome.runtime.sendMessage extend(request, handler: "getHelpDialogHtml"), (response) ->
-      HelpDialog.toggle {html: response}
+    HelpDialog.toggle showAllCommandDetails: true
 
   saveOptions = ->
     Option.saveOptions()