diff --git a/docs/api/menu.md b/docs/api/menu.md index 4c118879610f..227535ad13c3 100644 --- a/docs/api/menu.md +++ b/docs/api/menu.md @@ -302,27 +302,20 @@ browser windows. ## Menu Item Position -You can make use of `position` and `id` to control how the item will be placed -when building a menu with `Menu.buildFromTemplate`. +You can make use of `before`, `after`, `beforeGroupContaining`, `afterGroupContaining` and `id` to control how the item will be placed when building a menu with `Menu.buildFromTemplate`. -The `position` attribute of `MenuItem` has the form `[placement]=[id]`, where -`placement` is one of `before`, `after`, or `endof` and `id` is the unique ID of -an existing item in the menu: - -* `before` - Inserts this item before the id referenced item. If the +* `before` - Inserts this item before the item with the specified label. If the referenced item doesn't exist the item will be inserted at the end of - the menu. -* `after` - Inserts this item after id referenced item. If the referenced - item doesn't exist the item will be inserted at the end of the menu. -* `endof` - Inserts this item at the end of the logical group containing - the id referenced item (groups are created by separator items). If - the referenced item doesn't exist, a new separator group is created with - the given id and this item is inserted after that separator. + the menu. Also implies that the menu item in question should be placed in the same “group” as the item. +* `after` - Inserts this item after the item with the specified label. If the + referenced item doesn't exist the item will be inserted at the end of + the menu. Also implies that the menu item in question should be placed in the same “group” as the item. +* `beforeGroupContaining` - Provides a means for a single context menu to declare + the placement of their containing group before the containing group of the item with the specified label. +* `afterGroupContaining` - Provides a means for a single context menu to declare + the placement of their containing group after the containing group of the item with the specified label. -When an item is positioned, all un-positioned items are inserted after -it until a new item is positioned. So if you want to position a group of -menu items in the same location you only need to specify a position for -the first item. +By default, items will be inserted in the order they exist in the template unless one of the specified positioning keywords is used. ### Examples @@ -330,11 +323,10 @@ Template: ```javascript [ - {label: '4', id: '4'}, - {label: '5', id: '5'}, - {label: '1', id: '1', position: 'before=4'}, - {label: '2', id: '2'}, - {label: '3', id: '3'} + { id: '1', label: 'one' }, + { id: '2', label: 'two' }, + { id: '3', label: 'three' }, + { id: '4', label: 'four' } ] ``` @@ -345,19 +337,39 @@ Menu: - 2 - 3 - 4 -- 5 ``` Template: ```javascript [ - {label: 'a', position: 'endof=letters'}, - {label: '1', position: 'endof=numbers'}, - {label: 'b', position: 'endof=letters'}, - {label: '2', position: 'endof=numbers'}, - {label: 'c', position: 'endof=letters'}, - {label: '3', position: 'endof=numbers'} + { id: '1', label: 'one' }, + { type: 'separator' }, + { id: '3', label: 'three', beforeGroupContaining: ['1'] }, + { id: '4', label: 'four', afterGroupContaining: ['2'] }, + { type: 'separator' }, + { id: '2', label: 'two' } +] +``` + +Menu: + +```sh +- 3 +- 4 +- --- +- 1 +- --- +- 2 +``` + +Template: + +```javascript +[ + { id: '1', label: 'one', after: ['3'] }, + { id: '2', label: 'two', before: ['1'] }, + { id: '3', label: 'three' } ] ``` @@ -365,13 +377,9 @@ Menu: ```sh - --- -- a -- b -- c -- --- -- 1 -- 2 - 3 +- 2 +- 1 ``` [AboutInformationPropertyListFiles]: https://developer.apple.com/library/ios/documentation/general/Reference/InfoPlistKeyReference/Articles/AboutInformationPropertyListFiles.html diff --git a/filenames.gypi b/filenames.gypi index 0ba5a9d73b6a..43f019e945c0 100644 --- a/filenames.gypi +++ b/filenames.gypi @@ -23,6 +23,7 @@ 'lib/browser/api/in-app-purchase.js', 'lib/browser/api/menu-item-roles.js', 'lib/browser/api/menu-item.js', + 'lib/browser/api/menu-utils.js', 'lib/browser/api/menu.js', 'lib/browser/api/module-list.js', 'lib/browser/api/navigation-controller.js', diff --git a/lib/browser/api/menu-utils.js b/lib/browser/api/menu-utils.js new file mode 100644 index 000000000000..283e09933d1c --- /dev/null +++ b/lib/browser/api/menu-utils.js @@ -0,0 +1,168 @@ +function splitArray (arr, predicate) { + const result = arr.reduce((multi, item) => { + const current = multi[multi.length - 1] + if (predicate(item)) { + if (current.length > 0) multi.push([]) + } else { + current.push(item) + } + return multi + }, [[]]) + + if (result[result.length - 1].length === 0) { + return result.slice(0, result.length - 1) + } + return result +} + +function joinArrays (arrays, joiner) { + return arrays.reduce((joined, arr, i) => { + if (i > 0 && arr.length) { + joined.push(joiner) + } + return joined.concat(arr) + }, []) +} + +function pushOntoMultiMap (map, key, value) { + if (!map.has(key)) { + map.set(key, []) + } + map.get(key).push(value) +} + +function indexOfGroupContainingID (groups, id, ignoreGroup) { + return groups.findIndex( + candidateGroup => + candidateGroup !== ignoreGroup && + candidateGroup.some( + candidateItem => candidateItem.id === id + ) + ) +} + +// Sort nodes topologically using a depth-first approach. Encountered cycles +// are broken. +function sortTopologically (originalOrder, edgesById) { + const sorted = [] + const marked = new Set() + + const visit = (mark) => { + if (marked.has(mark)) return + marked.add(mark) + const edges = edgesById.get(mark) + if (edges != null) { + edges.forEach(visit) + } + sorted.push(mark) + } + + 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 toIDs = [...(item.before || []), ...(item.after || [])] + for (const id of toIDs) { + const index = indexOfGroupContainingID(groups, id, group) + if (index === -1) continue + const mergeTarget = groups[index] + + mergeTarget.push(...group) + groups.splice(i, 1) + return true + } + } + } + return false +} + +function mergeGroups (groups) { + let merged = true + while (merged) { + merged = attemptToMergeAGroup(groups) + } + return groups +} + +function sortItemsInGroup (group) { + const originalOrder = group.map((node, i) => i) + const edges = new Map() + const idToIndex = new Map(group.map((item, i) => [item.id, i])) + + group.forEach((item, i) => { + if (item.before) { + item.before.forEach(toID => { + const to = idToIndex.get(toID) + if (to != null) { + pushOntoMultiMap(edges, to, i) + } + }) + } + if (item.after) { + item.after.forEach(toID => { + const to = idToIndex.get(toID) + 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 id of item.beforeGroupContaining) { + const to = indexOfGroupContainingID(groups, id, group) + if (to !== -1) { + pushOntoMultiMap(edges, to, i) + return + } + } + } + if (item.afterGroupContaining) { + for (const id of item.afterGroupContaining) { + const to = indexOfGroupContainingID(groups, id, 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 sortMenuItems (menuItems) { + const isSeparator = (item) => item.type === 'separator' + + // Split the items into their implicit groups based upon separators. + const groups = splitArray(menuItems, isSeparator) + const mergedGroups = mergeGroups(groups) + const mergedGroupsWithSortedItems = mergedGroups.map(sortItemsInGroup) + const sortedGroups = sortGroups(mergedGroupsWithSortedItems) + + const joined = joinArrays(sortedGroups, { type: 'separator' }) + return joined +} + +module.exports = {sortMenuItems} diff --git a/lib/browser/api/menu.js b/lib/browser/api/menu.js index 4edbc3a10b97..dcc446587913 100644 --- a/lib/browser/api/menu.js +++ b/lib/browser/api/menu.js @@ -1,6 +1,7 @@ 'use strict' const {TopLevelWindow, MenuItem, webContents} = require('electron') +const {sortMenuItems} = require('./menu-utils') const EventEmitter = require('events').EventEmitter const v8Util = process.atomBinding('v8_util') const bindings = process.atomBinding('menu') @@ -160,18 +161,9 @@ Menu.buildFromTemplate = function (template) { const menu = new Menu() const filtered = removeExtraSeparators(template) + const sorted = sortTemplate(filtered) - const positioned = [] - let idx = 0 - - // sort template by position - filtered.forEach(item => { - idx = (item.position) ? indexToInsertByPosition(positioned, item.position) : idx += 1 - positioned.splice(idx, 0, item) - }) - - // add each item from positioned menu to application menu - positioned.forEach((item) => { + sorted.forEach((item) => { if (typeof item !== 'object') { throw new TypeError('Invalid template for MenuItem') } @@ -183,6 +175,17 @@ Menu.buildFromTemplate = function (template) { /* Helper Functions */ +function sortTemplate (template) { + const sorted = sortMenuItems(template) + for (let id in sorted) { + const item = sorted[id] + if (Array.isArray(item.submenu)) { + item.submenu = sortTemplate(item.submenu) + } + } + return sorted +} + // Search between separators to find a radio menu item and return its group id function generateGroupId (items, pos) { if (pos > 0) { @@ -200,45 +203,6 @@ function generateGroupId (items, pos) { return groupIdIndex } -function indexOfItemById (items, id) { - const foundItem = items.find(item => item.id === id) || -1 - return items.indexOf(foundItem) -} - -function indexToInsertByPosition (items, position) { - if (!position) return items.length - - const [query, id] = position.split('=') // parse query and id from position - const idx = indexOfItemById(items, id) // calculate initial index of item - - // warn if query doesn't exist - if (idx === -1 && query !== 'endof') { - console.warn(`Item with id ${id} is not found`) - return items.length - } - - // compute new index based on query - const queries = { - after: (index) => { - index += 1 - return index - }, - endof: (index) => { - if (index === -1) { - items.push({id, type: 'separator'}) - index = items.length - 1 - } - - index += 1 - while (index < items.length && items[index].type !== 'separator') index += 1 - return index - } - } - - // return new index if needed, or original indexOfItemById - return (query in queries) ? queries[query](idx) : idx -} - function removeExtraSeparators (items) { // fold adjacent separators together let ret = items.filter((e, idx, arr) => { diff --git a/spec/api-menu-spec.js b/spec/api-menu-spec.js index 9aff30beeb6b..4a25e3ce4d7c 100644 --- a/spec/api-menu-spec.js +++ b/spec/api-menu-spec.js @@ -2,6 +2,7 @@ const assert = require('assert') const {ipcRenderer, remote} = require('electron') const {BrowserWindow, Menu, MenuItem} = remote +const {sortMenuItems} = require('../lib/browser/api/menu-utils') const {closeWindow} = require('./window-helpers') describe('Menu module', () => { @@ -37,91 +38,445 @@ describe('Menu module', () => { }) }) - describe('Menu.buildFromTemplate should reorder based on item position specifiers', () => { + describe('Menu sorting and building', () => { + describe('sorts groups', () => { + it('does a simple sort', () => { + const items = [ + { + label: 'two', + id: '2', + afterGroupContaining: ['1'] }, + { type: 'separator' }, + { + id: '1', + label: 'one' + } + ] + + const expected = [ + { + id: '1', + label: 'one' + }, + { type: 'separator' }, + { + id: '2', + label: 'two', + afterGroupContaining: ['1'] + } + ] + + assert.deepEqual(sortMenuItems(items), expected) + }) + + it('resolves cycles by ignoring things that conflict', () => { + const items = [ + { + id: '2', + label: 'two', + afterGroupContaining: ['1'] + }, + { type: 'separator' }, + { + id: '1', + label: 'one', + afterGroupContaining: ['2'] + } + ] + + const expected = [ + { + id: '1', + label: 'one', + afterGroupContaining: ['2'] + }, + { type: 'separator' }, + { + id: '2', + label: 'two', + afterGroupContaining: ['1'] + } + ] + + assert.deepEqual(sortMenuItems(items), expected) + }) + + it('ignores references to commands that do not exist', () => { + const items = [ + { + id: '1', + label: 'one' + }, + { type: 'separator' }, + { + id: '2', + label: 'two', + afterGroupContaining: ['does-not-exist'] + } + ] + + const expected = [ + { + id: '1', + label: 'one' + }, + { type: 'separator' }, + { + id: '2', + label: 'two', + afterGroupContaining: ['does-not-exist'] + } + ] + + assert.deepEqual(sortMenuItems(items), expected) + }) + + it('only respects the first matching [before|after]GroupContaining rule in a given group', () => { + const items = [ + { + id: '1', + label: 'one' + }, + { type: 'separator' }, + { + id: '3', + label: 'three', + beforeGroupContaining: ['1'] + }, + { + id: '4', + label: 'four', + afterGroupContaining: ['2'] + }, + { type: 'separator' }, + { + id: '2', + label: 'two' + } + ] + + const expected = [ + { + id: '3', + label: 'three', + beforeGroupContaining: ['1'] + }, + { + id: '4', + label: 'four', + afterGroupContaining: ['2'] + }, + { type: 'separator' }, + { + id: '1', + label: 'one' + }, + { type: 'separator' }, + { + id: '2', + label: 'two' + } + ] + + assert.deepEqual(sortMenuItems(items), expected) + }) + }) + + describe('moves an item to a different group by merging groups', () => { + it('can move a group of one item', () => { + const items = [ + { + id: '1', + label: 'one' + }, + { type: 'separator' }, + { + id: '2', + label: 'two' + }, + { type: 'separator' }, + { + id: '3', + label: 'three', + after: ['1'] + }, + { type: 'separator' } + ] + + const expected = [ + { + id: '1', + label: 'one' + }, + { + id: '3', + label: 'three', + after: ['1'] + }, + { type: 'separator' }, + { + id: '2', + label: 'two' + } + ] + + assert.deepEqual(sortMenuItems(items), expected) + }) + + it("moves all items in the moving item's group", () => { + const items = [ + { + id: '1', + label: 'one' + }, + { type: 'separator' }, + { + id: '2', + label: 'two' + }, + { type: 'separator' }, + { + id: '3', + label: 'three', + after: ['1'] + }, + { + id: '4', + label: 'four' + }, + { type: 'separator' } + ] + + const expected = [ + { + id: '1', + label: 'one' + }, + { + id: '3', + label: 'three', + after: ['1'] + }, + { + id: '4', + label: 'four' + }, + { type: 'separator' }, + { + id: '2', + label: 'two' + } + ] + + assert.deepEqual(sortMenuItems(items), expected) + }) + + it("ignores positions relative to commands that don't exist", () => { + const items = [ + { + id: '1', + label: 'one' + }, + { type: 'separator' }, + { + id: '2', + label: 'two' + }, + { type: 'separator' }, + { + id: '3', + label: 'three', + after: ['does-not-exist'] + }, + { + id: '4', + label: 'four', + after: ['1'] + }, + { type: 'separator' } + ] + + const expected = [ + { + id: '1', + label: 'one' + }, + { + id: '3', + label: 'three', + after: ['does-not-exist'] + }, + { + id: '4', + label: 'four', + after: ['1'] + }, + { type: 'separator' }, + { + id: '2', + label: 'two' + } + ] + + assert.deepEqual(sortMenuItems(items), expected) + }) + + it('can handle recursive group merging', () => { + const items = [ + { + id: '1', + label: 'one', + after: ['3'] + }, + { + id: '2', + label: 'two', + before: ['1'] + }, + { + id: '3', + label: 'three' + } + ] + + const expected = [ + { + id: '3', + label: 'three' + }, + { + id: '2', + label: 'two', + before: ['1'] + }, + { + id: '1', + label: 'one', + after: ['3'] + } + ] + + assert.deepEqual(sortMenuItems(items), expected) + }) + + it('can merge multiple groups when given a list of before/after commands', () => { + const items = [ + { + id: '1', + label: 'one' + }, + { type: 'separator' }, + { + id: '2', + label: 'two' + }, + { type: 'separator' }, + { + id: '3', + label: 'three', + after: ['1', '2'] + } + ] + + const expected = [ + { + id: '2', + label: 'two' + }, + { + id: '1', + label: 'one' + }, + { + id: '3', + label: 'three', + after: ['1', '2'] + } + ] + + assert.deepEqual(sortMenuItems(items), expected) + }) + + it('can merge multiple groups based on both before/after commands', () => { + const items = [ + { + id: '1', + label: 'one' + }, + { type: 'separator' }, + { + id: '2', + label: 'two' + }, + { type: 'separator' }, + { + id: '3', + label: 'three', + after: ['1'], + before: ['2'] + } + ] + + const expected = [ + { + id: '1', + label: 'one' + }, + { + id: '3', + label: 'three', + after: ['1'], + before: ['2'] + }, + { + id: '2', + label: 'two' + } + ] + + assert.deepEqual(sortMenuItems(items), expected) + }) + }) + it('should position before existing item', () => { const menu = Menu.buildFromTemplate([ { - label: '2', - id: '2' + id: '2', + label: 'two' }, { - label: '3', - id: '3' + id: '3', + label: 'three' }, { - label: '1', id: '1', - position: 'before=2' + label: 'one', + before: ['2'] } ]) - assert.equal(menu.items[0].label, '1') - assert.equal(menu.items[1].label, '2') - assert.equal(menu.items[2].label, '3') + + assert.equal(menu.items[0].label, 'one') + assert.equal(menu.items[1].label, 'two') + assert.equal(menu.items[2].label, 'three') }) it('should position after existing item', () => { const menu = Menu.buildFromTemplate([ { - label: '1', - id: '1' - }, { - label: '3', - id: '3' - }, { - label: '2', id: '2', - position: 'after=1' - } - ]) - assert.equal(menu.items[0].label, '1') - assert.equal(menu.items[1].label, '2') - assert.equal(menu.items[2].label, '3') - }) - - it('should position at endof existing separator groups', () => { - const menu = Menu.buildFromTemplate([ + label: 'two', + after: ['1'] + }, { - label: 'first', - id: 'first' - }, { - type: 'separator', - id: 'numbers' - }, { - label: 'a', - id: 'a', - position: 'endof=letters' - }, { - type: 'separator', - id: 'letters' - }, { - label: '1', id: '1', - position: 'endof=numbers' + label: 'one' }, { - label: 'b', - id: 'b', - position: 'endof=letters' - }, { - label: '2', - id: '2', - position: 'endof=numbers' - }, { - label: 'c', - id: 'c', - position: 'endof=letters' - }, { - label: '3', id: '3', - position: 'endof=numbers' + label: 'three' } ]) - assert.equal(menu.items[1].id, 'numbers') - assert.equal(menu.items[2].label, '1') - assert.equal(menu.items[3].label, '2') - assert.equal(menu.items[4].label, '3') - assert.equal(menu.items[5].id, 'letters') - assert.equal(menu.items[6].label, 'a') - assert.equal(menu.items[7].label, 'b') - assert.equal(menu.items[8].label, 'c') + assert.equal(menu.items[0].label, 'one') + assert.equal(menu.items[1].label, 'two') + assert.equal(menu.items[2].label, 'three') }) it('should filter excess menu separators', () => { @@ -168,69 +523,32 @@ describe('Menu module', () => { assert.equal(menuTwo.items[2].label, 'c') }) - it('should create separator group if endof does not reference existing separator group', () => { - const menu = Menu.buildFromTemplate([ - { - label: 'a', - id: 'a', - position: 'endof=letters' - }, { - label: '1', - id: '1', - position: 'endof=numbers' - }, { - label: 'b', - id: 'b', - position: 'endof=letters' - }, { - label: '2', - id: '2', - position: 'endof=numbers' - }, { - label: 'c', - id: 'c', - position: 'endof=letters' - }, { - label: '3', - id: '3', - position: 'endof=numbers' - } - ]) - assert.equal(menu.items[0].id, 'letters') - assert.equal(menu.items[1].label, 'a') - assert.equal(menu.items[2].label, 'b') - assert.equal(menu.items[3].label, 'c') - assert.equal(menu.items[4].id, 'numbers') - assert.equal(menu.items[5].label, '1') - assert.equal(menu.items[6].label, '2') - assert.equal(menu.items[7].label, '3') - }) - it('should continue inserting items at next index when no specifier is present', () => { const menu = Menu.buildFromTemplate([ { - label: '4', - id: '4' + id: '2', + label: 'two' }, { - label: '5', - id: '5' + id: '3', + label: 'three' + }, { + id: '4', + label: 'four' + }, { + id: '5', + label: 'five' }, { - label: '1', id: '1', - position: 'before=4' - }, { - label: '2', - id: '2' - }, { - label: '3', - id: '3' + label: 'one', + before: ['2'] } ]) - assert.equal(menu.items[0].label, '1') - assert.equal(menu.items[1].label, '2') - assert.equal(menu.items[2].label, '3') - assert.equal(menu.items[3].label, '4') - assert.equal(menu.items[4].label, '5') + + assert.equal(menu.items[0].label, 'one') + assert.equal(menu.items[1].label, 'two') + assert.equal(menu.items[2].label, 'three') + assert.equal(menu.items[3].label, 'four') + assert.equal(menu.items[4].label, 'five') }) }) }) @@ -243,7 +561,7 @@ describe('Menu module', () => { submenu: [ { label: 'Enter Fullscreen', - accelerator: 'Control+Command+F', + accelerator: 'ControlCommandF', id: 'fullScreen' } ]