Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add menu item order control #12362

Merged
merged 16 commits into from May 5, 2018
78 changes: 43 additions & 35 deletions docs/api/menu.md
Expand Up @@ -302,39 +302,31 @@ 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. 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.
* `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.

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.
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.

By default, items will be inserted in the order they exist in the template unless one of the specified positioning keywords is used.

### Examples

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' }
]
```

Expand All @@ -345,33 +337,49 @@ 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
- ---
- a
- b
- c
- 3
- 4
- ---
- 1
- ---
- 2
```

Template:

```javascript
[
{ id: '1', label: 'one', after: ['3'] },
{ id: '2', label: 'two', before: ['1'] },
{ id: '3', label: 'three' }
]
```

Menu:

```sh
- ---
- 3
- 2
- 1
```

[AboutInformationPropertyListFiles]: https://developer.apple.com/library/ios/documentation/general/Reference/InfoPlistKeyReference/Articles/AboutInformationPropertyListFiles.html
Expand Down
1 change: 1 addition & 0 deletions filenames.gypi
Expand Up @@ -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',
Expand Down
168 changes: 168 additions & 0 deletions lib/browser/api/menu-utils.js
@@ -0,0 +1,168 @@
function splitArray (arr, predicate) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really want this to be a reduce (not a fan of a forEach creating a new array)

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

Not sure if that's ^^ the best way to do it but would love to see the forEach go away 😄

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++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can be a const here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't that not let me increment i?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

derp sorry

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}
64 changes: 14 additions & 50 deletions 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')
Expand Down Expand Up @@ -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')
}
Expand All @@ -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) {
Expand All @@ -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) => {
Expand Down