Default window position now recorded and used for new windows #5102

bsclifton · 2016-10-24T23:37:30Z

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).

Default window position now recorded and used for new windows

renamed setDefaultWindowSize => defaultWindowParamsChanged
created new state (code will backfill value by trying to read the new value and if null, then try the old value)
defaultWindowParamsChanged now saves x/y and uses when creating window
consolidated duplicate BrowserWindow event handlers from appStore.js in main.js
window resize handler now has a sliding 1 second timer to avoid spamming events (causing extreme slowness)

Fixes #3754
Fixes #3247
Fixes #586

Auditors: @bridiver, @bbondy
cc: @BrendanEich (I know this one has got you a few times- should be g2g after this!)

Test Plan:

for #3754

Launch Brave and go to Preferences
Set "Brave starts with" to "My homepage" and specify duckduckgo.com
Move window somewhere specific and resize it too
Close the browser
Launch browser and notice window size and position is correct (even though no window state was saved)

for #3247
6. Launch Brave on macOS
8. Only have one window open (close others; verify via the Window menu)
9. Move window somewhere specific and resize it to a weird (but memorable) size
10. Close the only window
11. Pick "New Window" from the file menu
12. Notice window goes to position of last window and assumes size of last window

for #586
13. Go to slashdot.org
14. Resize the window horizontally as fast as you possibly can!
15. Notice that while it could be better, it's MUCH faster than it is now
16. Move the window somewhere specific

Upgrade test plan:

Launch Brave (version 0.12.6) and go to Preferences
Set "Brave starts with" to "My homepage" and specify duckduckgo.com
Resize the window to a very specific size
Close the browser
Install Brave version 0.12.7
Launch Brave and notice that the window has the same dimensions as before (remember, position was not saved prior to this fix).

bridiver · 2016-10-25T16:40:21Z

js/components/main.js

+        const size = event.sender.getSize()
+        // NOTE: the default window size is whatever the last window resize was
+        appActions.defaultWindowParamsChanged(size)
+        windowActions.saveSize(size)


do we need this second action? Can defaultWindowParamsChanged also save them?

if not it's ok, just wanted to check

I think with the updated design that we'll move to (where app state has control over window state), it would be a no brainer as one action.

With our system now, does the WindowStore also receive AppStore events? (if so, would it be possible for us to add the window id in there and then fetch the session object, based on that?)

I'm actually not sure if the window store receives app events, but if you need the window or session those are available in the browser process. DM me for details

bridiver · 2016-10-25T16:41:50Z

js/components/main.js

      windowActions.onFocusChanged(false)
    })
-
+    // NOTE: setTimeout is used to avoid spamming events
    let moveTimeout = null
    currentWindow.on('move', function (event) {
      if (moveTimeout) {
        clearTimeout(moveTimeout)
      }
      moveTimeout = setTimeout(function () {


won't this still call the method just as many times, but delayed by 1 second? Can we use debounce here instead?

bridiver · 2016-10-25T16:44:02Z

js/components/main.js

+      resizeTimeout = setTimeout(function () {
+        const size = event.sender.getSize()
+        // NOTE: the default window size is whatever the last window resize was
+        appActions.defaultWindowParamsChanged(size)


be careful with these inside events. This is ok because it's wrapped in a setTimeout (see below for possible change to debounce which would also be ok). Otherwise they should be wrapped in setImmediate. No change needed for that, just a reminder.

bridiver · 2016-10-25T16:44:48Z

js/actions/appActions.js

   */
-  setDefaultWindowSize: function (size) {
+  defaultWindowParamsChanged: function (size, position) {


++ on declarative event naming

bridiver · 2016-10-25T16:46:35Z

js/components/main.js

      windowActions.onFocusChanged(false)
    })
-
+    // NOTE: setTimeout is used to avoid spamming events
    let moveTimeout = null
    currentWindow.on('move', function (event) {
      if (moveTimeout) {


re: my comment below, I didn't see this at first, but I still think debounce would be better. Can move and resize call a common method?

that would work well with debounce because you can can do method = method.debounce(...) or similar

@bridiver

- renamed setDefaultWindowSize => defaultWindowParamsChanged - created new state (code will backfill value by trying to read the new value and if null, then try the old value) - defaultWindowParamsChanged now saves x/y and uses when creating window - consolidated duplicate BrowserWindow event handlers from `appStore.js` in `main.js` - window resize handler now has a sliding 1 second timer to avoid spamming events (causing extreme slowness) (NOTE: this is a squashed rebase and includes the debounce implementation, per feedback by @bridiver) Fixes #3754 Fixes #3247 Fixes #586 Auditors: @bridiver, @bbondy cc: @BrendanEich (I know this one has got you a few times- should be g2g after this!) Test Plan: for #3754 1. Launch Brave and go to Preferences 2. Set "Brave starts with" to "My homepage" and specify duckduckgo.com 3. Move window somewhere specific and resize it too 4. Close the browser 5. Launch browser and notice window size and position is correct (even though no window state was saved) for #3247 6. Launch Brave on macOS 8. Only have one window open (close others; verify via the Window menu) 9. Move window somewhere specific and resize it to a weird (but memorable) size 10. Close the only window 11. Pick "New Window" from the file menu 12. Notice window goes to position of last window and assumes size of last window for #586 13. Go to slashdot.org 14. Resize the window horizontally as fast as you possibly can! 15. Notice that while it could be better, it's MUCH faster than it is now 16. Move the window somewhere specific Upgrade test plan: 1. Launch Brave (version 0.12.6) and go to Preferences 2. Set "Brave starts with" to "My homepage" and specify duckduckgo.com 3. Resize the window to a very specific size 4. Close the browser 5. Install Brave version 0.12.7 6. Lauch Brave and notice that the window has the same dimensions as before (remember, position was not saved prior to this fix).

bsclifton · 2016-10-25T18:53:48Z

implemented debounce feedback and rebased; issue to track the cleanup for the window methods can be found here #5127 😄

bbondy · 2016-10-26T01:19:06Z

thanks, merging!

srirambv · 2016-10-26T14:54:15Z

Test Plan: for #3754

Launch Brave and go to Preferences

Set "Brave starts with" to "My homepage" and specify duckduckgo.com

Move window somewhere specific and resize it too

Close the browser

Launch browser and notice window size and position is correct (even though no window state was saved)

Only remembers the size but not the window position.

luixxiul · 2016-10-26T17:03:27Z

@srirambv because this was not merged in 0.12.7 Preview1

bsclifton added the bug label Oct 24, 2016

bsclifton added this to the 0.12.7dev milestone Oct 24, 2016

bsclifton assigned bridiver and bbondy Oct 24, 2016

bsclifton mentioned this pull request Oct 25, 2016

content doesn't fill fast enough when you resize a browser window horizontally #586

Closed

bridiver reviewed Oct 25, 2016

View reviewed changes

bbondy merged commit 1188a53 into brave:master Oct 26, 2016

This was referenced Oct 26, 2016

Manual tests for OS X 0.12.7 Preview 1 #5154

Closed

Manual tests for Windows x64 0.12.7 Preview 1 #5155

Closed

Manual tests for Ubuntu 0.12.7 Preview 1 #5156

Closed

luixxiul mentioned this pull request Oct 26, 2016

Manual tests for Windows ia-32 0.12.7 Preview 1 #5162

Closed

bsclifton deleted the fix-position-homepage branch October 26, 2016 17:52

This was referenced Oct 27, 2016

Manual tests for OS X 0.12.7 RC1 #5184

Closed

Manual tests for Ubuntu 0.12.7 RC 1 #5185

Closed

Manual tests for Windows x64 0.12.7 RC1 #5186

Closed

Manual tests for Windows ia-32 0.12.7 RC1 #5187

Closed

srirambv added the QA/checked-Win64 label Oct 27, 2016

luixxiul added the QA/checked-Win32 label Oct 27, 2016

alexwykoff added the QA/checked-macOS label Oct 28, 2016

This was referenced Oct 28, 2016

Manual tests for OS X 0.12.7 RC 2 #5215

Closed

Manual tests for Ubuntu 0.12.7 RC 2 #5216

Closed

This was referenced Oct 28, 2016

Manual tests for Windows ia-32 0.12.7 RC 2 #5217

Closed

Manual tests for Windows x64 0.12.7 RC 2 #5218

Closed

This was referenced Nov 8, 2016

Maximize / Fullscreen status is not saved #3138

Closed

window size not remembered after restart #5267

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default window position now recorded and used for new windows #5102

Default window position now recorded and used for new windows #5102

bsclifton commented Oct 24, 2016 •

edited

Loading

bridiver Oct 25, 2016

bridiver Oct 25, 2016

bsclifton Oct 25, 2016 •

edited

Loading

bridiver Oct 25, 2016

bridiver Oct 25, 2016

bridiver Oct 25, 2016

bridiver Oct 25, 2016

bridiver Oct 25, 2016

bridiver Oct 25, 2016

bsclifton commented Oct 25, 2016

bbondy commented Oct 26, 2016

srirambv commented Oct 26, 2016

luixxiul commented Oct 26, 2016

Default window position now recorded and used for new windows #5102

Default window position now recorded and used for new windows #5102

Conversation

bsclifton commented Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton Oct 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton commented Oct 25, 2016

bbondy commented Oct 26, 2016

srirambv commented Oct 26, 2016

luixxiul commented Oct 26, 2016

bsclifton commented Oct 24, 2016 •

edited

Loading

bsclifton Oct 25, 2016 •

edited

Loading