From 14c39620f07fc9b53eb01759816674d5cc688ab5 Mon Sep 17 00:00:00 2001
From: "trop[bot]" <trop[bot]@users.noreply.github.com>
Date: Mon, 11 Feb 2019 13:15:00 -0800
Subject: [PATCH] feat: allow Menu.buildFromTemplate() to accept MenuItems
 (backport: 5-0-x) (#16783)

* feat: allow Menu.buildFromTemplate to accept MenuItems

* add another spec

* fix linter error

* add submenu spec
---
 docs/api/menu.md        |  5 ++-
 lib/browser/api/menu.js | 14 ++++++--
 spec/api-menu-spec.js   | 74 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 88 insertions(+), 5 deletions(-)

diff --git a/docs/api/menu.md b/docs/api/menu.md
index 63c0739efb9d5..e89091d4a7583 100644
--- a/docs/api/menu.md
+++ b/docs/api/menu.md
@@ -52,15 +52,14 @@ for more information on macOS' native actions.
 
 #### `Menu.buildFromTemplate(template)`
 
-* `template` MenuItemConstructorOptions[]
+* `template` (MenuItemConstructorOptions | MenuItem)[]
 
 Returns `Menu`
 
 Generally, the `template` is an array of `options` for constructing a
 [MenuItem](menu-item.md). The usage can be referenced above.
 
-You can also attach other fields to the element of the `template` and they
-will become properties of the constructed menu items.
+You can also attach other fields to the element of the `template` and they will become properties of the constructed menu items.
 
 ### Instance Methods
 
diff --git a/lib/browser/api/menu.js b/lib/browser/api/menu.js
index 2a977538c2027..b7a7ce80b3699 100644
--- a/lib/browser/api/menu.js
+++ b/lib/browser/api/menu.js
@@ -168,7 +168,13 @@ Menu.buildFromTemplate = function (template) {
   const sorted = sortTemplate(filtered)
 
   const menu = new Menu()
-  sorted.forEach((item) => menu.append(new MenuItem(item)))
+  sorted.forEach(item => {
+    if (item instanceof MenuItem) {
+      menu.append(item)
+    } else {
+      menu.append(new MenuItem(item))
+    }
+  })
 
   return menu
 }
@@ -178,7 +184,11 @@ Menu.buildFromTemplate = function (template) {
 // validate the template against having the wrong attribute
 function areValidTemplateItems (template) {
   return template.every(item =>
-    item != null && typeof item === 'object' && (item.hasOwnProperty('label') || item.hasOwnProperty('role') || item.type === 'separator'))
+    item != null &&
+    typeof item === 'object' &&
+    (item.hasOwnProperty('label') ||
+     item.hasOwnProperty('role') ||
+     item.type === 'separator'))
 }
 
 function sortTemplate (template) {
diff --git a/spec/api-menu-spec.js b/spec/api-menu-spec.js
index 7b9a8a410b86d..a1a2a6f946c7e 100644
--- a/spec/api-menu-spec.js
+++ b/spec/api-menu-spec.js
@@ -21,6 +21,37 @@ describe('Menu module', () => {
       expect(menu.items[0].extra).to.equal('field')
     })
 
+    it('should be able to accept only MenuItems', () => {
+      const menu = Menu.buildFromTemplate([
+        new MenuItem({ label: 'one' }),
+        new MenuItem({ label: 'two' })
+      ])
+      expect(menu.items[0].label).to.equal('one')
+      expect(menu.items[1].label).to.equal('two')
+    })
+
+    it('should be able to accept only MenuItems in a submenu', () => {
+      const menu = Menu.buildFromTemplate([
+        {
+          label: 'one',
+          submenu: [
+            new MenuItem({ label: 'two' })
+          ]
+        }
+      ])
+      expect(menu.items[0].label).to.equal('one')
+      expect(menu.items[0].submenu.items[0].label).to.equal('two')
+    })
+
+    it('should be able to accept MenuItems and plain objects', () => {
+      const menu = Menu.buildFromTemplate([
+        new MenuItem({ label: 'one' }),
+        { label: 'two' }
+      ])
+      expect(menu.items[0].label).to.equal('one')
+      expect(menu.items[1].label).to.equal('two')
+    })
+
     it('does not modify the specified template', () => {
       const template = [{ label: 'text', submenu: [{ label: 'sub' }] }]
       const result = ipcRenderer.sendSync('eval', `const template = [{label: 'text', submenu: [{label: 'sub'}]}]\nrequire('electron').Menu.buildFromTemplate(template)\ntemplate`)
@@ -90,6 +121,21 @@ describe('Menu module', () => {
           expect(sortMenuItems(items)).to.deep.equal(expected)
         })
 
+        it('does a simple sort with MenuItems', () => {
+          const firstItem = new MenuItem({ id: '1', label: 'one' })
+          const secondItem = new MenuItem({
+            label: 'two',
+            id: '2',
+            afterGroupContaining: ['1']
+          })
+          const sep = new MenuItem({ type: 'separator' })
+
+          const items = [ secondItem, sep, firstItem ]
+          const expected = [ firstItem, sep, secondItem ]
+
+          expect(sortMenuItems(items)).to.deep.equal(expected)
+        })
+
         it('resolves cycles by ignoring things that conflict', () => {
           const items = [
             {
@@ -571,6 +617,34 @@ describe('Menu module', () => {
         expect(menu.items[3].label).to.equal('four')
         expect(menu.items[4].label).to.equal('five')
       })
+
+      it('should continue inserting MenuItems at next index when no specifier is present', () => {
+        const menu = Menu.buildFromTemplate([
+          new MenuItem({
+            id: '2',
+            label: 'two'
+          }), new MenuItem({
+            id: '3',
+            label: 'three'
+          }), new MenuItem({
+            id: '4',
+            label: 'four'
+          }), new MenuItem({
+            id: '5',
+            label: 'five'
+          }), new MenuItem({
+            id: '1',
+            label: 'one',
+            before: ['2']
+          })
+        ])
+
+        expect(menu.items[0].label).to.equal('one')
+        expect(menu.items[1].label).to.equal('two')
+        expect(menu.items[2].label).to.equal('three')
+        expect(menu.items[3].label).to.equal('four')
+        expect(menu.items[4].label).to.equal('five')
+      })
     })
   })