From e329340a8edf212bd42a6b5622d065c156170ebf Mon Sep 17 00:00:00 2001 From: petemill Date: Thu, 5 Apr 2018 10:07:53 -0700 Subject: [PATCH] Some menu items which need a window will create / show one if needed Fix #13689 This bug was caused because menu items were using a hidden window for their new tabs. We should _not_ use `electron{.remote}.BrowserWindow.getAllWindows()` or `.get{Active, Focused}Window` directly. There are two reasons that could have bad results: 1. We create BrowserWindows which are not normal tabbed renderer windows (e.g. some code that runs on Bookmark creation) 2. We have a hidden window sometimes (the Buffer Window) Instead call `app/browser/window.js`: `getAllRendererWindows` and `getActiveWindowId`. Fix for main menu actions, which can be called when there are no windows, but when searching for `BrowserWindow.` Test Plan: - Check all menu items open new tab in currently active window - Check all menu items open new tab in currently active window, when app is not focused - Check all menu items open new tab in new window if there is no visible window - Check tabs in all windows are persisted on restart when app / windows close - Check process quits in Windows when last visible window is closed --- app/browser/reducers/tabsReducer.js | 14 ++++++++++++-- app/browser/windows.js | 24 +++++++++++++++++------- app/common/commonMenu.js | 23 +++++------------------ 3 files changed, 34 insertions(+), 27 deletions(-) diff --git a/app/browser/reducers/tabsReducer.js b/app/browser/reducers/tabsReducer.js index 1bfa0da25fd..c8e4d2a56bc 100644 --- a/app/browser/reducers/tabsReducer.js +++ b/app/browser/reducers/tabsReducer.js @@ -12,6 +12,7 @@ const windowActions = require('../../../js/actions/windowActions') // State const tabState = require('../../common/state/tabState') +const windowState = require('../../common/state/windowState') const siteSettings = require('../../../js/state/siteSettings') const siteSettingsState = require('../../common/state/siteSettingsState') const {frameOptsFromFrame} = require('../../../js/state/frameStateUtil') @@ -23,6 +24,7 @@ const appConstants = require('../../../js/constants/appConstants') const windowConstants = require('../../../js/constants/windowConstants') const dragTypes = require('../../../js/constants/dragTypes') const tabActionConsts = require('../../common/constants/tabAction') +const appActions = require('../../../js/actions/appActions') // Utils const tabs = require('../tabs') @@ -141,8 +143,16 @@ const tabsReducer = (state, action, immutableAction) => { const senderWindowId = action.getIn(['senderWindowId']) if (senderWindowId != null) { action = action.setIn(['createProperties', 'windowId'], senderWindowId) - } else if (BrowserWindow.getActiveWindow()) { - action = action.setIn(['createProperties', 'windowId'], BrowserWindow.getActiveWindow().id) + } else { + // no specified window, so use active one, or create one + const activeWindowId = windows.getActiveWindowId() + if (activeWindowId === windowState.WINDOW_ID_NONE) { + setImmediate(() => appActions.newWindow(action.get('createProperties'))) + // this action will get dispatched again + // once the new window is ready to have tabs + break + } + action = action.setIn(['createProperties', 'windowId'], activeWindowId) } } diff --git a/app/browser/windows.js b/app/browser/windows.js index 7cd5ef45ddc..bf69b7141ce 100644 --- a/app/browser/windows.js +++ b/app/browser/windows.js @@ -348,7 +348,7 @@ const api = { // that will not get saved to state as the last-closed window which should be restored // since we won't save state if there are no frames. if (!platformUtil.isDarwin() && api.getBufferWindow()) { - const remainingWindows = api.getAllRendererWindows().filter(win => win !== api.getBufferWindow()) + const remainingWindows = api.getAllRendererWindows() if (!remainingWindows.length) { api.closeBufferWindow() } @@ -784,21 +784,31 @@ const api = { return currentWindows[windowId] }, - getActiveWindowId: () => { - if (BrowserWindow.getFocusedWindow()) { - return BrowserWindow.getFocusedWindow().id + getActiveWindow: () => { + const focusedWindow = BrowserWindow.getFocusedWindow() + if (api.getAllRendererWindows().includes(focusedWindow)) { + return focusedWindow } - return windowState.WINDOW_ID_NONE + return null + }, + + getActiveWindowId: () => { + const activeWindow = api.getActiveWindow() + return activeWindow ? activeWindow.id : windowState.WINDOW_ID_NONE }, /** * Provides an array of all Browser Windows which are actual * main windows (not background workers), and are not destroyed */ - getAllRendererWindows: () => { + getAllRendererWindows: (includingBufferWindow = false) => { return Object.keys(currentWindows) .map(key => currentWindows[key]) - .filter(win => win && !win.isDestroyed()) + .filter(win => + win && + !win.isDestroyed() && + (includingBufferWindow || win !== api.getBufferWindow()) + ) }, on: (...args) => publicEvents.on(...args), diff --git a/app/common/commonMenu.js b/app/common/commonMenu.js index bf28b204621..20328849067 100644 --- a/app/common/commonMenu.js +++ b/app/common/commonMenu.js @@ -14,25 +14,12 @@ const communityURL = 'https://community.brave.com/' const isDarwin = process.platform === 'darwin' const electron = require('electron') -let BrowserWindow -if (process.type === 'browser') { - BrowserWindow = electron.BrowserWindow -} else { - BrowserWindow = electron.remote.BrowserWindow -} - const ensureAtLeastOneWindow = (frameOpts) => { - if (process.type === 'browser') { - if (BrowserWindow.getAllWindows().length === 0) { - appActions.newWindow(frameOpts || {}) - return - } - } - - if (!frameOpts) { - return - } - + // If this action is dispatched from a renderer window (windows) + // it will create the tab in the current window + // If it was dispatched by the browser (mac / linux) + // then it will create the tab in the active window + // or a new window if there is no active window appActions.createTabRequested(frameOpts) }