Support deleting plugin settings when uninstalling plugins #3219
Conversation
|
🥷 Code experts: jjw24 Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughThe changes update the plugin management system by enhancing the uninstallation and update logic. The Changes
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Flow.Launcher.Core/Plugin/PluginManager.cs (1)
546-556: Consider adding error handling for directory operations.The directory deletion operation could fail due to various reasons (e.g., file locks, permissions). Consider adding try-catch blocks to handle potential exceptions gracefully.
if (removePluginSettings) { var assemblyLoader = new PluginAssemblyLoader(plugin.ExecuteFilePath); var assembly = assemblyLoader.LoadAssemblyAndDependencies(); var assemblyName = assembly.GetName().Name; var directoryPath = Path.Combine(DataLocation.DataDirectory(), JsonStorage<object>.DirectoryName, Constant.Plugins, assemblyName); if (Directory.Exists(directoryPath)) { + try + { Directory.Delete(directoryPath, true); + } + catch (Exception ex) + { + Log.Exception(nameof(PluginManager), $"Failed to delete plugin settings directory for {plugin.Name}", ex); + } } }Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs (1)
685-689: LGTM! Consider adding documentation to clarify parameters.The implementation looks good and aligns with the PR objectives. The user prompt is clear and consistent with the codebase's style.
Consider adding a comment to explain the difference between
removeSettingsandremovePluginSettingsparameters:private void Uninstall(PluginMetadata plugin) { try { + // removeSettings: Remove plugin's system settings + // removePluginSettings: Remove plugin's user settings from %APPDATA%\FlowLauncher\Settings\Plugins\{Plugin_AssemblyName} var removePluginSettings = Context.API.ShowMsgBox( Context.API.GetTranslation("plugin_pluginsmanager_keep_plugin_settings_subtitle"), Context.API.GetTranslation("plugin_pluginsmanager_keep_plugin_settings_title"), button: MessageBoxButton.YesNo) == MessageBoxResult.No; PluginManager.UninstallPlugin(plugin, removeSettings: true, removePluginSettings: removePluginSettings);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Flow.Launcher.Core/Plugin/PluginManager.cs(5 hunks)Plugins/Flow.Launcher.Plugin.PluginsManager/Languages/en.xaml(2 hunks)Plugins/Flow.Launcher.Plugin.PluginsManager/Languages/zh-cn.xaml(2 hunks)Plugins/Flow.Launcher.Plugin.PluginsManager/Languages/zh-tw.xaml(2 hunks)Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (5)
Flow.Launcher.Core/Plugin/PluginManager.cs (2)
458-460: LGTM! Method signature updated to support plugin settings deletion.The new
removePluginSettingsparameter with a default value offalsemaintains backward compatibility while adding support for plugin settings deletion.
443-443: LGTM! UpdatePlugin method updated to maintain settings.The
UpdatePluginmethod correctly passesfalsefor bothremoveSettingsandremovePluginSettingsto preserve settings during updates.Plugins/Flow.Launcher.Plugin.PluginsManager/Languages/en.xaml (1)
18-19: LGTM! Clear and consistent English localization strings.The strings are well-written and clearly communicate the purpose to users.
Plugins/Flow.Launcher.Plugin.PluginsManager/Languages/zh-cn.xaml (1)
16-17: LGTM! Clear and consistent Simplified Chinese localization strings.The translations maintain the same meaning as the English strings while being natural in Chinese.
Plugins/Flow.Launcher.Plugin.PluginsManager/Languages/zh-tw.xaml (1)
16-17: LGTM! Clear and consistent Traditional Chinese localization strings.The translations are accurate and maintain consistency with other language versions.
|
Hey @Jack251970, are you in flow's Discord? |
Yes? |
|
Can you message me in Discord - jjw24 |
Already sent. |
| @@ -13,6 +13,8 @@ | |||
| <system:String x:Key="plugin_pluginsmanager_installing_plugin">Installing Plugin</system:String> | |||
| <system:String x:Key="plugin_pluginsmanager_install_from_web">下載並安裝 {0}</system:String> | |||
| <system:String x:Key="plugin_pluginsmanager_uninstall_title">解除安裝擴充功能</system:String> | |||
| <system:String x:Key="plugin_pluginsmanager_keep_plugin_settings_title">保留插件設置</system:String> | |||
There was a problem hiding this comment.
FYI, language files other than English need to be added by Crowdin. This may exist until next Crowdin PR update, which will revert this to English or new translation.
There was a problem hiding this comment.
You mean, I need to revert all changes in non-English languages files?
There was a problem hiding this comment.
Or I need to add the strings as the same string as English to other non-English files?
There was a problem hiding this comment.
seem that the former is the right choice🤔
There was a problem hiding this comment.
I have used the former way as I check the previous commits.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@jjw24 And every time you uninstall one plugin, you will get one prompts for this. So uninstall multiple plugins will get many prompts. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs (2)
685-689: LGTM! The implementation aligns well with the PR objectives.The changes effectively implement the feature to optionally delete plugin settings during uninstallation:
- Clear user prompt asking whether to keep plugin settings
- Proper handling of the user's choice when calling
UninstallPluginHowever, consider the following enhancement:
The message box text could be more explicit about the consequences of the choice. Here's a suggested improvement:
- Context.API.GetTranslation("plugin_pluginsmanager_keep_plugin_settings_subtitle"), + Context.API.GetTranslation("plugin_pluginsmanager_keep_plugin_settings_subtitle") + + Environment.NewLine + + Context.API.GetTranslation("These settings will be permanently deleted if you choose No."),
681-697: Consider enhancing the bulk uninstallation experience.When uninstalling multiple plugins without auto-restart, users will see multiple prompts (one for settings and one for confirmation per plugin). Consider adding a bulk operation option that allows users to apply the same settings choice to all plugins being uninstalled.
Here's a suggested approach:
private void Uninstall(PluginMetadata plugin) { + // Use a static field to remember the user's choice during bulk operations + private static bool? _bulkUninstallKeepSettings = null; try { - var removePluginSettings = Context.API.ShowMsgBox( - Context.API.GetTranslation("plugin_pluginsmanager_keep_plugin_settings_subtitle"), - Context.API.GetTranslation("plugin_pluginsmanager_keep_plugin_settings_title"), - button: MessageBoxButton.YesNo) == MessageBoxResult.No; + var removePluginSettings = true; + if (_bulkUninstallKeepSettings == null) + { + var result = Context.API.ShowMsgBox( + Context.API.GetTranslation("plugin_pluginsmanager_keep_plugin_settings_subtitle") + + Environment.NewLine + + "Apply this choice to all plugins being uninstalled?", + Context.API.GetTranslation("plugin_pluginsmanager_keep_plugin_settings_title"), + button: MessageBoxButton.YesNoCancel); + + if (result == MessageBoxResult.Cancel) + return; + + removePluginSettings = result == MessageBoxResult.No; + _bulkUninstallKeepSettings = removePluginSettings; + } + else + { + removePluginSettings = _bulkUninstallKeepSettings.Value; + } PluginManager.UninstallPlugin(plugin, removePluginFromSettings: true, removePluginSettings: removePluginSettings); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher.Core/Plugin/PluginManager.cs(4 hunks)Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher.Core/Plugin/PluginManager.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
This comment has been minimized.
This comment has been minimized.
…and settings-removed plugins
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Core/Plugin/PluginManager.cs(3 hunks)Flow.Launcher.Infrastructure/Storage/PluginJsonStorage.cs(2 hunks)Flow.Launcher/PublicAPIInstance.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher.Core/Plugin/PluginManager.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (2)
Flow.Launcher.Infrastructure/Storage/PluginJsonStorage.cs (1)
8-9: LGTM! Good use of readonly field.The
AssemblyNamefield is correctly marked as readonly to prevent modifications after initialization.Flow.Launcher/PublicAPIInstance.cs (1)
192-207: Verify the impact on plugin settings removal.The implementation removes plugin settings from memory but doesn't delete the settings directory. Consider calling
DeleteDirectoryafter removing the settings.Run this script to check if any plugin settings directories remain after uninstallation:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for orphaned plugin settings directories. # Search for references to plugin settings deletion rg -A 5 "RemovePluginSettings|DeleteDirectory" # Search for plugin uninstallation code ast-grep --pattern $'UninstallPlugin($$$) { $$$ }'Length of output: 2213
Plugin settings directory deletion is correctly chained.
The RemovePluginSettings method only removes the in‑memory plugin settings. The PluginManager in Flow.Launcher.Core/Plugin/PluginManager.cs subsequently retrieves the returned plugin storage object and, if not null, calls its DeleteDirectory method (defined in Flow.Launcher.Infrastructure/Storage/PluginJsonStorage.cs) to delete the settings directory.
- Flow.Launcher/PublicAPIInstance.cs: RemovePluginSettings only removes the in‑memory entry.
- Flow.Launcher.Core/Plugin/PluginManager.cs: After invoking RemovePluginSettings, it calls DeleteDirectory on the plugin storage if one exists.
- Flow.Launcher.Infrastructure/Storage/PluginJsonStorage.cs: DeleteDirectory deletes the settings directory.
No changes are needed as the chain of deletion is already implemented.
This comment has been minimized.
This comment has been minimized.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. If the flagged items are 🤯 false positivesIf items relate to a ...
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Flow.Launcher.Core/Plugin/JsonRPCPluginBase.cs (1)
165-171: Consider adding error handling for directory deletion.While the directory existence check is good, the deletion operation could fail due to file locks or permissions.
Apply this diff to add error handling:
public void DeletePluginSettingsDirectory() { if (Directory.Exists(SettingDirectory)) { - Directory.Delete(SettingDirectory, true); + try + { + Directory.Delete(SettingDirectory, true); + } + catch (Exception e) + { + Log.Exception($"|JsonRPCPluginBase.DeletePluginSettingsDirectory|Failed to delete settings directory for {Context.CurrentPluginMetadata.Name}", e); + } } }Flow.Launcher.Core/Plugin/PluginManager.cs (1)
547-589: Consider extracting plugin settings removal logic into separate methods.The plugin settings removal logic is quite complex and could benefit from being split into separate methods for .NET and JSON RPC plugins.
Apply this diff to improve code organization:
+private static void RemoveDotNetPluginSettings(PluginMetadata plugin) +{ + var assemblyLoader = new PluginAssemblyLoader(plugin.ExecuteFilePath); + var assembly = assemblyLoader.LoadAssemblyAndDependencies(); + var assemblyName = assembly.GetName().Name; + + // if user want to remove the plugin settings, we cannot call save method for the plugin json storage instance of this plugin + // so we need to remove it from the api instance + var method = API.GetType().GetMethod("RemovePluginSettings"); + var pluginJsonStorage = method?.Invoke(API, new object[] { assemblyName }); + + // if there exists a json storage for current plugin, we need to delete the directory path + if (pluginJsonStorage != null) + { + var deleteMethod = pluginJsonStorage.GetType().GetMethod("DeleteDirectory"); + try + { + deleteMethod?.Invoke(pluginJsonStorage, null); + } + catch (Exception e) + { + Log.Exception($"|PluginManager.UninstallPlugin|Failed to delete plugin json folder for {plugin.Name}", e); + } + } +} + +private static void RemoveJsonRpcPluginSettings(PluginMetadata plugin) +{ + var pluginPair = AllPlugins.FirstOrDefault(p => p.Metadata.ID == plugin.ID); + if (pluginPair != null && pluginPair.Plugin is JsonRPCPlugin jsonRpcPlugin) + { + try + { + jsonRpcPlugin.DeletePluginSettingsDirectory(); + } + catch (Exception e) + { + Log.Exception($"|PluginManager.UninstallPlugin|Failed to delete plugin json folder for {plugin.Name}", e); + } + } +} + internal static void UninstallPlugin(PluginMetadata plugin, bool removePluginFromSettings, bool removePluginSettings, bool checkModified) { if (checkModified && PluginModified(plugin.ID)) { throw new ArgumentException($"Plugin {plugin.Name} has been modified"); } if (removePluginSettings) { - if (AllowedLanguage.IsDotNet(plugin.Language)) // for the plugin in .NET, we can use assembly loader - { - var assemblyLoader = new PluginAssemblyLoader(plugin.ExecuteFilePath); - var assembly = assemblyLoader.LoadAssemblyAndDependencies(); - var assemblyName = assembly.GetName().Name; - - // if user want to remove the plugin settings, we cannot call save method for the plugin json storage instance of this plugin - // so we need to remove it from the api instance - var method = API.GetType().GetMethod("RemovePluginSettings"); - var pluginJsonStorage = method?.Invoke(API, new object[] { assemblyName }); - - // if there exists a json storage for current plugin, we need to delete the directory path - if (pluginJsonStorage != null) - { - var deleteMethod = pluginJsonStorage.GetType().GetMethod("DeleteDirectory"); - try - { - deleteMethod?.Invoke(pluginJsonStorage, null); - } - catch (Exception e) - { - Log.Exception($"|PluginManager.UninstallPlugin|Failed to delete plugin json folder for {plugin.Name}", e); - } - } - } - else // the plugin with json prc interface - { - var pluginPair = AllPlugins.FirstOrDefault(p => p.Metadata.ID == plugin.ID); - if (pluginPair != null && pluginPair.Plugin is JsonRPCPlugin jsonRpcPlugin) - { - try - { - jsonRpcPlugin.DeletePluginSettingsDirectory(); - } - catch (Exception e) - { - Log.Exception($"|PluginManager.UninstallPlugin|Failed to delete plugin json folder for {plugin.Name}", e); - } - } - } + if (AllowedLanguage.IsDotNet(plugin.Language)) + RemoveDotNetPluginSettings(plugin); + else + RemoveJsonRpcPluginSettings(plugin); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher.Core/Plugin/JsonRPCPluginBase.cs(2 hunks)Flow.Launcher.Core/Plugin/PluginManager.cs(3 hunks)
🔇 Additional comments (6)
Flow.Launcher.Core/Plugin/JsonRPCPluginBase.cs (2)
47-48: LGTM!The
SettingDirectoryproperty correctly combines the plugin settings directory path with the plugin name.
50-50: LGTM!Good refactoring to reuse the
SettingDirectoryproperty for constructing the settings file path.Flow.Launcher.Core/Plugin/PluginManager.cs (4)
442-442: LGTM!The method call correctly uses the new parameters, ensuring plugin settings are preserved during updates.
457-459: LGTM!The parameter renaming improves clarity by distinguishing between removing the plugin from settings and removing the plugin's settings directory.
524-532: LGTM!Good addition of error handling for temp folder deletion with proper logging.
591-595: LGTM!The plugin removal logic is correctly guarded by the
removePluginFromSettingsparameter.
Jack251970
changed the title
|
@taooceros Regarding the reflection issue you mentioned, I’m planning to improve the FL plugin settings path mangement in the future. And these reflection codes will be removed. |
I see. This seems pretty ambitious. Looking forward to it! |
Allow user to delete plugin settings (under %APPDATA%\FlowLauncher\Settings\Plugins{Plugin_AssemblyName}) when uninstalling plugins.
Test
MessageBoxEx behaviour when deleting plugin: Every time you uninstall the plugin, it will create one dialog for that.
Delete settings behaviour for dotnet (I guess C# and F# should be the same, so I just test for C#), python, nodejs plugins: When uninstalling all types of plugins, if you don not let FL keep the plugin settings, the plugin settings files will be deleted.