Skip to content

Commit

Permalink
Save window appState into windowState when closing a window
Browse files Browse the repository at this point in the history
Resolves brave#8600
Resolves brave#9709
Resolves brave#3754
Resolves brave#6602

Auditors:

Test Plan:
  • Loading branch information
NejcZdovc committed Aug 14, 2017
1 parent 36d9141 commit deffe88
Show file tree
Hide file tree
Showing 15 changed files with 215 additions and 71 deletions.
2 changes: 1 addition & 1 deletion app/browser/reducers/tabsReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'))
Expand Down
16 changes: 7 additions & 9 deletions app/browser/reducers/windowsReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
23 changes: 9 additions & 14 deletions app/browser/windows.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {}
Expand Down Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -189,6 +191,7 @@ const api = {
LocalShortcuts.register(win)

appActions.windowCreated(windowValue)
windowActions.onWindowUpdate(windowId, windowValue)
})
win.once('closed', () => {
cleanupWindow(windowId)
Expand All @@ -198,7 +201,7 @@ const api = {
updateWindowDebounce(windowId)
})
win.on('focus', () => {
updateWindowDebounce(windowId)
updateWindowDebounce(windowId, true)
})
win.on('show', () => {
updateWindowDebounce(windowId)
Expand All @@ -207,7 +210,7 @@ const api = {
updateWindowDebounce(windowId)
})
win.on('maximize', () => {
updateWindowDebounce(windowId)
updateWindowDebounce(windowId, true)
})
win.on('unmaximize', () => {
updateWindowDebounce(windowId)
Expand All @@ -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)
Expand Down
1 change: 0 additions & 1 deletion app/renderer/components/main/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 3 additions & 15 deletions js/actions/appActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,11 @@ const appActions = {
})
},

windowUpdated: function (windowValue) {
windowUpdated: function (windowValue, updateDefault) {
dispatch({
actionType: appConstants.APP_WINDOW_UPDATED,
windowValue
windowValue,
updateDefault
})
},

Expand Down Expand Up @@ -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
Expand Down
10 changes: 10 additions & 0 deletions js/actions/windowActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -1203,6 +1203,16 @@ const windowActions = {
bookmarkKey,
type
})
},

onWindowUpdate: function (windowId, windowValue) {
dispatch({
actionType: windowConstants.WINDOW_ON_WINDOW_UPDATE,
queryInfo: {
windowId
},
windowValue
})
}
}

Expand Down
1 change: 0 additions & 1 deletion js/constants/appConstants.js
Original file line number Diff line number Diff line change
Expand Up @@ -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: _,
Expand Down
3 changes: 2 additions & 1 deletion js/constants/windowConstants.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
2 changes: 1 addition & 1 deletion js/contextMenus.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
})
Expand Down
2 changes: 1 addition & 1 deletion js/entry.js
Original file line number Diff line number Diff line change
Expand Up @@ -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())
})

Expand Down
67 changes: 45 additions & 22 deletions js/stores/appStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
3 changes: 3 additions & 0 deletions js/stores/windowStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -798,6 +798,9 @@ const doAction = (action) => {
}
break
}
case windowConstants.WINDOW_ON_WINDOW_UPDATE:
windowState = windowState.setIn(['appData'], action.windowValue)
break
default:
break
}
Expand Down
44 changes: 39 additions & 5 deletions test/misc-components/windowTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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
})
})
})
})

Expand Down
Loading

0 comments on commit deffe88

Please sign in to comment.