eclipse-theiaGH-6530: Set the shutdown hook to beforeunload.

- `unload` was not triggered in electron. (See: electron/electron#7278 (comment)) - Exposed `ShellLayoutRestorer` APIs. - Added logging. - Fixed typos. Closes eclipse-theia#6530. Signed-off-by: Akos Kitta <kittaakos@typefox.io>
akosyakov · Feb 24, 2020 · d4d2432 · d4d2432
1 parent 69d5aef
commit d4d2432
Show file tree

Hide file tree

Showing 4 changed files with 36 additions and 17 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ## v0.13.0
 
+- [core] Switched the frontend application's shutdown hook from `window.unload` to `window.beforeunload`. [#6530](https://github.com/eclipse-theia/theia/issues/6530)
 - [scm] added handling when opening diff-editors to respect preference `workbench.list.openMode` [#6481](https://github.com/eclipse-theia/theia/pull/6481)
 
 Breaking changes:

diff --git a/packages/core/src/browser/frontend-application-state.ts b/packages/core/src/browser/frontend-application-state.ts
@@ -14,9 +14,10 @@
  * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
  ********************************************************************************/
 
-import { injectable } from 'inversify';
+import { injectable, inject } from 'inversify';
 import { Emitter, Event } from '../common/event';
 import { Deferred } from '../common/promise-util';
+import { ILogger } from '../common/logger';
 
 export type FrontendApplicationState =
     'init'
@@ -29,6 +30,9 @@ export type FrontendApplicationState =
 @injectable()
 export class FrontendApplicationStateService {
 
+    @inject(ILogger)
+    protected readonly logger: ILogger;
+
     private _state: FrontendApplicationState = 'init';
 
     protected deferred: { [state: string]: Deferred<void> } = {};
@@ -43,11 +47,13 @@ export class FrontendApplicationStateService {
             if (this.deferred[this._state] === undefined) {
                 this.deferred[this._state] = new Deferred();
             }
+            const oldState = this._state;
             this._state = state;
             if (this.deferred[state] === undefined) {
                 this.deferred[state] = new Deferred();
             }
             this.deferred[state].resolve();
+            this.logger.info(`Changed application state from '${oldState}' to '${this._state}'.`);
             this.stateChanged.fire(state);
         }
     }

diff --git a/packages/core/src/browser/frontend-application.ts b/packages/core/src/browser/frontend-application.ts
@@ -58,7 +58,7 @@ export interface FrontendApplicationContribution {
     /**
      * Called when an application is stopped or unloaded.
      *
-     * Note that this is implemented using `window.unload` which doesn't allow any asynchronous code anymore.
+     * Note that this is implemented using `window.beforeunload` which doesn't allow any asynchronous code anymore.
      * I.e. this is the last tick.
      */
     onStop?(app: FrontendApplication): void;
@@ -161,7 +161,7 @@ export class FrontendApplication {
      * Register global event listeners.
      */
     protected registerEventListeners(): void {
-        window.addEventListener('unload', () => {
+        window.addEventListener('beforeunload', () => {
             this.stateService.state = 'closing_window';
             this.layoutRestorer.storeLayout(this);
             this.stopContributions();
@@ -320,6 +320,7 @@ export class FrontendApplication {
      * Stop the frontend application contributions. This is called when the window is unloaded.
      */
     protected stopContributions(): void {
+        this.logger.info('>>> Stopping contributions....');
         for (const contribution of this.contributions.getContributions()) {
             if (contribution.onStop) {
                 try {
@@ -329,6 +330,7 @@ export class FrontendApplication {
                 }
             }
         }
+        this.logger.info('<<< All contributions have been stopped.');
     }
 
     protected async measure<T>(name: string, fn: () => MaybePromise<T>): Promise<T> {

diff --git a/packages/core/src/browser/shell/shell-layout-restorer.ts b/packages/core/src/browser/shell/shell-layout-restorer.ts
@@ -20,7 +20,7 @@ import { FrontendApplication } from '../frontend-application';
 import { WidgetManager, WidgetConstructionOptions } from '../widget-manager';
 import { StorageService } from '../storage-service';
 import { ILogger } from '../../common/logger';
-import { CommandContribution, CommandRegistry } from '../../common/command';
+import { CommandContribution, CommandRegistry, Command } from '../../common/command';
 import { ThemeService } from '../theming';
 import { ContributionProvider } from '../../common/contribution-provider';
 import { MaybePromise } from '../../common/types';
@@ -109,16 +109,17 @@ export interface ApplicationShellLayoutMigration {
     onWillInflateWidget?(desc: WidgetDescription, context: ApplicationShellLayoutMigrationContext): MaybePromise<WidgetDescription | undefined>;
 }
 
-const resetLayoutCommand = {
+export const RESET_LAYOUT: Command = {
     id: 'reset.layout',
     category: 'View',
     label: 'Reset Workbench Layout'
 };
 
 @injectable()
 export class ShellLayoutRestorer implements CommandContribution {
-    private storageKey = 'layout';
-    private shouldStoreLayout: boolean = true;
+
+    protected storageKey = 'layout';
+    protected shouldStoreLayout: boolean = true;
 
     @inject(ContributionProvider) @named(ApplicationShellLayoutMigration)
     protected readonly migrations: ContributionProvider<ApplicationShellLayoutMigration>;
@@ -129,22 +130,28 @@ export class ShellLayoutRestorer implements CommandContribution {
         @inject(StorageService) protected storageService: StorageService) { }
 
     registerCommands(commands: CommandRegistry): void {
-        commands.registerCommand(resetLayoutCommand, {
-            execute: async () => {
-                this.shouldStoreLayout = false;
-                this.storageService.setData(this.storageKey, undefined);
-                ThemeService.get().reset(); // Theme service cannot use DI, so the current theme ID is stored elsewhere. Hence the explicit reset.
-                window.location.reload(true);
-            }
+        commands.registerCommand(RESET_LAYOUT, {
+            execute: async () => this.resetLayout()
         });
     }
 
+    protected async resetLayout(): Promise<void> {
+        this.logger.info('>>> Resetting layout...');
+        this.shouldStoreLayout = false;
+        this.storageService.setData(this.storageKey, undefined);
+        ThemeService.get().reset(); // Theme service cannot use DI, so the current theme ID is stored elsewhere. Hence the explicit reset.
+        this.logger.info('<<< The layout has been successfully reset.');
+        window.location.reload(true);
+    }
+
     storeLayout(app: FrontendApplication): void {
         if (this.shouldStoreLayout) {
             try {
+                this.logger.info('>>> Storing the layout...');
                 const layoutData = app.shell.getLayoutData();
                 const serializedLayoutData = this.deflate(layoutData);
                 this.storageService.setData(this.storageKey, serializedLayoutData);
+                this.logger.info('<<< The layout has been successfully stored.');
             } catch (error) {
                 this.storageService.setData(this.storageKey, undefined);
                 this.logger.error('Error during serialization of layout data', error);
@@ -153,12 +160,15 @@ export class ShellLayoutRestorer implements CommandContribution {
     }
 
     async restoreLayout(app: FrontendApplication): Promise<boolean> {
+        this.logger.info('>>> Restoring the layout state...');
         const serializedLayoutData = await this.storageService.getData<string>(this.storageKey);
         if (serializedLayoutData === undefined) {
+            this.logger.info('<<< Nothing to restore.');
             return false;
         }
         const layoutData = await this.inflate(serializedLayoutData);
         await app.shell.setLayoutData(layoutData);
+        this.logger.info('<<< The layout has been successfully restored.');
         return true;
     }
 
@@ -227,7 +237,7 @@ export class ShellLayoutRestorer implements CommandContribution {
             } else {
                 console.warn(`Layout version ${layoutVersion} is ahead current layout version ${applicationShellLayoutVersion}, trying to load anyway...`);
             }
-            console.info(`Please use '${resetLayoutCommand.label}' command if the layout looks bogus.`);
+            console.info(`Please use '${RESET_LAYOUT.label}' command if the layout looks bogus.`);
         }
 
         const migrations = this.migrations.getContributions()
@@ -246,7 +256,7 @@ export class ShellLayoutRestorer implements CommandContribution {
     protected async fireWillInflateLayout(context: ShellLayoutRestorer.InflateContext): Promise<void> {
         for (const migration of context.migrations) {
             if (migration.onWillInflateLayout) {
-                // don't catch exceptions, if one migrarion fails all should fail.
+                // don't catch exceptions, if one migration fails all should fail.
                 await migration.onWillInflateLayout(context);
             }
         }
@@ -284,7 +294,7 @@ export class ShellLayoutRestorer implements CommandContribution {
     protected async fireWillInflateWidget(desc: WidgetDescription, context: ShellLayoutRestorer.InflateContext): Promise<WidgetDescription> {
         for (const migration of context.migrations) {
             if (migration.onWillInflateWidget) {
-                // don't catch exceptions, if one migrarion fails all should fail.
+                // don't catch exceptions, if one migration fails all should fail.
                 const migrated = await migration.onWillInflateWidget(desc, context);
                 if (migrated) {
                     if (migrated.innerWidgetState && typeof migrated.innerWidgetState !== 'string') {