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

Delay restart after installing/uninstalling/updating plugins #2369

Merged
merged 22 commits into from
Nov 19, 2023

Conversation

VictoriousRaptor
Copy link
Contributor

@VictoriousRaptor VictoriousRaptor commented Sep 29, 2023

Feature

  • Delay restart after installing/uninstalling/updating plugins.

Changes

Core

  • Move Install/Uninstall plugin logic to Core.PluginManager
  • Disallow modifying a plugin twice in a session.

Plugins Manger

  • New option for Plugins Manger: AutoRestartAfterChanging. Default is false. (Should we set to true to keep current behavior?)
  • Exclude installed plugins from pm install
  • Exclude modified(i.e. updated/installed/uninstalled in this session) plugins from pm install and pm update results
  • Show plugin remote icons in pm Install results.
  • Add glyphs for pm context menu
  • Reduce messageboxes to improve consistency
  • Fix BUG: Downloading plugins makes error when original zip file existing #2419

Tests:

  • Different message boxes and notification are shown when AutoRestartAfterChanging == false.
  • plugins can be installed/uninstalled after restart
  • Uninstall a plugin, then install again before restarting not allowed.
  • Installing twice before restarting not allowed.
  • Existing zip deleted when downloading zip.

@@ -136,13 +146,11 @@ internal async Task InstallOrUpdateAsync(UserPlugin plugin)
{
await Http.DownloadAsync(plugin.UrlDownload, filePath).ConfigureAwait(false);

Context.API.ShowMsg(Context.API.GetTranslation("plugin_pluginsmanager_downloading_plugin"),
string.Format(Context.API.GetTranslation("plugin_pluginsmanager_download_success"), plugin.Name));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Download success notification removed.

@VictoriousRaptor VictoriousRaptor marked this pull request as ready for review October 6, 2023 08:30
@VictoriousRaptor VictoriousRaptor requested review from taooceros, jjw24, Garulf, JohnTheGr8 and deefrawley and removed request for taooceros October 6, 2023 08:34
@VictoriousRaptor VictoriousRaptor self-assigned this Oct 6, 2023
@VictoriousRaptor VictoriousRaptor added this to the 1.17.0 milestone Oct 6, 2023
@VictoriousRaptor VictoriousRaptor added enhancement New feature or request kind/ux related to user experience labels Oct 6, 2023
@jjw24 jjw24 added the review in progress Indicates that a review is in progress for this PR label Oct 12, 2023
@VictoriousRaptor
Copy link
Contributor Author

Oh no looks like spell check is really broken...

@jjw24
Copy link
Member

jjw24 commented Oct 14, 2023

We will need to handle an obscure scenario where user chooses to do manual restart, install a plugin and then for some reason uninstalls the same plugin, this will freak flow out a little. Same thing but for vice versa will also happen- uninstall and install the same plugin.

@@ -107,15 +107,15 @@ public interface IPublicAPI
/// </summary>
/// <param name="title">Message title</param>
/// <param name="subTitle">Message subtitle</param>
/// <param name="iconPath">Message icon path (relative path to your plugin folder)</param>
/// <param name="iconPath">Full path to icon</param>
Copy link
Member

Choose a reason for hiding this comment

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

Is this comment change correct? Is it relative or full path? I thought it's still some like ./img/icon.png?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the full path.

Copy link
Member

Choose a reason for hiding this comment

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

relative path also works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

relative path also works

Are you sure? iirc I changed this line cuz I found a Python plugin had no icon. maybe it's a Python side issue?

Copy link
Member

Choose a reason for hiding this comment

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

public string IcoPath
{
get { return _icoPath; }
set
{
// As a standard this property will handle prepping and converting to absolute local path for icon image processing
if (!string.IsNullOrEmpty(value)
&& !string.IsNullOrEmpty(PluginDirectory)
&& !Path.IsPathRooted(value)
&& !value.StartsWith("http://", StringComparison.OrdinalIgnoreCase)
&& !value.StartsWith("https://", StringComparison.OrdinalIgnoreCase))
{
_icoPath = Path.Combine(PluginDirectory, value);
}
else
{
_icoPath = value;
}
}

Maybe the json parser doesn't abide this setter?

@jjw24
Copy link
Member

jjw24 commented Oct 15, 2023

We will need to handle an obscure scenario where user chooses to do manual restart, install a plugin and then for some reason uninstalls the same plugin, this will freak flow out a little. Same thing but for vice versa will also happen- uninstall and install the same plugin.

Best I can think of that is easy to solve is persisting three type of lists installed/uninstalled/updated in memory and each action should check against this list first.

@jjw24 jjw24 mentioned this pull request Nov 7, 2023
3 tasks
VictoriousRaptor and others added 2 commits November 8, 2023 21:51
Co-authored-by: Jeremy Wu <[email protected]>
This reverts commit 0c8729f.
@VictoriousRaptor
Copy link
Contributor Author

We will need to handle an obscure scenario where user chooses to do manual restart, install a plugin and then for some reason uninstalls the same plugin, this will freak flow out a little. Same thing but for vice versa will also happen- uninstall and install the same plugin.

Best I can think of that is easy to solve is persisting three type of lists installed/uninstalled/updated in memory and each action should check against this list first.

You can't uninstall right after installing since the installed plugin is not added to list yet. Installing after uninstalling throws exception though.

@VictoriousRaptor VictoriousRaptor added the bug Something isn't working label Nov 12, 2023
@jjw24
Copy link
Member

jjw24 commented Nov 12, 2023

hey @VictoriousRaptor looks like you added merging the plugins manager functionality to this PR, is it possible to branch this change out so this PR is just for the delay restart ?

Copy link
Member

@jjw24 jjw24 left a comment

Choose a reason for hiding this comment

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

👍

@jjw24 jjw24 removed bug Something isn't working review in progress Indicates that a review is in progress for this PR labels Nov 19, 2023
@jjw24 jjw24 merged commit 4efac50 into Flow-Launcher:dev Nov 19, 2023
@VictoriousRaptor VictoriousRaptor deleted the delay-restart-2 branch November 19, 2023 06:13
@VictoriousRaptor VictoriousRaptor added the bug Something isn't working label Nov 20, 2023
@jjw24 jjw24 removed the bug Something isn't working label Feb 4, 2024
@jjw24 jjw24 linked an issue Aug 17, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Refactor enhancement New feature or request kind/ux related to user experience
Projects
None yet
3 participants