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

discussion: electron main fixup #8056

Closed
wants to merge 7 commits into from
Closed

discussion: electron main fixup #8056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
93d158b
Use `window` instead of `globalThis`.
Jun 19, 2020
b65d482
Resolved incorrect rebase, updated output URI.
Jun 19, 2020
2ec5183
inject the frontend app config from the gen code
Jun 19, 2020
657be51
removed redundant app name.
Jun 19, 2020
bab72e0
fixed app termination when closing the last window
Jun 19, 2020
b865b52
simplified `stop` abstraction.
Jun 19, 2020
61d6251
aligned the behavior with master.
Jun 19, 2020
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
11 changes: 4 additions & 7 deletions dev-packages/application-manager/src/generator/frontend-generator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,14 +136,11 @@ process.env.LC_NUMERIC = 'C';

const { default: electronApplicationModule } = require('@theia/electron/lib/electron-main/electron-application-module');
const { ElectronApplication, ElectronApplicationGlobals } = require('@theia/electron/lib/electron-main/electron-application');
const { FrontendApplicationConfigProvider } = require('@theia/core/lib/browser/frontend-application-config-provider');
const { Container } = require('inversify');
const { resolve } = require('path');
const { app } = require('electron');

FrontendApplicationConfigProvider.set(${this.prettyStringify(this.pck.props.frontend.config)});

const applicationName = \`${this.pck.props.frontend.config.applicationName}\`;
const config = ${this.prettyStringify(this.pck.props.frontend.config)};
const isSingleInstance = ${this.pck.props.backend.config.singleInstance === true ? 'true' : 'false'};

if (isSingleInstance && !app.requestSingleInstanceLock()) {
Expand All @@ -155,7 +152,6 @@ if (isSingleInstance && !app.requestSingleInstanceLock()) {
const container = new Container();
container.load(electronApplicationModule);
container.bind(ElectronApplicationGlobals).toConstantValue({
THEIA_APPLICATION_NAME: applicationName,
THEIA_APP_PROJECT_PATH: resolve(__dirname, '..', '..'),
THEIA_BACKEND_MAIN_PATH: resolve(__dirname, '..', 'backend', 'main.js'),
THEIA_FRONTEND_HTML_PATH: resolve(__dirname, '..', '..', 'lib', 'index.html'),
Expand All @@ -169,7 +165,7 @@ function load(raw) {

async function start() {
const application = container.get(ElectronApplication);
await application.start();
await application.start(config);
}

module.exports = Promise.resolve()${this.compileElectronMainModuleImports(electronMainModules)}
Expand All @@ -178,7 +174,8 @@ module.exports = Promise.resolve()${this.compileElectronMainModuleImports(electr
if (reason) {
console.error(reason);
}
}); `;
});
`;
}

}
4 changes: 2 additions & 2 deletions packages/core/src/browser/frontend-application-config-provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,14 @@ export class FrontendApplicationConfigProvider {
throw new Error('The configuration is already set.');
}
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const globalObject = globalThis as any;
const globalObject = window as any;
const key = FrontendApplicationConfigProvider.KEY;
globalObject[key] = config;
}

private static doGet(): FrontendApplicationConfig | undefined {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const globalObject = globalThis as any;
const globalObject = window as any;
const key = FrontendApplicationConfigProvider.KEY;
return globalObject[key];
}
Expand Down
21 changes: 0 additions & 21 deletions packages/core/src/common/promise-util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,24 +43,3 @@ export function timeout(ms: number, token = CancellationToken.None): Promise<voi
});
return deferred.promise;
}

/**
* Wrap a promise to know synchronously if it is finished or not.
*/
export class SyncPromise<T> {

/** Promise that will resolve when finished. */
readonly promise: Promise<T>;
readonly finished = false;

constructor(promise: T | PromiseLike<T>) {
this.promise = Promise.resolve(promise).then(value => {
(this.finished as boolean) = true;
return value;
}, error => {
(this.finished as boolean) = true;
throw error;
});
}

}
146 changes: 38 additions & 108 deletions packages/electron/src/electron-main/electron-application.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,30 +18,21 @@ import { ContributionProvider } from '@theia/core/lib/common/contribution-provid
import { MaybePromise } from '@theia/core/lib/common/types';
import URI from '@theia/core/lib/common/uri';
import { ElectronSecurityToken } from '@theia/core/lib/electron-common/electron-token';
import { ChildProcess, fork, ForkOptions } from 'child_process';
import { fork, ForkOptions } from 'child_process';
import * as electron from 'electron';
import { app, BrowserWindow, BrowserWindowConstructorOptions, Event as ElectronEvent, shell, dialog } from 'electron';
import { realpathSync } from 'fs';
import { inject, injectable, named } from 'inversify';
import { AddressInfo } from 'net';
import * as path from 'path';
import { Argv } from 'yargs';
import { FileUri } from '@theia/core/lib/node';
import { SyncPromise } from '@theia/core/lib/common/promise-util';
import { FrontendApplicationConfigProvider } from '@theia/core/lib/browser/frontend-application-config-provider';
import { Deferred } from '@theia/core/lib/common/promise-util';
import { FrontendApplicationConfig } from '@theia/application-package';
const Storage = require('electron-store');
const createYargs: (argv?: string[], cwd?: string) => Argv = require('yargs/yargs');

/**
* Options passed to the main/default command handler.
*/
export interface MainCommandOptions {

/**
* By default, the first positional argument. Should be a file or a folder.
*/
file?: string

}

/**
Expand All @@ -52,17 +43,15 @@ export interface MainCommandOptions {
* 2. The app is already running but user relaunches it, `secondInstance` is true.
*/
export interface ExecutionParams {
secondInstance: boolean
argv: string[]
cwd: string
readonly secondInstance: boolean;
readonly argv: string[];
}

export const ElectronApplicationGlobals = Symbol('ElectronApplicationSettings');
export interface ElectronApplicationGlobals {
THEIA_APPLICATION_NAME: string
THEIA_APP_PROJECT_PATH: string
THEIA_BACKEND_MAIN_PATH: string
THEIA_FRONTEND_HTML_PATH: string
readonly THEIA_APP_PROJECT_PATH: string
readonly THEIA_BACKEND_MAIN_PATH: string
readonly THEIA_FRONTEND_HTML_PATH: string
}

export const ElectronMainContribution = Symbol('ElectronApplicationContribution');
Expand All @@ -75,9 +64,9 @@ export interface ElectronMainContribution {
*/
onStart?(application?: ElectronApplication): MaybePromise<void>;
/**
* The application is stopping.
* The application is stopping. Contributions must perform only synchronous operations.
*/
onStop?(application?: ElectronApplication): MaybePromise<void>;
onStop?(application?: ElectronApplication): void;
}

@injectable()
Expand All @@ -94,46 +83,25 @@ export class ElectronApplication {

protected readonly electronStore = new Storage();

/**
* When the application is stopping, this field will contain the task encapsulating the stopping process.
* This field is undefined otherwise.
*/
protected stoppingTask: SyncPromise<void> | undefined;

/**
* Note: There is no backend process while in `devMode`.
*/
protected backendProcess: ChildProcess | undefined;

protected _backendPort: number | undefined;
protected config: FrontendApplicationConfig;
readonly backendPort = new Deferred<number>();

get backendPort(): number {
if (typeof this._backendPort === 'undefined') {
throw new Error('backend port is not set');
}
return this._backendPort;
}

async start(): Promise<void> {
async start(config: FrontendApplicationConfig): Promise<void> {
this.config = config;
this.hookApplicationEvents();
this._backendPort = await this.startBackend();
const port = await this.startBackend();
this.backendPort.resolve(port);
await app.whenReady();
await this.attachElectronSecurityToken(this.backendPort);
await this.attachElectronSecurityToken(await this.backendPort.promise);
await this.startContributions();
await this.launch({
secondInstance: false,
argv: process.argv,
cwd: process.cwd(),
argv: process.argv
});
}

async launch(params: ExecutionParams): Promise<void> {
createYargs(params.argv, params.cwd)
.command('$0 [<file>]', false,
cmd => cmd
.positional('file', { type: 'string' }),
args => this.handleMainCommand(params, { file: args.file }),
).parse();
this.handleMainCommand(params);
}

/**
Expand All @@ -142,12 +110,9 @@ export class ElectronApplication {
* @param options
*/
async createWindow(options: BrowserWindowConstructorOptions): Promise<BrowserWindow> {
if (this.isStopping()) {
throw new Error('cannot create new windows when the app is stopping.');
}
const electronWindow = new BrowserWindow(options);
this.attachWebContentsNewWindow(electronWindow);
this.attachReadyToShow(electronWindow);
this.attachWebContentsNewWindow(electronWindow);
this.attachSaveWindowState(electronWindow);
this.attachWillPreventUnload(electronWindow);
this.attachGlobalShortcuts(electronWindow);
Expand All @@ -161,35 +126,20 @@ export class ElectronApplication {
return electronWindow;
}

async openWindowWithWorkspace(workspace: string): Promise<BrowserWindow> {
const uri = (await this.createWindowUri()).withFragment(workspace);
const electronWindow = await this.createWindow(this.getBrowserWindowOptions());
electronWindow.loadURL(uri.toString(true));
return electronWindow;
}

/**
* "Gently" close all windows, application will not stop if a `beforeunload` handler returns `false`.
*/
requestStop(): void {
app.quit();
}

isStopping(): boolean {
return typeof this.stoppingTask !== 'undefined';
}

protected async handleMainCommand(params: ExecutionParams, options: MainCommandOptions): Promise<void> {
if (typeof options.file === 'undefined') {
await this.openDefaultWindow();
} else {
await this.openWindowWithWorkspace(realpathSync(path.resolve(params.cwd, options.file)));
}
protected async handleMainCommand(params: ExecutionParams): Promise<void> {
await this.openDefaultWindow();
}

protected async createWindowUri(): Promise<URI> {
return FileUri.create(this.globals.THEIA_FRONTEND_HTML_PATH)
.withQuery(`port=${this.backendPort}`);
const port = await this.backendPort.promise;
return FileUri.create(this.globals.THEIA_FRONTEND_HTML_PATH).withQuery(`port=${port}`);
}

protected getBrowserWindowOptions(): BrowserWindowConstructorOptions {
Expand All @@ -200,7 +150,7 @@ export class ElectronApplication {
return {
...windowState,
show: false,
title: this.globals.THEIA_APPLICATION_NAME,
title: this.config.applicationName,
minWidth: 200,
minHeight: 120,
};
Expand Down Expand Up @@ -297,7 +247,7 @@ export class ElectronApplication {
* Catch certain keybindings to prevent reloading the window using keyboard shortcuts.
*/
protected attachGlobalShortcuts(electronWindow: BrowserWindow): void {
if (FrontendApplicationConfigProvider.get().electron?.disallowReloadKeybinding) {
if (this.config.electron?.disallowReloadKeybinding) {
const accelerators = ['CmdOrCtrl+R', 'F5'];
electronWindow.on('focus', () => {
for (const accelerator of accelerators) {
Expand Down Expand Up @@ -336,18 +286,20 @@ export class ElectronApplication {
const address: AddressInfo = await require(this.globals.THEIA_BACKEND_MAIN_PATH);
return address.port;
} else {
const backendProcess = this.backendProcess = fork(this.globals.THEIA_BACKEND_MAIN_PATH, backendArgv, await this.getForkOptions());
const backendProcess = fork(this.globals.THEIA_BACKEND_MAIN_PATH, backendArgv, await this.getForkOptions());
return new Promise((resolve, reject) => {
const timeout = setTimeout(console.warn, 5000, 'backend is taking a long time to start, something could be wrong?');
// The backend server main file is also supposed to send the resolved http(s) server port via IPC.
backendProcess.on('message', (address: AddressInfo) => {
clearTimeout(timeout);
resolve(address.port);
});
backendProcess.on('error', error => {
clearTimeout(timeout);
reject(error);
});
app.on('quit', () => {
// If we forked the process for the clusters, we need to manually terminate it.
// See: https://github.com/eclipse-theia/theia/issues/835
process.kill(backendProcess.pid);
});
});
}
}
Expand All @@ -356,7 +308,6 @@ export class ElectronApplication {
return {
env: {
...process.env,
ELECTRON_RUN_AS_NODE: 1,
[ElectronSecurityToken]: JSON.stringify(this.electronSecurityToken),
},
};
Expand All @@ -376,25 +327,15 @@ export class ElectronApplication {
protected hookApplicationEvents(): void {
app.on('will-quit', this.onWillQuit.bind(this));
app.on('second-instance', this.onSecondInstance.bind(this));
app.on('window-all-closed', () => this.requestStop.bind(this));
}

protected onWillQuit(event: ElectronEvent): void {
if (typeof this.stoppingTask === 'undefined') {
event.preventDefault();
this.stoppingTask = new SyncPromise(this.stop());
// Once the stopping process is done, we should quit for real:
this.stoppingTask.promise.then(
() => app.quit(),
() => app.quit(),
);
} else if (!this.stoppingTask.finished) {
event.preventDefault();
}
// `stoppingTask.finished === true`: let the process quit.
this.stopContributions();
}

protected async onSecondInstance(event: ElectronEvent, argv: string[], cwd: string): Promise<void> {
await this.launch({ argv, cwd, secondInstance: true });
Copy link
Member Author

@paul-marechal paul-marechal Jun 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Why remove cwd???

My approach was to pass everything Electron gives us because here's how the second instance API works:

  1. You can do cd some/place && app . and it will open /some/place as a workspace because the app knows what's the cwd when you do app .
  2. Now the app is already running, and you open a new terminal and do cd somewhere/else && app .. Here it will trigger the second-instance mechanism and forward the cwd used to invoke the second instance to the already running process. If we don't have the new cwd we cannot resolve app . to the correct location.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove cwd???

Work in progress; otherwise, I could not start the app without a workspace error on macOS.

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, can you please fix the macOS behavior on your branch, and I'll just rebase this PR from yours and get back the cwd support. What you wrote 👆 sounds good to me, a very nice addition, but it does not work on macOS 😕 We must verify the bundled app behavior too.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • You can do cd some/place && app . and it will open /some/place as a workspace because the app knows what's the cwd when you do app .
  • Now the app is already running, and you open a new terminal and do cd somewhere/else && app .. Here it will trigger the second-instance mechanism and forward the cwd used to invoke the second instance to the already running process. If we don't have the new cwd we cannot resolve app . to the correct location.

@marechal-p, VS Code handles it differently, right?

cat `which code`
#!/usr/bin/env bash
#
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License. See License.txt in the project root for license information.

function realpath() { python -c "import os,sys; print(os.path.realpath(sys.argv[1]))" "$0"; }
CONTENTS="$(dirname "$(dirname "$(dirname "$(dirname "$(realpath "$0")")")")")"
ELECTRON="$CONTENTS/MacOS/Electron"
CLI="$CONTENTS/Resources/app/out/cli.js"
ELECTRON_RUN_AS_NODE=1 "$ELECTRON" "$CLI" "$@"
exit $?

So the approach you have implemented does not work on macOS; when I start the app from a terminal in dev mode with yarn, or I launch the electron backend launch config from VS Code, without specifying any file my options will fall back to the electron process: "/Users/akos.kitta/Desktop/theia/node_modules/electron/dist/Electron.app/Contents/MacOS/Electron" and this cannot be opened for sure.

Is there a chance to support this new feature once it works in a follow-up PR?

CC: @akosyakov

protected async onSecondInstance(event: ElectronEvent, argv: string[]): Promise<void> {
await this.launch({ argv, secondInstance: true });
}

protected async startContributions(): Promise<void> {
Expand All @@ -407,23 +348,12 @@ export class ElectronApplication {
await Promise.all(promises);
}

protected async stopContributions(): Promise<void> {
const promises = [];
protected stopContributions(): void {
for (const contribution of this.electronApplicationContributions.getContributions()) {
if (contribution.onStop) {
promises.push(contribution.onStop(this));
contribution.onStop(this);
}
}
await Promise.all(promises);
}

protected async stop(): Promise<void> {
try {
await this.stopContributions();
} catch (error) {
console.error(error);
process.exitCode = error && error.code || 1;
}
}

}
8 changes: 7 additions & 1 deletion packages/output/src/common/output-uri.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,13 @@ export namespace OutputUri {
}

export function create(name: string): URI {
return new URI(name).withScheme(SCHEME);
if (!name) {
throw new Error("'name' must be defined.");
}
if (!name.trim().length) {
throw new Error("'name' must contain at least one non-whitespace character.");
}
return new URI(encodeURIComponent(name)).withScheme(SCHEME);
}

export function channelName(uri: string | URI): string {
Expand Down