Merge pull request #12697 from atom/ns-upgrade-atom-keymap

Improve support for international keyboards

package.json

@@ -15,7 +15,7 @@
   "electronVersion": "1.3.6",
   "dependencies": {
     "async": "0.2.6",
-    "atom-keymap": "6.3.2",
+    "atom-keymap": "6.3.5",
     "atom-ui": "0.4.1",
     "babel-core": "5.8.38",
     "cached-run-in-this-context": "0.4.1",

spec/menu-manager-spec.coffee

@@ -53,6 +53,9 @@ describe "MenuManager", ->
       expect(menu.template[originalItemCount]).toEqual {label: "A", submenu: [{label: "B", command: "b"}]}
 
   describe "::update()", ->
+    originalPlatform = process.platform
+    afterEach -> Object.defineProperty process, 'platform', value: originalPlatform
+
     it "sends the current menu template and associated key bindings to the browser process", ->
       spyOn(menu, 'sendToBrowserProcess')
       menu.add [{label: "A", submenu: [{label: "B", command: "b"}]}]
@@ -75,6 +78,47 @@ describe "MenuManager", ->
 
       runs -> expect(menu.sendToBrowserProcess.argsForCall[0][1]['b']).toBeUndefined()
 
+    it "omits key bindings that could conflict with AltGraph characters on macOS", ->
+      spyOn(menu, 'sendToBrowserProcess')
+      menu.add [{label: "A", submenu: [
+        {label: "B", command: "b"},
+        {label: "C", command: "c"}
+        {label: "D", command: "d"}
+      ]}]
+
+      atom.keymaps.add 'test', 'atom-workspace':
+        'alt-b': 'b'
+        'alt-shift-C': 'c'
+        'alt-cmd-d': 'd'
+
+      waits 50
+
+      runs ->
+        expect(menu.sendToBrowserProcess.argsForCall[0][1]['b']).toBeUndefined()
+        expect(menu.sendToBrowserProcess.argsForCall[0][1]['c']).toBeUndefined()
+        expect(menu.sendToBrowserProcess.argsForCall[0][1]['d']).toEqual(['alt-cmd-d'])
+
+    it "omits key bindings that could conflict with AltGraph characters on Windows", ->
+      Object.defineProperty process, 'platform', value: 'win32'
+      spyOn(menu, 'sendToBrowserProcess')
+      menu.add [{label: "A", submenu: [
+        {label: "B", command: "b"},
+        {label: "C", command: "c"}
+        {label: "D", command: "d"}
+      ]}]
+
+      atom.keymaps.add 'test', 'atom-workspace':
+        'ctrl-alt-b': 'b'
+        'ctrl-alt-shift-C': 'c'
+        'ctrl-alt-cmd-d': 'd'
+
+      waits 50
+
+      runs ->
+        expect(menu.sendToBrowserProcess.argsForCall[0][1]['b']).toBeUndefined()
+        expect(menu.sendToBrowserProcess.argsForCall[0][1]['c']).toBeUndefined()
+        expect(menu.sendToBrowserProcess.argsForCall[0][1]['d']).toEqual(['ctrl-alt-cmd-d'])
+
   it "updates the application menu when a keymap is reloaded", ->
     spyOn(menu, 'update')
     keymapPath = path.join(__dirname, 'fixtures', 'packages', 'package-with-keymaps', 'keymaps', 'keymap-1.cson')

src/menu-manager.coffee

@@ -143,17 +143,20 @@ class MenuManager
   # Public: Refreshes the currently visible menu.
   update: ->
     clearImmediate(@pendingUpdateOperation) if @pendingUpdateOperation?
-    @pendingUpdateOperation = setImmediate =>
-      includedBindings = []
-      unsetKeystrokes = new Set
 
-      for binding in @keymapManager.getKeyBindings() when @includeSelector(binding.selector)
-        includedBindings.push(binding)
+    @pendingUpdateOperation = setImmediate =>
+      unsetKeystrokes = new Set
+      for binding in @keymapManager.getKeyBindings()
         if binding.command is 'unset!'
           unsetKeystrokes.add(binding.keystrokes)
 
       keystrokesByCommand = {}
-      for binding in includedBindings when not unsetKeystrokes.has(binding.keystrokes)
+      for binding in @keymapManager.getKeyBindings()
+        continue unless @includeSelector(binding.selector)
+        continue if unsetKeystrokes.has(binding.keystrokes)
+        continue if binding.keystrokes.includes(' ')
+        continue if process.platform is 'darwin' and /^alt-(shift-)?.$/.test(binding.keystrokes)
+        continue if process.platform is 'win32' and /^ctrl-alt-(shift-)?.$/.test(binding.keystrokes)
         keystrokesByCommand[binding.command] ?= []
         keystrokesByCommand[binding.command].unshift binding.keystrokes
 
@@ -176,21 +179,7 @@ class MenuManager
   unmerge: (menu, item) ->
     MenuHelpers.unmerge(menu, item)
 
-  # macOS can't handle displaying accelerators for multiple keystrokes.
-  # If they are sent across, it will stop processing accelerators for the rest
-  # of the menu items.
-  filterMultipleKeystroke: (keystrokesByCommand) ->
-    filtered = {}
-    for key, bindings of keystrokesByCommand
-      for binding in bindings
-        continue if binding.indexOf(' ') isnt -1
-
-        filtered[key] ?= []
-        filtered[key].push(binding)
-    filtered
-
   sendToBrowserProcess: (template, keystrokesByCommand) ->
-    keystrokesByCommand = @filterMultipleKeystroke(keystrokesByCommand)
     ipcRenderer.send 'update-application-menu', template, keystrokesByCommand
 
   # Get an {Array} of {String} classes for the given element.