diff --git a/spec/context-menu-manager-spec.coffee b/spec/context-menu-manager-spec.coffee index 035d56a61..70ab4b3e7 100644 --- a/spec/context-menu-manager-spec.coffee +++ b/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"] + }] + }]) diff --git a/spec/menu-sort-helpers-spec.js b/spec/menu-sort-helpers-spec.js new file mode 100644 index 000000000..86f00b37e --- /dev/null +++ b/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' } + ]) + }) + }) +}) diff --git a/src/context-menu-manager.coffee b/src/context-menu-manager.coffee index 10a7a3bdb..9cff5497b 100644 --- a/src/context-menu-manager.coffee +++ b/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 diff --git a/src/menu-helpers.js b/src/menu-helpers.js index 398feb40e..12598764e 100644 --- a/src/menu-helpers.js +++ b/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)) diff --git a/src/menu-sort-helpers.js b/src/menu-sort-helpers.js new file mode 100644 index 000000000..259f8321e --- /dev/null +++ b/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}