From 25b2724ab94e775229fe40e32d9409f0f5dd10c4 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 22 Jun 2016 10:29:49 -0700 Subject: [PATCH 01/19] Add menu item role defaults --- filenames.gypi | 1 + lib/browser/api/menu-item-roles.js | 65 ++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+) create mode 100644 lib/browser/api/menu-item-roles.js diff --git a/filenames.gypi b/filenames.gypi index af7080821..6b086e1f8 100644 --- a/filenames.gypi +++ b/filenames.gypi @@ -21,6 +21,7 @@ 'lib/browser/api/ipc-main.js', 'lib/browser/api/menu.js', 'lib/browser/api/menu-item.js', + 'lib/browser/api/menu-item-roles.js', 'lib/browser/api/navigation-controller.js', 'lib/browser/api/power-monitor.js', 'lib/browser/api/power-save-blocker.js', diff --git a/lib/browser/api/menu-item-roles.js b/lib/browser/api/menu-item-roles.js new file mode 100644 index 000000000..13484e5da --- /dev/null +++ b/lib/browser/api/menu-item-roles.js @@ -0,0 +1,65 @@ +module.exports = { + undo: { + label: 'Undo', + accelerator: 'CmdOrCtrl+Z', + method: 'undo' + }, + redo: { + label: 'Redo', + accelerator: 'Shift+CmdOrCtrl+Z', + method: 'redo' + }, + cut: { + label: 'Cut', + accelerator: 'CmdOrCtrl+X', + method: 'cut' + }, + copy: { + label: 'Copy', + accelerator: 'CmdOrCtrl+C', + method: 'copy' + }, + paste: { + label: 'Paste', + accelerator: 'CmdOrCtrl+V', + method: 'paste' + }, + pasteandmatchstyle: { + label: 'Paste and Match Style', + accelerator: 'Shift+Command+V', + method: 'pasteAndMatchStyle' + }, + selectall: { + label: 'Select All', + accelerator: 'CmdOrCtrl+A', + method: 'selectAll' + }, + minimize: { + label: 'Minimize', + accelerator: 'CmdOrCtrl+M', + method: 'minimize' + }, + close: { + label: 'Close', + accelerator: 'CmdOrCtrl+W', + method: 'close' + }, + delete: { + label: 'Delete', + accelerator: 'Delete', + method: 'delete' + }, + quit: { + get label () { + const {app} = require('electron') + return process.platform === 'win32' ? 'Exit' : `Quit ${app.getName()}` + }, + accelerator: process.platform === 'win32' ? null : 'Command+Q', + method: 'quit' + }, + togglefullscreen: { + label: 'Toggle Full Screen', + accelerator: process.platform === 'darwin' ? 'Ctrl+Command+F' : 'F11', + method: 'toggleFullScreen' + } +} From 13a6d32ee9ab48fdb5d81d6e5c5a2d106a9e7c20 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 22 Jun 2016 10:45:01 -0700 Subject: [PATCH 02/19] Add default label/accelerator to role menu items --- lib/browser/api/menu-item-roles.js | 14 +++++++++++++- lib/browser/api/menu-item.js | 6 ++++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/lib/browser/api/menu-item-roles.js b/lib/browser/api/menu-item-roles.js index 13484e5da..f26e9c5a5 100644 --- a/lib/browser/api/menu-item-roles.js +++ b/lib/browser/api/menu-item-roles.js @@ -1,4 +1,4 @@ -module.exports = { +const roles = { undo: { label: 'Undo', accelerator: 'CmdOrCtrl+Z', @@ -63,3 +63,15 @@ module.exports = { method: 'toggleFullScreen' } } + +exports.getDefaultLabel = function (role) { + if (roles.hasOwnProperty(role)) { + return roles[role].label + } else { + return '' + } +} + +exports.getDefaultAccelerator = function (role) { + if (roles.hasOwnProperty(role)) return roles[role].accelerator +} diff --git a/lib/browser/api/menu-item.js b/lib/browser/api/menu-item.js index 264be8179..ba980960b 100644 --- a/lib/browser/api/menu-item.js +++ b/lib/browser/api/menu-item.js @@ -1,5 +1,7 @@ 'use strict' +const roles = require('./menu-item-roles') + let nextCommandId = 0 // Maps role to methods of webContents @@ -58,11 +60,11 @@ const MenuItem = function (options) { this.overrideReadOnlyProperty('type', 'normal') this.overrideReadOnlyProperty('role') - this.overrideReadOnlyProperty('accelerator') + this.overrideReadOnlyProperty('accelerator', roles.getDefaultAccelerator(this.role)) this.overrideReadOnlyProperty('icon') this.overrideReadOnlyProperty('submenu') - this.overrideProperty('label', '') + this.overrideProperty('label', roles.getDefaultLabel(this.role)) this.overrideProperty('sublabel', '') this.overrideProperty('enabled', true) this.overrideProperty('visible', true) From 566a407b363934ee003be09151a4bf9b3a5e8a89 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 22 Jun 2016 10:46:17 -0700 Subject: [PATCH 03/19] Use default labels and accelerators --- default_app/main.js | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/default_app/main.js b/default_app/main.js index 8b2fb1bbf..085708cb4 100644 --- a/default_app/main.js +++ b/default_app/main.js @@ -52,46 +52,30 @@ app.once('ready', () => { label: 'Edit', submenu: [ { - label: 'Undo', - accelerator: 'CmdOrCtrl+Z', role: 'undo' }, { - label: 'Redo', - accelerator: 'Shift+CmdOrCtrl+Z', role: 'redo' }, { type: 'separator' }, { - label: 'Cut', - accelerator: 'CmdOrCtrl+X', role: 'cut' }, { - label: 'Copy', - accelerator: 'CmdOrCtrl+C', role: 'copy' }, { - label: 'Paste', - accelerator: 'CmdOrCtrl+V', role: 'paste' }, { - label: 'Paste and Match Style', - accelerator: 'Shift+Command+V', role: 'pasteandmatchstyle' }, { - label: 'Delete', - accelerator: 'Delete', role: 'delete' }, { - label: 'Select All', - accelerator: 'CmdOrCtrl+A', role: 'selectall' } ] @@ -107,9 +91,7 @@ app.once('ready', () => { } }, { - label: 'Toggle Full Screen', role: 'togglefullscreen', - accelerator: process.platform === 'darwin' ? 'Ctrl+Command+F' : 'F11' }, { label: 'Toggle Developer Tools', @@ -125,13 +107,9 @@ app.once('ready', () => { role: 'window', submenu: [ { - label: 'Minimize', - accelerator: 'CmdOrCtrl+M', role: 'minimize' }, { - label: 'Close', - accelerator: 'CmdOrCtrl+W', role: 'close' } ] From 888068b59717fe1e6ae14c178b216a30cd61cce3 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 22 Jun 2016 10:47:25 -0700 Subject: [PATCH 04/19] Add default help/window labels --- default_app/main.js | 2 -- lib/browser/api/menu-item-roles.js | 6 ++++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/default_app/main.js b/default_app/main.js index 085708cb4..45ff78e1b 100644 --- a/default_app/main.js +++ b/default_app/main.js @@ -103,7 +103,6 @@ app.once('ready', () => { ] }, { - label: 'Window', role: 'window', submenu: [ { @@ -115,7 +114,6 @@ app.once('ready', () => { ] }, { - label: 'Help', role: 'help', submenu: [ { diff --git a/lib/browser/api/menu-item-roles.js b/lib/browser/api/menu-item-roles.js index f26e9c5a5..c78ff8aed 100644 --- a/lib/browser/api/menu-item-roles.js +++ b/lib/browser/api/menu-item-roles.js @@ -61,6 +61,12 @@ const roles = { label: 'Toggle Full Screen', accelerator: process.platform === 'darwin' ? 'Ctrl+Command+F' : 'F11', method: 'toggleFullScreen' + }, + help: { + label: 'Help' + }, + window: { + label: 'Window' } } From c0562d16d58c2ee727da493c70c2730d15fefafb Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 22 Jun 2016 13:09:39 -0700 Subject: [PATCH 05/19] Add more role defaults --- default_app/main.js | 16 ---------------- lib/browser/api/menu-item-roles.js | 29 +++++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/default_app/main.js b/default_app/main.js index 45ff78e1b..a484c030d 100644 --- a/default_app/main.js +++ b/default_app/main.js @@ -151,14 +151,12 @@ app.once('ready', () => { label: 'Electron', submenu: [ { - label: 'About Electron', role: 'about' }, { type: 'separator' }, { - label: 'Services', role: 'services', submenu: [] }, @@ -166,49 +164,36 @@ app.once('ready', () => { type: 'separator' }, { - label: 'Hide Electron', - accelerator: 'Command+H', role: 'hide' }, { - label: 'Hide Others', - accelerator: 'Command+Alt+H', role: 'hideothers' }, { - label: 'Show All', role: 'unhide' }, { type: 'separator' }, { - label: 'Quit ' + app.getName(), - accelerator: 'Command+Q', role: 'quit' } ] }) template[3].submenu = [ { - label: 'Close', - accelerator: 'CmdOrCtrl+W', role: 'close' }, { - label: 'Minimize', - accelerator: 'CmdOrCtrl+M', role: 'minimize' }, { - label: 'Zoom', role: 'zoom' }, { type: 'separator' }, { - label: 'Bring All to Front', role: 'front' } ] @@ -219,7 +204,6 @@ app.once('ready', () => { label: 'File', submenu: [ { - label: 'Exit', role: 'quit' } ] diff --git a/lib/browser/api/menu-item-roles.js b/lib/browser/api/menu-item-roles.js index c78ff8aed..a5373c65a 100644 --- a/lib/browser/api/menu-item-roles.js +++ b/lib/browser/api/menu-item-roles.js @@ -67,6 +67,35 @@ const roles = { }, window: { label: 'Window' + }, + services: { + label: 'Services' + }, + zoom: { + label: 'Zoom' + }, + front: { + label: 'Bring All to Front' + }, + about: { + get label () { + const {app} = require('electron') + return `About ${app.getName()}` + } + }, + hide: { + get label () { + const {app} = require('electron') + return return `Hide ${app.getName()}` + }, + accelerator: 'Command+H' + }, + hideothers: { + label: 'Hide Others', + accelerator: 'Command+Alt+H' + }, + unhide: { + label: 'Show All' } } From 653370974a5029ce88bced81a553f155a7eebd2c Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 22 Jun 2016 13:15:20 -0700 Subject: [PATCH 06/19] :art: Sort roles alphabetically --- lib/browser/api/menu-item-roles.js | 120 ++++++++++++++--------------- 1 file changed, 60 insertions(+), 60 deletions(-) diff --git a/lib/browser/api/menu-item-roles.js b/lib/browser/api/menu-item-roles.js index a5373c65a..6d6b0371e 100644 --- a/lib/browser/api/menu-item-roles.js +++ b/lib/browser/api/menu-item-roles.js @@ -1,23 +1,51 @@ const roles = { - undo: { - label: 'Undo', - accelerator: 'CmdOrCtrl+Z', - method: 'undo' + about: { + get label () { + const {app} = require('electron') + return `About ${app.getName()}` + } }, - redo: { - label: 'Redo', - accelerator: 'Shift+CmdOrCtrl+Z', - method: 'redo' + close: { + label: 'Close', + accelerator: 'CmdOrCtrl+W', + method: 'close' + }, + copy: { + label: 'Copy', + accelerator: 'CmdOrCtrl+C', + method: 'copy' }, cut: { label: 'Cut', accelerator: 'CmdOrCtrl+X', method: 'cut' }, - copy: { - label: 'Copy', - accelerator: 'CmdOrCtrl+C', - method: 'copy' + delete: { + label: 'Delete', + accelerator: 'Delete', + method: 'delete' + }, + front: { + label: 'Bring All to Front' + }, + help: { + label: 'Help' + }, + hide: { + get label () { + const {app} = require('electron') + return `Hide ${app.getName()}` + }, + accelerator: 'Command+H' + }, + hideothers: { + label: 'Hide Others', + accelerator: 'Command+Alt+H' + }, + minimize: { + label: 'Minimize', + accelerator: 'CmdOrCtrl+M', + method: 'minimize' }, paste: { label: 'Paste', @@ -29,26 +57,6 @@ const roles = { accelerator: 'Shift+Command+V', method: 'pasteAndMatchStyle' }, - selectall: { - label: 'Select All', - accelerator: 'CmdOrCtrl+A', - method: 'selectAll' - }, - minimize: { - label: 'Minimize', - accelerator: 'CmdOrCtrl+M', - method: 'minimize' - }, - close: { - label: 'Close', - accelerator: 'CmdOrCtrl+W', - method: 'close' - }, - delete: { - label: 'Delete', - accelerator: 'Delete', - method: 'delete' - }, quit: { get label () { const {app} = require('electron') @@ -57,45 +65,37 @@ const roles = { accelerator: process.platform === 'win32' ? null : 'Command+Q', method: 'quit' }, + redo: { + label: 'Redo', + accelerator: 'Shift+CmdOrCtrl+Z', + method: 'redo' + }, + selectall: { + label: 'Select All', + accelerator: 'CmdOrCtrl+A', + method: 'selectAll' + }, + services: { + label: 'Services' + }, togglefullscreen: { label: 'Toggle Full Screen', accelerator: process.platform === 'darwin' ? 'Ctrl+Command+F' : 'F11', method: 'toggleFullScreen' }, - help: { - label: 'Help' + undo: { + label: 'Undo', + accelerator: 'CmdOrCtrl+Z', + method: 'undo' + }, + unhide: { + label: 'Show All' }, window: { label: 'Window' }, - services: { - label: 'Services' - }, zoom: { label: 'Zoom' - }, - front: { - label: 'Bring All to Front' - }, - about: { - get label () { - const {app} = require('electron') - return `About ${app.getName()}` - } - }, - hide: { - get label () { - const {app} = require('electron') - return return `Hide ${app.getName()}` - }, - accelerator: 'Command+H' - }, - hideothers: { - label: 'Hide Others', - accelerator: 'Command+Alt+H' - }, - unhide: { - label: 'Show All' } } From 66f2fb2fe4ae6c0b39991db4078097067a3ae048 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 22 Jun 2016 13:48:26 -0700 Subject: [PATCH 07/19] Add execute helper to roles file --- lib/browser/api/menu-item-roles.js | 57 +++++++++++++++++++++++------- lib/browser/api/menu-item.js | 49 +++---------------------- 2 files changed, 50 insertions(+), 56 deletions(-) diff --git a/lib/browser/api/menu-item-roles.js b/lib/browser/api/menu-item-roles.js index 6d6b0371e..947bf4d28 100644 --- a/lib/browser/api/menu-item-roles.js +++ b/lib/browser/api/menu-item-roles.js @@ -8,22 +8,22 @@ const roles = { close: { label: 'Close', accelerator: 'CmdOrCtrl+W', - method: 'close' + windowMethod: 'close' }, copy: { label: 'Copy', accelerator: 'CmdOrCtrl+C', - method: 'copy' + webContentsMethod: 'copy' }, cut: { label: 'Cut', accelerator: 'CmdOrCtrl+X', - method: 'cut' + webContentsMethod: 'cut' }, delete: { label: 'Delete', accelerator: 'Delete', - method: 'delete' + webContentsMethod: 'delete' }, front: { label: 'Bring All to Front' @@ -45,17 +45,17 @@ const roles = { minimize: { label: 'Minimize', accelerator: 'CmdOrCtrl+M', - method: 'minimize' + windowMethod: 'minimize' }, paste: { label: 'Paste', accelerator: 'CmdOrCtrl+V', - method: 'paste' + webContentsMethod: 'paste' }, pasteandmatchstyle: { label: 'Paste and Match Style', accelerator: 'Shift+Command+V', - method: 'pasteAndMatchStyle' + webContentsMethod: 'pasteAndMatchStyle' }, quit: { get label () { @@ -63,17 +63,17 @@ const roles = { return process.platform === 'win32' ? 'Exit' : `Quit ${app.getName()}` }, accelerator: process.platform === 'win32' ? null : 'Command+Q', - method: 'quit' + appMethod: 'quit' }, redo: { label: 'Redo', accelerator: 'Shift+CmdOrCtrl+Z', - method: 'redo' + webContentsMethod: 'redo' }, selectall: { label: 'Select All', accelerator: 'CmdOrCtrl+A', - method: 'selectAll' + webContentsMethod: 'selectAll' }, services: { label: 'Services' @@ -81,12 +81,14 @@ const roles = { togglefullscreen: { label: 'Toggle Full Screen', accelerator: process.platform === 'darwin' ? 'Ctrl+Command+F' : 'F11', - method: 'toggleFullScreen' + windowMethod: function (window) { + window.setFullScreen(!window.isFullScreen()) + } }, undo: { label: 'Undo', accelerator: 'CmdOrCtrl+Z', - method: 'undo' + webContentsMethod: 'undo' }, unhide: { label: 'Show All' @@ -110,3 +112,34 @@ exports.getDefaultLabel = function (role) { exports.getDefaultAccelerator = function (role) { if (roles.hasOwnProperty(role)) return roles[role].accelerator } + +exports.execute = function (role, focusedWindow) { + if (!roles.hasOwnProperty(role)) return false + if (process.platform === 'darwin') return false + + const {appMethod, webContentsMethod, windowMethod} = roles[role] + + if (appMethod) { + app[appMethod]() + return true + } + + if (focusedWindow != null) { + if (windowMethod) { + if (typeof windowMethod === 'function') { + windowMethod(focusedWindow) + } else { + focusedWindow[windowMethod]() + } + return true + } else if (webContentsMethod) { + const {webContents} = focusedWindow + if (webContents) { + webContents[webContentsMethod]() + } + return true + } + } + + return false +} diff --git a/lib/browser/api/menu-item.js b/lib/browser/api/menu-item.js index ba980960b..d2d8e6519 100644 --- a/lib/browser/api/menu-item.js +++ b/lib/browser/api/menu-item.js @@ -4,35 +4,6 @@ const roles = require('./menu-item-roles') let nextCommandId = 0 -// Maps role to methods of webContents -const rolesMap = { - undo: 'undo', - redo: 'redo', - cut: 'cut', - copy: 'copy', - paste: 'paste', - pasteandmatchstyle: 'pasteAndMatchStyle', - selectall: 'selectAll', - minimize: 'minimize', - close: 'close', - delete: 'delete', - quit: 'quit', - togglefullscreen: 'toggleFullScreen' -} - -// Maps methods that should be called directly on the BrowserWindow instance -const methodInBrowserWindow = { - minimize: true, - close: true, - toggleFullScreen: function (window) { - window.setFullScreen(!window.isFullScreen()) - } -} - -const methodInApp = { - quit: true -} - const MenuItem = function (options) { const {app, Menu} = require('electron') @@ -83,22 +54,12 @@ const MenuItem = function (options) { this.checked = !this.checked } - if (this.role && rolesMap[this.role] && process.platform !== 'darwin' && focusedWindow != null) { - const methodName = rolesMap[this.role] - if (methodInApp[methodName]) { - return app[methodName]() - } else if (typeof methodInBrowserWindow[methodName] === 'function') { - return methodInBrowserWindow[methodName](focusedWindow) - } else if (methodInBrowserWindow[methodName]) { - return focusedWindow[methodName]() - } else { - const {webContents} = focusedWindow - return webContents != null ? webContents[methodName]() : void 0 + if (!roles.execute(this.role)) { + if (typeof click === 'function') { + click(this, focusedWindow, event) + } else if (typeof this.selector === 'string' && process.platform === 'darwin') { + Menu.sendActionToFirstResponder(this.selector) } - } else if (typeof click === 'function') { - return click(this, focusedWindow, event) - } else if (typeof this.selector === 'string' && process.platform === 'darwin') { - return Menu.sendActionToFirstResponder(this.selector) } } } From 30e3a6ed8363ff99b5c301c2b866e981b25d8b89 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 22 Jun 2016 13:59:49 -0700 Subject: [PATCH 08/19] Add app require --- lib/browser/api/menu-item-roles.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/browser/api/menu-item-roles.js b/lib/browser/api/menu-item-roles.js index 947bf4d28..a6a985a5e 100644 --- a/lib/browser/api/menu-item-roles.js +++ b/lib/browser/api/menu-item-roles.js @@ -120,6 +120,7 @@ exports.execute = function (role, focusedWindow) { const {appMethod, webContentsMethod, windowMethod} = roles[role] if (appMethod) { + const {app} = require('electron') app[appMethod]() return true } From c6dc6a8905e3db5bf99035094d42d399325a21d9 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 22 Jun 2016 14:02:31 -0700 Subject: [PATCH 09/19] Specify focused window to execute call --- lib/browser/api/menu-item.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/browser/api/menu-item.js b/lib/browser/api/menu-item.js index d2d8e6519..2f96fdd31 100644 --- a/lib/browser/api/menu-item.js +++ b/lib/browser/api/menu-item.js @@ -54,7 +54,7 @@ const MenuItem = function (options) { this.checked = !this.checked } - if (!roles.execute(this.role)) { + if (!roles.execute(this.role, focusedWindow)) { if (typeof click === 'function') { click(this, focusedWindow, event) } else if (typeof this.selector === 'string' && process.platform === 'darwin') { From 5096d7835f71f5d79bd6df4edeadadb489669d17 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 22 Jun 2016 14:07:46 -0700 Subject: [PATCH 10/19] Add spec for default role label/accelerator --- spec/api-menu-spec.js | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/spec/api-menu-spec.js b/spec/api-menu-spec.js index d6ac56475..a6c56a9a4 100644 --- a/spec/api-menu-spec.js +++ b/spec/api-menu-spec.js @@ -395,4 +395,28 @@ describe('menu module', function () { }, /Invalid submenu/) }) }) + + describe('MenuItem role', function () { + it('includes a default label and accelerator', function () { + var item = new MenuItem({role: 'close'}) + assert.equal(item.label, 'Close') + assert.equal(item.accelerator, 'CmdOrCtrl+W') + + var item = new MenuItem({role: 'close', label: 'Other'}) + assert.equal(item.label, 'Other') + assert.equal(item.accelerator, 'CmdOrCtrl+W') + + var item = new MenuItem({role: 'close', accelerator: 'D'}) + assert.equal(item.label, 'Close') + assert.equal(item.accelerator, 'D') + + var item = new MenuItem({role: 'close', label: 'C', accelerator: 'D'}) + assert.equal(item.label, 'C') + assert.equal(item.accelerator, 'D') + + var item = new MenuItem({role: 'help'}) + assert.equal(item.label, 'Help') + assert.equal(item.accelerator, undefined) + }) + }) }) From c98f419bc805a4feddaccd299191e721a900b757 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 22 Jun 2016 14:10:38 -0700 Subject: [PATCH 11/19] Doc role defaults --- docs/api/menu-item.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/api/menu-item.md b/docs/api/menu-item.md index 0d05d81bc..3a9a565a9 100644 --- a/docs/api/menu-item.md +++ b/docs/api/menu-item.md @@ -39,6 +39,9 @@ It is best to specify `role` for any menu item that matches a standard role, rather than trying to manually implement the behavior in a `click` function. The built-in `role` behavior will give the best native experience. +The `label` and `accelerator` are optional when using a `role` and will default +to appropriate values for each platform. + The `role` property can have following values: * `undo` From 58c1d38c96499e4e4f3b498704d88b3feb29a004 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 22 Jun 2016 14:13:12 -0700 Subject: [PATCH 12/19] Remove lint errors --- default_app/main.js | 2 +- lib/browser/api/menu-item.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/default_app/main.js b/default_app/main.js index a484c030d..dd887e0de 100644 --- a/default_app/main.js +++ b/default_app/main.js @@ -91,7 +91,7 @@ app.once('ready', () => { } }, { - role: 'togglefullscreen', + role: 'togglefullscreen' }, { label: 'Toggle Developer Tools', diff --git a/lib/browser/api/menu-item.js b/lib/browser/api/menu-item.js index 2f96fdd31..8fb1b51d1 100644 --- a/lib/browser/api/menu-item.js +++ b/lib/browser/api/menu-item.js @@ -5,7 +5,7 @@ const roles = require('./menu-item-roles') let nextCommandId = 0 const MenuItem = function (options) { - const {app, Menu} = require('electron') + const {Menu} = require('electron') this.selector = options.selector this.type = options.type From ece319a6871bc76c358db8b1a033ac878cb1e5a9 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 22 Jun 2016 14:14:32 -0700 Subject: [PATCH 13/19] :art: --- lib/browser/api/menu-item-roles.js | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/lib/browser/api/menu-item-roles.js b/lib/browser/api/menu-item-roles.js index a6a985a5e..aaebd5622 100644 --- a/lib/browser/api/menu-item-roles.js +++ b/lib/browser/api/menu-item-roles.js @@ -125,21 +125,21 @@ exports.execute = function (role, focusedWindow) { return true } - if (focusedWindow != null) { - if (windowMethod) { - if (typeof windowMethod === 'function') { - windowMethod(focusedWindow) - } else { - focusedWindow[windowMethod]() - } - return true - } else if (webContentsMethod) { - const {webContents} = focusedWindow - if (webContents) { - webContents[webContentsMethod]() - } - return true + if (windowMethod && focusedWindow != null) { + if (typeof windowMethod === 'function') { + windowMethod(focusedWindow) + } else { + focusedWindow[windowMethod]() } + return true + } + + if (webContentsMethod && focusedWindow != null) { + const {webContents} = focusedWindow + if (webContents) { + webContents[webContentsMethod]() + } + return true } return false From b7afe44a5c16b1e876b08a32cca731c0c777d5e0 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 22 Jun 2016 14:22:15 -0700 Subject: [PATCH 14/19] Add assert for role with app name in label --- spec/api-menu-spec.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/api-menu-spec.js b/spec/api-menu-spec.js index a6c56a9a4..7d1044b23 100644 --- a/spec/api-menu-spec.js +++ b/spec/api-menu-spec.js @@ -417,6 +417,10 @@ describe('menu module', function () { var item = new MenuItem({role: 'help'}) assert.equal(item.label, 'Help') assert.equal(item.accelerator, undefined) + + var item = new MenuItem({role: 'about'}) + assert.equal(item.label, 'About Electron Test') + assert.equal(item.accelerator, undefined) }) }) }) From 4dbdcad05ea841021a906b305161beaced5c0042 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 22 Jun 2016 14:37:16 -0700 Subject: [PATCH 15/19] Remove label/accelerators with role defaults --- docs/api/menu.md | 39 ++------------------------------------- 1 file changed, 2 insertions(+), 37 deletions(-) diff --git a/docs/api/menu.md b/docs/api/menu.md index 7dea2c6b4..d54469545 100644 --- a/docs/api/menu.md +++ b/docs/api/menu.md @@ -39,45 +39,30 @@ const template = [ label: 'Edit', submenu: [ { - label: 'Undo', - accelerator: 'CmdOrCtrl+Z', role: 'undo' }, { - label: 'Redo', - accelerator: 'Shift+CmdOrCtrl+Z', role: 'redo' }, { type: 'separator' }, { - label: 'Cut', - accelerator: 'CmdOrCtrl+X', role: 'cut' }, { - label: 'Copy', - accelerator: 'CmdOrCtrl+C', role: 'copy' }, { - label: 'Paste', - accelerator: 'CmdOrCtrl+V', role: 'paste' }, { - label: 'Paste and Match Style', - accelerator: 'Shift+Command+V', role: 'pasteandmatchstyle' }, { - label: 'Delete', role: 'delete' }, { - label: 'Select All', - accelerator: 'CmdOrCtrl+A', role: 'selectall' }, ] @@ -93,12 +78,7 @@ const template = [ } }, { - label: 'Toggle Full Screen', - accelerator: process.platform === 'darwin' ? 'Ctrl+Command+F' : 'F11', - click(item, focusedWindow) { - if (focusedWindow) - focusedWindow.setFullScreen(!focusedWindow.isFullScreen()); - } + role: 'togglefullscreen' }, { label: 'Toggle Developer Tools', @@ -111,23 +91,17 @@ const template = [ ] }, { - label: 'Window', role: 'window', submenu: [ { - label: 'Minimize', - accelerator: 'CmdOrCtrl+M', role: 'minimize' }, { - label: 'Close', - accelerator: 'CmdOrCtrl+W', role: 'close' }, ] }, { - label: 'Help', role: 'help', submenu: [ { @@ -144,14 +118,12 @@ if (process.platform === 'darwin') { label: name, submenu: [ { - label: 'About ' + name, role: 'about' }, { type: 'separator' }, { - label: 'Services', role: 'services', submenu: [] }, @@ -159,26 +131,19 @@ if (process.platform === 'darwin') { type: 'separator' }, { - label: 'Hide ' + name, - accelerator: 'Command+H', role: 'hide' }, { - label: 'Hide Others', - accelerator: 'Command+Alt+H', role: 'hideothers' }, { - label: 'Show All', role: 'unhide' }, { type: 'separator' }, { - label: 'Quit', - accelerator: 'Command+Q', - click() { app.quit(); } + role: 'quit' }, ] }); From be642612c02b8e7fba92b2417a135a7b092d939d Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 22 Jun 2016 15:21:45 -0700 Subject: [PATCH 16/19] Export app before requiring modules --- lib/browser/api/app.js | 12 ++++++------ lib/browser/api/menu-item-roles.js | 6 ++---- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/lib/browser/api/app.js b/lib/browser/api/app.js index 09835a792..34210f420 100644 --- a/lib/browser/api/app.js +++ b/lib/browser/api/app.js @@ -1,12 +1,15 @@ 'use strict' +const bindings = process.atomBinding('app') +const {app} = bindings + +// Only one app object permitted. +module.exports = app + const electron = require('electron') const {deprecate, Menu} = electron const {EventEmitter} = require('events') -const bindings = process.atomBinding('app') -const {app} = bindings - Object.setPrototypeOf(app, EventEmitter.prototype) let appPath = null @@ -67,6 +70,3 @@ process.atomBinding('download_item')._setWrapDownloadItem((downloadItem) => { // downloadItem is an EventEmitter. Object.setPrototypeOf(downloadItem, EventEmitter.prototype) }) - -// Only one App object pemitted. -module.exports = app diff --git a/lib/browser/api/menu-item-roles.js b/lib/browser/api/menu-item-roles.js index aaebd5622..0df6126a1 100644 --- a/lib/browser/api/menu-item-roles.js +++ b/lib/browser/api/menu-item-roles.js @@ -1,7 +1,8 @@ +const {app} = require('electron') + const roles = { about: { get label () { - const {app} = require('electron') return `About ${app.getName()}` } }, @@ -33,7 +34,6 @@ const roles = { }, hide: { get label () { - const {app} = require('electron') return `Hide ${app.getName()}` }, accelerator: 'Command+H' @@ -59,7 +59,6 @@ const roles = { }, quit: { get label () { - const {app} = require('electron') return process.platform === 'win32' ? 'Exit' : `Quit ${app.getName()}` }, accelerator: process.platform === 'win32' ? null : 'Command+Q', @@ -120,7 +119,6 @@ exports.execute = function (role, focusedWindow) { const {appMethod, webContentsMethod, windowMethod} = roles[role] if (appMethod) { - const {app} = require('electron') app[appMethod]() return true } From 6165908ba7c0d6712d16939329d96596fbdd4a57 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 22 Jun 2016 15:26:17 -0700 Subject: [PATCH 17/19] Incorporate review feedback --- lib/browser/api/menu-item-roles.js | 33 ++++++++++++++++-------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/lib/browser/api/menu-item-roles.js b/lib/browser/api/menu-item-roles.js index 0df6126a1..232c3e05b 100644 --- a/lib/browser/api/menu-item-roles.js +++ b/lib/browser/api/menu-item-roles.js @@ -3,27 +3,26 @@ const {app} = require('electron') const roles = { about: { get label () { - return `About ${app.getName()}` + return process.platform === 'linux' ? 'About' : `About ${app.getName()}` } }, close: { label: 'Close', - accelerator: 'CmdOrCtrl+W', + accelerator: 'CommandOrControl+W', windowMethod: 'close' }, copy: { label: 'Copy', - accelerator: 'CmdOrCtrl+C', + accelerator: 'CommandOrControl+C', webContentsMethod: 'copy' }, cut: { label: 'Cut', - accelerator: 'CmdOrCtrl+X', + accelerator: 'CommandOrControl+X', webContentsMethod: 'cut' }, delete: { label: 'Delete', - accelerator: 'Delete', webContentsMethod: 'delete' }, front: { @@ -44,34 +43,38 @@ const roles = { }, minimize: { label: 'Minimize', - accelerator: 'CmdOrCtrl+M', + accelerator: 'CommandOrControl+M', windowMethod: 'minimize' }, paste: { label: 'Paste', - accelerator: 'CmdOrCtrl+V', + accelerator: 'CommandOrControl+V', webContentsMethod: 'paste' }, pasteandmatchstyle: { label: 'Paste and Match Style', - accelerator: 'Shift+Command+V', + accelerator: 'Shift+CommandOrControl+V', webContentsMethod: 'pasteAndMatchStyle' }, quit: { get label () { - return process.platform === 'win32' ? 'Exit' : `Quit ${app.getName()}` + switch (process.platform) { + case 'darwin': return `Quit ${app.getName()}` + case 'win32': return 'Exit' + default: return 'Quit' + } }, accelerator: process.platform === 'win32' ? null : 'Command+Q', appMethod: 'quit' }, redo: { label: 'Redo', - accelerator: 'Shift+CmdOrCtrl+Z', + accelerator: 'Shift+CommandOrControl+Z', webContentsMethod: 'redo' }, selectall: { label: 'Select All', - accelerator: 'CmdOrCtrl+A', + accelerator: 'CommandOrControl+A', webContentsMethod: 'selectAll' }, services: { @@ -86,7 +89,7 @@ const roles = { }, undo: { label: 'Undo', - accelerator: 'CmdOrCtrl+Z', + accelerator: 'CommandOrControl+Z', webContentsMethod: 'undo' }, unhide: { @@ -100,7 +103,7 @@ const roles = { } } -exports.getDefaultLabel = function (role) { +exports.getDefaultLabel = (role) => { if (roles.hasOwnProperty(role)) { return roles[role].label } else { @@ -108,11 +111,11 @@ exports.getDefaultLabel = function (role) { } } -exports.getDefaultAccelerator = function (role) { +exports.getDefaultAccelerator = (role) => { if (roles.hasOwnProperty(role)) return roles[role].accelerator } -exports.execute = function (role, focusedWindow) { +exports.execute = (role, focusedWindow) => { if (!roles.hasOwnProperty(role)) return false if (process.platform === 'darwin') return false From 813e528350a78730375f26d10d6aa1c0659f2f3d Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 22 Jun 2016 15:35:58 -0700 Subject: [PATCH 18/19] Update expected accelerator --- spec/api-menu-spec.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/api-menu-spec.js b/spec/api-menu-spec.js index 7d1044b23..d78b5b4e3 100644 --- a/spec/api-menu-spec.js +++ b/spec/api-menu-spec.js @@ -400,11 +400,11 @@ describe('menu module', function () { it('includes a default label and accelerator', function () { var item = new MenuItem({role: 'close'}) assert.equal(item.label, 'Close') - assert.equal(item.accelerator, 'CmdOrCtrl+W') + assert.equal(item.accelerator, 'CommandOrControl+W') var item = new MenuItem({role: 'close', label: 'Other'}) assert.equal(item.label, 'Other') - assert.equal(item.accelerator, 'CmdOrCtrl+W') + assert.equal(item.accelerator, 'CommandOrControl+W') var item = new MenuItem({role: 'close', accelerator: 'D'}) assert.equal(item.label, 'Close') @@ -418,9 +418,9 @@ describe('menu module', function () { assert.equal(item.label, 'Help') assert.equal(item.accelerator, undefined) - var item = new MenuItem({role: 'about'}) - assert.equal(item.label, 'About Electron Test') - assert.equal(item.accelerator, undefined) + var item = new MenuItem({role: 'hide'}) + assert.equal(item.label, 'Hide Electron Test') + assert.equal(item.accelerator, 'Command+H') }) }) }) From c6869972091c305c288c780a9722ae27f10aa433 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 22 Jun 2016 16:56:45 -0700 Subject: [PATCH 19/19] Ctrl -> Control for consistency --- lib/browser/api/menu-item-roles.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/browser/api/menu-item-roles.js b/lib/browser/api/menu-item-roles.js index 232c3e05b..082099818 100644 --- a/lib/browser/api/menu-item-roles.js +++ b/lib/browser/api/menu-item-roles.js @@ -82,7 +82,7 @@ const roles = { }, togglefullscreen: { label: 'Toggle Full Screen', - accelerator: process.platform === 'darwin' ? 'Ctrl+Command+F' : 'F11', + accelerator: process.platform === 'darwin' ? 'Control+Command+F' : 'F11', windowMethod: function (window) { window.setFullScreen(!window.isFullScreen()) }