Fix window maximization when using splash screen #14219
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Avoid eager restore of maximized state in `TheiaElectronWindow.init` when a splash screen is configured (i.e. `preventAutomaticShow` config option is false) Calling `window.maximize()` implicity also makes the window visibile. If `preventAutomaticShow` is true we have to make sure to invoke `restoreMaximizeState` after the main window is ready. Otherwise both the splashscreen and the preload window will be visible at the same time Fixes eclipse-theia#14218
sdirix
reviewed
Oct 9, 2024
Comment on lines
+199
to
+201
| if (state === 'ready' && this.options.preventAutomaticShow) { | ||
| this.restoreMaximizedState(); | ||
| } |
There was a problem hiding this comment.
I'm not sure that here is the best place to put the maximize logic. I agree that this fixes the issue, however this depends on the fact that we know that the main application will call show once the application state is 'ready'.
What do you think of the following:
- If a Window is immediately shown (i.e.
this.options.show === true || typeof this.options.show === 'undefined'), we directly call therestoreMaximizedState - If a Window is not immediately shown we
- return a
ProxyofBrowserWindow - The proxy overrides the
showofBrowserWindow - On the first time
showis called, we also implicitly callrestoreMaximizedState
- return a
There was a problem hiding this comment.
IMO using a proxy approach here is kind of an overkill. Maybe it would be better to move the maximization logic into the main application entirely. Here we know exactly when the main window will be shown and can invoke the maximization at this point if nencessary.
It could look like this:
diff
diff --git a/packages/core/src/electron-main/electron-main-application.ts b/packages/core/src/electron-main/electron-main-application.ts
index b464c916a84..13f7fe127b7 100644
--- a/packages/core/src/electron-main/electron-main-application.ts
+++ b/packages/core/src/electron-main/electron-main-application.ts
@@ -334,19 +334,22 @@ export class ElectronMainApplication {
});
if (this.isShowSplashScreen()) {
console.log('Showing splash screen');
- this.configureAndShowSplashScreen(this.initialWindow);
+ this.configureAndShowSplashScreen(this.initialWindow, options.isMaximized);
}
// Show main window early if windows shall be shown early and splash screen is not configured
if (this.isShowWindowEarly() && !this.isShowSplashScreen()) {
console.log('Showing main window early');
this.initialWindow.show();
+ if (options.isMaximized) {
+ this.initialWindow.maximize();
+ }
}
});
}
}
- protected async configureAndShowSplashScreen(mainWindow: BrowserWindow): Promise<BrowserWindow> {
+ protected async configureAndShowSplashScreen(mainWindow: BrowserWindow, isMaximized?: boolean): Promise<BrowserWindow> {
const splashScreenOptions = this.getSplashScreenOptions()!;
console.debug('SplashScreen options', splashScreenOptions);
@@ -382,6 +385,9 @@ export class ElectronMainApplication {
cancelTokenSource.cancel();
if (!mainWindow.isVisible()) {
mainWindow.show();
+ if (isMaximized) {
+ mainWindow.maximize();
+ }
}
splashScreenWindow.close();
};
diff --git a/packages/core/src/electron-main/theia-electron-window.ts b/packages/core/src/electron-main/theia-electron-window.ts
index 720865f8f59..4faa5852fb7 100644
--- a/packages/core/src/electron-main/theia-electron-window.ts
+++ b/packages/core/src/electron-main/theia-electron-window.ts
@@ -86,7 +86,6 @@ export class TheiaElectronWindow {
if (!this.options.preventAutomaticShow) {
this.attachReadyToShow();
}
- this.restoreMaximizedState();
this.attachCloseListeners();
this.trackApplicationState();
this.attachReloadListener();
@@ -185,14 +184,6 @@ export class TheiaElectronWindow {
return TheiaRendererAPI.requestClose(this.window.webContents, reason);
}
- protected restoreMaximizedState(): void {
- if (this.options.isMaximized) {
- this._window.maximize();
- } else {
- this._window.unmaximize();
- }
- }
-
protected trackApplicationState(): void {
this.toDispose.push(TheiaRendererAPI.onApplicationStateChanged(this.window.webContents, state => {
this.applicationState = state;WDYT?
Sidenote: options.show is currently set to false by default in Theia and not considered at all in the initialization logic.
I think we should keep it that way.
There was a problem hiding this comment.
Fine by me in general. We just need to make sure that when the window is shown automatically, without the main application calling show itself, we also restore the maximized state.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What it does
Avoid eager restore of maximized state in
TheiaElectronWindow.initwhen a splash screen is configured (i.e.preventAutomaticShowconfig option is false)Calling
window.maximize()implicity also makes the window visibile. IfpreventAutomaticShowis true we have to make sure to invokerestoreMaximizeStateafter the main window is ready. Otherwise both the splashscreen and the preload window will be visible at the same timeFixes #14218
How to test
Only the splash screen should be shown, main window should remain invisible until the splash screen is hidden.
Follow-ups
Review checklist
Reminder for reviewers