From 23d44e322d4265b925758103a7fb5802ebefae8c Mon Sep 17 00:00:00 2001 From: Milan Burda Date: Tue, 15 Jan 2019 21:35:53 +0100 Subject: [PATCH] feat: use default-app behavior in packaged apps (#16310) Unify the behavior between default app and packaged apps: - create default application menu unless the app has one - default window-all-closed handling unless the app handles the event --- default_app/main.js | 17 +---- docs/api/menu.md | 7 +- filenames.gni | 2 +- lib/browser/api/menu.js | 2 + .../menu.js => lib/browser/default-menu.js | 3 +- lib/browser/init.js | 12 ++++ spec/api-app-spec.js | 67 ++++++++++++++----- spec/fixtures/api/default-menu/main.js | 32 +++++++++ spec/fixtures/api/default-menu/package.json | 4 ++ spec/fixtures/api/window-all-closed/main.js | 20 ++++++ .../api/window-all-closed/package.json | 4 ++ 11 files changed, 133 insertions(+), 37 deletions(-) rename default_app/menu.js => lib/browser/default-menu.js (91%) create mode 100644 spec/fixtures/api/default-menu/main.js create mode 100644 spec/fixtures/api/default-menu/package.json create mode 100644 spec/fixtures/api/window-all-closed/main.js create mode 100644 spec/fixtures/api/window-all-closed/package.json diff --git a/default_app/main.js b/default_app/main.js index f8a71e347fb7..a4d0938abf35 100644 --- a/default_app/main.js +++ b/default_app/main.js @@ -7,8 +7,6 @@ const Module = require('module') const path = require('path') const url = require('url') -const { setDefaultApplicationMenu } = require('./menu') - // Parse command line options. const argv = process.argv.slice(1) @@ -59,18 +57,6 @@ if (nextArgIsRequire) { process.exit(1) } -// Quit when all windows are closed and no other one is listening to this. -app.on('window-all-closed', () => { - if (app.listeners('window-all-closed').length === 1 && !option.interactive) { - app.quit() - } -}) - -// Create default menu. -app.once('ready', () => { - setDefaultApplicationMenu() -}) - // Set up preload modules if (option.modules.length > 0) { Module._preloadModules(option.modules) @@ -142,6 +128,9 @@ function startRepl () { process.exit(1) } + // prevent quitting + app.on('window-all-closed', () => {}) + const repl = require('repl') repl.start('> ').on('exit', () => { process.exit(0) diff --git a/docs/api/menu.md b/docs/api/menu.md index 4e31779f7982..415c8fc52cb8 100644 --- a/docs/api/menu.md +++ b/docs/api/menu.md @@ -25,10 +25,11 @@ indicate which letter should get a generated accelerator. For example, using opens the associated menu. The indicated character in the button label gets an underline. The `&` character is not displayed on the button label. -Passing `null` will remove the menu bar on Windows and Linux but has no -effect on macOS. +Passing `null` will suppress the default menu. On Windows and Linux, +this has the additional effect of removing the menu bar from the window. -**Note:** This API has to be called after the `ready` event of `app` module. +**Note:** The default menu will be created automatically if the app does not set one. +It contains standard items such as `File`, `Edit`, `View`, `Window` and `Help`. #### `Menu.getApplicationMenu()` diff --git a/filenames.gni b/filenames.gni index 19476915d2a8..4004edccf133 100644 --- a/filenames.gni +++ b/filenames.gni @@ -35,6 +35,7 @@ filenames = { "lib/browser/api/web-contents.js", "lib/browser/api/web-contents-view.js", "lib/browser/chrome-extension.js", + "lib/browser/default-menu.js", "lib/browser/guest-view-manager.js", "lib/browser/guest-window-manager.js", "lib/browser/init.js", @@ -92,7 +93,6 @@ filenames = { "default_app/icon.png", "default_app/index.html", "default_app/main.js", - "default_app/menu.js", "default_app/package.json", "default_app/renderer.js", "default_app/styles.css", diff --git a/lib/browser/api/menu.js b/lib/browser/api/menu.js index 1b282a4a188d..2a977538c202 100644 --- a/lib/browser/api/menu.js +++ b/lib/browser/api/menu.js @@ -145,6 +145,8 @@ Menu.setApplicationMenu = function (menu) { } applicationMenu = menu + v8Util.setHiddenValue(global, 'applicationMenuSet', true) + if (process.platform === 'darwin') { if (!menu) return menu._callMenuWillShow() diff --git a/default_app/menu.js b/lib/browser/default-menu.js similarity index 91% rename from default_app/menu.js rename to lib/browser/default-menu.js index 7dd71017791a..09ed05bcdcb2 100644 --- a/default_app/menu.js +++ b/lib/browser/default-menu.js @@ -1,11 +1,12 @@ 'use strict' const { shell, Menu } = require('electron') +const v8Util = process.atomBinding('v8_util') const isMac = process.platform === 'darwin' const setDefaultApplicationMenu = () => { - if (Menu.getApplicationMenu()) return + if (v8Util.getHiddenValue(global, 'applicationMenuSet')) return const helpMenu = { role: 'help', diff --git a/lib/browser/init.js b/lib/browser/init.js index fc526e1f0654..c50c5f0d1eb8 100644 --- a/lib/browser/init.js +++ b/lib/browser/init.js @@ -184,5 +184,17 @@ if (currentPlatformSupportsAppIndicator()) { process.env.XDG_CURRENT_DESKTOP = 'Unity' } +// Quit when all windows are closed and no other one is listening to this. +app.on('window-all-closed', () => { + if (app.listenerCount('window-all-closed') === 1) { + app.quit() + } +}) + +const { setDefaultApplicationMenu } = require('@electron/internal/browser/default-menu') + +// Create default menu. +app.once('ready', setDefaultApplicationMenu) + // Finally load app's main.js and transfer control to C++. Module._load(path.join(packagePath, mainStartupScript), Module, true) diff --git a/spec/api-app-spec.js b/spec/api-app-spec.js index 05e118601906..9084c7ec4c23 100644 --- a/spec/api-app-spec.js +++ b/spec/api-app-spec.js @@ -1179,12 +1179,12 @@ describe('app module', () => { describe('commandLine.hasSwitch (existing argv)', () => { it('returns true when present', async () => { - const { hasSwitch } = await runCommandLineTestApp('--foobar') + const { hasSwitch } = await runTestApp('command-line', '--foobar') expect(hasSwitch).to.be.true() }) it('returns false when not present', async () => { - const { hasSwitch } = await runCommandLineTestApp() + const { hasSwitch } = await runTestApp('command-line') expect(hasSwitch).to.be.false() }) }) @@ -1207,31 +1207,62 @@ describe('app module', () => { describe('commandLine.getSwitchValue (existing argv)', () => { it('returns the value when present', async () => { - const { getSwitchValue } = await runCommandLineTestApp('--foobar=test') + const { getSwitchValue } = await runTestApp('command-line', '--foobar=test') expect(getSwitchValue).to.equal('test') }) it('returns an empty string when present without value', async () => { - const { getSwitchValue } = await runCommandLineTestApp('--foobar') + const { getSwitchValue } = await runTestApp('command-line', '--foobar') expect(getSwitchValue).to.equal('') }) it('returns an empty string when not present', async () => { - const { getSwitchValue } = await runCommandLineTestApp() + const { getSwitchValue } = await runTestApp('command-line') expect(getSwitchValue).to.equal('') }) }) - - async function runCommandLineTestApp (...args) { - const appPath = path.join(__dirname, 'fixtures', 'api', 'command-line') - const electronPath = remote.getGlobal('process').execPath - const appProcess = cp.spawn(electronPath, [appPath, ...args]) - - let output = '' - appProcess.stdout.on('data', (data) => { output += data }) - - await emittedOnce(appProcess.stdout, 'end') - - return JSON.parse(output) - } }) + +describe('default behavior', () => { + describe('application menu', () => { + it('creates the default menu if the app does not set it', async () => { + const result = await runTestApp('default-menu') + expect(result).to.equal(false) + }) + + it('does not create the default menu if the app sets a custom menu', async () => { + const result = await runTestApp('default-menu', '--custom-menu') + expect(result).to.equal(true) + }) + + it('does not create the default menu if the app sets a null menu', async () => { + const result = await runTestApp('default-menu', '--null-menu') + expect(result).to.equal(true) + }) + }) + + describe('window-all-closed', () => { + it('quits when the app does not handle the event', async () => { + const result = await runTestApp('window-all-closed') + expect(result).to.equal(false) + }) + + it('does not quit when the app handles the event', async () => { + const result = await runTestApp('window-all-closed', '--handle-event') + expect(result).to.equal(true) + }) + }) +}) + +async function runTestApp (name, ...args) { + const appPath = path.join(__dirname, 'fixtures', 'api', name) + const electronPath = remote.getGlobal('process').execPath + const appProcess = cp.spawn(electronPath, [appPath, ...args]) + + let output = '' + appProcess.stdout.on('data', (data) => { output += data }) + + await emittedOnce(appProcess.stdout, 'end') + + return JSON.parse(output) +} diff --git a/spec/fixtures/api/default-menu/main.js b/spec/fixtures/api/default-menu/main.js new file mode 100644 index 000000000000..786d9ab4b0d3 --- /dev/null +++ b/spec/fixtures/api/default-menu/main.js @@ -0,0 +1,32 @@ +const { app, Menu } = require('electron') + +function output (value) { + process.stdout.write(JSON.stringify(value)) + process.stdout.end() + + app.quit() +} + +try { + let expectedMenu + + if (app.commandLine.hasSwitch('custom-menu')) { + expectedMenu = new Menu() + Menu.setApplicationMenu(expectedMenu) + } else if (app.commandLine.hasSwitch('null-menu')) { + expectedMenu = null + Menu.setApplicationMenu(null) + } + + app.on('ready', () => { + setImmediate(() => { + try { + output(Menu.getApplicationMenu() === expectedMenu) + } catch (error) { + output(null) + } + }) + }) +} catch (error) { + output(null) +} diff --git a/spec/fixtures/api/default-menu/package.json b/spec/fixtures/api/default-menu/package.json new file mode 100644 index 000000000000..7e253de58f85 --- /dev/null +++ b/spec/fixtures/api/default-menu/package.json @@ -0,0 +1,4 @@ +{ + "name": "default-menu", + "main": "main.js" +} diff --git a/spec/fixtures/api/window-all-closed/main.js b/spec/fixtures/api/window-all-closed/main.js new file mode 100644 index 000000000000..67450d93e751 --- /dev/null +++ b/spec/fixtures/api/window-all-closed/main.js @@ -0,0 +1,20 @@ +const { app, BrowserWindow } = require('electron') + +let handled = false + +if (app.commandLine.hasSwitch('handle-event')) { + app.on('window-all-closed', () => { + handled = true + app.quit() + }) +} + +app.on('quit', () => { + process.stdout.write(JSON.stringify(handled)) + process.stdout.end() +}) + +app.on('ready', () => { + const win = new BrowserWindow() + win.close() +}) diff --git a/spec/fixtures/api/window-all-closed/package.json b/spec/fixtures/api/window-all-closed/package.json new file mode 100644 index 000000000000..6486c5bc6e98 --- /dev/null +++ b/spec/fixtures/api/window-all-closed/package.json @@ -0,0 +1,4 @@ +{ + "name": "window-all-closed", + "main": "main.js" +}