From f415f5aeee358c13a615b8632ff3eff372c5a7ff Mon Sep 17 00:00:00 2001 From: NejcZdovc Date: Sun, 13 Aug 2017 12:56:27 +0200 Subject: [PATCH] Save window appState into windowState when closing a window Resolves #8600 Resolves #9709 Resolves #3754 Resolves #6602 Auditors: Test Plan: --- app/browser/reducers/tabsReducer.js | 2 +- app/browser/reducers/windowsReducer.js | 16 ++--- app/browser/windows.js | 23 +++---- app/common/state/windowState.js | 19 +++++- app/renderer/components/main/main.js | 1 - js/actions/appActions.js | 18 +---- js/actions/windowActions.js | 10 +++ js/constants/appConstants.js | 1 - js/constants/windowConstants.js | 3 +- js/contextMenus.js | 2 +- js/entry.js | 2 +- js/stores/appStore.js | 67 +++++++++++++------ js/stores/windowStore.js | 3 + test/misc-components/windowTest.js | 44 ++++++++++-- .../app/browser/reducers/tabsReducerTest.js | 1 + 15 files changed, 140 insertions(+), 72 deletions(-) diff --git a/app/browser/reducers/tabsReducer.js b/app/browser/reducers/tabsReducer.js index 782bf95d726..917a2110435 100644 --- a/app/browser/reducers/tabsReducer.js +++ b/app/browser/reducers/tabsReducer.js @@ -368,7 +368,7 @@ const tabsReducer = (state, action, immutableAction) => { if (dragData && dragData.get('type') === dragTypes.TAB) { const frame = dragData.get('data') const frameOpts = frameOptsFromFrame(frame).toJS() - const browserOpts = { positionByMouseCursor: true } + const browserOpts = { positionByMouseCursor: true, checkMaximized: true } frameOpts.indexByFrameKey = dragData.getIn(['dragOverData', 'draggingOverKey']) frameOpts.prependIndexByFrameKey = dragData.getIn(['dragOverData', 'draggingOverLeftHalf']) tabs.moveTo(state, frame.get('tabId'), frameOpts, browserOpts, dragData.get('dropWindowId')) diff --git a/app/browser/reducers/windowsReducer.js b/app/browser/reducers/windowsReducer.js index 358ca04b709..c1819623432 100644 --- a/app/browser/reducers/windowsReducer.js +++ b/app/browser/reducers/windowsReducer.js @@ -39,15 +39,13 @@ const windowsReducer = (state, action, immutableAction) => { break case appConstants.APP_WINDOW_UPDATED: state = windowState.maybeCreateWindow(state, action) - break - case appConstants.APP_DEFAULT_WINDOW_PARAMS_CHANGED: - if (action.get('size')) { - state = state.setIn(['defaultWindowParams', 'width'], action.getIn(['size', 0])) - state = state.setIn(['defaultWindowParams', 'height'], action.getIn(['size', 1])) - } - if (action.get('position')) { - state = state.setIn(['defaultWindowParams', 'x'], action.getIn(['position', 0])) - state = state.setIn(['defaultWindowParams', 'y'], action.getIn(['position', 1])) + if (action.get('updateDefault')) { + state = state + .setIn(['defaultWindowParams', 'width'], action.getIn(['windowValue', 'width'])) + .setIn(['defaultWindowParams', 'height'], action.getIn(['windowValue', 'height'])) + .setIn(['defaultWindowParams', 'x'], action.getIn(['windowValue', 'x'])) + .setIn(['defaultWindowParams', 'y'], action.getIn(['windowValue', 'y'])) + .setIn(['defaultWindowParams', 'maximized'], action.getIn(['windowValue', 'state']) === 'maximized') } break case windowConstants.WINDOW_SHOULD_SET_TITLE: diff --git a/app/browser/windows.js b/app/browser/windows.js index b692e228fbc..13efea9d692 100644 --- a/app/browser/windows.js +++ b/app/browser/windows.js @@ -18,6 +18,7 @@ const windowState = require('../common/state/windowState') const Immutable = require('immutable') const pinnedSitesState = require('../common/state/pinnedSitesState') const pinnedSitesUtil = require('../common/lib/pinnedSitesUtil') +const windowActions = require('../../js/actions/windowActions') // TODO(bridiver) - set window uuid let currentWindows = {} @@ -55,10 +56,11 @@ const getWindowValue = (windowId) => { } } -const updateWindow = (windowId) => { +const updateWindow = (windowId, updateDefault = false) => { const windowValue = getWindowValue(windowId) if (windowValue) { - appActions.windowUpdated(windowValue) + appActions.windowUpdated(windowValue, updateDefault) + windowActions.onWindowUpdate(windowId, windowValue) } } @@ -189,6 +191,7 @@ const api = { LocalShortcuts.register(win) appActions.windowCreated(windowValue) + windowActions.onWindowUpdate(windowId, windowValue) }) win.once('closed', () => { cleanupWindow(windowId) @@ -198,7 +201,7 @@ const api = { updateWindowDebounce(windowId) }) win.on('focus', () => { - updateWindowDebounce(windowId) + updateWindowDebounce(windowId, true) }) win.on('show', () => { updateWindowDebounce(windowId) @@ -207,7 +210,7 @@ const api = { updateWindowDebounce(windowId) }) win.on('maximize', () => { - updateWindowDebounce(windowId) + updateWindowDebounce(windowId, true) }) win.on('unmaximize', () => { updateWindowDebounce(windowId) @@ -219,18 +222,10 @@ const api = { updateWindowDebounce(windowId) }) win.on('resize', () => { - updateWindowDebounce(windowId) - const size = win.getSize() - const position = win.getPosition() - // NOTE: the default window size is whatever the last window resize was - appActions.defaultWindowParamsChanged(size, position) + updateWindowDebounce(windowId, true) }) win.on('move', () => { - updateWindowMove(windowId) - const size = win.getSize() - const position = win.getPosition() - // NOTE: the default window position is whatever the last window move was - appActions.defaultWindowParamsChanged(size, position) + updateWindowMove(windowId, true) }) win.on('enter-full-screen', () => { updateWindowDebounce(windowId) diff --git a/app/common/state/windowState.js b/app/common/state/windowState.js index 8b9505b7c78..39301c6d000 100644 --- a/app/common/state/windowState.js +++ b/app/common/state/windowState.js @@ -109,7 +109,14 @@ const api = { index = parseInt(index) assert.ok(index >= 0, 'index must be positive') state = validateState(state) - return state.set('windows', state.get('windows').delete(index)) + const window = state.getIn(['windows', index]) + + if (window == null) { + return state + } + + state = state.set('windows', state.get('windows').delete(index)) + return state.set('lastClosedWindow', window) }, removeWindow: (state, action) => { @@ -182,6 +189,16 @@ const api = { !windowState.getIn(['ui', 'noScriptInfo', 'isVisible']) && frame && !frame.getIn(['security', 'loginRequiredDetail']) && !windowState.getIn(['ui', 'menubar', 'selectedIndex']) + }, + + getLastClosedWindow: (state, windowId) => { + state = validateState(state) + + if (state.getIn(['lastClosedWindow', 'windowId']) === windowId) { + return state.get('lastClosedWindow') + } + + return null } } diff --git a/app/renderer/components/main/main.js b/app/renderer/components/main/main.js index 3276e4b0649..ca5c1daf4eb 100644 --- a/app/renderer/components/main/main.js +++ b/app/renderer/components/main/main.js @@ -440,7 +440,6 @@ class Main extends React.Component { } }, { passive: true }) - // disable dnd by default window.addEventListener('dragover', function (event) { // allow webviews to handle dnd diff --git a/js/actions/appActions.js b/js/actions/appActions.js index 85223478229..0bf62002f06 100644 --- a/js/actions/appActions.js +++ b/js/actions/appActions.js @@ -65,10 +65,11 @@ const appActions = { }) }, - windowUpdated: function (windowValue) { + windowUpdated: function (windowValue, updateDefault) { dispatch({ actionType: appConstants.APP_WINDOW_UPDATED, - windowValue + windowValue, + updateDefault }) }, @@ -304,19 +305,6 @@ const appActions = { }) }, - /** - * Sets the default window size / position - * @param {Array} size - [width, height] - * @param {Array} position - [x, y] - */ - defaultWindowParamsChanged: function (size, position) { - dispatch({ - actionType: appConstants.APP_DEFAULT_WINDOW_PARAMS_CHANGED, - size, - position - }) - }, - /** * Sets the etag value for a downloaded data file. * This is used for keeping track of when to re-download adblock and tracking diff --git a/js/actions/windowActions.js b/js/actions/windowActions.js index 0caad9b0b77..a1887f23fdc 100644 --- a/js/actions/windowActions.js +++ b/js/actions/windowActions.js @@ -1203,6 +1203,16 @@ const windowActions = { bookmarkKey, type }) + }, + + onWindowUpdate: function (windowId, windowValue) { + dispatch({ + actionType: windowConstants.WINDOW_ON_WINDOW_UPDATE, + queryInfo: { + windowId + }, + windowValue + }) } } diff --git a/js/constants/appConstants.js b/js/constants/appConstants.js index 313fe92effc..3d0bfcc1262 100644 --- a/js/constants/appConstants.js +++ b/js/constants/appConstants.js @@ -18,7 +18,6 @@ const appConstants = { APP_REMOVE_HISTORY_SITE: _, APP_MERGE_DOWNLOAD_DETAIL: _, // Sets an individual download detail APP_CLEAR_COMPLETED_DOWNLOADS: _, // Removes all completed downloads - APP_DEFAULT_WINDOW_PARAMS_CHANGED: _, APP_SET_DATA_FILE_ETAG: _, APP_SET_DATA_FILE_LAST_CHECK: _, APP_SET_RESOURCE_ENABLED: _, diff --git a/js/constants/windowConstants.js b/js/constants/windowConstants.js index 40c56fcef02..4b38ce4515a 100644 --- a/js/constants/windowConstants.js +++ b/js/constants/windowConstants.js @@ -108,7 +108,8 @@ const windowConstants = { WINDOW_ON_BOOKMARK_ADDED: _, WINDOW_ON_ADD_BOOKMARK_FOLDER: _, WINDOW_ON_EDIT_BOOKMARK_FOLDER: _, - WINDOW_ON_BOOKMARK_FOLDER_CLOSE: _ + WINDOW_ON_BOOKMARK_FOLDER_CLOSE: _, + WINDOW_ON_WINDOW_UPDATE: _ } module.exports = mapValuesByKeys(windowConstants) diff --git a/js/contextMenus.js b/js/contextMenus.js index df27106947e..94368dfc3d6 100644 --- a/js/contextMenus.js +++ b/js/contextMenus.js @@ -488,7 +488,7 @@ function tabTemplateInit (frameProps) { template.push({ label: locale.translation('detach'), click: (item) => { - const browserOpts = { positionByMouseCursor: true } + const browserOpts = { positionByMouseCursor: true, checkMaximized: true } appActions.tabMoved(tabId, frameProps.toJS(), browserOpts, -1) } }) diff --git a/js/entry.js b/js/entry.js index 12e060b627b..21504886a68 100644 --- a/js/entry.js +++ b/js/entry.js @@ -74,7 +74,7 @@ ipc.on(messages.CLEAR_CLOSED_FRAMES, () => { windowActions.clearClosedFrames() }) -window.addEventListener('beforeunload', function (e) { +window.addEventListener('beforeunload', function () { ipc.send(messages.LAST_WINDOW_STATE, windowStore.getState().toJS()) }) diff --git a/js/stores/appStore.js b/js/stores/appStore.js index eeefc8a2455..c05e846bb24 100644 --- a/js/stores/appStore.js +++ b/js/stores/appStore.js @@ -78,20 +78,22 @@ const navbarHeight = () => { */ const setWindowDimensions = (browserOpts, defaults, immutableWindowState) => { assert(isImmutable(immutableWindowState)) - if (immutableWindowState.getIn(['ui', 'size'])) { - browserOpts.width = firstDefinedValue(browserOpts.width, immutableWindowState.getIn(['ui', 'size', 0])) - browserOpts.height = firstDefinedValue(browserOpts.height, immutableWindowState.getIn(['ui', 'size', 1])) - } - browserOpts.width = firstDefinedValue(browserOpts.width, browserOpts.innerWidth, defaults.width) - // height and innerHeight are the frame webview size - browserOpts.height = firstDefinedValue(browserOpts.height, browserOpts.innerHeight) - if (typeof browserOpts.height === 'number') { - // add navbar height to get total height for BrowserWindow - browserOpts.height = browserOpts.height + navbarHeight() + if (immutableWindowState.getIn(['appData'])) { + browserOpts.width = firstDefinedValue(browserOpts.width, immutableWindowState.getIn(['appData', 'width'])) + browserOpts.height = firstDefinedValue(browserOpts.height, immutableWindowState.getIn(['appData', 'height'])) } else { - // no inner height so check outer height or use default - browserOpts.height = firstDefinedValue(browserOpts.outerHeight, defaults.height) + browserOpts.width = firstDefinedValue(browserOpts.width, browserOpts.innerWidth, defaults.width) + // height and innerHeight are the frame webview size + browserOpts.height = firstDefinedValue(browserOpts.height, browserOpts.innerHeight) + if (typeof browserOpts.height === 'number') { + // add navbar height to get total height for BrowserWindow + browserOpts.height = browserOpts.height + navbarHeight() + } else { + // no inner height so check outer height or use default + browserOpts.height = firstDefinedValue(browserOpts.outerHeight, defaults.height) + } } + return browserOpts } @@ -103,10 +105,10 @@ const setWindowPosition = (browserOpts, defaults, immutableWindowState) => { const screenPos = electron.screen.getCursorScreenPoint() browserOpts.x = screenPos.x browserOpts.y = screenPos.y - } else if (immutableWindowState.getIn(['ui', 'position'])) { + } else if (immutableWindowState.getIn(['appData'])) { // Position comes from window state - browserOpts.x = firstDefinedValue(browserOpts.x, immutableWindowState.getIn(['ui', 'position', 0])) - browserOpts.y = firstDefinedValue(browserOpts.y, immutableWindowState.getIn(['ui', 'position', 1])) + browserOpts.x = firstDefinedValue(browserOpts.x, immutableWindowState.getIn(['appData', 'left'])) + browserOpts.y = firstDefinedValue(browserOpts.y, immutableWindowState.getIn(['appData', 'top'])) } else if (typeof defaults.x === 'number' && typeof defaults.y === 'number') { // Position comes from the default position browserOpts.x = firstDefinedValue(browserOpts.x, defaults.x) @@ -119,11 +121,24 @@ const setWindowPosition = (browserOpts, defaults, immutableWindowState) => { return browserOpts } +const setMaximized = (browserOpts, immutableWindowState) => { + if (Object.keys(browserOpts).length > 0 && !browserOpts.checkMaximized) { + return false + } + + if (immutableWindowState.getIn(['appData'])) { + return immutableWindowState.getIn(['appData', 'state']) === 'maximized' + } + + return appState.getIn(['defaultWindowParams', 'maximized']) || false +} + const createWindow = (action) => { const frameOpts = (action.frameOpts && action.frameOpts.toJS()) || {} let browserOpts = (action.browserOpts && action.browserOpts.toJS()) || {} const immutableWindowState = action.restoredState || Immutable.Map() const defaults = windowDefaults() + const isMaximized = setMaximized(browserOpts, immutableWindowState) browserOpts = setWindowDimensions(browserOpts, defaults, immutableWindowState) browserOpts = setWindowPosition(browserOpts, defaults, immutableWindowState) @@ -132,9 +147,17 @@ const createWindow = (action) => { delete browserOpts.top const screen = electron.screen - const primaryDisplay = screen.getPrimaryDisplay() + let primaryDisplay = screen.getPrimaryDisplay() const parentWindowKey = browserOpts.parentWindowKey - const parentWindow = parentWindowKey ? BrowserWindow.fromId(parentWindowKey) : BrowserWindow.getFocusedWindow() + if (browserOpts.x != null && browserOpts.y != null) { + const matchingDisplay = screen.getDisplayMatching(browserOpts) + if (matchingDisplay != null) { + primaryDisplay = matchingDisplay + } + } + const parentWindow = parentWindowKey + ? BrowserWindow.fromId(parentWindowKey) + : BrowserWindow.getFocusedWindow() const bounds = parentWindow ? parentWindow.getBounds() : primaryDisplay.bounds // position on screen should be relative to focused window @@ -198,6 +221,10 @@ const createWindow = (action) => { windowProps.icon = path.join(__dirname, '..', '..', 'res', 'app.png') } + if (immutableWindowState.getIn(['appData', 'state']) === 'fullscreen') { + windowProps.fullscreen = true + } + const homepageSetting = getSetting(settings.HOMEPAGE) const startupSetting = getSetting(settings.STARTUP_MODE) const toolbarUserInterfaceScale = getSetting(settings.TOOLBAR_UI_SCALE) @@ -233,14 +260,10 @@ const createWindow = (action) => { frames = Immutable.fromJS([{}]) } - if (immutableWindowState.getIn(['ui', 'isMaximized'])) { + if (isMaximized) { mainWindow.maximize() } - if (immutableWindowState.getIn(['ui', 'isFullScreen'])) { - mainWindow.setFullScreen(true) - } - mainWindow.webContents.on('did-finish-load', (e) => { lastEmittedState = appState mainWindow.webContents.setZoomLevel(zoomLevel[toolbarUserInterfaceScale] || 0.0) diff --git a/js/stores/windowStore.js b/js/stores/windowStore.js index 7a9a7773952..979864e22e4 100644 --- a/js/stores/windowStore.js +++ b/js/stores/windowStore.js @@ -798,6 +798,9 @@ const doAction = (action) => { } break } + case windowConstants.WINDOW_ON_WINDOW_UPDATE: + windowState = windowState.setIn(['appData'], action.windowValue) + break default: break } diff --git a/test/misc-components/windowTest.js b/test/misc-components/windowTest.js index 5b31feeac93..04cdda99247 100644 --- a/test/misc-components/windowTest.js +++ b/test/misc-components/windowTest.js @@ -75,20 +75,31 @@ describe('application window', function () { describe('after resize', function () { Brave.beforeAll(this) + const width = 600 + const height = 700 before(function * () { yield this.app.client .waitForUrl(Brave.newTabUrl) .waitForBrowserWindow() .unmaximize() - .resizeWindow(600, 700) + .resizeWindow(width, height) .waitUntil(function () { return this.getAppState().then((val) => { return val && val.value && val.value.defaultWindowParams && - val.value.defaultWindowParams.width === 600 && - val.value.defaultWindowParams.height === 700 + val.value.defaultWindowParams.width === width && + val.value.defaultWindowParams.height === height + }) + }) + .waitUntil(function () { + return this.getWindowState().then((val) => { + return val && + val.value && + val.value.appData && + val.value.appData.width === width && + val.value.appData.height === height }) }) .newWindowAction() @@ -111,8 +122,31 @@ describe('application window', function () { it('has the width and height of the last window resize', function * () { yield this.app.client - .windowByIndex(1).browserWindow.getBounds().should.eventually.have.property('width').should.become(600) - .windowByIndex(1).browserWindow.getBounds().should.eventually.have.property('height').should.become(700) + .windowByIndex(1).browserWindow.getBounds().should.eventually.have.property('width').should.become(width) + .windowByIndex(1).browserWindow.getBounds().should.eventually.have.property('height').should.become(height) + }) + + it('check if new window has the same state data', function * () { + yield this.app.client + .waitForBrowserWindow() + .waitUntil(function () { + return this.getAppState().then((val) => { + return val && + val.value && + val.value.defaultWindowParams && + val.value.defaultWindowParams.width === width && + val.value.defaultWindowParams.height === height + }) + }) + .waitUntil(function () { + return this.getWindowState().then((val) => { + return val && + val.value && + val.value.appData && + val.value.appData.width === width && + val.value.appData.height === height + }) + }) }) }) diff --git a/test/unit/app/browser/reducers/tabsReducerTest.js b/test/unit/app/browser/reducers/tabsReducerTest.js index e42432ea37b..667478a9f3a 100644 --- a/test/unit/app/browser/reducers/tabsReducerTest.js +++ b/test/unit/app/browser/reducers/tabsReducerTest.js @@ -715,6 +715,7 @@ describe('tabsReducer unit tests', function () { }) // Passes browser options for position by mouse cursor assert.deepEqual(args[3], { + checkMaximized: true, positionByMouseCursor: true }) // Dropping on window ID is -1