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

Fix plugin icon URLs in electron & Fix custom icon themes #7583

Merged
merged 1 commit into from
Apr 23, 2020

Conversation

owlJaeger
Copy link
Contributor

@owlJaeger owlJaeger commented Apr 15, 2020

Signed-off-by: Luca Jaeger owl.jaeger@gmail.com

What it does

Fixes Issues:

#6834 Fixed by:

  • plugin-protocol.ts:
    • Replace PluginModel.packagePath with a backend-created FileUri passed on as PluginModel.packageUri instead of PluginModel.packagePath
  • scanner-vscode.ts & scanner-theia.ts:
    • Pass on a backend-generated FileUri through the PluginModel.packageUri property.
  • plugin-api-rpc.ts:
    • Updated the empty Model to include packageUri
  • plugin-icon-theme-service.ts:
    • Updated to use packageUri instead of packagePath and fix the relativePath being undefined on Windows.
    • Using Endpoints to fix it for electron specifically.

#7040 Fixed by:

How to test

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added electron issues related to the electron target vscode issues related to VSCode compatibility labels Apr 15, 2020
@vince-fugnitto
Copy link
Member

Please be sure to properly sign the Eclipse ECA, I see that your sign-off email is not yet registered:

Screen Shot 2020-04-15 at 12 40 06 PM

@owlJaeger owlJaeger force-pushed the master branch 2 times, most recently from 963679b to 7bd9d28 Compare April 15, 2020 16:50
@owlJaeger
Copy link
Contributor Author

Should be registered now

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I verified the functionality present on electron and it works very well:

  • I am able to successfully update/render file-icon themes.
  • I am able to successfully view icons provided by plugins (such as gitlens)
Screen Shot 2020-04-15 at 3 13 23 PM

I also tested the browser to see if there were any regressions.

Additional Info

  • macOS

packages/core/src/common/uri.ts Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@owlJaeger
Copy link
Contributor Author

Do these changes meet your expectations @akosyakov?

@akosyakov
Copy link
Member

@owlJaeger it looks very promising, i'm going to test it.

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

It looks good code-wise to me and I've tested that icons are available for themes, commands, trees and view containers in browser/electron targets on Linux. I have not tested on Windows.

I think we should mark PluginModel.packagePath as deprecated and tell to use packageUri instead. Clean up can be done later.

@owlJaeger
Copy link
Contributor Author

Shall I update the Changelog for that? I see there are more deprecation warnings under the breaking changes section.

@akosyakov
Copy link
Member

Shall I update the Changelog for that? I see there are more deprecation warnings under the breaking changes section.

Yes, you can mention it. Don't delete code yet, only deprecate.

@akosyakov
Copy link
Member

@kittaakos Would you be able to check whether on Windows you have icons for electron?

@owlJaeger owlJaeger force-pushed the master branch 2 times, most recently from 7ea0382 to b6b009c Compare April 20, 2020 09:51
@owlJaeger
Copy link
Contributor Author

owlJaeger commented Apr 20, 2020

Updated to include the deprecation warning and rebased to include newest commits from theia/master. Now with the JS-Doc tag.

Signed-off-by: Luca Jaeger <owl.jaeger@gmail.com>
@akosyakov
Copy link
Member

merging tomorrow if no objections?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
electron issues related to the electron target vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants