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

[Proposal] Improve plugin icons #221

Closed
wants to merge 25 commits into from
Closed

[Proposal] Improve plugin icons #221

wants to merge 25 commits into from

Conversation

vitaliy-guliy
Copy link
Contributor

@vitaliy-guliy vitaliy-guliy commented Sep 10, 2019

What does this PR do?

Sets proper icons for each plugin

Screenshot from 2019-09-10 15-37-55

Screenshot from 2019-09-10 15-38-24

The PR has to be finished, it's only first steps to demonstrate how it may look.
To test how it works, just add following JSON file as a custom plugin registry to your Che-Theia

https://raw.githubusercontent.com/vitaliy-guliy/che-plugin-registry/master/v3/plugins/plugins.json

Currently PNG images are not allowed to be used in the registry. We need to something with that because almost all the VS Code extensions use this format for icons.

Issue to allow PNG images eclipse-che/che#14502
Issue for this improvement eclipse-che/che#14510

@vitaliy-guliy
Copy link
Contributor Author

@benoitf we don't include the images in our repository as we don't include the third party .vsix binaries. Do we need to take care about that?

@benoitf
Copy link
Contributor

benoitf commented Sep 10, 2019

@vitaliy-guliy well images are hosted on your github account. We still to know licensing to do that.

@vitaliy-guliy
Copy link
Contributor Author

@benoitf yep. I forgot to mention that in the PR description.
https://raw.githubusercontent.com/vitaliy-guliy/che-plugin-registry/master/v3/plugins/eclipse-che-logo.png
This logo I found on our sites and have been reworked it a bit.
I think we need to upload it somewhere to be public.

Let's say. I will create a dedicated PR wit this one image and place it in v3/plugins directory
https://github.com/eclipse/che-plugin-registry/tree/master/v3/plugins

After updating https://che-plugin-registry.openshift.io/ this image will be accessible by URI
https://che-plugin-registry.openshift.io/v3/plugins/eclipse-che-logo.png

And only then I will update the links to this image and update this PR. Is it ok?

@mmorhun
Copy link
Contributor

mmorhun commented Sep 11, 2019

@vitaliy-guliy I would like to have at least separate folder for images.

@vitaliy-guliy
Copy link
Contributor Author

vitaliy-guliy commented Sep 11, 2019

@benoitf @mmorhun
v3/plugins for plugins
v3/images for images

v3/images will contain only one image (at least for now)

@amisevsk
Copy link
Contributor

After updating https://che-plugin-registry.openshift.io/ this image will be accessible by URI
https://che-plugin-registry.openshift.io/v3/plugins/eclipse-che-logo.png

We should not depend on che-plugin-registry.openshift.io since the primary priority there is supporting hosted Che. For now, no updates will be rolled out there until the next release.

In the future, new Che deploys will depend on deploying an instance of the registries, since we cannot guarantee version compatibility between the hosted registry and the version of Che you're installing.

@amisevsk
Copy link
Contributor

amisevsk commented Sep 11, 2019

The official Che logo should be hosted somewhere on eclipse.org, IMO.

Edit: there is also https://github.com/eclipse/che/blob/master/dashboard/src/assets/branding/loader.svg

@benoitf
Copy link
Contributor

benoitf commented Sep 11, 2019

also in case of private networks/ airgap we may not have access to external URLs so all resources should be part of the registry. (che-plugin-registry.openshift.io or eclipse.org may not be reachable at all)

@vitaliy-guliy
Copy link
Contributor Author

I don't see any problem if the plugin metadata refers the icon on the same host.

In our case file

https://che-plugin-registry.openshift.io/v3/plugins/eclipse/che-machine-exec-plugin/latest/meta.yaml

could refer on

https://che-plugin-registry.openshift.io/v3/images/eclipse-che-logo.png

The official Che logo should be hosted somewhere on eclipse.org, IMO.

I don't mind. But also I don't see any issues to host the image on che-plugin-registry.openshift.io

@vitaliy-guliy
Copy link
Contributor Author

also in case of private networks/ airgap we may not have access to external URLs so all resources should be part of the registry. (che-plugin-registry.openshift.io or eclipse.org may not be reachable at all)

In that case the plugin management will not work at all and you will not be able even to start the worksace.

If you want to run Che inside your private network, you need to rework you plugin registry because each metafile describing VS Code / Theia extensions already refers on external .vsix / .theia

@benoitf
Copy link
Contributor

benoitf commented Sep 11, 2019

@vitaliy-guliy yes in that case vsix files are stored in the registry as well

@amisevsk
Copy link
Contributor

also in case of private networks/ airgap we may not have access to external URLs so all resources should be part of the registry. (che-plugin-registry.openshift.io or eclipse.org may not be reachable at all)

This is still not really possible to do; hosting the icon in the registry requires knowing the URL of the registry before building. I'm considering adding this to #211 but it's not clear that there's a good way to do this without changing the deploy process significantly.

@amisevsk
Copy link
Contributor

amisevsk commented Sep 11, 2019

I don't see any problem if the plugin metadata refers the icon on the same host.

Most deploys of Che should not refer to che.openshift.io at all. We can use your patch for hosted Che but any other deployment of Che will come with its own registries; these icons would no longer be on the same host.

The reason this is an issue is eclipse-che/che#14261, eclipse-che/che#14266, and eclipse-che/che#14267

@vitaliy-guliy
Copy link
Contributor Author

@benoitf then it's not a problem. You may use default icon for all the plugins. If this is a private network, that could be normal.
But anyway you have to rework/fix all the meta files in the plugin registry in that case.

@vitaliy-guliy
Copy link
Contributor Author

@amisevsk we are talking about default icon or for the icons for VS Code extensions?

@vitaliy-guliy
Copy link
Contributor Author

I still don't understand why it's an issue to refer on the VS Code extension icon having a direct reference on the VS Code binary.

@amisevsk
Copy link
Contributor

amisevsk commented Sep 11, 2019

@vitaliy-guliy Sorry the conversation is getting a little muddled. I'm referring to

https://raw.githubusercontent.com/vitaliy-guliy/che-plugin-registry/master/v3/plugins/eclipse-che-logo.png
This logo I found on our sites and have been reworked it a bit.
I think we need to upload it somewhere to be public.

Let's say. I will create a dedicated PR wit this one image and place it in v3/plugins directory
https://github.com/eclipse/che-plugin-registry/tree/master/v3/plugins

After updating https://che-plugin-registry.openshift.io/ this image will be accessible by URI
https://che-plugin-registry.openshift.io/v3/plugins/eclipse-che-logo.png

And only then I will update the links to this image and update this PR. Is it ok?

The logo cannot be hosted on https://che-plugin-registry.openshift.io, since this repo should not depend on that registry being in a certain state. If we want to host the new logo .png, it should be pushed to either https://github.com/eclipse/che/tree/master/dashboard/src/assets/branding or the eclipse.org site.

I have no issue with referring to icons in other repositories for plugins.

@vitaliy-guliy
Copy link
Contributor Author

@amisevsk
I see two options here.

The first one

The logo cannot be hosted on https://che-plugin-registry.openshift.io, since this repo should not depend on that registry being in a certain state

There could be a relative reference. It will solve problem with referring on https://che-plugin-registry.openshift.io
Che-Theia knows the direct URI to meta.yaml. It can find the image by relative path.

The second one

it should be pushed to either https://github.com/eclipse/che/tree/master/dashboard/src/assets/branding

In that case what should be in meta.yaml and how we can calculate the icon URI?
Che-Theia is running in container and knows nothing about dashboard.

@amisevsk
Copy link
Contributor

There could be a relative reference. It will solve problem with referring on https://che-plugin-registry.openshift.io
Che-Theia knows the direct URI to meta.yaml. It can find the image by relative path.

If Theia can handle relative references, then that would work. It would make implementing icon caching for #211 a lot easier as well

In that case what should be in meta.yaml and how we can calculate the icon URI?
Che-Theia is running in container and knows nothing about dashboard.

In this case, the meta.yaml would just refer to the raw file on github: e.g. https://raw.githubusercontent.com/eclipse/che/master/dashboard/src/assets/branding/loader.svg

Che branding and icons should definitely be hosted somewhere in general.

@vitaliy-guliy
Copy link
Contributor Author

If Theia can handle relative references, then that would work.

We can implement that. It's not difficult.

It would make implementing icon caching for #211 a lot easier as well

Icons are loaded by the browser. I believe it will cache everything.

In this case, the meta.yaml would just refer to the raw file on github: e.g.
https://raw.githubusercontent.com/eclipse/che/master/dashboard/src/assets/branding/loader.svg
Che branding and icons should definitely be hosted somewhere in general.

Well. Then let's choose from options

@amisevsk
Copy link
Contributor

amisevsk commented Sep 12, 2019

I'm +1 on implementing relative paths in Theia regardless of what we do. I think in an ideal case the Che logo is available on some static site but have no problem storing it in every registry.

@nickboldt
Copy link
Contributor

nickboldt commented Sep 26, 2019

For airgap productization we need icons cached in the plugin registry. +1 to cache images inside the plugin registry, perhaps as the default behaviour too.

I think the simplest / most logical approach is:

  • download the vsix (as in Add option to cache .vsix and .theia artifacts during build #211)
  • unpack vsix: look for .png or .svg file, if found use that icon file - no need to cache anything else in the plugin registry container
  • if no vsix or no image found in the vsix archive, pull the referenced image in the meta.yaml while building the plugin reg container
  • it none found or specified, fall back to generic/default image (which should also be in the plugin registry if not present in Theia itself)

If PR #211 gets merged we can reuse /resources/ folder for icons. If not and there's a compelling reason to keep 'em separated (to borrow a line from The Offspring) ... then that works too.

Re: vsix files already containing an icon, note that they may be in a different location in the archive (root folder, extension/ folder) ...

image

But for cases where there's no visx file... like https://github.com/eclipse/che-plugin-registry/blob/master/v3/plugins/eclipse/che-machine-exec-plugin/latest/meta.yaml ... then we need to fall back to the external reference (fetched while building the container, as noted above)

@nickboldt
Copy link
Contributor

what are copyrights on the images ?

They'd be covered by the same extension/LICENSE.txt in the .vsix archive. Eg., for ms-python, (c) Microsoft, and MIT License.

For fabric8-analytics, Apache 2.0 License.

image

So if it's safe to include the vsix in an offline/airgap plugin registry, the image is safe too.

@nickboldt
Copy link
Contributor

Is this still in progress? Needs rebasing and a correctly signed ECA on the commit if so.

@nickboldt
Copy link
Contributor

This is so old it still contains /latest/meta.yaml (from before Che 7.3) files so I'm going to close it as won't fix. Feel free to rebase and reopen if it's still something we want to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants