Hide windows until they are either rendered or a timeout expires #11764
Conversation
Codecov Report
@@ 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
|
|
@petemill did you maybe define wrong fix issue number? |
|
Yeah @NejcZdovc thanks, edited. Will edit commit message before merge. |
426a5c3 to
ad50233
Compare
| @@ -35,6 +35,8 @@ const appDispatcher = require('../../../js/dispatcher/appDispatcher') | |||
| const isDarwin = platformUtil.isDarwin() | |||
| const isWindows = platformUtil.isWindows() | |||
|
|
|||
| const TIMEOUT_WINDOW_SHOW_MS = 5000 | |||
There was a problem hiding this comment.
I think this is fine here, but for future reference you may consider js/constants/config.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()) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 😄
a65094f to
172ea85
Compare
| windowsReducer(state, action) | ||
| fakeTimers.tick(0) | ||
| // ensure the window api was asked to create the frame | ||
| const actualCreateWindowFrameArg = spy.args[0][3] |
There was a problem hiding this comment.
this is an interesting way to check the params- any reason you'd do this over something like assert(spy.withArgs(args_here).calledOnce)?
There was a problem hiding this comment.
@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!
There was a problem hiding this comment.
@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.
| break | ||
| case appConstants.APP_WINDOW_READY: | ||
| windows.windowReady(action.get('windowId')) | ||
| break | ||
| case appConstants.APP_WINDOW_RENDERED: |
There was a problem hiding this comment.
Great having an event for this! 😄
| @@ -111,6 +120,26 @@ const updatePinnedTabs = (win) => { | |||
| } | |||
| } | |||
|
|
|||
| function showDeferredShowWindow (win) { | |||
There was a problem hiding this comment.
much better encapsulation this go-around 😄 👍
172ea85 to
6634d64
Compare
|
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 |
|
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 |
| const windowInfoState = immutableWindowState.get('windowInfo') | ||
| if (windowInfoState) { | ||
| browserOpts.width = firstDefinedValue(browserOpts.width, windowInfoState.get('width')) | ||
| browserOpts.height = firstDefinedValue(browserOpts.height, windowInfoState.get('windowInfo')) |
There was a problem hiding this comment.
Here's the bug causing the height issue 🐛 👀 !!
should be:
browserOpts.height = firstDefinedValue(browserOpts.height, windowInfoState.get('height'))
There was a problem hiding this comment.
oh good spot @bsclifton, I remember deleting that code 🤦♂️
eb6ecba to
c30b3e2
Compare
app/browser/windows.js
Outdated
| // passed to BrowserWindow constructor | ||
| } | ||
| // let store know there's a new window | ||
| // so it can subscrive to state updates |
test/unit/lib/fakeWindow.js
Outdated
| getId () { | ||
| return this.id | ||
| // cannot be a class since sinon has | ||
| // trouble stubbing the constructtor for a class |
test/unit/lib/fakeElectron.js
Outdated
| } | ||
| } | ||
|
|
||
| // function getFakeDisplay |
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
…g w/ height Auditors: @petemill Test Plan: `npm run unittest -- --grep="APP_NEW_WINDOW"`
c30b3e2 to
ca54c13
Compare
Hide windows until they are either rendered or a timeout expires
Hide windows until they are either rendered or a timeout expires
Hide windows until they are either rendered or a timeout expires
|
@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):
|
|
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 😄 |
Fix #8128
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:
One complication is with macOS fullscreen windows for a couple reasons.
showproperty 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)Thoughts for the future:
Testing
Automated
npm run unittest -- --grep "window API unit tests"npm run unittest -- --grep "windowsReducer"Manual
throw new Error()inentry.js, it should open after 5 secondsSubmitter Checklist:
git rebase -ito squash commits (if needed).Reviewer Checklist:
Tests