[Electron] Use absolute path for plugins running in Electron. #7202

Johannes-B-stock · 2020-02-24T11:15:48Z

Signed-off-by: Johannes Birkenstock johannes.birkenstock@siemens.com

What it does

Fixes #7040

It fixes this bug by using absolute paths for plugins that are running in Electron.

How to test

Run any vscode extension that uses its own images in the theia electron app.

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

packages/plugin-ext/src/common/plugin-protocol.ts

vince-fugnitto

Thank you for your contribution @Johannes-B-stock!
Please be sure to sign the Eclipse ECA so we can accept your changes.

packages/plugin-ext/src/common/plugin-protocol.ts

Livven · 2020-03-04T17:02:22Z

We added a fix for sidebar icons (viewsContainers).

Regarding the ECA: I already created a private Eclipse account previously. Is there any way to add my work email to that or should I create a new work Eclipse account in order to sign the ECA?

vince-fugnitto · 2020-03-04T17:04:23Z

We added a fix for sidebar icons (viewsContainers).

Regarding the ECA: I already created a private Eclipse account previously. Is there any way to add my work email to that or should I create a new work Eclipse account in order to sign the ECA?

Would it be possible to squash the pull-request? (it'd also make it easier to handle the ECA)
You can keep a co-author if you'd like.

For the Eclipse ECA, it requires that your sign-off is identical to an email that is registered in Eclipse.

vince-fugnitto

I confirmed that assets (like the sidebar icon) are correctly loaded in Electron 🍾

Fixes eclipse-theia#7040. Signed-off-by: Johannes Birkenstock <johannes.birkenstock@siemens.com> Signed-off-by: Liwen Guo <liwen.guo@siemens.com>

Livven · 2020-03-04T17:22:38Z

Would it be possible to squash the pull-request? (it'd also make it easier to handle the ECA)
You can keep a co-author if you'd like.

Done.

For the Eclipse ECA, it requires that your sign-off is identical to an email that is registered in Eclipse.

Thanks for the info. Actually noticed it only takes the commit author anyway so since I am only committer now I don't need to sign it. Yay 🎉

vince-fugnitto · 2020-03-11T17:24:00Z

@Livven is the pull-request ready for another round of review?
cc @lmcbout it should fix the icon issues we experienced in Electron.

vince-fugnitto · 2020-03-11T17:37:54Z

I verified and it looks like #6834 is not fixed by the change.
The issue #7040 is successfully fixed 👍

Livven · 2020-03-11T17:46:28Z

@vince-fugnitto Sure, nothing changed since #7202 (review) except that the commits have been squashed.

vince-fugnitto

The pull-request correctly fixed #7040.
@akosyakov did you have any objections with the following changes?

akosyakov · 2020-03-11T19:19:15Z

It looks good to me, wonder whether we still need hostedPlugin endpoint, but it is a matter for another PR.

vince-fugnitto · 2020-03-11T19:23:23Z

It looks good to me, wonder whether we still need hostedPlugin endpoint, but it is a matter for another PR.

Will merge tomorrow if there are no objections.

akosyakov · 2020-03-11T19:32:21Z

Will merge tomorrow if there are no objections.

@vince-fugnitto It would be better if someone from RedHat have a look cc @benoitf @azatsarynnyy
The issue is that PackagePath.toUrl used to create a URI which can be used to fetch content, it is not impossible, so it breaks any client which rely on it. Technically it is still URI, but not external.

Another thing it can be that some products expect that hostedPlugin endpoint is called to fetch images and look up them from somewhere else. It would break them as well. I wonder why we could not keep using hostedPlugin endpoint?

akosyakov · 2020-03-11T19:34:16Z

packages/plugin-ext/src/common/plugin-protocol.ts

@@ -58,7 +60,7 @@ export interface PluginPackage {
 }
 export namespace PluginPackage {
    export function toPluginUrl(pck: PluginPackage, relativePath: string): string {
-        return `hostedPlugin/${getPluginId(pck)}/${encodeURIComponent(relativePath)}`;
+        return URI.file(pck.packagePath + '/' + relativePath).toString();


could we do here instead file?${URI.file(pck.packagePath + '/' + relativePath).toString()} to avoid breaking all clients

That's actually a minimal not breaking change.

Actually, i think we should revert changes and only update clients to something like:

new Endpoint({ path: iconUrl }).getRestUrl().toString()

It won't break anyone and fix clients in this repo.

akosyakov · 2020-03-12T07:53:02Z

dev-packages/application-package/src/environment.ts

-        // Se we look into the `process.env` as well. `is-electron` does not do it for us.
-        return isElectron() || typeof process !== 'undefined' && typeof process.env === 'object' && !!process.env.THEIA_ELECTRON_VERSION;
+        // So we look into the `process.env` as well. `is-electron` does not do it for us.
+        return isElectron() || typeof process !== 'undefined' && typeof process.env === 'object' && (!!process.env.THEIA_ELECTRON_VERSION || !!process.env.ELECTRON_RUN_AS_NODE);


how this change related to a PR?

It was used for the previous fix where toPluginUrl would return a different value depending on whether it was running in Electron or not. ELECTRON_RUN_AS_NODE covers the VS Code extension process.

So it's not necessary anymore, but it might still be an improvement (or break other things, or both, or neither). I'd keep the typo fix at least though ;)

akosyakov

just making sure that it does not get merged without doing #7202 (comment) to avoid breaking changes

owlJaeger · 2020-04-13T23:19:28Z

I am looking for a fix for some issues fixed by this pull while sticking with /hostedPlugin. The issue with electron is that you can't just leave it how it is now with returning just /hostedPlugin/... because the window doesn't point to localhost:port but instead to a file. The solution would be to not return /hostedPlugin/... but to prepend http://localhost:port and return http://localhost:port/hostedPlugin/.... I am currently trying to achieve this in a elegant way but everything I've tried had issues or was a bit hacky... In the end I think it boils down to not being able to access the current url at any given time anywhere in the program... Is there a elegant way to access the electron window url and the browser url at the same time? I've also tried to set a process.env variable but with partial success. Sometimes the variable was just empty... The backend-application.ts logs a message that contains all the information needed. But the port is not stored and thus not accessible... Do you think writing it to a temporary file would be a good idea? The electron and browser urls contain every information we should need.

akosyakov · 2020-04-14T07:23:19Z

The minimal solution is #7202 (comment). Someone has to update this PR or send a new PR.

owlJaeger · 2020-04-14T10:55:43Z

I'll test it out. There are some other things needing a small fix but I've noted them and will test everything. The last time I've created an endpoint inside plugin-protocol.ts it failed because window.location was not accessible and thus the browser Version was then broken. I'll test it with a clean new repository and if it fails again I'll divide the browser and electron return-statements. If it passes all my tests I'll create a new pull request to get it fixed as quickly as possible. Is there a deadline for which commits will reach the next release?

owlJaeger · 2020-04-15T16:22:33Z

I've created a pull request: #7583 to fix the there described issues. The PR is continuing the hostedPlugin use. Was a bad idea to try and use the Endpoint inside plugin-protocol.ts. With this pull Icons should be showing fine.

akosyakov · 2020-04-20T09:24:42Z

Closing in favor of #7583

kittaakos reviewed Feb 24, 2020

View reviewed changes