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

Feature: Add support for local plugin installation #2520

Merged
merged 4 commits into from May 19, 2024

Conversation

flooxo
Copy link
Contributor

@flooxo flooxo commented Feb 1, 2024

This PR closes #2252.

Description

The new method uses plugin.UrlDownload to store the local path of the zip file.
First, the corresponding values are read from the plugin.json file. Then it is installed like any other plugin, except that the zip file is not downloaded.

Changes

Add a new method so that plugins can now also be installed locally from a zip file
Example: pm install c:\plugin.zip.

Signed-off-by: Florian Grabmeier <flo.grabmeier@gmail.com>
@flooxo
Copy link
Contributor Author

flooxo commented Feb 1, 2024

Open questions

  • The icon is not displayed correctly in the installation command because the path from the zip file cannot be used
  • How should update recommendations be handled (should it also be possible to use pm update <localPath>, as this is automatically suggested if the plugin is already installed)
  • After the installation the zip is always deleted, which may not be desired, how to avoid this?

@jjw24
Copy link
Member

jjw24 commented Feb 4, 2024

I am thinking:

The icon is not displayed correctly in the installation command because the path from the zip file cannot be used

Use a custom icon, or can we load the zip icon?

How should update recommendations be handled (should it also be possible to use pm update , as this is automatically suggested if the plugin is already installed)

Detect if it's local path, think Explorer plugin already has code to detect if it's local/network path. We can reuse it but move it to the SharedCommands folder in the Plugin project.

After the installation the zip is always deleted, which may not be desired, how to avoid this?

Cleaning up the zip file is by design when downloaded from URL. If local/network path is detected, skip this step.

Signed-off-by: Florian Grabmeier <flo.grabmeier@gmail.com>
@flooxo
Copy link
Contributor Author

flooxo commented Feb 4, 2024

Use a custom icon, or can we load the zip icon?

The problem with the icon is that the path to the png would be correct, but it has to be unzipped before it can be displayed. For this it would have to be temporary stored. I think the easiest way is to simply display a custom icon

Detect if it's local path, think Explorer plugin already has code to detect if it's local/network path. We can reuse it but move it to the SharedCommands folder in the Plugin project.

Unfortunately I have not found a method in Explorer to detect if it is a local path (currently I use File.Exsits and sometimes Path.GetTempPath() to check if its a local file)
But there is a lot of redundant code in the update, because especially the prompts are always the same for update from a local file, a single update and update all.

Cleaning up the zip file is by design when downloaded from URL. If local/network path is detected, skip this step.

Thanks

This comment has been minimized.

@jjw24
Copy link
Member

jjw24 commented Feb 5, 2024

The problem with the icon is that the path to the png would be correct, but it has to be unzipped before it can be displayed. For this it would have to be temporary stored. I think the easiest way is to simply display a custom icon

Yeah that's what I mean, custom icon or just load the icon of the zip file?

@flooxo
Copy link
Contributor Author

flooxo commented Feb 5, 2024

Yes, thanks, I have done that

@flooxo flooxo marked this pull request as ready for review February 5, 2024 19:39
@flooxo
Copy link
Contributor Author

flooxo commented Feb 6, 2024

Where are the other icons from? Maybe I need to either add attribution (but I don't know where) or use a different icon since this icon uses the "flaticon license" which is free for commercial use but requires attribution

@Garulf
Copy link
Member

Garulf commented Feb 6, 2024

@onesounds could you please lend a hand with icons?

@jjw24
Copy link
Member

jjw24 commented Feb 6, 2024

Unfortunately I have not found a method in Explorer to detect if it is a local path (currently I use File.Exsits and sometimes Path.GetTempPath() to check if its a local file)
But there is a lot of redundant code in the update, because especially the prompts are always the same for update from a local file, a single update and update all.

@flooxo this is what you need:

public static bool IsLocationPathString(this string querySearchString)

@Garulf
Copy link
Member

Garulf commented Feb 11, 2024

The new method uses plugin.UrlDownload to store the local path of the zip file. First, the corresponding values are read from the plugin.json file. Then it is installed like any other plugin, except that the zip file is not downloaded.

Could you explain why we need a local path in the plugin's manifest? Shouldn't allowing pm install <local path> be enough?

@flooxo
Copy link
Contributor Author

flooxo commented Feb 20, 2024

The local path is required because when calling InstallOrUpdateAsync it is no longer possible to access the local path as this is not passed anywhere. However, the local path determines whether the plugin must be installed or downloaded first. And also in the case of updating the existing plugin, the local path must be reused to suggest pm update <local path>. If that makes sense :)

@flooxo
Copy link
Contributor Author

flooxo commented Feb 20, 2024

@flooxo this is what you need:

public static bool IsLocationPathString(this string querySearchString)

Thanks! But this function does not check whether the location exists, which is crucial.

@Garulf
Copy link
Member

Garulf commented Feb 20, 2024

Would it be best to use pm install at the new zip file for updates?

@flooxo
Copy link
Contributor Author

flooxo commented Feb 20, 2024

What would be the advantage of this?
The difference is that once PluginManager.UpdatePlugin and otherwise PluginManager.InstallPlugin is used. And update ensures that the old version is deleted first. So I would say pm update is fitting here

@jjw24
Copy link
Member

jjw24 commented Feb 21, 2024

@flooxo this is what you need:

public static bool IsLocationPathString(this string querySearchString)

Thanks! But this function does not check whether the location exists, which is crucial.

This method is best used to check if the query string is a location path, if not then proceed to download, if it is then you do a separate call to check if the file actually exists. This is so you don't end up checking file exists on disk per every query string.

So call that function on the passed in query string, if is location path then call this

public static bool FileExists(this string filePath)

@jjw24
Copy link
Member

jjw24 commented Feb 21, 2024

What would be the advantage of this? The difference is that once PluginManager.UpdatePlugin and otherwise PluginManager.InstallPlugin is used. And update ensures that the old version is deleted first. So I would say pm update is fitting here

Not sure if I am understanding correctly, but I would say the expected user experience would be to use pm install <file_path> to install and pm update <file_path> to update.

@jjw24
Copy link
Member

jjw24 commented Feb 21, 2024

Just a thought, whether pm update is a necessity here? If you need to constantly update your plugin from local file path, it would be when you are developing a plugin? Wouldn't be better to just dump/install the plugin into the plugins folder and open up the project from there anyway? Just a question more or less.

@flooxo
Copy link
Contributor Author

flooxo commented Feb 24, 2024

Yes, sure. I've already thought about this point, but I think if you can install a plugin locally, then it should also be possible to update it for the sake of completeness. Of course, it would be easier to simply drag it into the folder.

@jjw24 jjw24 added this to the Future milestone Apr 18, 2024
@jjw24
Copy link
Member

jjw24 commented May 11, 2024

Hey @flooxo I would like to make a couple changes and refactor the code on this PR, do you like me to commit them here directly or show you via a new PR?

@flooxo
Copy link
Contributor Author

flooxo commented May 11, 2024

Hey @flooxo I would like to make a couple changes and refactor the code on this PR, do you like me to commit them here directly or show you via a new PR?

Feel free to commit them directly here to keep things simple :)

@jjw24 jjw24 added the enhancement New feature or request label May 16, 2024
@jjw24 jjw24 modified the milestones: Future, 1.19.0 May 16, 2024
@jjw24 jjw24 modified the milestone: 1.19.0 May 16, 2024
@jjw24
Copy link
Member

jjw24 commented May 16, 2024

@flooxo hey I pushed my changes, they include:

  • the zip file no longer gets deleted. Added an extra property to UserPlugin class to hold and determine a local install.
  • moved the zip file read method into utilities class so no need to write the code in with the install/update methods
  • created a new method to determine zip file path

Let me know if you see any issues with them.

@flooxo
Copy link
Contributor Author

flooxo commented May 19, 2024

Nice, LGTM :)

@jjw24 jjw24 enabled auto-merge May 19, 2024 22:44
@jjw24 jjw24 merged commit 1bbe030 into Flow-Launcher:dev May 19, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pm install url does not work with a local path
3 participants