Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WS-689] minimize white flashes #101

Merged
merged 3 commits into from
Aug 15, 2023
Merged

Conversation

szymonkaliski
Copy link
Member

Why

We got feedback that on Windows we can see a white box flash before the app loads.

The reason is that when we create a new window it defaults to being visible, and we set the size and other properties later on. This PR modifies it so new window starts invisible, and we flip it to visible after we go through the initialization process.

In my non-scientific tests on Windows VM this doesn't show a white box flash, but we still get a white window before we start loading the app. As far as I can tell we're doing everything correctly with backgroundColor set from the start, and this might be a Windows thing -- it's not only us and the Electron GitHub is full with issues like this one electron/electron#861

Another solution would be to keep the window hidden until we load the URL and go through the first render as described here https://www.electronjs.org/docs/latest/api/browser-window#using-the-ready-to-show-event but in my tests it takes a while and makes the app feel slow to load.

What changed

Made the window hidden on start, and show after we set up the size etc.

Test plan

Open the app on Windows, notice that we don't flash a white box before the Window opens properly.

@linear
Copy link

linear bot commented Aug 15, 2023

Comment on lines 120 to 123
// Bypass the browser's cache when initially loading the remote URL
// in order to ensure that we load the latest web build.
// See: https://github.com/electron/electron/issues/1360#issuecomment-156506130
window.loadURL(url, { extraHeaders: "pragma: no-cache\n" });
Copy link
Contributor

Choose a reason for hiding this comment

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

can test this later today but I don't think we should be loading the URL before we set the listeners below. feels like bad practice given that events can in theory come in at any time.

we should figure out what about this is the bottleneck here? is it the finding and setting bounds? if so, we should just move it before that because I doubt setting the listeners takes a noticeable amount of time

Copy link
Member Author

@szymonkaliski szymonkaliski Aug 15, 2023

Choose a reason for hiding this comment

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

ah good catch, yea let me move this after we set the events, one sec -> done ✅

we should figure out what about this is the bottleneck here? is it the finding and setting bounds? if so, we should just move it before that because I doubt setting the listeners takes a noticeable amount of time

not sure what you mean by bottleneck here:

  • if you're asking about using the ready-to-show event linked above, then the bottleneck is network connection + how fast repl-it-web loads -- we're basically waiting for the website to load before we show the window, which is perceivable and feels as if the app was sluggish
  • if you're asking why we still get white screen even though backgroundColor is set, then I guess it's platform (as in Windows) specific, but I couldn't find very clear answer anywhere really

Copy link
Contributor

@sergeichestakov sergeichestakov left a comment

Choose a reason for hiding this comment

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

awesome yeah I see the problem now. doesn't seem like the issue is / can be totally fixed but this is definitely better. thanks!

@szymonkaliski szymonkaliski merged commit dc863aa into main Aug 15, 2023
4 checks passed
@sergeichestakov sergeichestakov deleted the sk/minimize-white-flashes branch August 15, 2023 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants