Hide windows until they are either rendered or a timeout expires #11764

petemill · 2017-11-02T09:41:32Z

Waits for React to do its initial render of components, before firing an event to the browser process, which will show the window.

If the event is not received within 5 seconds (hard-coded, but in config.js), the window is shown (and will probably be a white screen).

Also:

Separates parsing of APP_NEW_WINDOW action properties, and window creation / visibility logic.
Moves window creation logic to windows API.
Creates tests for existing APP_NEW_WINDOW action handler logic in windowsReducer
Creates tests for new windows.createWindow logic
Fixes test for downloadsReducer with not restoring a stub required by windowsReducer test

One complication is with macOS fullscreen windows for a couple reasons.

macOS (or muon) will ignore the show property and open the window immediately if the window is fullscreen. This code counteracts that by removing the fullscreen property and then setting it once the window is ready (or the timeout happens)
When starting the app with many windows in the state to be opened, some of which are fullscreen and some of which are non-fullscreen on the same displays as a fullscreen window(s), the non fullscreen windows will open in the fullscreen space, which can be annoying for a user. I've gone between just having those windows show immediately and some more complex logic than what I have right now which attempted to open all non-fullscreen windows before opening fullscreen windows. The complex logic did not work because at that point, macOS would not put more than 1 window fullscreen per display even if the state was instructing it to. master currently supports that, and I didn't want to break it so right now is a balance (using a setTimeout of 100ms for fullscreen windows) between being able to open many fullscreen windows on app startup and preventing combined window spaces in most circumstances.

Thoughts for the future:

Set a default (non-react) html content which displays that there has been an error, and provides tips for how to proceed.
Remove the white background and put a gray one in (BrowserWindow has a background-color property)
If fading in the windows is desired on some platforms where it is normal, we may need to bring back muon's transparency property in BrowserWindow, or use window handles.

Testing

Automated

npm run unittest -- --grep "window API unit tests"
npm run unittest -- --grep "windowsReducer"

Manual

Make sure windows open at app startup
Make sure windows loaded from state at app startup remember their locations and fullscreen status
macOS / linux: when in a fullscreen window, ensure new windows opened are also fullscreen
Try to have a window that errors (put throw new Error() in entry.js, it should open after 5 seconds
Ideally if you have a profile that takes more than 5 seconds to open a window, you could observe that the window opens in a not-so-ready state after 5 seconds, but I have not been able to produce such a state.

Submitter Checklist:

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).
Tagged reviewers and labelled the pull request as needed.

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

codecov-io · 2017-11-02T10:02:44Z

Codecov Report

Merging #11764 into master will increase coverage by 0.33%.
The diff coverage is 70.46%.

@@            Coverage Diff             @@
##           master   #11764      +/-   ##
==========================================
+ Coverage   53.83%   54.17%   +0.33%     
==========================================
  Files         275      275              
  Lines       26058    26140      +82     
  Branches     4185     4192       +7     
==========================================
+ Hits        14029    14162     +133     
+ Misses      12029    11978      -51

Flag	Coverage Δ
#unittest	`54.17% <70.46%> (+0.33%)`	⬆️

Impacted Files	Coverage Δ
js/constants/config.js	`55.55% <ø> (ø)`	⬆️
js/constants/appConstants.js	`100% <ø> (ø)`	⬆️
js/actions/appActions.js	`17.88% <0%> (-1.29%)`	⬇️
app/browser/windows.js	`35.43% <64.07%> (+13.52%)`	⬆️
app/browser/reducers/windowsReducer.js	`80.53% <88.63%> (+59.18%)`	⬆️
app/browser/api/ledger.js	`44.3% <0%> (-4.76%)`	⬇️
app/browser/api/ledgerNotifications.js	`59.41% <0%> (-2.95%)`	⬇️
app/sessionStoreShutdown.js	`86.7% <0%> (+0.63%)`	⬆️
... and 4 more

NejcZdovc · 2017-11-02T10:09:56Z

@petemill did you maybe define wrong fix issue number?

petemill · 2017-11-02T10:25:47Z

Yeah @NejcZdovc thanks, edited. Will edit commit message before merge.

bsclifton · 2017-11-20T06:36:46Z

app/browser/reducers/windowsReducer.js

@@ -35,6 +35,8 @@ const appDispatcher = require('../../../js/dispatcher/appDispatcher')
 const isDarwin = platformUtil.isDarwin()
 const isWindows = platformUtil.isWindows()

+const TIMEOUT_WINDOW_SHOW_MS = 5000


I think this is fine here, but for future reference you may consider js/constants/config.js 😄

moved to config.js

bsclifton · 2017-11-20T06:45:54Z

app/browser/reducers/windowsReducer.js

+    // fullscreen once it is ready to be shown
+    // (browserOpts.fullscreen may already be set when loading from saved state,
+    // so this just sets it for other scenarios)
+    if (isDarwin && parentWindow && parentWindow.isFullScreen()) {


this logic (all of what is in this setImmediate) might be better to pull out to a discrete function. This would help with unit testing and could help with readability. Something like getCreateWindowArgs (passing what's needed). Then whatever is returned is directly passed to the call new BrowserWindow

Unit tests would then be a great way to capture the behavior (of setting fullscreen to false) and the tests themselves would act as documentation (if someone makes a change which breaks the test). Could we get some unit tests for this?

I have split all the window creation, showing, deferred showing, and timeout showing logic to a different place - app/browser/windows.js which seemed an appropriate place which deals with calling functions on a muon BrowserWindow (and children). It's also the same place that the whenRendered action is handled, so that nicely puts all the logic in one place, including the temporary but ugly win.__blah properties - so they are at least encapsulated in one module.

I created tests for this new logic in windowsTest.js handling:

showing the window immediately

showing the window if the timout expires

showing the window on whenRendered firing for the window

ensuring the window is fullscreen or maximized on each of those scenarios

I also created tests for most of the existing action handler logic for APP_NEW_WINDOW in windowReducerTest.js, of which there were no existing tests, including tests for:

creating the right tabs in the new window

window position and size

maximizing the window

bsclifton

Manually tested on Windows and macOS- this looks noticeably better 😄 I think it would be important to capture some tests for windowReducer because 1) this is important functionality and also 2) because of the complexity of the comments here

I can definitely help with tests- let me know 😄

bsclifton · 2017-11-21T23:59:39Z

test/unit/app/browser/reducers/windowsReducerTest.js

+      windowsReducer(state, action)
+      fakeTimers.tick(0)
+      // ensure the window api was asked to create the frame
+      const actualCreateWindowFrameArg = spy.args[0][3]


this is an interesting way to check the params- any reason you'd do this over something like assert(spy.withArgs(args_here).calledOnce)?

@bsclifton In this case, sure that more expressive function could be used. I was just being consistent with the other tests here, which need to inspect the windowOptions object argument for property values, and not necessarily enforce every property on the object argument. If there's a nicer way to do that vs being tied to the argument order I'd love to learn!

@bsclifton I updated the couple of tests I could using that syntax. It looks nicer, thanks. Only downside is the error messages aren't as expressive as using the chai.assert functions.

bsclifton · 2017-11-22T00:04:28Z

app/browser/reducers/windowsReducer.js

      break
    case appConstants.APP_WINDOW_READY:
      windows.windowReady(action.get('windowId'))
      break
+    case appConstants.APP_WINDOW_RENDERED:


Great having an event for this! 😄

bsclifton · 2017-11-22T00:05:29Z

app/browser/windows.js

@@ -111,6 +120,26 @@ const updatePinnedTabs = (win) => {
  }
 }

+function showDeferredShowWindow (win) {


much better encapsulation this go-around 😄 👍

petemill · 2017-11-22T00:56:35Z

Just noticed that multiple homepage tabs, as specified in https://community.brave.com/t/how-to-set-up-multiple-home-pages/9998 is not working in master (or this branch). I'll see if it's an easy fix we can bundle in this, or create the issue
edit: won't fix in this PR as not sure if intentional. Logged in #12057

bsclifton · 2017-11-22T05:55:51Z

Found a problem- I grabbed latest (saw there was a rebase / force push after I last looked at code) and tried it out. The sizes are not being saved for the windows ☹️ Here's a clip: basically the height for the window on the right is not saved

bsclifton · 2017-11-22T06:05:30Z

app/browser/reducers/windowsReducer.js

+  const windowInfoState = immutableWindowState.get('windowInfo')
+  if (windowInfoState) {
+    browserOpts.width = firstDefinedValue(browserOpts.width, windowInfoState.get('width'))
+    browserOpts.height = firstDefinedValue(browserOpts.height, windowInfoState.get('windowInfo'))


Here's the bug causing the height issue 🐛 👀 !!
should be:
browserOpts.height = firstDefinedValue(browserOpts.height, windowInfoState.get('height'))

oh good spot @bsclifton, I remember deleting that code 🤦‍♂️

bsclifton · 2017-11-22T07:14:51Z

app/browser/windows.js

+      // passed to BrowserWindow constructor
+    }
+    // let store know there's a new window
+    // so it can subscrive to state updates


s/subscrive/subscribe

bsclifton · 2017-11-22T07:16:01Z

test/unit/lib/fakeWindow.js

-  getId () {
-    return this.id
+// cannot be a class since sinon has
+// trouble stubbing the constructtor for a class


s/constructtor/constructor

bsclifton · 2017-11-22T07:16:22Z

test/unit/lib/fakeElectron.js

  }
 }

+// function getFakeDisplay


can this be removed?

bsclifton

Beautiful job with this! The functionality works as expected, code looks great - the refactoring is nice and lends itself well to unit testing. Huge thanks for putting the extra effort in to write all those tests ❤️

Separates parsing of APP_NEW_WINDOW action properties, and window creation / visibility logic. Moves window creation logic to windows API. Creates tests for existing APP_NEW_WINDOW action handler logic in windowsReducer Creates tests for new windows.createWindow logic Fixes test for downloadsReducer with not restoring a stub required by windowsReducer test Fix #8128

@petemill

…g w/ height Auditors: @petemill Test Plan: `npm run unittest -- --grep="APP_NEW_WINDOW"`

Hide windows until they are either rendered or a timeout expires

bsclifton · 2017-11-22T17:11:31Z

master 01e7624
0.21.x 57c20b6
0.20.x 0b27319
~~0.19.x need some help merging this one, @petemill 😄 (if possible- if too difficult, we can push back to 0.20.x)~~

Hide windows until they are either rendered or a timeout expires

petemill · 2017-11-22T18:15:38Z

@bsclifton I've got a successful merge on 0.19.x (pushed to origin/0.19.x-hide-white-windows) - complication was that all the window creation logic had been moved and edited from appStore.js to windowReducer.js previous to this in #9963 and #10458, both of which were only merged to 0.20.x. This could be a risk, but we've actually introduced more unit tests here, so up to you. If we do keep the 0.19.x merge, I would say incorporate the test plan from those PRs, but they both don't have one, so this actually would merge those fixes and create a test plan for them 🤷‍♂️ 😄

I had to introduce all windowsReducerTests and windowsTests - and all the ones we created pass. However, there were two that were created before this PR that do not pass (and I removed them):

windowsReducerTest.js/APP_WINDOW_UPDATED/updateDefault is true is not passing because saving window dimensions is not working very well in 0.19.x. On the current 0.19.x, your screencap above doesn't work, i.e. the app only remembers one of the window positions, and layers all windows at that position, on top of eachother. This PR does not fix that, since it already had been fixed in master (in Fixes window restore and fullscreen #10458). I removed that test.
windowsTest.js/updatePinnedTabs these tests were added as part of the Stop losing pinned tabs in a window when adding or removing another pinned tab #10531 fix, that is not in 0.19.x, so I removed them.

bsclifton · 2017-11-22T19:00:31Z

Based on the above (and in the interest of reducing complexity), let's push this to 0.20.x 👍

Thanks for taking the time to evaluate the merge 😄

petemill added the product/user-happiness label Nov 2, 2017

petemill added this to the Triage Backlog milestone Nov 2, 2017

petemill self-assigned this Nov 2, 2017

petemill requested review from bridiver, bbondy and NejcZdovc November 2, 2017 09:41

lucidNTR mentioned this pull request Nov 2, 2017

New windows are just white for a bit. #8128

Closed

petemill mentioned this pull request Nov 2, 2017

Tabs transitions / drag & drop / realtime tear-off #11720

Closed

32 tasks

bsclifton modified the milestones: Triage Backlog, 0.19.x Hotfix 6 (Release channel) Nov 7, 2017

luixxiul added the QA/test-plan-specified label Nov 13, 2017

bsclifton force-pushed the hide-white-windows branch from 426a5c3 to ad50233 Compare November 20, 2017 05:27

bsclifton self-requested a review November 20, 2017 05:35

bsclifton reviewed Nov 20, 2017

View reviewed changes

bsclifton suggested changes Nov 20, 2017

View reviewed changes

petemill force-pushed the hide-white-windows branch 3 times, most recently from a65094f to 172ea85 Compare November 21, 2017 21:46

bsclifton reviewed Nov 22, 2017

View reviewed changes

petemill force-pushed the hide-white-windows branch from 172ea85 to 6634d64 Compare November 22, 2017 00:49

petemill mentioned this pull request Nov 22, 2017

Multiple home pages not working for additional new windows #12057

Open

bsclifton reviewed Nov 22, 2017

View reviewed changes

bsclifton force-pushed the hide-white-windows branch from eb6ecba to c30b3e2 Compare November 22, 2017 07:08

bsclifton reviewed Nov 22, 2017

View reviewed changes

test/unit/lib/fakeElectron.js Outdated

}

}

// function getFakeDisplay

Copy link

Member

bsclifton Nov 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be removed?

bsclifton approved these changes Nov 22, 2017

View reviewed changes

petemill and others added 2 commits November 22, 2017 09:04

Updated unit tests for argument parsing for window creation; fixed bu…

ca54c13

…g w/ height Auditors: @petemill Test Plan: `npm run unittest -- --grep="APP_NEW_WINDOW"`

petemill force-pushed the hide-white-windows branch from c30b3e2 to ca54c13 Compare November 22, 2017 17:04

bsclifton merged commit 01e7624 into master Nov 22, 2017

bsclifton deleted the hide-white-windows branch November 22, 2017 17:09

bsclifton added a commit that referenced this pull request Nov 22, 2017

Merge pull request #11764 from brave/hide-white-windows

57c20b6

Hide windows until they are either rendered or a timeout expires

bsclifton added a commit that referenced this pull request Nov 22, 2017

Merge pull request #11764 from brave/hide-white-windows

0b27319

Hide windows until they are either rendered or a timeout expires

petemill pushed a commit that referenced this pull request Nov 22, 2017

Merge pull request #11764 from brave/hide-white-windows

5f67f44

Hide windows until they are either rendered or a timeout expires

bsclifton modified the milestones: 0.19.x Hotfix 6 (Release channel), 0.20.x (Beta Channel) Nov 22, 2017

petemill mentioned this pull request Nov 24, 2017

Correct a mockery path that was preventing forcing the darwin platform in windowsTest #12091

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide windows until they are either rendered or a timeout expires #11764

Hide windows until they are either rendered or a timeout expires #11764

petemill commented Nov 2, 2017 •

edited by bsclifton

Loading

codecov-io commented Nov 2, 2017 •

edited

Loading

NejcZdovc commented Nov 2, 2017

petemill commented Nov 2, 2017

bsclifton Nov 20, 2017

petemill Nov 21, 2017

bsclifton Nov 20, 2017

bsclifton Nov 20, 2017

petemill Nov 21, 2017

bsclifton left a comment

bsclifton Nov 21, 2017

petemill Nov 22, 2017

petemill Nov 22, 2017

bsclifton Nov 22, 2017

bsclifton Nov 22, 2017

petemill commented Nov 22, 2017 •

edited

Loading

bsclifton commented Nov 22, 2017

bsclifton Nov 22, 2017 •

edited

Loading

petemill Nov 22, 2017

bsclifton Nov 22, 2017

bsclifton Nov 22, 2017

bsclifton Nov 22, 2017

bsclifton left a comment

bsclifton commented Nov 22, 2017 •

edited

Loading

petemill commented Nov 22, 2017

bsclifton commented Nov 22, 2017

Hide windows until they are either rendered or a timeout expires #11764

Hide windows until they are either rendered or a timeout expires #11764

Conversation

petemill commented Nov 2, 2017 • edited by bsclifton Loading

Testing

Automated

Manual

codecov-io commented Nov 2, 2017 • edited Loading

Codecov Report

NejcZdovc commented Nov 2, 2017

petemill commented Nov 2, 2017

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 left a 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

petemill commented Nov 22, 2017 • edited Loading

bsclifton commented Nov 22, 2017

bsclifton Nov 22, 2017 • 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

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton commented Nov 22, 2017 • edited Loading

petemill commented Nov 22, 2017

bsclifton commented Nov 22, 2017

petemill commented Nov 2, 2017 •

edited by bsclifton

Loading

codecov-io commented Nov 2, 2017 •

edited

Loading

petemill commented Nov 22, 2017 •

edited

Loading

bsclifton Nov 22, 2017 •

edited

Loading

bsclifton commented Nov 22, 2017 •

edited

Loading