electron: use inversify in main process #7964

paul-marechal · 2020-06-04T15:03:15Z

Closes #3364
Closes #7406
Closes #7964

Combined work with @kittaakos.

What it does

This PR contributes a JSON-RPC communication scheme between the electron-main process and the renderer processes. It is reusing the same architecture as the websocket-based connection between frontend/backend, but adapted to work over electron's ipc apis. Currently there isn't scoped connections, meaning that each renderer process will interact with the same singleton in the electron-main process, but for the current use-cases this is sufficient.

How to test

To verify:

yarn --cwd ./examples/electron start -> Should open previous workspace, if any. Otherwise, opens a no-workspace app.
yarn --cwd ./examples/electron start ~/Desktop/ -> Opens your app with a workspace to the Desktop.
yarn --cwd ./examples/electron start ../.. -> Opens the app to with Theia as the workspace.
yarn --cwd ./examples/electron start path/to/missing -> Opens the app without a workspace. See log: Could not resolve the workspace path.
mkdir ~/Desktop/path\ with\ space && yarn --cwd ./examples/electron start ~/Desktop/path\ with\ space -> Open the app with the desired workspace. The workspace contains spaces in the FS path.

Check updater sample
Check File > Settings > Check for Update... -> You will see, there are no updates.
Execute the Electron Updater Sample: Mock - Available command and wait a bit.
You should receive a notification, about new updates. Click on yes, the electron-main process should print something to the backend console.
Refresh the browser window, you should see Disposed a sample updater client. AND Registered a new sample updater client.
Open a second workspace, you should see Registered a new sample updater client.. Close the new window, you should see Disposed a sample updater client.

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

paul-marechal · 2020-06-04T15:08:15Z

@akosyakov here is Electron to use inversify.

This commit adds the bare minimum required to run Electron, as well as a communication scheme between frontend and electron main process (via IpcMain and IpcRenderer Electron APIs).

No communication is happening between electron main process and the backend process at the moment. My concern in that regard is that the backend heavily pollutes its stdout/stderr, so I am not really sure it would be possible to use those streams for IPC. But I also don't see a use for such a communication right now?

packages/electron/src/electron-main/delete.electron-cli.ts

packages/electron/src/electron-main/delete.tmp.js

paul-marechal · 2020-06-09T18:10:57Z

Are we waiting for #7968 before considering this PR?

kittaakos · 2020-06-11T14:38:19Z

Are we waiting for #7968 before considering this PR?

Yes, please. Of course, if you have a very good reason, we can start with this, but first, it should be marked as ready for the review.

As for the verification, I want to extend the APIs you have created and want to add support for the electron-updater, if that is feasible, we can be sure the electron-main customization is sufficient in Theia. Thoughts?

paul-marechal · 2020-06-11T14:57:24Z

Of course, if you have a very good reason, we can start with this, but first, it should be marked as ready for the review.

I just want either you or Anton to go over the new contributions points and messaging utilities to make sure the design is ok. I didn't want to get crazy with a design that would be refused. If the current approach is fine with you, I can clean the PR and mark it as ready for final review.

As for the verification, I want to extend the APIs you have created and want to add support for the electron-updater, if that is feasible, we can be sure the electron-main customization is sufficient in Theia.

I am sorry, I don't understand what you mean. Are you asking if you will be able to add support for electron-updater using the new APIs? Or are you asking to add such support in this PR?

kittaakos · 2020-06-11T15:04:33Z

I am sorry, I don't understand what you mean. Are you asking if you will be able to add support for electron-updater using the new APIs?

Yes.

Or are you asking to add such support in this PR?

No no, it's out of context.

paul-marechal · 2020-06-11T15:05:27Z

Hm, I am not familiar with what using electron-updater implies to do, so I cannot tell. Let me look.

paul-marechal · 2020-06-11T15:07:24Z

@kittaakos I don't see anything that would prevent it, as it seems it doesn't require changes to the Electron main process logic to work. Correct me if I am wrong.

kittaakos · 2020-06-11T15:09:14Z

Let me look.

You do not have to, just if you're interested.

Here is an example, I want to see the notifications about new updates, do you want to install and so on in the Theia notification center, and not some native dialogs. So that's why your change is super important. Without injection, it is not possible.

paul-marechal · 2020-06-11T15:16:19Z

Here is an example

From that example, it seems like you will be able to nicely use it after this patch.

kittaakos

I added a few comments. I am going to start with the integration and see how it works. What I definitely miss is the electron application as an argument to the start, stop, and hookApplicationEvents methods. Let me check how well does the API work during the integration.

packages/electron/package.json

packages/electron/src/common/electron-window-protocol.ts

packages/electron/src/electron-main/delete.electron-cli.ts

packages/electron/src/electron-main/delete.tmp.js

packages/electron/src/electron-main/electron-application.ts

dev-packages/application-manager/src/generator/frontend-generator.ts

kittaakos · 2020-06-12T09:58:03Z

When I start the electron app as usual (yarn --cwd ./examples/electron start) I see the following error notification and no workspace is opened:

Edit:

Then I set a workspace,
Stopped the app,
Started the app,
Had the same error notification, no workspace was opened.

kittaakos · 2020-06-12T11:04:36Z

I am experiencing #6499 again. If I refresh the browser window, the onDidCloseConnection is not called.

kittaakos · 2020-06-12T11:19:01Z

@marechal-p, please help me with this. I have tried to add a new extension that contributes to the new electronMain extension-point. I made the following changes, rebuilt everything, but nothing happens when I start the app. What am I doing wrong?

examples/electron-api-samples/package.json:

...
  "dependencies": {
    "@theia/core": "^1.2.0",
    "@theia/electron": "^1.2.0"
  },
  "theiaExtensions": [
    {
      "electronMain": "lib/electron-main/update/electron-updater-module"
    }
  ],
...

examples/electron-api-samples/src/electron-main/update/electron-updater-module.ts:

import { ContainerModule } from 'inversify';
import { ElectronApplicationContribution } from '@theia/electron/lib/electron-main/electron-application';
import { ElectronUpdaterContribution } from './electron-updater-contribution';

export default new ContainerModule(bind => {
    bind(ElectronApplicationContribution).to(ElectronUpdaterContribution).inSingletonScope();
});

examples/electron-api-samples/src/electron-main/update/electron-updater-contribution.ts:

import { injectable } from 'inversify';
import { ElectronApplicationContribution } from '@theia/electron/lib/electron-main/electron-application';

@injectable()
export class ElectronUpdaterContribution implements ElectronApplicationContribution {

    async start(): Promise<void> {
        console.log('start');
    }

    async stop(): Promise<void> {
        console.log('stop');
    }

}

And in the example/electron/package.json, I made this change:

diff --git a/examples/electron/package.json b/examples/electron/package.json
index e2dddb190..405bbf2b1 100644
--- a/examples/electron/package.json
+++ b/examples/electron/package.json
@@ -22,6 +22,7 @@
     "@theia/editor": "^1.2.0",
     "@theia/editor-preview": "^1.2.0",
     "@theia/electron": "^1.2.0",
+    "@theia/electron-api-samples": "^1.2.0",
     "@theia/file-search": "^1.2.0",
     "@theia/filesystem": "^1.2.0",
     "@theia/getting-started": "^1.2.0",

I pushed the code to https://github.com/eclipse-theia/theia/compare/electron-updater-example

paul-marechal · 2020-06-16T21:32:43Z

@kittaakos thank you very much for the review, it's encouraging! I still need to investigate the issues you reported about the connection not being closed, as well as why contributions are not picked up.

kittaakos · 2020-06-17T06:29:35Z

well as why contributions are not picked up.

I am not sure if this is an issue with the changeset, or I misunderstood something.

👍 The PR looks very promising, let me know if you need someone to try it out again.

kittaakos · 2020-07-16T11:24:51Z

I am going to rebase this from the master after #7968.

It's done. I have verified the electron-main contributions and the dummy updater. It was OK.

paul-marechal · 2020-07-21T14:04:30Z

I'm ok with the current changes, let's get this merged right after the release? I'll need someone to approve since I cannot do it on my own PRs :)

kittaakos · 2020-07-21T14:30:51Z

I'm ok with the current changes, let's get this merged right after the release?

I totally agree. Let's schedule another review after 30.7.

I'll need someone to approve since I cannot do it on my own PRs :)

Same for me. 😄

kittaakos · 2020-08-03T11:33:50Z

@marechal-p, do you have time to update this PR from the master? I want to merge it ASAP. If you don't have time, I can make the required changes later today. Thank you!

kittaakos · 2020-08-03T15:37:20Z

I can make the required changes later today.

I am going to take care of the update.

kittaakos · 2020-08-03T16:16:22Z

I did the update:

resolved the version conflicts and
adjusted the changelog.

I gave the app another try on macOS with Node.js v12.14.1:

I verified the lifecycle of the new electronMain contribution: I checked how they behave (onStart and onStop) when the window open/close/refresh happens.
I tried the dummy electron updater sample. It worked.
I also checked the basic TS functionality with the browser example after a successful rebuild:browser command. It worked fine.

I reviewed @marechal-p's contribution several times by drafting the following PRs:

Here is another one:

discussion: electron main fixup #8056

The changes from the PRs as mentioned above had been merged back to here.

Here is another review from Windows env. Thanks a lot for your help, @mcgordonite 👍

Support for electron-main process customization with DI #8125 (review)

I am going to leave this PR open for another few days then I'll merge it without further notice. Let me know if anyone has any concerns.

paul-marechal · 2020-08-04T19:01:15Z

Just tested on Windows that we can launch the Electron app and open new windows. There was a bug where the maximized state was not applied back, I just pushed the fix for it.

kittaakos · 2020-08-05T12:49:30Z

There was a bug where the maximized state was not applied back, I just pushed the fix for it.

Thanks for the heads-up and the fix, @marechal-p. I did not know we planned to change it. What about these:

theia/dev-packages/application-manager/src/generator/frontend-generator.ts

Lines 192 to 197 in 6086b28

    
                   const persistedWindowOptionsAdditions = electronStore.get('windowOptions', {}); 
        
                   const windowOptionsAdditions = { 
        
                       ...defaultWindowOptionsAdditions, 
        
                       ...persistedWindowOptionsAdditions 
        
                   };

theia/dev-packages/application-manager/src/generator/frontend-generator.ts

Line 209 in 6086b28

...windowOptionsAdditions,

I could not find the corresponding logic in the new code. Did we just lose the Added ability to change windowOptions & it's defaults feature?

paul-marechal · 2020-08-05T13:13:14Z

@kittaakos the mechanism for changing default options is simply different now: One has to rebind the ElectronMainApplication component and override the getDefaultBrowserWindowOptions method. cc @owlJaeger

(It was also one of the reason why this refactoring was needed: the old mechanism was waiting for the frontend to send a message back to the electron main process in order to drive it, now you can directly add the logic within)

paul-marechal · 2020-08-05T13:21:23Z

@kittaakos at the very least I added back the window options coming from the package.json configuration field. When I first started the PR a lot of patches were not merged so these things must have slept through, my bad.

kittaakos · 2020-08-05T13:26:01Z

@kittaakos the mechanism for changing default options is simply different now: One has to rebind the ElectronMainApplication component and override the getDefaultBrowserWindowOptions method. cc @owlJaeger

We cannot break existing behavior. It was not even mentioned anywhere.

ElectronFrontendApplicationConfig#windowOptions still an existing prop and ignored.

is simply different now

No. It is not simply different. A downstream Theia consumer could define window properties at build time in the package.json. With this change, if one wants to ship two apps, two extensions are required. With this PR the windows config becomes a runtime thing.

Could you please make this PR compliment with the master? Let's change one thing in the PR: support injection in the electron-main. Do you have time to do it, or should I do the revert?

paul-marechal · 2020-08-05T13:35:19Z

A downstream Theia consumer could define window properties at build time in the package.json.

This has been brought back.

theia/dev-packages/application-manager/src/generator/frontend-generator.ts

Lines 309 to 314 in 6086b28

    
               ipcMain.on('set-window-options', (event, options) => { 
        
                   electronStore.set('windowOptions', options); 
        
               }); 
        
               ipcMain.on('get-persisted-window-options-additions', event => { 
        
                   event.returnValue = electronStore.get('windowOptions', {}); 
        
               });

@kittaakos regarding the workaround IPC code, is it ok in this case to make this breaking change? When I mentioned getting rid of it to @owlJaeger that made the PR to introduce it he seemed favorable to the idea. I think that it would be better to get rid of workarounds.

kittaakos · 2020-08-05T13:55:25Z

is it ok

I do not mind, but you have to ask the community.

paul-marechal · 2020-08-05T13:58:54Z

Mentions of the IPC events being temporary:

Added ability to change windowOptions & it's defaults. Moved disallowReloadKeybinding inside electron subsection #7803 (comment)
Added ability to change windowOptions & it's defaults. Moved disallowReloadKeybinding inside electron subsection #7803 (comment)

Going through with this change then.

owlJaeger · 2020-08-05T14:31:42Z

regarding the workaround IPC code, is it ok in this case to make this breaking change?

I do not mind it breaking the currently used workarounds. I am looking forward to switching to the extensible solution by rebinding ElectronMainApplication as it should yield all the needed functionality the workarounds were providing. 👍

kittaakos · 2020-08-06T09:54:30Z

We have a conflict again. I am going to make another rebase and merge it tomorrow.

Update: it's done.

All the logic for the electron main process currently has to be added to our generators, making it hard to extend without committing to Theia. This commit re-arranges the way Electron is launched to allow people to more easily change the behavior of their application. Add a basic CLI to open a workspace by doing `app path/to/workspace`. CLI can be overriden by application makers by extending and rebinding `ElectronApplication.launch` and handling yourself the `ExecutionParams`. Added a dummy electron-updater sample. Breaking-Change: Removed the `set-window-options` and `get-persisted-window-options-additions` Electron IPC handlers from the Electron Main process. Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com> Co-Authored-By: Akos Kitta <kittaakos@typefox.io>

kittaakos

Thank you for all of you helping with this PR. Special thanks to @marechal-p for initiating this change.

If you discover any regressions, please file a bug and name me. Thanks!

paul-marechal requested a review from akosyakov June 4, 2020 15:03

paul-marechal force-pushed the mp/electron-main branch from 26ba1dd to 90007cd Compare June 4, 2020 15:03

paul-marechal added electron issues related to the electron target extensibility issues to simplify ability to extend Theia extension system issues related to the extension system labels Jun 4, 2020

paul-marechal commented Jun 4, 2020

View reviewed changes

packages/electron/src/electron-main/delete.electron-cli.ts Outdated Show resolved Hide resolved

packages/electron/src/electron-main/delete.tmp.js Outdated Show resolved Hide resolved

kittaakos self-requested a review June 4, 2020 16:09

kittaakos reviewed Jun 12, 2020

View reviewed changes

packages/electron/src/electron-main/electron-application.ts Outdated Show resolved Hide resolved

kittaakos reviewed Jun 12, 2020

View reviewed changes

packages/electron/src/electron-main/electron-application.ts Outdated Show resolved Hide resolved

kittaakos reviewed Jun 12, 2020

View reviewed changes

dev-packages/application-manager/src/generator/frontend-generator.ts Show resolved Hide resolved

kittaakos reviewed Jun 12, 2020

View reviewed changes

dev-packages/application-manager/src/generator/frontend-generator.ts Outdated Show resolved Hide resolved

paul-marechal force-pushed the mp/electron-main branch 3 times, most recently from b3a027c to d4ebff3 Compare June 16, 2020 19:54

paul-marechal force-pushed the mp/electron-main branch from d4ebff3 to c796cc7 Compare June 16, 2020 21:36

kittaakos mentioned this pull request Jul 20, 2020

Electron security considerations #2018

Closed

kittaakos force-pushed the mp/electron-main branch from 24c8747 to d8a2a2f Compare August 3, 2020 15:52

kittaakos force-pushed the mp/electron-main branch from d8a2a2f to a3fb16d Compare August 3, 2020 16:18

paul-marechal force-pushed the mp/electron-main branch from a3fb16d to 3b700e9 Compare August 4, 2020 18:59

paul-marechal force-pushed the mp/electron-main branch from 3b700e9 to eb1cc99 Compare August 5, 2020 13:20

paul-marechal force-pushed the mp/electron-main branch from eb1cc99 to 79c3ad8 Compare August 5, 2020 13:58

kittaakos force-pushed the mp/electron-main branch from 79c3ad8 to 99d88d2 Compare August 6, 2020 10:05

kittaakos self-requested a review August 7, 2020 13:35

kittaakos approved these changes Aug 7, 2020

View reviewed changes

kittaakos merged commit e95311d into master Aug 7, 2020

paul-marechal deleted the mp/electron-main branch August 7, 2020 15:24

kittaakos mentioned this pull request Nov 3, 2020

Regression when setting the app name for electron #8701

Open

vince-fugnitto mentioned this pull request Jan 21, 2021

Improve documentation for the theia extensions of package.json #8976

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

electron: use inversify in main process #7964

electron: use inversify in main process #7964

paul-marechal commented Jun 4, 2020 •

edited by kittaakos

Loading

paul-marechal commented Jun 4, 2020

paul-marechal commented Jun 9, 2020

kittaakos commented Jun 11, 2020

paul-marechal commented Jun 11, 2020 •

edited

Loading

kittaakos commented Jun 11, 2020

paul-marechal commented Jun 11, 2020 •

edited

Loading

paul-marechal commented Jun 11, 2020 •

edited

Loading

kittaakos commented Jun 11, 2020

paul-marechal commented Jun 11, 2020

kittaakos left a comment

kittaakos commented Jun 12, 2020 •

edited

Loading

kittaakos commented Jun 12, 2020

kittaakos commented Jun 12, 2020 •

edited

Loading

paul-marechal commented Jun 16, 2020

kittaakos commented Jun 17, 2020

kittaakos commented Jul 16, 2020

paul-marechal commented Jul 21, 2020

kittaakos commented Jul 21, 2020

kittaakos commented Aug 3, 2020

kittaakos commented Aug 3, 2020

kittaakos commented Aug 3, 2020

paul-marechal commented Aug 4, 2020

kittaakos commented Aug 5, 2020

paul-marechal commented Aug 5, 2020 •

edited

Loading

paul-marechal commented Aug 5, 2020 •

edited

Loading

kittaakos commented Aug 5, 2020

paul-marechal commented Aug 5, 2020 •

edited

Loading

kittaakos commented Aug 5, 2020

paul-marechal commented Aug 5, 2020

owlJaeger commented Aug 5, 2020 •

edited

Loading

kittaakos commented Aug 6, 2020 •

edited

Loading

kittaakos left a comment

electron: use inversify in main process #7964

electron: use inversify in main process #7964

Conversation

paul-marechal commented Jun 4, 2020 • edited by kittaakos Loading

What it does

How to test

Review checklist

Reminder for reviewers

paul-marechal commented Jun 4, 2020

paul-marechal commented Jun 9, 2020

kittaakos commented Jun 11, 2020

paul-marechal commented Jun 11, 2020 • edited Loading

kittaakos commented Jun 11, 2020

paul-marechal commented Jun 11, 2020 • edited Loading

paul-marechal commented Jun 11, 2020 • edited Loading

kittaakos commented Jun 11, 2020

paul-marechal commented Jun 11, 2020

kittaakos left a comment

Choose a reason for hiding this comment

kittaakos commented Jun 12, 2020 • edited Loading

kittaakos commented Jun 12, 2020

kittaakos commented Jun 12, 2020 • edited Loading

paul-marechal commented Jun 16, 2020

kittaakos commented Jun 17, 2020

kittaakos commented Jul 16, 2020

paul-marechal commented Jul 21, 2020

kittaakos commented Jul 21, 2020

kittaakos commented Aug 3, 2020

kittaakos commented Aug 3, 2020

kittaakos commented Aug 3, 2020

paul-marechal commented Aug 4, 2020

kittaakos commented Aug 5, 2020

paul-marechal commented Aug 5, 2020 • edited Loading

paul-marechal commented Aug 5, 2020 • edited Loading

kittaakos commented Aug 5, 2020

paul-marechal commented Aug 5, 2020 • edited Loading

kittaakos commented Aug 5, 2020

paul-marechal commented Aug 5, 2020

owlJaeger commented Aug 5, 2020 • edited Loading

kittaakos commented Aug 6, 2020 • edited Loading

kittaakos left a comment

Choose a reason for hiding this comment

paul-marechal commented Jun 4, 2020 •

edited by kittaakos

Loading

paul-marechal commented Jun 11, 2020 •

edited

Loading

paul-marechal commented Jun 11, 2020 •

edited

Loading

paul-marechal commented Jun 11, 2020 •

edited

Loading

kittaakos commented Jun 12, 2020 •

edited

Loading

kittaakos commented Jun 12, 2020 •

edited

Loading

paul-marechal commented Aug 5, 2020 •

edited

Loading

paul-marechal commented Aug 5, 2020 •

edited

Loading

paul-marechal commented Aug 5, 2020 •

edited

Loading

owlJaeger commented Aug 5, 2020 •

edited

Loading

kittaakos commented Aug 6, 2020 •

edited

Loading