Merge pull request #11968 from electron/refactor-menu-popup

Refactor menu.popup

docs/api/menu.md

@@ -59,10 +59,10 @@ will become properties of the constructed menu items.
 
 The `menu` object has the following instance methods:
 
-#### `menu.popup([browserWindow, options, callback])`
+#### `menu.popup(options)`
 
-* `browserWindow` [BrowserWindow](browser-window.md) (optional) - Default is the focused window.
-* `options` Object (optional)
+* `options` Object
+  * `window` [BrowserWindow](browser-window.md) (optional) - Default is the focused window.
   * `x` Number (optional) - Default is the current mouse cursor position.
     Must be declared if `y` is declared.
   * `y` Number (optional) - Default is the current mouse cursor position.
@@ -70,7 +70,7 @@ The `menu` object has the following instance methods:
   * `positioningItem` Number (optional) _macOS_ - The index of the menu item to
     be positioned under the mouse cursor at the specified coordinates. Default
     is -1.
-* `callback` Function (optional) - Called when menu is closed.
+  * `callback` Function (optional) - Called when menu is closed.
 
 Pops up this menu as a context menu in the [`BrowserWindow`](browser-window.md).
 
@@ -259,7 +259,7 @@ menu.append(new MenuItem({label: 'MenuItem2', type: 'checkbox', checked: true}))
 
 window.addEventListener('contextmenu', (e) => {
   e.preventDefault()
-  menu.popup(remote.getCurrentWindow())
+  menu.popup({window: remote.getCurrentWindow()})
 }, false)
 </script>
 ```

lib/browser/api/menu.js

@@ -46,63 +46,31 @@ Menu.prototype._init = function () {
   this.delegate = delegate
 }
 
-Menu.prototype.popup = function (window, x, y, positioningItem) {
-  let [newX, newY, newPosition, newWindow] = [x, y, positioningItem, window]
-  let opts
-  let callback
-
-  // menu.popup(x, y, positioningItem)
-  if (window != null && !(window instanceof BrowserWindow)) {
-    [newPosition, newY, newX, newWindow] = [y, x, window, null]
-  }
-
-  // menu.popup([w], x, y, callback)
-  if (typeof newPosition === 'function') {
-    callback = newPosition
-  }
-
-  // menu.popup({}) || menu.popup(window, callback)
-  if ((window != null && window.constructor === Object) ||
-      (x && typeof x === 'function')) {
-    opts = window
-    callback = arguments[1]
-  // menu.popup(window, {})
-  } else if (x && typeof x === 'object') {
-    opts = x
-    callback = arguments[2]
-  }
+Menu.prototype.popup = function (options) {
+  let {window, x, y, positioningItem, callback} = options
 
-  if (opts) {
-    newX = opts.x
-    newY = opts.y
-    newPosition = opts.positioningItem
-  }
-  if (typeof callback !== 'function') {
-    callback = () => {}
-  }
+  // no callback passed
+  if (!callback || typeof callback !== 'function') callback = () => {}
 
   // set defaults
-  if (typeof newX !== 'number') newX = -1
-  if (typeof newY !== 'number') newY = -1
-  if (typeof newPosition !== 'number') newPosition = -1
-  if (!newWindow || (newWindow && newWindow.constructor !== BrowserWindow)) {
-    newWindow = BrowserWindow.getFocusedWindow()
-
-    // No window focused?
-    if (!newWindow) {
-      const browserWindows = BrowserWindow.getAllWindows()
-
-      if (browserWindows && browserWindows.length > 0) {
-        newWindow = browserWindows[0]
-      } else {
-        throw new Error(`Cannot open Menu without a BrowserWindow present`)
-      }
+  if (typeof x !== 'number') x = -1
+  if (typeof y !== 'number') y = -1
+  if (typeof positioningItem !== 'number') positioningItem = -1
+
+  // find which window to use
+  const wins = BrowserWindow.getAllWindows()
+  if (!wins || wins.indexOf(window) === -1) {
+    window = BrowserWindow.getFocusedWindow()
+    if (!window && wins && wins.length > 0) {
+      window = wins[0]
+    }
+    if (!window) {
+      throw new Error(`Cannot open Menu without a BrowserWindow present`)
     }
   }
 
-  this.popupAt(newWindow, newX, newY, newPosition, callback)
-
-  return { browserWindow: newWindow, x: newX, y: newY, position: newPosition }
+  this.popupAt(window, x, y, positioningItem, callback)
+  return { browserWindow: window, x, y, position: positioningItem }
 }
 
 Menu.prototype.closePopup = function (window) {

lib/browser/api/web-contents.js

@@ -299,7 +299,12 @@ WebContents.prototype._init = function () {
   this.on('pepper-context-menu', function (event, params, callback) {
     // Access Menu via electron.Menu to prevent circular require.
     const menu = electron.Menu.buildFromTemplate(params.menu)
-    menu.popup(event.sender.getOwnerBrowserWindow(), params.x, params.y, callback)
+    menu.popup({
+      window: event.sender.getOwnerBrowserWindow(),
+      x: params.x,
+      y: params.y,
+      callback
+    })
   })
 
   // The devtools requests the webContents to reload.

lib/renderer/inspector.js

@@ -63,7 +63,7 @@ const createMenu = function (x, y, items) {
 
   // The menu is expected to show asynchronously.
   setTimeout(function () {
-    menu.popup(remote.getCurrentWindow())
+    menu.popup({window: remote.getCurrentWindow()})
   })
 }
 

package.json

@@ -12,7 +12,7 @@
     "dugite": "^1.45.0",
     "electabul": "~0.0.4",
     "electron-docs-linter": "^2.3.4",
-    "electron-typescript-definitions": "1.3.1",
+    "electron-typescript-definitions": "^1.3.2",
     "github": "^9.2.0",
     "husky": "^0.14.3",
     "minimist": "^1.2.0",

spec/api-menu-spec.js

@@ -334,21 +334,21 @@ describe('Menu module', () => {
 
     it('should emit menu-will-show event', (done) => {
       menu.on('menu-will-show', () => { done() })
-      menu.popup(w)
+      menu.popup({window: w})
     })
 
     it('should emit menu-will-close event', (done) => {
       menu.on('menu-will-close', () => { done() })
-      menu.popup(w)
+      menu.popup({window: w})
       menu.closePopup()
     })
 
     it('returns immediately', () => {
-      const { browserWindow, x, y } = menu.popup(w, {x: 100, y: 101})
-
-      assert.equal(browserWindow, w)
-      assert.equal(x, 100)
-      assert.equal(y, 101)
+      const input = {window: w, x: 100, y: 101}
+      const output = menu.popup(input)
+      assert.equal(output.x, input.x)
+      assert.equal(output.y, input.y)
+      assert.equal(output.browserWindow, input.window)
     })
 
     it('works without a given BrowserWindow and options', () => {
@@ -359,42 +359,21 @@ describe('Menu module', () => {
       assert.equal(y, 101)
     })
 
-    it('works without a given BrowserWindow', () => {
-      const { browserWindow, x, y } = menu.popup(100, 101)
-
-      assert.equal(browserWindow.constructor.name, 'BrowserWindow')
-      assert.equal(x, 100)
-      assert.equal(y, 101)
-    })
-
-    it('works without a given BrowserWindow and 0 options', () => {
-      const { browserWindow, x, y } = menu.popup(0, 1)
-
-      assert.equal(browserWindow.constructor.name, 'BrowserWindow')
-      assert.equal(x, 0)
-      assert.equal(y, 1)
-    })
-
-    it('works with a given BrowserWindow and no options', () => {
-      const { browserWindow, x, y } = menu.popup(w, 100, 101)
+    it('works with a given BrowserWindow, options and callback', (done) => {
+      const {x, y} = menu.popup({
+        window: w,
+        x: 100,
+        y: 101,
+        callback: () => done()
+      })
 
-      assert.equal(browserWindow, w)
       assert.equal(x, 100)
       assert.equal(y, 101)
-    })
-
-    it('works with a given BrowserWindow, no options, and a callback', (done) => {
-      menu.popup(w, () => done())
       menu.closePopup()
     })
 
-    it('calls the callback', (done) => {
-      menu.popup({}, () => done())
-      menu.closePopup()
-    })
-
-    it('works with old style', (done) => {
-      menu.popup(w, 100, 101, () => done())
+    it('works with a given BrowserWindow, no options, and a callback', (done) => {
+      menu.popup({window: w, callback: () => done()})
       menu.closePopup()
     })
   })