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

Extend multi-window support to Electron (#11642) #49

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,10 @@ module.exports = Promise.resolve()${this.compileElectronMainModuleImports(electr
// Only process messages from Theia main window
if (e.source === window.opener) {
// Delegate message to iframe
document.getElementsByTagName('iframe').item(0).contentWindow.postMessage({ ...e.data }, '*');
const f = document.getElementsByTagName('iframe');
if (f) {
f.item(0).contentWindow.postMessage({ ...e.data }, '*');
}
}
});
</script>
Expand Down
1 change: 1 addition & 0 deletions examples/electron/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
"@theia/scm": "1.30.0",
"@theia/scm-extra": "1.30.0",
"@theia/search-in-workspace": "1.30.0",
"@theia/secondary-window": "1.30.0",
"@theia/task": "1.30.0",
"@theia/terminal": "1.30.0",
"@theia/timeline": "1.30.0",
Expand Down
3 changes: 3 additions & 0 deletions examples/electron/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,9 @@
{
"path": "../../packages/search-in-workspace"
},
{
"path": "../../packages/secondary-window"
},
{
"path": "../../packages/task"
},
Expand Down
16 changes: 11 additions & 5 deletions packages/core/src/browser/color-application-contribution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export class ColorApplicationContribution implements FrontendApplicationContribu
protected readonly onDidChangeEmitter = new Emitter<void>();
readonly onDidChange = this.onDidChangeEmitter.event;
private readonly windows: Set<Window> = new Set();
protected readonly toUpdate = new Map<Window, DisposableCollection>();

@inject(ColorRegistry)
protected readonly colors: ColorRegistry;
Expand Down Expand Up @@ -61,21 +62,26 @@ export class ColorApplicationContribution implements FrontendApplicationContribu
this.windows.add(win);
this.updateWindow(win);
this.onDidChangeEmitter.fire();
return Disposable.create(() => this.windows.delete(win));
return Disposable.create(() => {
this.toUpdate.delete(win);
this.windows.delete(win);
});
}

protected readonly toUpdate = new DisposableCollection();
protected update(): void {
this.toUpdate.dispose();
this.windows.forEach(win => this.updateWindow(win));
this.onDidChangeEmitter.fire();
}

protected updateWindow(win: Window): void {
this.toUpdate.get(win)?.dispose();
const theme = 'theia-' + this.themeService.getCurrentTheme().type;

const localDisposables = new DisposableCollection();
this.toUpdate.set(win, localDisposables);

win.document.body.classList.add(theme);
this.toUpdate.push(Disposable.create(() => win.document.body.classList.remove(theme)));
localDisposables.push(Disposable.create(() => win.document.body.classList.remove(theme)));

const documentElement = win.document.documentElement;
if (documentElement) {
Expand All @@ -84,7 +90,7 @@ export class ColorApplicationContribution implements FrontendApplicationContribu
if (variable) {
const { name, value } = variable;
documentElement.style.setProperty(name, value);
this.toUpdate.push(Disposable.create(() => documentElement.style.removeProperty(name)));
localDisposables.push(Disposable.create(() => documentElement.style.removeProperty(name)));
}
}
}
Expand Down
45 changes: 32 additions & 13 deletions packages/core/src/browser/secondary-window-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
// SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
// *****************************************************************************

import debounce = require('lodash.debounce');
import { inject, injectable } from 'inversify';
import { BoxLayout, BoxPanel, ExtractableWidget, Widget } from './widgets';
import { MessageService } from '../common/message-service';
Expand All @@ -23,6 +22,8 @@ import { Emitter } from '../common/event';
import { SecondaryWindowService } from './window/secondary-window-service';
import { KeybindingRegistry } from './keybinding';
import { ColorApplicationContribution } from './color-application-contribution';
import debounce = require('lodash.debounce');
import { Saveable } from './saveable';

/** Widget to be contained directly in a secondary window. */
class SecondaryWindowRootWidget extends Widget {
Expand All @@ -40,7 +41,6 @@ class SecondaryWindowRootWidget extends Widget {
}

/**
* Offers functionality to move a widget out of the main window to a newly created window.
* Widgets must explicitly implement the `ExtractableWidget` interface to support this.
*
* This handler manages the opened secondary windows and sets up messaging between them and the Theia main window.
Expand Down Expand Up @@ -139,13 +139,32 @@ export class SecondaryWindowHandler {
return;
}

const newWindow = this.secondaryWindowService.createSecondaryWindow(closed => {
this.applicationShell.closeWidget(widget.id);
const extIndex = this.secondaryWindows.indexOf(closed);
if (extIndex > -1) {
this.secondaryWindows.splice(extIndex, 1);
}
});
const newWindow = this.secondaryWindowService.createSecondaryWindow(
() => {
const saveable = Saveable.get(widget);
return !!saveable && saveable.dirty && saveable.autoSave === 'off';
},
async trySaving => {
widget.isClosing = true;
// if trySaving is true, we should let the widget decide what to do
const saveable = Saveable.get(widget);
let closeOptions = undefined;
if (!trySaving) {
closeOptions = {
save: !!saveable && saveable.dirty && saveable.autoSave !== 'off'
};
}

await this.applicationShell.closeWidget(widget.id, closeOptions);
widget.isClosing = false;
return widget.isDisposed;
Comment on lines +158 to +160

Choose a reason for hiding this comment

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

await this.applicationShell.closeWidget(widget.id, closeOptions); closes the secondary window that the widget resides in with the changes at application-shell.ts https://github.com/eclipsesource/theia/pull/49/files#diff-cb4b87cc3dc274963d24f6fd5682ad9c89cd51d94726100eb2e3c92421e5c171R1565-R1566

Afterwards, the widget is accessed to set its isClosing flag. Maybe this could cause problems with IllegalAccess?
This is especially intereseting because this was not the case on the last known working state at cdamus@2b3d733

The changes stem from the browser save handling as far as I can see but affect the electron implementation in this way, too. It is called here: https://github.com/eclipsesource/theia/pull/49/files#diff-563e180229ce37170c43089f2a51e72ab90eead76b88b8a4300365d8373f93c3R51

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All I can say is that this shouldn't be a problem, since the widget is an object that lives in the context of the main browser window. But it's a diff I should look at.

},
closedWindow => {
const extIndex = this.secondaryWindows.indexOf(closedWindow);
if (extIndex > -1) {
this.secondaryWindows.splice(extIndex, 1);
}
});

if (!newWindow) {
this.messageService.error('The widget could not be moved to a secondary window because the window creation failed. Please make sure to allow popups.');
Expand All @@ -156,7 +175,6 @@ export class SecondaryWindowHandler {

const mainWindowTitle = document.title;
newWindow.onload = () => {
this.keybindings.registerEventListeners(newWindow);
// Use the widget's title as the window title
// Even if the widget's label were malicious, this should be safe against XSS because the HTML standard defines this is inserted via a text node.
// See https://html.spec.whatwg.org/multipage/dom.html#document.title
Expand All @@ -167,11 +185,14 @@ export class SecondaryWindowHandler {
console.error('Could not find dom element to attach to in secondary window');
return;
}

const disposKeybindingsListener = this.keybindings.registerEventListeners(newWindow);
const unregisterWithColorContribution = this.colorAppContribution.registerWindow(newWindow);

widget.secondaryWindow = newWindow;
const rootWidget = new SecondaryWindowRootWidget();
rootWidget.addClass('secondary-widget-root');
rootWidget.id = 'root-' + widget.id;
Widget.attach(rootWidget, element);
rootWidget.addWidget(widget);
widget.show();
Expand All @@ -181,11 +202,9 @@ export class SecondaryWindowHandler {

// Close the window if the widget is disposed, e.g. by a command closing all widgets.
widget.disposed.connect(() => {
disposKeybindingsListener.dispose();
unregisterWithColorContribution.dispose();
this.removeWidget(widget);
if (!newWindow.closed) {
newWindow.close();
}
});

// debounce to avoid rapid updates while resizing the secondary window
Expand Down
23 changes: 14 additions & 9 deletions packages/core/src/browser/shell/application-shell.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import { FrontendApplicationStateService } from '../frontend-application-state';
import { TabBarToolbarRegistry, TabBarToolbarFactory } from './tab-bar-toolbar';
import { ContextKeyService } from '../context-key-service';
import { Emitter } from '../../common/event';
import { waitForRevealed, waitForClosed, PINNED_CLASS } from '../widgets';
import { waitForRevealed, waitForClosed, PINNED_CLASS, ExtractableWidget } from '../widgets';
import { CorePreferences } from '../core-preferences';
import { BreadcrumbsRendererFactory } from '../breadcrumbs/breadcrumbs-renderer';
import { Deferred } from '../../common/promise-util';
Expand Down Expand Up @@ -1561,14 +1561,19 @@ export class ApplicationShell extends Widget {
if (!current) {
return undefined;
}
const saveableOptions = options && { shouldSave: () => options.save };
const pendingClose = SaveableWidget.is(current)
? current.closeWithSaving(saveableOptions)
: (current.close(), waitForClosed(current));
await Promise.all([
pendingClose,
this.pendingUpdates
]);

if (ExtractableWidget.is(current) && current.secondaryWindow && !current.isClosing) {
current.secondaryWindow!.close();

Choose a reason for hiding this comment

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

With the current.secondaryWindow check, the ! shouldn't be necessary.

Suggested change
current.secondaryWindow!.close();
current.secondaryWindow.close();

Choose a reason for hiding this comment

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

Suggested change
current.secondaryWindow!.close();
current.secondaryWindow.close();

} else {
const saveableOptions = options && { shouldSave: () => options.save };
const pendingClose = SaveableWidget.is(current)
? current.closeWithSaving(saveableOptions)
: (current.close(), waitForClosed(current));
await Promise.all([
pendingClose,
this.pendingUpdates
]);
}
return stack[0] || current;
}

Expand Down
3 changes: 3 additions & 0 deletions packages/core/src/browser/widgets/extractable-widget.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ import { Widget } from './widget';
export interface ExtractableWidget extends Widget {
/** Set to `true` to mark the widget to be extractable. */
isExtractable: boolean;

/** State variable to keep track of recursive attempty to close the secondary window */

Choose a reason for hiding this comment

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

Suggested change
/** State variable to keep track of recursive attempty to close the secondary window */
/** State variable to keep track of recursive attempts to close the secondary window */

isClosing: boolean;
/** The secondary window that the window was extracted to or `undefined` if it is not yet extracted. */
secondaryWindow: Window | undefined;
}
Expand Down
7 changes: 4 additions & 3 deletions packages/core/src/browser/widgets/widget.ts
Original file line number Diff line number Diff line change
Expand Up @@ -336,16 +336,17 @@ export function waitForHidden(widget: Widget): Promise<void> {
}

function waitForVisible(widget: Widget, visible: boolean, attached?: boolean): Promise<void> {
const win = widget.node.ownerDocument.defaultView || window;
if ((typeof attached !== 'boolean' || widget.isAttached === attached) &&
(widget.isVisible === visible || (widget.node.style.visibility !== 'hidden') === visible)
) {
return new Promise(resolve => window.requestAnimationFrame(() => resolve()));
return new Promise(resolve => win.requestAnimationFrame(() => resolve()));
}
return new Promise(resolve => {
const waitFor = () => window.requestAnimationFrame(() => {
const waitFor = () => win.requestAnimationFrame(() => {
if ((typeof attached !== 'boolean' || widget.isAttached === attached) &&
(widget.isVisible === visible || (widget.node.style.visibility !== 'hidden') === visible)) {
window.requestAnimationFrame(() => resolve());
win.requestAnimationFrame(() => resolve());
} else {
waitFor();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,33 +46,44 @@ export class DefaultSecondaryWindowService implements SecondaryWindowService {
});
}

createSecondaryWindow(onClose?: (closedWin: Window) => void): Window | undefined {
const win = this.doCreateSecondaryWindow(onClose);
createSecondaryWindow(wouldLoseStateOnClosing: () => boolean, tryCloseWidget: (trySaving: boolean) => Promise<boolean>, closed: (win: Window) => void): Window | undefined {
const id = this.nextWindowId();
const win = this.doCreateSecondaryWindow(id, wouldLoseStateOnClosing, tryCloseWidget, closed);
if (win) {
this.secondaryWindows.push(win);
}
return win;
}

protected doCreateSecondaryWindow(onClose?: (closedWin: Window) => void): Window | undefined {
const win = window.open(DefaultSecondaryWindowService.SECONDARY_WINDOW_URL, this.nextWindowId(), 'popup');
protected doCreateSecondaryWindow(id: string, wouldLoseStateOnClosing: () => boolean, tryCloseWidget: (trySaving: boolean) => Promise<boolean>,
closed: (win: Window) => void): Window | undefined {
const win = window.open(DefaultSecondaryWindowService.SECONDARY_WINDOW_URL, id, 'popup');

if (win) {
// Add the unload listener after the dom content was loaded because otherwise the unload listener is called already on open in some browsers (e.g. Chrome).
win.addEventListener('DOMContentLoaded', () => {
win.addEventListener('beforeunload', evt => {
if (wouldLoseStateOnClosing()) {
evt.returnValue = '';
evt.preventDefault();
return '';
}
}, { capture: true });
win.addEventListener('unload', () => {
this.handleWindowClosed(win, onClose);
tryCloseWidget(false);
this.handleWindowClosed(win);
closed(win);
});
});
}
return win ?? undefined;
}

protected handleWindowClosed(win: Window, onClose?: (closedWin: Window) => void): void {
protected handleWindowClosed(win: Window): void {
const extIndex = this.secondaryWindows.indexOf(win);
if (extIndex > -1) {
this.secondaryWindows.splice(extIndex, 1);
};
onClose?.(win);
}

focus(win: Window): void {
Expand Down
6 changes: 4 additions & 2 deletions packages/core/src/browser/window/secondary-window-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,12 @@ export interface SecondaryWindowService {
* Creates a new secondary window for a widget to be extracted from the application shell.
* The created window is closed automatically when the current theia instance is closed.
*
* @param onClose optional callback that is invoked when the secondary window is closed
* @param wouldLoseStateOnClosing callback to determine whether save is necessary
* @param tryCloseWidget callback to try and close the widget. If the promise returns true, the window will be closed.
* @param onClosed optional callback that is invoked when the secondary window is closed
* @returns the created window or `undefined` if it could not be created
*/
createSecondaryWindow(onClose?: (win: Window) => void): Window | undefined;
createSecondaryWindow(wouldLoseStateOnClosing: () => boolean, tryCloseWidget: (trySaving: boolean) => Promise<boolean>, closed: (win: Window) => void): Window | undefined;

/** Handles focussing the given secondary window in the browser and on Electron. */
focus(win: Window): void;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,32 +14,53 @@
// SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
// *****************************************************************************

import { BrowserWindow } from '../../../electron-shared/electron';
import { ipcRenderer, BrowserWindow } from '../../../electron-shared/electron';
import * as electronRemote from '../../../electron-shared/@electron/remote';
import { injectable } from 'inversify';
import { injectable, postConstruct } from 'inversify';
import { DefaultSecondaryWindowService } from '../../browser/window/default-secondary-window-service';
import { CloseSecondaryRequestArguments, CLOSE_SECONDARY_REQUESTED_SIGNAL } from '../../electron-common/messaging/electron-messages';

@injectable()
export class ElectronSecondaryWindowService extends DefaultSecondaryWindowService {
protected electronWindows: Map<string, BrowserWindow> = new Map();

protected override doCreateSecondaryWindow(onClose?: (closedWin: Window) => void): Window | undefined {
const id = this.nextWindowId();
private electronWindows: Map<string, BrowserWindow> = new Map();
private electronWindowsById: Map<string, () => Promise<boolean>> = new Map();

@postConstruct()
override init(): void {
super.init();
ipcRenderer.addListener(CLOSE_SECONDARY_REQUESTED_SIGNAL, (_sender, args: CloseSecondaryRequestArguments) => this.handleCloseRequestedEvent(args));
}

protected async handleCloseRequestedEvent(event: CloseSecondaryRequestArguments): Promise<void> {
const safeToClose = await this.safeToClose(event.windowId);
if (safeToClose) {
ipcRenderer.send(event.confirmChannel);
} else {
ipcRenderer.send(event.cancelChannel);
}
}

protected override doCreateSecondaryWindow(id: string, wouldLoseStateOnClosing: () => boolean, tryCloseWidget: (trySaving: boolean) => Promise<boolean>,
closed: (win: Window) => void): Window | undefined {
let win: Window | undefined = undefined;
electronRemote.getCurrentWindow().webContents.once('did-create-window', newElectronWindow => {
newElectronWindow.setMenuBarVisibility(false);
// newElectronWindow.setMenuBarVisibility(false);

Choose a reason for hiding this comment

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

Either comment this back in or remove the line completely. I think hiding the menu in secondary windows generally makes sense with the current state (i.e. it's just the default electron menu)

this.electronWindows.set(id, newElectronWindow);
newElectronWindow.on('closed', () => {
const electronId = newElectronWindow.id.toString();
this.electronWindowsById.set(electronId, () => tryCloseWidget(true));
const closedHandler = () => {
if (closed) {
closed(win!);
}

this.electronWindows.delete(id);
const browserWin = this.secondaryWindows.find(w => w.name === id);
if (browserWin) {
this.handleWindowClosed(browserWin, onClose);
} else {
console.warn(`Could not execute proper close handling for secondary window '${id}' because its frontend window could not be found.`);
};
});
this.electronWindowsById.delete(electronId);
};
newElectronWindow.once('closed', closedHandler);
});
const win = window.open(DefaultSecondaryWindowService.SECONDARY_WINDOW_URL, id);
return win ?? undefined;
win = window.open(DefaultSecondaryWindowService.SECONDARY_WINDOW_URL, id, 'popup') || undefined;
return win;
}

override focus(win: Window): void {
Expand All @@ -54,4 +75,13 @@ export class ElectronSecondaryWindowService extends DefaultSecondaryWindowServic
console.warn(`There is no known secondary window '${win.name}'. Thus, the window could not be focussed.`);
}
}

safeToClose(windowId: string): Promise<boolean> {
const closingHandler = this.electronWindowsById.get(windowId);
if (closingHandler) {
return closingHandler!();
} else {
return Promise.resolve(true);
}
}
}
Loading