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

GH-8186: Workaround for application.confirmExit #8732

Merged
merged 1 commit into from
Nov 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
<a name="breaking_changes_1.8.0">[Breaking Changes:](#breaking_changes_1.8.0)</a>

- [file-search] Deprecate dependency on `@theia/process` and replaced its usage by node's `child_process` api.
- [electron] Removed `attachWillPreventUnload` method from the Electron main application. The `confirmExit` logic is handled on the frontend. [#8732](https://github.com/eclipse-theia/theia/pull/8732)

## v1.7.0 - 29/10/2020

Expand Down
7 changes: 5 additions & 2 deletions packages/core/src/browser/frontend-application.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import { ShellLayoutRestorer, ApplicationShellLayoutMigrationError } from './she
import { FrontendApplicationStateService } from './frontend-application-state';
import { preventNavigation, parseCssTime, animationFrame } from './browser';
import { CorePreferences } from './core-preferences';
import { WindowService } from './window/window-service';

/**
* Clients can implement to get a callback for contributing widgets to a shell on start.
Expand Down Expand Up @@ -96,6 +97,9 @@ export class FrontendApplication {
@inject(CorePreferences)
protected readonly corePreferences: CorePreferences;

@inject(WindowService)
protected readonly windowsService: WindowService;

constructor(
@inject(CommandRegistry) protected readonly commands: CommandRegistry,
@inject(MenuModelRegistry) protected readonly menus: MenuModelRegistry,
Expand Down Expand Up @@ -182,8 +186,7 @@ export class FrontendApplication {
*/
protected registerEventListeners(): void {
this.registerCompositionEventListeners(); /* Hotfix. See above. */

window.addEventListener('beforeunload', () => {
this.windowsService.onUnload(() => {
this.stateService.state = 'closing_window';
this.layoutRestorer.storeLayout(this);
this.stopContributions();
Expand Down
28 changes: 23 additions & 5 deletions packages/core/src/browser/window/default-window-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
********************************************************************************/

import { inject, injectable, named } from 'inversify';
import { Event, Emitter } from '../../common';
import { CorePreferences } from '../core-preferences';
import { ContributionProvider } from '../../common/contribution-provider';
import { FrontendApplicationContribution, FrontendApplication } from '../frontend-application';
Expand All @@ -25,6 +26,11 @@ export class DefaultWindowService implements WindowService, FrontendApplicationC

protected frontendApplication: FrontendApplication;

protected onUnloadEmitter = new Emitter<void>();
get onUnload(): Event<void> {
return this.onUnloadEmitter.event;
}

@inject(CorePreferences)
protected readonly corePreferences: CorePreferences;

Expand All @@ -34,11 +40,7 @@ export class DefaultWindowService implements WindowService, FrontendApplicationC

onStart(app: FrontendApplication): void {
this.frontendApplication = app;
window.addEventListener('beforeunload', event => {
if (!this.canUnload()) {
return this.preventUnload(event);
}
});
this.registerUnloadListeners();
}

openNewWindow(url: string): undefined {
Expand All @@ -61,6 +63,22 @@ export class DefaultWindowService implements WindowService, FrontendApplicationC
return confirmExit !== 'always';
}

/**
* Implement the mechanism to detect unloading of the page.
*/
protected registerUnloadListeners(): void {
window.addEventListener('beforeunload', event => {
if (!this.canUnload()) {
return this.preventUnload(event);
}
});
// In a browser, `unload` is correctly fired when the page unloads, unlike Electron.
// If `beforeunload` is cancelled, the user will be prompted to leave or stay.
// If the user stays, the page won't be unloaded, so `unload` is not fired.
// If the user leaves, the page will be unloaded, so `unload` is fired.
window.addEventListener('unload', () => this.onUnloadEmitter.fire());
}

/**
* Ask the user to confirm if they want to unload the window. Prevent it if they do not.
* @param event The beforeunload event
Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/browser/window/test/mock-window-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
********************************************************************************/
import { injectable } from 'inversify';
import { Event } from '../../../common/event';
import { WindowService } from '../window-service';

@injectable()
export class MockWindowService implements WindowService {
openNewWindow(): undefined { return undefined; }
canUnload(): boolean { return true; }
get onUnload(): Event<void> { return Event.None; }
kittaakos marked this conversation as resolved.
Show resolved Hide resolved
}
8 changes: 8 additions & 0 deletions packages/core/src/browser/window/window-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
********************************************************************************/

import { Event } from '../../common/event';

export interface NewWindowOptions {
readonly external?: boolean;
}
Expand All @@ -38,4 +40,10 @@ export interface WindowService {
*/
canUnload(): boolean;

/**
* Fires when the `window` unloads. The unload event is inevitable. On this event, the frontend application can save its state and release resource.
* Saving the state and releasing any resources must be a synchronous call. Any asynchronous calls invoked after emitting this event might be ignored.
*/
readonly onUnload: Event<void>;

}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
********************************************************************************/

import { injectable, inject } from 'inversify';
import { remote } from 'electron';
import { NewWindowOptions } from '../../browser/window/window-service';
import { DefaultWindowService } from '../../browser/window/default-window-service';
import { ElectronMainWindowService } from '../../electron-common/electron-main-window-service';
Expand All @@ -30,9 +31,33 @@ export class ElectronWindowService extends DefaultWindowService {
return undefined;
}

protected preventUnload(event: BeforeUnloadEvent): string | void {
// The user will be shown a confirmation dialog by the will-prevent-unload handler in the Electron main script
event.returnValue = false;
registerUnloadListeners(): void {
window.addEventListener('beforeunload', event => {
// Either we can unload, or the user confirms that he wants to quit
if (this.canUnload() || this.shouldUnload()) {
// We are unloading
delete event.returnValue;
this.onUnloadEmitter.fire();
} else {
// The user wants to stay, let's prevent unloading
return this.preventUnload(event);
}
});
}

/**
* When preventing `beforeunload` on Electron, no popup is shown.
* This method implements a modal to ask the user if he wants to quit the page.
*/
protected shouldUnload(): boolean {
const electronWindow = remote.getCurrentWindow();
const response = remote.dialog.showMessageBoxSync(electronWindow, {
type: 'question',
buttons: ['Yes', 'No'],
title: 'Confirm',
message: 'Are you sure you want to quit?',
detail: 'Any unsaved changes will not be saved.'
});
return response === 0; // 'Yes', close the window.
}
}
23 changes: 1 addition & 22 deletions packages/core/src/electron-main/electron-main-application.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
********************************************************************************/

import { inject, injectable, named } from 'inversify';
import { session, screen, globalShortcut, app, BrowserWindow, BrowserWindowConstructorOptions, Event as ElectronEvent, dialog } from 'electron';
import { session, screen, globalShortcut, app, BrowserWindow, BrowserWindowConstructorOptions, Event as ElectronEvent } from 'electron';
import * as path from 'path';
import { Argv } from 'yargs';
import { AddressInfo } from 'net';
Expand Down Expand Up @@ -213,7 +213,6 @@ export class ElectronMainApplication {
const electronWindow = new BrowserWindow(options);
this.attachReadyToShow(electronWindow);
this.attachSaveWindowState(electronWindow);
this.attachWillPreventUnload(electronWindow);
this.attachGlobalShortcuts(electronWindow);
this.restoreMaximizedState(electronWindow, options);
return electronWindow;
Expand Down Expand Up @@ -339,26 +338,6 @@ export class ElectronMainApplication {
electronWindow.on('move', saveWindowStateDelayed);
}

/**
* Catch window closing event and display a confirmation window.
*/
protected attachWillPreventUnload(electronWindow: BrowserWindow): void {
// Fired when a beforeunload handler tries to prevent the page unloading
electronWindow.webContents.on('will-prevent-unload', async event => {
const { response } = await dialog.showMessageBox(electronWindow, {
type: 'question',
buttons: ['Yes', 'No'],
title: 'Confirm',
message: 'Are you sure you want to quit?',
detail: 'Any unsaved changes will not be saved.'
});
if (response === 0) { // 'Yes'
// This ignores the beforeunload callback, allowing the page to unload
event.preventDefault();
}
});
}

/**
* Catch certain keybindings to prevent reloading the window using keyboard shortcuts.
*/
Expand Down