-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Open initial electron window early and reuse it later #12897
Open initial electron window early and reuse it later #12897
Conversation
a456a9a
to
7c18dce
Compare
This is a great start, but it would also be great to fix the problem instead of showing a fake window. Also, how will this work when multiple windows have to be restored when the app starts? According to my profiling, a lot of time is spent with the backend starting and loading the plugin contributions and the languages, especially when there are many, and parsing the JSON files takes time. It would be great to move the plugin loading to a worker. This is what it takes currently: deployplugin.mp4My initial tweak to overcome it is to avoid |
@@ -242,6 +245,16 @@ export class ElectronMainApplication { | |||
return this.didUseNativeWindowFrameOnStart.get(webContents.id) ? 'native' : 'custom'; | |||
} | |||
|
|||
protected showInitialWindow(): void { | |||
if (!process.env.SKIP_INITIAL_WINDOW) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can one configure this when there is a final bundled app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the property to the electron frontend app config to make this configurable downstream.
theia/dev-packages/application-package/src/application-props.ts
Lines 37 to 45 in f038673
export interface Partial { | |
/** | |
* Override or add properties to the electron `windowOptions`. | |
* | |
* Defaults to `{}`. | |
*/ | |
readonly windowOptions?: BrowserWindowConstructorOptions; | |
} |
In my opinion it's always better to show something instead of showing nothing. So opening a window as early as is technically possible will always be a good solution as the user gets the earliest visual feedback possible. Obviously it would also be great to show meaningful content as fast as possible, so any performance gain there is certainly welcomed. However I don't see why we should not open the Electron window at the earliest possible point. It's not a "fake", it's just early without content. You can also compare this to VS Code where they also open the window at the earliest point and only gradually populate it with content. It certainly helps for the perception of being fast, even if the app is actually usable only a few seconds later. |
@kittaakos @sdirix I think we're talking about different things here: one is the perception of performance and the other is effective performance. Both contribute to user satisfaction. Things like putting up a splash screen do not make loading faster, but users still want to see something is happening. Ideally, we would show some loading progress on the "early window". |
I agree to all said above. @kittaakos It is worth mentioning that we are currently investigating the start up performance in general, this PR is just one piece. It seems you are working in the same area. I believe it would make sense to align a bit to share ideas and avoid duplicate work. Are you interested in this? If so, maybe you join the next dev call and maybe we can schedule a break out session there for interested parties? |
Sure. Thank you! I saved the date of the next dev call. |
For references, this is the epic to track all current streams of work around start-up performance improvements: #12924 |
@@ -242,6 +245,16 @@ export class ElectronMainApplication { | |||
return this.didUseNativeWindowFrameOnStart.get(webContents.id) ? 'native' : 'custom'; | |||
} | |||
|
|||
protected showInitialWindow(): void { | |||
if (!process.env.SKIP_INITIAL_WINDOW) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the property to the electron frontend app config to make this configurable downstream.
theia/dev-packages/application-package/src/application-props.ts
Lines 37 to 45 in f038673
export interface Partial { | |
/** | |
* Override or add properties to the electron `windowOptions`. | |
* | |
* Defaults to `{}`. | |
*/ | |
readonly windowOptions?: BrowserWindowConstructorOptions; | |
} |
@@ -303,6 +316,7 @@ export class ElectronMainApplication { | |||
return { | |||
show: false, | |||
title: this.config.applicationName, | |||
backgroundColor: nativeTheme.shouldUseDarkColors ? '#1E1E1E' : '#FFFFFF', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A similar dispatch is already at:
const value = window.localStorage.getItem(DEFAULT_BACKGROUND_COLOR_STORAGE_KEY) || (dark ? '#1E1E1E' : '#FFFFFF'); |
Could you single-source them somewhere here? Thanks
theia/dev-packages/application-package/src/application-props.ts
Lines 48 to 63 in f038673
export type DefaultTheme = string | Readonly<{ light: string, dark: string }>; | |
export namespace DefaultTheme { | |
export function defaultForOSTheme(theme: DefaultTheme): string { | |
if (typeof theme === 'string') { | |
return theme; | |
} | |
if ( | |
typeof window !== 'undefined' && | |
window.matchMedia && | |
window.matchMedia('(prefers-color-scheme: dark)').matches | |
) { | |
return theme.dark; | |
} | |
return theme.light; | |
} | |
} |
7c18dce
to
1d630a6
Compare
@kittaakos Thank you very much for the helpful review! I've incorporated the changes you suggested and also enhanced the change so that we store the user's background color in the electron store to match the theme color with the initial window's background color. Unfortunately, I had to rebase to make the changes you suggested. Sorry if that makes a re-review more cumbersome. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I verified 1d630a6, and it worked for me. Thank you!
Why is it required to set the window background?
@@ -30,6 +30,8 @@ import { WindowTitleService } from '../../browser/window/window-title-service'; | |||
|
|||
import '../../../src/electron-browser/menu/electron-menu-style.css'; | |||
import { MenuDto } from '../../electron-common/electron-api'; | |||
import { ThemeService } from '../../browser/theming'; | |||
import { ThemeChangeEvent } from 'src/common/theme'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import { ThemeChangeEvent } from 'src/common/theme'; | |
import { ThemeChangeEvent } from '../../common/theme'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nice catch, thank you!
1d630a6
to
bd18c66
Compare
Quite some time (500ms to 1000ms) is needed until the frontend actually starts rendering and during that time the window is just empty showing the default system background. Imho it looks faster and better to already show the empty window with the background color that matches the expected background color until the actual app is loaded. It also avoids unpleasant flickering. |
I understand why it is set in the electron main. Here: But how is opening and reusing the first window requires to set the background on theme change: Feel free to merge it though. I only want to understand what's the catch here. Thanks |
This is to match the background color if the user switched to a different theme. Whenever the user switches theme, we notify the Electron window and it stores the background color into the electron store, so it can be restored on next start correctly. Or am I misunderstanding the question? |
No, you're right. Thanks for the explanation |
@kittaakos Great, thanks! Can you please re-approve the rebase, if you agree to merge? Thank you! |
This avoids having a rather long pause from Theia start until something is visibly coming up. In my initial experiment, I observe the empty window showing up ~1.5s earlier than usual. This behavior can be controlled via the application config in the application's package.json. For instance, to turn off, use: ``` "theia": { "target": "electron", "frontend": { "config": { "applicationName": "Theia Electron Example", "electron": { "showWindowEarly": false } } } } ``` Also, we store the background color of the current color theme to be able to show the correct background color in the initial window. Contributed on behalf of STMicroelectronics. Change-Id: Ib8a6e8221c4f48605c865afc120d62adf6eef902
bd18c66
to
f4bf07e
Compare
What it does
This avoids having a rather long pause from Theia start until something is visibly coming up. In my initial experiment, I observe the empty window showing up ~1.5s earlier than usual.
This behavior can be controlled via the application config in the application's package.json. For instance, to turn off, use:
To mitigate flickering, we set a background color of the window, which shows before any page is loaded, according to the expected background color of the Theia app. Also, we store the background color of the current color theme to be able to show the correct background color in the initial window.
In future changes, we may consider loading a very simple static web page mimicking the basic workbench structure before we switch to the actual application and in the Theia app don't show a spinner but a loading indicator on top of the basic workbench structure. With that, the application rather builds up visually instead of flickering between nothing, the spinner and the actual app.
Contributed on behalf of STMicroelectronics.
Without this change
without.this.change.mp4
With this change
with.this.change.mp4
How to test
Build the example electron app with
"showWindowEarly": false
and with"showWindowEarly": true
to compare the differences.In addition, it is worth testing the following aspects:
Review checklist
Reminder for reviewers