Save window appState into windowState when closing a window

Resolves brave#8600 Resolves brave#9709 Resolves brave#3754 Resolves brave#6602 Auditors: Test Plan:
NejcZdovc · Aug 14, 2017 · deffe88 · deffe88
1 parent 36d9141
commit deffe88
Show file tree

Hide file tree

Showing 15 changed files with 215 additions and 71 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
+            })
+          })
       })
     })