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.

Sign up for GitHub

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

Jump to bottom

Fix window maximization when using splash screen #14219

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix window maximization when using splash screen #14219

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/core/src/electron-main/theia-electron-window.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,8 @@ export class TheiaElectronWindow {
this._window.setMenuBarVisibility(false);
if (!this.options.preventAutomaticShow) {
this.attachReadyToShow();
this.restoreMaximizedState();
}
this.restoreMaximizedState();
this.attachCloseListeners();
this.trackApplicationState();
this.attachReloadListener();
Expand Down Expand Up @@ -196,6 +196,9 @@ export class TheiaElectronWindow {
protected trackApplicationState(): void {
this.toDispose.push(TheiaRendererAPI.onApplicationStateChanged(this.window.webContents, state => {
this.applicationState = state;
if (state === 'ready' && this.options.preventAutomaticShow) {
this.restoreMaximizedState();
}
Comment on lines +199 to +201
Copy link
Member

Choose a reason for hiding this comment

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

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 the restoreMaximizedState
  • If a Window is not immediately shown we
    • return a Proxy of BrowserWindow
    • The proxy overrides the show of BrowserWindow
    • On the first time show is called, we also implicitly call restoreMaximizedState

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

}));
}

Expand Down
Loading