From 38bb9baac5dc2bd625279705193a9f9dbf8364bc Mon Sep 17 00:00:00 2001 From: Felix Rieseberg Date: Fri, 8 Dec 2017 14:36:29 -0800 Subject: [PATCH 01/11] :wrench: Oh wow, that looks wrong --- lib/browser/api/menu.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/browser/api/menu.js b/lib/browser/api/menu.js index a340f74abcf..1cf1a101340 100644 --- a/lib/browser/api/menu.js +++ b/lib/browser/api/menu.js @@ -63,10 +63,10 @@ Menu.prototype.popup = function (window, x, y, positioningItem) { } // set defaults - if (typeof x !== 'number') newX = -1 - if (typeof y !== 'number') newY = -1 - if (typeof positioningItem !== 'number') newPosition = -1 if (!window) newWindow = BrowserWindow.getFocusedWindow() + if (typeof newX !== 'number') newX = -1 + if (typeof newY !== 'number') newY = -1 + if (typeof newPosition !== 'number') newPosition = -1 this.popupAt(newWindow, newX, newY, newPosition) } From bd6767fac628f51677bc3c3cd26da611ea1e0148 Mon Sep 17 00:00:00 2001 From: Felix Rieseberg Date: Fri, 8 Dec 2017 14:36:52 -0800 Subject: [PATCH 02/11] :wrench: Always find a window (or error) --- lib/browser/api/menu.js | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/browser/api/menu.js b/lib/browser/api/menu.js index 1cf1a101340..bec3cce7644 100644 --- a/lib/browser/api/menu.js +++ b/lib/browser/api/menu.js @@ -63,10 +63,23 @@ Menu.prototype.popup = function (window, x, y, positioningItem) { } // set defaults - if (!window) newWindow = BrowserWindow.getFocusedWindow() 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`) + } + } + } this.popupAt(newWindow, newX, newY, newPosition) } From 955564abd7bdccb318803166bfbb164910df859d Mon Sep 17 00:00:00 2001 From: Felix Rieseberg Date: Fri, 8 Dec 2017 14:37:16 -0800 Subject: [PATCH 03/11] :wrench: Allow for `menu.popup({})` --- lib/browser/api/menu.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/browser/api/menu.js b/lib/browser/api/menu.js index bec3cce7644..f50fee6ecb4 100644 --- a/lib/browser/api/menu.js +++ b/lib/browser/api/menu.js @@ -45,6 +45,7 @@ Menu.prototype._init = function () { Menu.prototype.popup = function (window, x, y, positioningItem) { let [newX, newY, newPosition, newWindow] = [x, y, positioningItem, window] + let opts // menu.popup(x, y, positioningItem) if (!window) { @@ -54,9 +55,16 @@ Menu.prototype.popup = function (window, x, y, positioningItem) { } } + // menu.popup({}) + if (window && typeof window === 'object' && window.constructor && + window.constructor.name !== 'BrowserWindow') { + opts = window // menu.popup(window, {}) - if (x && typeof x === 'object') { - const opts = x + } else if (x && typeof x === 'object') { + opts = x + } + + if (opts) { newX = opts.x newY = opts.y newPosition = opts.positioningItem From abd56eda6ffe6895315c6719c2ec13d2f3a6df33 Mon Sep 17 00:00:00 2001 From: Felix Rieseberg Date: Fri, 8 Dec 2017 14:40:51 -0800 Subject: [PATCH 04/11] :wrench: Cleanup --- lib/browser/api/menu.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/browser/api/menu.js b/lib/browser/api/menu.js index f50fee6ecb4..d15457238c0 100644 --- a/lib/browser/api/menu.js +++ b/lib/browser/api/menu.js @@ -56,8 +56,7 @@ Menu.prototype.popup = function (window, x, y, positioningItem) { } // menu.popup({}) - if (window && typeof window === 'object' && window.constructor && - window.constructor.name !== 'BrowserWindow') { + if (typeof window === 'object' && window.constructor !== BrowserWindow) { opts = window // menu.popup(window, {}) } else if (x && typeof x === 'object') { From 725f6c97d659a924282f4651e14fe4591228e104 Mon Sep 17 00:00:00 2001 From: Felix Rieseberg Date: Fri, 8 Dec 2017 14:52:21 -0800 Subject: [PATCH 05/11] :construction_worker: Add a spec --- spec/api-menu-spec.js | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/spec/api-menu-spec.js b/spec/api-menu-spec.js index 538c640e712..f57617b8771 100644 --- a/spec/api-menu-spec.js +++ b/spec/api-menu-spec.js @@ -283,9 +283,19 @@ describe('Menu module', () => { }) it('returns immediately', () => { - menu.popup(w, {x: 100, y: 100, async: true}) + menu.popup(w, {x: 100, y: 100}) menu.closePopup(w) }) + + it('works without a given BrowserWindow and options', () => { + menu.popup({x: 100, y: 100}) + menu.closePopup() + }) + + it('works without a given BrowserWindow', () => { + menu.popup(100, 100) + menu.closePopup() + }) }) describe('Menu.setApplicationMenu', () => { From 93b46116f4aaed61c67c782f64055db9501f7a56 Mon Sep 17 00:00:00 2001 From: Felix Rieseberg Date: Mon, 11 Dec 2017 13:30:35 -0800 Subject: [PATCH 06/11] :wrench: Fix value shift --- lib/browser/api/menu.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/browser/api/menu.js b/lib/browser/api/menu.js index d15457238c0..33e0b1e4ce6 100644 --- a/lib/browser/api/menu.js +++ b/lib/browser/api/menu.js @@ -48,11 +48,8 @@ Menu.prototype.popup = function (window, x, y, positioningItem) { let opts // menu.popup(x, y, positioningItem) - if (!window) { - // shift over values - if (typeof window !== 'object' || window.constructor !== BrowserWindow) { - [newPosition, newY, newX, newWindow] = [y, x, window, null] - } + if (window && typeof window !== 'object' || window.constructor !== BrowserWindow) { + [newPosition, newY, newX, newWindow] = [y, x, window, null] } // menu.popup({}) From 22e9d665d293e75353c3de1146d4acb7e4316436 Mon Sep 17 00:00:00 2001 From: Felix Rieseberg Date: Mon, 11 Dec 2017 13:54:33 -0800 Subject: [PATCH 07/11] :wrench: Menu returns its properties now --- lib/browser/api/menu.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/browser/api/menu.js b/lib/browser/api/menu.js index 33e0b1e4ce6..d4a2aafb793 100644 --- a/lib/browser/api/menu.js +++ b/lib/browser/api/menu.js @@ -86,6 +86,8 @@ Menu.prototype.popup = function (window, x, y, positioningItem) { } this.popupAt(newWindow, newX, newY, newPosition) + + return { browserWindow: newWindow, x: newX, y: newY, position: newPosition } } Menu.prototype.closePopup = function (window) { From dfd7598d485b7dfd585bd9f645b85946c71dfed7 Mon Sep 17 00:00:00 2001 From: Felix Rieseberg Date: Mon, 11 Dec 2017 13:54:43 -0800 Subject: [PATCH 08/11] :construction_worker: Hence, better testing --- spec/api-menu-spec.js | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/spec/api-menu-spec.js b/spec/api-menu-spec.js index f57617b8771..3c6353db0ce 100644 --- a/spec/api-menu-spec.js +++ b/spec/api-menu-spec.js @@ -283,19 +283,44 @@ describe('Menu module', () => { }) it('returns immediately', () => { - menu.popup(w, {x: 100, y: 100}) + const { browserWindow, x, y } = menu.popup(w, {x: 100, y: 100}) + + assert.equal(browserWindow, w) + assert.equal(x, 100) + assert.equal(y, 100) + menu.closePopup(w) }) it('works without a given BrowserWindow and options', () => { - menu.popup({x: 100, y: 100}) + const { browserWindow, x, y } = menu.popup({x: 100, y: 100}) + + assert.equal(browserWindow.constructor.name, 'BrowserWindow') + assert.equal(x, 100) + assert.equal(y, 100) + menu.closePopup() }) it('works without a given BrowserWindow', () => { - menu.popup(100, 100) + const { browserWindow, x, y } = menu.popup(100, 100) + + assert.equal(browserWindow.constructor.name, 'BrowserWindow') + assert.equal(x, 100) + assert.equal(y, 100) + menu.closePopup() }) + + it('works with a given BrowserWindow and no options', () => { + const { browserWindow, x, y } = menu.popup(w, 100, 100) + + assert.equal(browserWindow, w) + assert.equal(x, 100) + assert.equal(y, 100) + + menu.closePopup(w) + }) }) describe('Menu.setApplicationMenu', () => { From f7ebfff8ae39f74e6a88fbf5586778099d3b279e Mon Sep 17 00:00:00 2001 From: Felix Rieseberg Date: Mon, 11 Dec 2017 14:05:07 -0800 Subject: [PATCH 09/11] :construction_worker: Properly test x vs y --- spec/api-menu-spec.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/spec/api-menu-spec.js b/spec/api-menu-spec.js index 3c6353db0ce..b61db17baac 100644 --- a/spec/api-menu-spec.js +++ b/spec/api-menu-spec.js @@ -283,41 +283,41 @@ describe('Menu module', () => { }) it('returns immediately', () => { - const { browserWindow, x, y } = menu.popup(w, {x: 100, y: 100}) + const { browserWindow, x, y } = menu.popup(w, {x: 100, y: 101}) assert.equal(browserWindow, w) assert.equal(x, 100) - assert.equal(y, 100) + assert.equal(y, 101) menu.closePopup(w) }) it('works without a given BrowserWindow and options', () => { - const { browserWindow, x, y } = menu.popup({x: 100, y: 100}) + const { browserWindow, x, y } = menu.popup({x: 100, y: 101}) assert.equal(browserWindow.constructor.name, 'BrowserWindow') assert.equal(x, 100) - assert.equal(y, 100) + assert.equal(y, 101) menu.closePopup() }) it('works without a given BrowserWindow', () => { - const { browserWindow, x, y } = menu.popup(100, 100) + const { browserWindow, x, y } = menu.popup(100, 101) assert.equal(browserWindow.constructor.name, 'BrowserWindow') assert.equal(x, 100) - assert.equal(y, 100) + assert.equal(y, 101) menu.closePopup() }) it('works with a given BrowserWindow and no options', () => { - const { browserWindow, x, y } = menu.popup(w, 100, 100) + const { browserWindow, x, y } = menu.popup(w, 100, 101) assert.equal(browserWindow, w) assert.equal(x, 100) - assert.equal(y, 100) + assert.equal(y, 101) menu.closePopup(w) }) From 89b90be6a2429663c769be4837d90e95ae858e84 Mon Sep 17 00:00:00 2001 From: Felix Rieseberg Date: Mon, 11 Dec 2017 14:43:35 -0800 Subject: [PATCH 10/11] :wrench: Feedback --- lib/browser/api/menu.js | 4 ++-- spec/api-menu-spec.js | 10 ++++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/browser/api/menu.js b/lib/browser/api/menu.js index d4a2aafb793..787fd596f65 100644 --- a/lib/browser/api/menu.js +++ b/lib/browser/api/menu.js @@ -48,12 +48,12 @@ Menu.prototype.popup = function (window, x, y, positioningItem) { let opts // menu.popup(x, y, positioningItem) - if (window && typeof window !== 'object' || window.constructor !== BrowserWindow) { + if (window != null && !(window instanceof BrowserWindow)) { [newPosition, newY, newX, newWindow] = [y, x, window, null] } // menu.popup({}) - if (typeof window === 'object' && window.constructor !== BrowserWindow) { + if (window && window.constructor !== BrowserWindow) { opts = window // menu.popup(window, {}) } else if (x && typeof x === 'object') { diff --git a/spec/api-menu-spec.js b/spec/api-menu-spec.js index b61db17baac..75a77813a17 100644 --- a/spec/api-menu-spec.js +++ b/spec/api-menu-spec.js @@ -312,6 +312,16 @@ describe('Menu module', () => { menu.closePopup() }) + 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) + + menu.closePopup() + }) + it('works with a given BrowserWindow and no options', () => { const { browserWindow, x, y } = menu.popup(w, 100, 101) From 927c63b4770ead65313f381c34b3acda3cc26ba8 Mon Sep 17 00:00:00 2001 From: Felix Rieseberg Date: Mon, 11 Dec 2017 15:19:33 -0800 Subject: [PATCH 11/11] :wrench: Last round of feedback --- lib/browser/api/menu.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/browser/api/menu.js b/lib/browser/api/menu.js index 787fd596f65..78f1a4466a8 100644 --- a/lib/browser/api/menu.js +++ b/lib/browser/api/menu.js @@ -53,7 +53,7 @@ Menu.prototype.popup = function (window, x, y, positioningItem) { } // menu.popup({}) - if (window && window.constructor !== BrowserWindow) { + if (window != null && window.constructor === Object) { opts = window // menu.popup(window, {}) } else if (x && typeof x === 'object') {