From deffe88b7a69581bbaa18168d6d10b9a6e85766a 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/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 + .../browser/reducers/windowsReducerTest.js | 93 +++++++++++++++++++ 15 files changed, 215 insertions(+), 71 deletions(-) create mode 100644 test/unit/app/browser/reducers/windowsReducerTest.js 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/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 diff --git a/test/unit/app/browser/reducers/windowsReducerTest.js b/test/unit/app/browser/reducers/windowsReducerTest.js new file mode 100644 index 00000000000..20cd718dc61 --- /dev/null +++ b/test/unit/app/browser/reducers/windowsReducerTest.js @@ -0,0 +1,93 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +/* global describe, it, before, after, afterEach */ +const mockery = require('mockery') +const sinon = require('sinon') +const Immutable = require('immutable') +const assert = require('assert') +const fakeElectron = require('../../../lib/fakeElectron') +const fakeAdBlock = require('../../../lib/fakeAdBlock') + +const appConstants = require('../../../../../js/constants/appConstants') +require('../../../braveUnit') + +describe('windowsReducer unit test', function () { + let windowsReducer + + const fakeWindowState = { + maybeCreateWindow: (state, action) => state + } + + const state = Immutable.fromJS({ + windows: [], + defaultWindowParams: {} + }) + + before(function () { + mockery.enable({ + warnOnReplace: false, + warnOnUnregistered: false, + useCleanCache: true + }) + mockery.registerMock('electron', fakeElectron) + mockery.registerMock('ad-block', fakeAdBlock) + mockery.registerMock('../../common/state/windowState', fakeWindowState) + windowsReducer = require('../../../../../app/browser/reducers/windowsReducer') + }) + + after(function () { + mockery.disable() + }) + + describe('APP_WINDOW_UPDATED', function () { + let spy + + afterEach(function () { + spy.restore() + }) + + it('null case', function () { + spy = sinon.spy(fakeWindowState, 'maybeCreateWindow') + const newState = windowsReducer(state, { + actionType: appConstants.APP_WINDOW_UPDATED + }) + assert.equal(spy.calledOnce, true) + assert.deepEqual(state.getIn(['defaultWindowParams']).toJS(), newState.getIn(['defaultWindowParams']).toJS()) + }) + + it('updateDefault is false (we shouldnt update it)', function () { + spy = sinon.spy(fakeWindowState, 'maybeCreateWindow') + const newState = windowsReducer(state, { + actionType: appConstants.APP_WINDOW_UPDATED, + updateDefault: false + }) + assert.equal(spy.calledOnce, true) + assert.deepEqual(state.getIn(['defaultWindowParams']).toJS(), newState.getIn(['defaultWindowParams']).toJS()) + }) + + it('updateDefault is true', function () { + spy = sinon.spy(fakeWindowState, 'maybeCreateWindow') + const newState = windowsReducer(state, { + actionType: appConstants.APP_WINDOW_UPDATED, + updateDefault: true, + windowValue: { + width: 1, + height: 2, + x: 3, + y: 4, + state: 'maximized' + } + }) + assert.equal(spy.calledOnce, true) + assert.deepEqual({ + width: 1, + height: 2, + x: 3, + y: 4, + maximized: true + }, newState.getIn(['defaultWindowParams']).toJS()) + }) + }) +})