Merge pull request #16661 from captbaritone/context-menu-order-master

Enable packages to control order of context menu items

spec/context-menu-manager-spec.coffee

@@ -333,3 +333,46 @@ describe "ContextMenuManager", ->
             }
           ]
         ])
+
+  describe "::templateForEvent(target) (sorting)", ->
+    it "applies simple sorting rules", ->
+      contextMenu.add('.parent': [{
+        label: 'My Command',
+        command: "test:my-command",
+        after: ["test:my-other-command"]
+      }, {
+        label: 'My Other Command',
+        command: "test:my-other-command",
+      }])
+      dispatchedEvent = {target: parent}
+      expect(contextMenu.templateForEvent(dispatchedEvent)).toEqual([{
+        label: 'My Other Command',
+        command: 'test:my-other-command',
+      }, {
+        label: 'My Command',
+        command: 'test:my-command',
+        after: ["test:my-other-command"]
+      }])
+
+    it "applies sorting rules recursively to submenus", ->
+      contextMenu.add('.parent': [{
+        submenu: [{
+          label: 'My Command',
+          command: "test:my-command",
+          after: ["test:my-other-command"]
+        }, {
+          label: 'My Other Command',
+          command: "test:my-other-command",
+        }]
+      }])
+      dispatchedEvent = {target: parent}
+      expect(contextMenu.templateForEvent(dispatchedEvent)).toEqual([{
+        submenu: [{
+          label: 'My Other Command',
+          command: 'test:my-other-command',
+        }, {
+          label: 'My Command',
+          command: 'test:my-command',
+          after: ["test:my-other-command"]
+        }]
+      }])

spec/menu-sort-helpers-spec.js

@@ -0,0 +1,243 @@
+const {sortMenuItems} = require('../src/menu-sort-helpers')
+
+describe('contextMenu', () => {
+  describe('dedupes separators', () => {
+    it('preserves existing submenus', () => {
+      const items = [{ submenu: [] }]
+      expect(sortMenuItems(items)).toEqual(items)
+    })
+  })
+
+  describe('dedupes separators', () => {
+    it('trims leading separators', () => {
+      const items = [{ type: 'separator' }, { command: 'core:one' }]
+      const expected = [{ command: 'core:one' }]
+      expect(sortMenuItems(items)).toEqual(expected)
+    })
+
+    it('preserves separators at the begining of set two', () => {
+      const items = [
+        { command: 'core:one' },
+        { type: 'separator' }, { command: 'core:two' }
+      ]
+      const expected = [
+        { command: 'core:one' },
+        { type: 'separator' },
+        { command: 'core:two' }
+      ]
+      expect(sortMenuItems(items)).toEqual(expected)
+    })
+
+    it('trims trailing separators', () => {
+      const items = [{ command: 'core:one' }, { type: 'separator' }]
+      const expected = [{ command: 'core:one' }]
+      expect(sortMenuItems(items)).toEqual(expected)
+    })
+
+    it('removes duplicate separators across sets', () => {
+      const items = [
+        { command: 'core:one' }, { type: 'separator' },
+        { type: 'separator' }, { command: 'core:two' }
+      ]
+      const expected = [
+        { command: 'core:one' },
+        { type: 'separator' },
+        { command: 'core:two' }
+      ]
+      expect(sortMenuItems(items)).toEqual(expected)
+    })
+  })
+
+  describe('can move an item to a different group by merging groups', () => {
+    it('can move a group of one item', () => {
+      const items = [
+        { command: 'core:one' },
+        { type: 'separator' },
+        { command: 'core:two' },
+        { type: 'separator' },
+        { command: 'core:three', after: ['core:one'] },
+        { type: 'separator' }
+      ]
+      const expected = [
+        { command: 'core:one' },
+        { command: 'core:three', after: ['core:one'] },
+        { type: 'separator' },
+        { command: 'core:two' }
+      ]
+      expect(sortMenuItems(items)).toEqual(expected)
+    })
+
+    it("moves all items in the moving item's group", () => {
+      const items = [
+        { command: 'core:one' },
+        { type: 'separator' },
+        { command: 'core:two' },
+        { type: 'separator' },
+        { command: 'core:three', after: ['core:one'] },
+        { command: 'core:four' },
+        { type: 'separator' }
+      ]
+      const expected = [
+        { command: 'core:one' },
+        { command: 'core:three', after: ['core:one'] },
+        { command: 'core:four' },
+        { type: 'separator' },
+        { command: 'core:two' }
+      ]
+      expect(sortMenuItems(items)).toEqual(expected)
+    })
+
+    it("ignores positions relative to commands that don't exist", () => {
+      const items = [
+        { command: 'core:one' },
+        { type: 'separator' },
+        { command: 'core:two' },
+        { type: 'separator' },
+        { command: 'core:three', after: ['core:does-not-exist'] },
+        { command: 'core:four', after: ['core:one'] },
+        { type: 'separator' }
+      ]
+      const expected = [
+        { command: 'core:one' },
+        { command: 'core:three', after: ['core:does-not-exist'] },
+        { command: 'core:four', after: ['core:one'] },
+        { type: 'separator' },
+        { command: 'core:two' }
+      ]
+      expect(sortMenuItems(items)).toEqual(expected)
+    })
+
+    it('can handle recursive group merging', () => {
+      const items = [
+        { command: 'core:one', after: ['core:three'] },
+        { command: 'core:two', before: ['core:one'] },
+        { command: 'core:three' }
+      ]
+      const expected = [
+        { command: 'core:three' },
+        { command: 'core:two', before: ['core:one'] },
+        { command: 'core:one', after: ['core:three'] }
+      ]
+      expect(sortMenuItems(items)).toEqual(expected)
+    })
+
+    it('can merge multiple groups when given a list of before/after commands', () => {
+      const items = [
+        { command: 'core:one' },
+        { type: 'separator' },
+        { command: 'core:two' },
+        { type: 'separator' },
+        { command: 'core:three', after: ['core:one', 'core:two'] }
+      ]
+      const expected = [
+        { command: 'core:two' },
+        { command: 'core:one' },
+        { command: 'core:three', after: ['core:one', 'core:two'] }
+      ]
+      expect(sortMenuItems(items)).toEqual(expected)
+    })
+
+    it('can merge multiple groups based on both before/after commands', () => {
+      const items = [
+        { command: 'core:one' },
+        { type: 'separator' },
+        { command: 'core:two' },
+        { type: 'separator' },
+        { command: 'core:three', after: ['core:one'], before: ['core:two'] }
+      ]
+      const expected = [
+        { command: 'core:one' },
+        { command: 'core:three', after: ['core:one'], before: ['core:two'] },
+        { command: 'core:two' }
+      ]
+      expect(sortMenuItems(items)).toEqual(expected)
+    })
+  })
+
+  describe('sorts items within their ultimate group', () => {
+    it('does a simple sort', () => {
+      const items = [
+        { command: 'core:two', after: ['core:one'] },
+        { command: 'core:one' }
+      ]
+      expect(sortMenuItems(items)).toEqual([
+        { command: 'core:one' },
+        { command: 'core:two', after: ['core:one'] }
+      ])
+    })
+
+    it('resolves cycles by ignoring things that conflict', () => {
+      const items = [
+        { command: 'core:two', after: ['core:one'] },
+        { command: 'core:one', after: ['core:two'] }
+      ]
+      expect(sortMenuItems(items)).toEqual([
+        { command: 'core:one', after: ['core:two'] },
+        { command: 'core:two', after: ['core:one'] }
+      ])
+    })
+  })
+
+  describe('sorts groups', () => {
+    it('does a simple sort', () => {
+      const items = [
+        { command: 'core:two', afterGroupContaining: ['core:one'] },
+        { type: 'separator' },
+        { command: 'core:one' }
+      ]
+      expect(sortMenuItems(items)).toEqual([
+        { command: 'core:one' },
+        { type: 'separator' },
+        { command: 'core:two', afterGroupContaining: ['core:one'] }
+      ])
+    })
+
+    it('resolves cycles by ignoring things that conflict', () => {
+      const items = [
+        { command: 'core:two', afterGroupContaining: ['core:one'] },
+        { type: 'separator' },
+        { command: 'core:one', afterGroupContaining: ['core:two'] }
+      ]
+      expect(sortMenuItems(items)).toEqual([
+        { command: 'core:one', afterGroupContaining: ['core:two'] },
+        { type: 'separator' },
+        { command: 'core:two', afterGroupContaining: ['core:one'] }
+      ])
+    })
+
+    it('ignores references to commands that do not exist', () => {
+      const items = [
+        { command: 'core:one' },
+        { type: 'separator' },
+        {
+          command: 'core:two',
+          afterGroupContaining: ['core:does-not-exist']
+        }
+      ]
+      expect(sortMenuItems(items)).toEqual([
+        { command: 'core:one' },
+        { type: 'separator' },
+        { command: 'core:two', afterGroupContaining: ['core:does-not-exist'] }
+      ])
+    })
+
+    it('only respects the first matching [before|after]GroupContaining rule in a given group', () => {
+      const items = [
+        { command: 'core:one' },
+        { type: 'separator' },
+        { command: 'core:three', beforeGroupContaining: ['core:one'] },
+        { command: 'core:four', afterGroupContaining: ['core:two'] },
+        { type: 'separator' },
+        { command: 'core:two' }
+      ]
+      expect(sortMenuItems(items)).toEqual([
+        { command: 'core:three', beforeGroupContaining: ['core:one'] },
+        { command: 'core:four', afterGroupContaining: ['core:two'] },
+        { type: 'separator' },
+        { command: 'core:one' },
+        { type: 'separator' },
+        { command: 'core:two' }
+      ])
+    })
+  })
+})

src/context-menu-manager.coffee

@@ -5,6 +5,7 @@ fs = require 'fs-plus'
 {Disposable} = require 'event-kit'
 {remote} = require 'electron'
 MenuHelpers = require './menu-helpers'
+{sortMenuItems} = require './menu-sort-helpers'
 
 platformContextMenu = require('../package.json')?._atomMenu?['context-menu']
 
@@ -149,7 +150,7 @@ class ContextMenuManager
     @pruneRedundantSeparators(template)
     @addAccelerators(template)
 
-    template
+    return @sortTemplate(template)
 
   # Adds an `accelerator` property to items that have key bindings. Electron
   # uses this property to surface the relevant keymaps in the context menu.
@@ -175,6 +176,13 @@ class ContextMenuManager
         keepNextItemIfSeparator = true
         index++
 
+  sortTemplate: (template) ->
+    template = sortMenuItems(template)
+    for id, item of template
+      if Array.isArray(item.submenu)
+        item.submenu = @sortTemplate(item.submenu)
+    return template
+
   # Returns an object compatible with `::add()` or `null`.
   cloneItemForEvent: (item, event) ->
     return null if item.devMode and not @devMode

src/menu-helpers.js

@@ -83,7 +83,11 @@ function cloneMenuItem (item) {
     'submenu',
     'commandDetail',
     'role',
-    'accelerator'
+    'accelerator',
+    'before',
+    'after',
+    'beforeGroupContaining',
+    'afterGroupContaining'
   )
   if (item.submenu != null) {
     item.submenu = item.submenu.map(submenuItem => cloneMenuItem(submenuItem))

src/menu-sort-helpers.js

@@ -0,0 +1,186 @@
+// UTILS
+
+function splitArray (arr, predicate) {
+  let lastArr = []
+  const multiArr = [lastArr]
+  arr.forEach(item => {
+    if (predicate(item)) {
+      if (lastArr.length > 0) {
+        lastArr = []
+        multiArr.push(lastArr)
+      }
+    } else {
+      lastArr.push(item)
+    }
+  })
+  return multiArr
+}
+
+function joinArrays (arrays, joiner) {
+  const joinedArr = []
+  arrays.forEach((arr, i) => {
+    if (i > 0 && arr.length > 0) {
+      joinedArr.push(joiner)
+    }
+    joinedArr.push(...arr)
+  })
+  return joinedArr
+}
+
+const pushOntoMultiMap = (map, key, value) => {
+  if (!map.has(key)) {
+    map.set(key, [])
+  }
+  map.get(key).push(value)
+}
+
+function indexOfGroupContainingCommand (groups, command, ignoreGroup) {
+  return groups.findIndex(
+    candiateGroup =>
+      candiateGroup !== ignoreGroup &&
+      candiateGroup.some(
+        candidateItem => candidateItem.command === command
+      )
+  )
+}
+
+// Sort nodes topologically using a depth-first approach. Encountered cycles
+// are broken.
+function sortTopologically (originalOrder, edgesById) {
+  const sorted = []
+  const marked = new Set()
+
+  function visit (id) {
+    if (marked.has(id)) {
+      // Either this node has already been placed, or we have encountered a
+      // cycle and need to exit.
+      return
+    }
+    marked.add(id)
+    const edges = edgesById.get(id)
+    if (edges != null) {
+      edges.forEach(visit)
+    }
+    sorted.push(id)
+  }
+
+  originalOrder.forEach(visit)
+  return sorted
+}
+
+function attemptToMergeAGroup (groups) {
+  for (let i = 0; i < groups.length; i++) {
+    const group = groups[i]
+    for (const item of group) {
+      const toCommands = [...(item.before || []), ...(item.after || [])]
+      for (const command of toCommands) {
+        const index = indexOfGroupContainingCommand(groups, command, group)
+        if (index === -1) {
+          // No valid edge for this command
+          continue
+        }
+        const mergeTarget = groups[index]
+        // Merge with group containing `command`
+        mergeTarget.push(...group)
+        groups.splice(i, 1)
+        return true
+      }
+    }
+  }
+  return false
+}
+
+// Merge groups based on before/after positions
+// Mutates both the array of groups, and the individual group arrays.
+function mergeGroups (groups) {
+  let mergedAGroup = true
+  while (mergedAGroup) {
+    mergedAGroup = attemptToMergeAGroup(groups)
+  }
+  return groups
+}
+
+function sortItemsInGroup (group) {
+  const originalOrder = group.map((node, i) => i)
+  const edges = new Map()
+  const commandToIndex = new Map(group.map((item, i) => [item.command, i]))
+
+  group.forEach((item, i) => {
+    if (item.before) {
+      item.before.forEach(toCommand => {
+        const to = commandToIndex.get(toCommand)
+        if (to != null) {
+          pushOntoMultiMap(edges, to, i)
+        }
+      })
+    }
+    if (item.after) {
+      item.after.forEach(toCommand => {
+        const to = commandToIndex.get(toCommand)
+        if (to != null) {
+          pushOntoMultiMap(edges, i, to)
+        }
+      })
+    }
+  })
+
+  const sortedNodes = sortTopologically(originalOrder, edges)
+
+  return sortedNodes.map(i => group[i])
+}
+
+function findEdgesInGroup (groups, i, edges) {
+  const group = groups[i]
+  for (const item of group) {
+    if (item.beforeGroupContaining) {
+      for (const command of item.beforeGroupContaining) {
+        const to = indexOfGroupContainingCommand(groups, command, group)
+        if (to !== -1) {
+          pushOntoMultiMap(edges, to, i)
+          return
+        }
+      }
+    }
+    if (item.afterGroupContaining) {
+      for (const command of item.afterGroupContaining) {
+        const to = indexOfGroupContainingCommand(groups, command, group)
+        if (to !== -1) {
+          pushOntoMultiMap(edges, i, to)
+          return
+        }
+      }
+    }
+  }
+}
+
+function sortGroups (groups) {
+  const originalOrder = groups.map((item, i) => i)
+  const edges = new Map()
+
+  for (let i = 0; i < groups.length; i++) {
+    findEdgesInGroup(groups, i, edges)
+  }
+
+  const sortedGroupIndexes = sortTopologically(originalOrder, edges)
+  return sortedGroupIndexes.map(i => groups[i])
+}
+
+function isSeparator (item) {
+  return item.type === 'separator'
+}
+
+function sortMenuItems (menuItems) {
+  // Split the items into their implicit groups based upon separators.
+  const groups = splitArray(menuItems, isSeparator)
+  // Merge groups that contain before/after references to eachother.
+  const mergedGroups = mergeGroups(groups)
+  // Sort each individual group internally.
+  const mergedGroupsWithSortedItems = mergedGroups.map(sortItemsInGroup)
+  // Sort the groups based upon their beforeGroupContaining/afterGroupContaining
+  // references.
+  const sortedGroups = sortGroups(mergedGroupsWithSortedItems)
+  // Join the groups back
+  return joinArrays(sortedGroups, { type: 'separator' })
+}
+
+module.exports = {sortMenuItems}