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

Install vsix files from the explorer view (#13269) #13291

Merged

Conversation

dhuebner
Copy link
Member

What it does

Allows installing VSIX extensions from the file explorer using context menu.

  • Contributes a new command that uses selection service to get the *.vsix file instead of opening a dialog
  • Adds a @theia/navigator dependency to get the navigator menu ID (existing dependencies like @theia/plugin-ext already depends on the navigator package)
  • Refresh 'Installed' tree entries when extension view becomes visible, otherwise one will see updated extension count (badge), but not the new installed entry.
  • Other fixes: Added explicit canSelectFiles: true to dialog options, otherwise one can't select files in the electron app. Could be a problem with default handling in ElectronFileDialogService.toOpenDialogOptions()

How to test

Add a *.vsix to your workspace. Use the context menu to execute Install Extension VSIX action. You should see the 'Successfully installed' message. Go to the "Extensions" view and check the "Installed" list contains the new installed extension.

Follow-ups

When testing extension installation I noticed that when installing an already installed extension a Failed to install error is reported. This is because local file deployer rejects installing existing extensions, but because the installing routine can only check the number of installed extension (in this case it's 0) a misleading error is reported.
I think we need to change the installation routine return value to something more meaningful than just a number. With more information we can then to produce a better user feedback. See PluginDeployerImpl.deployPlugins().

Review checklist

Reminder for reviewers

@dhuebner dhuebner marked this pull request as ready for review January 19, 2024 13:35
Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

Thank you very much @dhuebner! I think this is a really nice contribution and it works well! I do have a few minor comments regarding naming and visibility, I'd be interested to get your take on that before we merge this change.

packages/vsx-registry/package.json Outdated Show resolved Hide resolved

protected override onAfterShow(msg: Message): void {
super.onAfterShow(msg);
if (this.options.id === VSXExtensionsSourceOptions.INSTALLED) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This works when closing and re-opening the view. Unfortunately it seems that we do not have a notification system for when extensions are deployed, at least none that I can see and implementing one is probably out of scope here.

Copy link
Member Author

@dhuebner dhuebner Feb 16, 2024

Choose a reason for hiding this comment

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

we do not have a notification system for when extensions are deployed

Yeah, I also found nothing there.
An other point I mentioned in the description is that, it would be nice to have an easy way to check if an extension is already installed. And what is even more important, to have a detailed return value when attempting to install an extension. Right now it is just a number of extensions being installed and if it is 0 an error is reported.

Copy link
Contributor

Choose a reason for hiding this comment

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

I fully agree, there is definitely room for improvement on the exposure of extension management. I remember there being a discussion somewhere about being notified properly for uninstallations but I can't find it right now. Do you mind logging a Feature Request for the extension of that API?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will do.

@dhuebner dhuebner force-pushed the huebner/install-from-file-13269 branch 2 times, most recently from 2bb8237 to f9deb0c Compare February 16, 2024 08:21
@dhuebner dhuebner force-pushed the huebner/install-from-file-13269 branch from f9deb0c to a433d42 Compare February 16, 2024 08:39
@dhuebner
Copy link
Member Author

@martin-fleck-at
Thank you very much for your review, it was really helpful!
I changed all the related places and think the PR is now even better :)

@dhuebner
Copy link
Member Author

@martin-fleck-at
Are here some remaining changes to be made?
Do you mind to look into my other PR #13344 as well? It is also kind of files related...

Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

Thank you very much for the update! Everything looks good to me now!

@martin-fleck-at martin-fleck-at merged commit f442b7d into eclipse-theia:master Feb 16, 2024
14 checks passed
@martin-fleck-at
Copy link
Contributor

@martin-fleck-at Are here some remaining changes to be made? Do you mind to look into my other PR #13344 as well? It is also kind of files related...

Sure, I can have a look.

@dhuebner dhuebner deleted the huebner/install-from-file-13269 branch February 16, 2024 13:26
@jfaltermeier jfaltermeier added this to the 1.47.0 milestone Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants