Improve Plugin Metadata & Path Management #3272
Conversation
…data for csharp plugins
…data for non-csharp plugins
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: no user but you matched threshold 10 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:
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:
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:
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:
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:
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:
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughThis pull request introduces several changes across the codebase. Directory and settings path constructions have been streamlined by replacing concatenated paths with new properties from a centralized data location class. Several methods have been refactored to remove unused dependencies and simplify error handling. Additionally, new XML documentation comments have been added for clarity, and a new event with accompanying delegate and event arguments has been introduced to improve result update notifications. Minor modifications to plugin initialization logic and a new file relocation method for cache files are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin
participant IResultUpdated
participant Consumer
Plugin->>IResultUpdated: Produce updated results
IResultUpdated-->>Consumer: Trigger ResultsUpdated event with event args
Consumer->>IResultUpdated: Process updated results
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 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: 4
🔭 Outside diff range comments (3)
Flow.Launcher/PublicAPIInstance.cs (2)
192-204:⚠️ Potential issue❓ Verification inconclusive
Breaking change: Return type changed from
objecttovoid.The method's return type has been changed from
objecttovoid, which could break existing plugins that rely on the returned value. Please ensure that this change is documented in the release notes and that all dependent plugins are updated accordingly.Run the following script to find any potential usages of the return value:
🏁 Script executed:
#!/bin/bash # Description: Search for usages of RemovePluginSettings that might be affected by the return type change. # Test: Search for method calls that assign the result to a variable or use it in an expression rg -A 5 'RemovePluginSettings\(' | rg -A 5 '(var|object|=|return)'Length of output: 376
Action required: Confirm External Plugin Compatibility Due to API Change
The
RemovePluginSettingsmethod inFlow.Launcher/PublicAPIInstance.csnow returnsvoidinstead ofobject. While our codebase search found no internal usages capturing a return value, please note that this change remains breaking for any external plugins that might rely on the previously returned object. It is essential to document this change in the release notes and ensure that all dependent plugins are updated accordingly.
- Location:
Flow.Launcher/PublicAPIInstance.cs(Lines: 192-204)- Action: Update documentation and notify plugin developers to verify compatibility with this API change.
192-204:⚠️ Potential issueBreaking change: Method return type changed from
objecttovoid.The change in return type of
RemovePluginSettingsfromobjecttovoidis a breaking change that could affect consumers of the public API. Consider:
- Maintaining backward compatibility by keeping the return type as
object.- Creating a new method with a different name that returns void.
- Incrementing the major version number to indicate a breaking change.
Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs (1)
54-66: 🛠️ Refactor suggestionImprove exception handling in DeserializeAsync
The current exception handling silently swallows errors. Consider:
- Logging the exception details
- Adding specific exception types instead of catching all exceptions
- catch (System.Exception) + catch (MemoryPackSerializationException ex) { - // Log.Exception($"|BinaryStorage.Deserialize|Deserialize error for file <{FilePath}>", e); + Log.Error($"|BinaryStorage.Deserialize|Failed to deserialize data: {ex.Message}"); return defaultData; }
🧹 Nitpick comments (7)
Flow.Launcher.Core/Plugin/PluginManager.cs (1)
35-35: Use target-typed new expression.The initialization can be simplified using C# 9.0's target-typed new expression.
-private static List<string> _modifiedPlugins = new(); +private static readonly List<string> _modifiedPlugins = [];Flow.Launcher.Plugin/PluginPair.cs (1)
45-45: Fix typo in documentation.There's a typo in the XML documentation.
- /// Get hash coode + /// Get hash codeFlow.Launcher.Plugin/SharedModels/MatchResult.cs (2)
46-62: Consider making RawScore and Score more consistentThe relationship between
RawScoreandScoreis well-implemented, but consider makingScoremore explicit by renaming it toFilteredScoreto better reflect its nature as a filtered version ofRawScore.-public int Score { get; private set; } +public int FilteredScore { get; private set; } public int RawScore { get { return _rawScore; } set { _rawScore = value; - Score = ScoreAfterSearchPrecisionFilter(_rawScore); + FilteredScore = ScoreAfterSearchPrecisionFilter(_rawScore); } }
97-113: Consider more descriptive enum namesThe
SearchPrecisionScoreenum values could be more descriptive to better reflect their precision levels.public enum SearchPrecisionScore { - Regular = 50, + High = 50, Low = 20, - None = 0 + Disabled = 0 }Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs (1)
9-15: Fix typo in XML documentationThere's a typo in the XML documentation.
- /// Stroage object using binary data + /// Storage object using binary dataFlow.Launcher.Infrastructure/UserSettings/DataLocation.cs (1)
32-32: Fix typo in property nameThere's a typo in the property name.
-public static readonly string SettingsDirectorty = Path.Combine(DataDirectory(), Constant.Settings); +public static readonly string SettingsDirectory = Path.Combine(DataDirectory(), Constant.Settings);Plugins/Flow.Launcher.Plugin.Program/Main.cs (1)
196-216: LGTM! Consider enhancing error handling.The
MoveFilemethod effectively handles file relocation with proper validation. Consider these improvements:
- Add try-catch block to handle IO exceptions.
- Log errors and file operations for debugging.
- Consider using
File.Moveoverload withoverwriteparameter for atomic operations.static bool MoveFile(string sourcePath, string destinationPath) { - if (!File.Exists(sourcePath)) - { - return false; - } - - if (File.Exists(destinationPath)) - { - File.Delete(sourcePath); - return false; - } - - var destinationDirectory = Path.GetDirectoryName(destinationPath); - if (!Directory.Exists(destinationDirectory) && (!string.IsNullOrEmpty(destinationDirectory))) - { - Directory.CreateDirectory(destinationDirectory); - } - File.Move(sourcePath, destinationPath); - return true; + try + { + if (!File.Exists(sourcePath)) + { + Log.Debug("|Flow.Launcher.Plugin.Program.Main|Source file not found", sourcePath); + return false; + } + + var destinationDirectory = Path.GetDirectoryName(destinationPath); + if (!string.IsNullOrEmpty(destinationDirectory)) + { + Directory.CreateDirectory(destinationDirectory); + } + + File.Move(sourcePath, destinationPath, overwrite: true); + Log.Debug("|Flow.Launcher.Plugin.Program.Main|File moved successfully", $"{sourcePath} -> {destinationPath}"); + return true; + } + catch (Exception ex) + { + Log.Exception("|Flow.Launcher.Plugin.Program.Main|Failed to move file", ex); + return false; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs(2 hunks)Flow.Launcher.Core/Plugin/JsonRPCPlugin.cs(1 hunks)Flow.Launcher.Core/Plugin/JsonRPCPluginBase.cs(1 hunks)Flow.Launcher.Core/Plugin/PluginConfig.cs(3 hunks)Flow.Launcher.Core/Plugin/PluginManager.cs(4 hunks)Flow.Launcher.Core/Plugin/PluginsLoader.cs(4 hunks)Flow.Launcher.Infrastructure/Constant.cs(1 hunks)Flow.Launcher.Infrastructure/Logger/Log.cs(1 hunks)Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs(3 hunks)Flow.Launcher.Infrastructure/Storage/JsonStorage.cs(1 hunks)Flow.Launcher.Infrastructure/Storage/PluginJsonStorage.cs(1 hunks)Flow.Launcher.Infrastructure/UserSettings/DataLocation.cs(1 hunks)Flow.Launcher.Infrastructure/UserSettings/PluginSettings.cs(1 hunks)Flow.Launcher.Plugin/ActionContext.cs(1 hunks)Flow.Launcher.Plugin/Interfaces/IResultUpdated.cs(1 hunks)Flow.Launcher.Plugin/Interfaces/ISettingProvider.cs(1 hunks)Flow.Launcher.Plugin/PluginInitContext.cs(1 hunks)Flow.Launcher.Plugin/PluginMetadata.cs(2 hunks)Flow.Launcher.Plugin/PluginPair.cs(2 hunks)Flow.Launcher.Plugin/Query.cs(2 hunks)Flow.Launcher.Plugin/Result.cs(0 hunks)Flow.Launcher.Plugin/SharedCommands/SearchWeb.cs(2 hunks)Flow.Launcher.Plugin/SharedCommands/ShellCommand.cs(2 hunks)Flow.Launcher.Plugin/SharedModels/MatchResult.cs(5 hunks)Flow.Launcher/Languages/en.xaml(1 hunks)Flow.Launcher/PublicAPIInstance.cs(1 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneAboutViewModel.cs(2 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Program/Main.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Sys/Main.cs(2 hunks)Plugins/Flow.Launcher.Plugin.WebSearch/Main.cs(1 hunks)
💤 Files with no reviewable changes (1)
- Flow.Launcher.Plugin/Result.cs
✅ Files skipped from review due to trivial changes (6)
- Flow.Launcher.Plugin/ActionContext.cs
- Flow.Launcher.Plugin/Interfaces/ISettingProvider.cs
- Flow.Launcher.Plugin/SharedCommands/SearchWeb.cs
- Flow.Launcher.Plugin/SharedCommands/ShellCommand.cs
- Flow.Launcher.Infrastructure/Constant.cs
- Flow.Launcher.Plugin/PluginInitContext.cs
👮 Files not reviewed due to content moderation or server errors (4)
- Flow.Launcher/PublicAPIInstance.cs
- Flow.Launcher.Infrastructure/Logger/Log.cs
- Plugins/Flow.Launcher.Plugin.Program/Main.cs
- Flow.Launcher.Core/Plugin/JsonRPCPlugin.cs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (40)
Flow.Launcher.Infrastructure/Storage/PluginJsonStorage.cs (1)
16-16: LGTM! Improved path management using centralized settings directory.The change enhances path management by using
DataLocation.PluginSettingsDirectoryinstead of manual path concatenation, aligning with the PR's goal of better plugin metadata management.Flow.Launcher.Infrastructure/Storage/JsonStorage.cs (1)
19-19: LGTM! Improved configuration management using centralized constants.The change reduces magic strings by using
Constant.Settingsinstead of a hardcoded string value.Plugins/Flow.Launcher.Plugin.Sys/Main.cs (1)
407-408: LGTM! Improved path references using centralized properties.The changes enhance maintainability by:
- Removing unnecessary local variables
- Using direct references to centralized path properties from DataLocation
Also applies to: 411-411, 433-434, 437-437
Flow.Launcher/Languages/en.xaml (1)
134-135: LGTM! Added clear error messages for plugin cache management.The new string resources provide user-friendly error messages for plugin cache removal failures, supporting the improved plugin cache directory management feature.
Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs (1)
423-423: LGTM! Improved path management.The change correctly uses the centralized
ThemesDirectoryproperty instead of manual path concatenation, which aligns with the PR's goal of improving path management.Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs (1)
117-120: LGTM! Proper metadata handling for non-.NET plugins.Setting
AssemblyNameto empty string for non-.NET plugins is correct as they don't have assemblies. This change aligns with the PR's goal of improving plugin metadata management.Flow.Launcher/SettingPages/ViewModels/SettingsPaneAboutViewModel.cs (1)
80-80: LGTM! Improved path management using centralized properties.The changes correctly use centralized properties from
DataLocationfor managing paths to settings and logs, which aligns with the PR's goal of improving path management.Also applies to: 86-86, 118-118
Flow.Launcher.Plugin/Query.cs (2)
5-7: LGTM! Added clear class documentation.The added XML documentation clearly describes the purpose of the Query class.
65-65: LGTM! Added JsonIgnore attribute to computed property.The addition of
[JsonIgnore]attribute toFirstSearchproperty is correct as it's a computed property that doesn't need to be serialized.Plugins/Flow.Launcher.Plugin.WebSearch/Main.cs (1)
185-186: LGTM! Good use of the new PluginSettingsDirectoryPath property.The change effectively utilizes the new
PluginSettingsDirectoryPathproperty for managing custom icons, which aligns well with the PR's objective of improving plugin path management.Flow.Launcher.Core/Plugin/PluginConfig.cs (1)
114-114: LGTM! Good use of modern C# syntax.The change to use the null-coalescing assignment operator (
??=) improves code readability while maintaining the same functionality.Flow.Launcher.Infrastructure/UserSettings/PluginSettings.cs (1)
35-35: LGTM! Performance improvement in dictionary access.The change from
ContainsKeytoTryGetValuepattern eliminates redundant dictionary lookups by combining the existence check and value retrieval into a single operation.Flow.Launcher.Core/Plugin/PluginsLoader.cs (2)
77-78: LGTM! Consistent AssemblyName handling across plugin types.The code properly sets the AssemblyName property for both .NET and non-.NET plugins, which aligns with the PR's objective of improving plugin metadata management.
Also applies to: 147-147, 165-165
139-150: LGTM! Improved code readability.The refactored plugin pair creation using block initialization improves code readability and maintainability.
Also applies to: 157-168
Flow.Launcher.Infrastructure/Logger/Log.cs (3)
15-15: LGTM! Improved path management.The changes effectively centralize path management by using constants and properties from
DataLocation, which aligns with the PR objectives. This reduces the need for manual path construction and improves maintainability.Also applies to: 21-21
15-15: LGTM! Path management improvements.The changes improve path management by:
- Using centralized constants for directory names.
- Using dedicated properties for directory paths.
- Reducing path concatenation operations.
Also applies to: 21-21
15-15: LGTM! Path management improvements.The changes improve path management by:
- Using a centralized constant for the log directory name.
- Using a dedicated property for the version-specific log directory path.
Also applies to: 21-21
Plugins/Flow.Launcher.Plugin.Program/Main.cs (5)
194-194: LGTM! Improved cache file management.The changes effectively handle the migration of cache files to the new location and initialize storage with the correct paths. The validation of the plugin cache directory before moving files is a good practice.
Also applies to: 219-229
194-229: LGTM! Cache directory management improvements.The changes improve cache management by:
- Validating cache directory before use.
- Moving old cache files to new location.
- Using dedicated cache directory path from plugin metadata.
194-194: LGTM! Directory validation.The validation ensures that the plugin cache directory exists before moving files.
218-224: LGTM! Cache file migration.The code correctly handles the migration of cache files from the old location to the new one.
226-229: LGTM! Storage initialization.The storage initialization correctly uses the new plugin cache directory path.
Flow.Launcher.Core/Plugin/JsonRPCPlugin.cs (3)
33-33: LGTM! Simplified settings path construction.The change effectively uses the new
PluginSettingsDirectoryPathproperty to simplify path construction, which aligns with the PR objectives to improve path management.
33-33: LGTM! Path management improvements.The change improves path management by using the dedicated
PluginSettingsDirectoryPathproperty, reducing path concatenation complexity.
33-33: LGTM! Path management improvements.The change improves path management by using the dedicated plugin settings directory path property.
Flow.Launcher.Core/Plugin/JsonRPCPluginBase.cs (1)
30-30: LGTM! Improved path management.The change simplifies path management by using the new
PluginSettingsDirectoryPathproperty fromPluginMetadata, which aligns with the PR objectives.Flow.Launcher.Core/Plugin/PluginManager.cs (4)
158-173: LGTM! Well-structured path management.The new method effectively manages plugin paths by:
- Using
AssemblyNamefor .NET plugins andNamefor others.- Consistently organizing settings and cache directories.
565-570: LGTM! Improved .NET plugin settings cleanup.The code correctly handles .NET plugin settings removal by:
- Using reflection to invoke
RemovePluginSettings.- Using
AssemblyNamefor accurate identification.
572-583: LGTM! Robust error handling for settings removal.The code properly handles settings directory removal with:
- Clear error logging.
- User-friendly error messages.
588-599: LGTM! Robust error handling for cache removal.The code properly handles cache directory removal with:
- Clear error logging.
- User-friendly error messages.
Flow.Launcher.Plugin/PluginMetadata.cs (4)
7-9: LGTM! Comprehensive XML documentation.The documentation is clear, concise, and follows XML documentation best practices.
Also applies to: 14-17, 19-22, 24-27, 29-32, 34-38, 40-43, 45-48, 50-53, 55-58, 60-63, 65-70
77-77: LGTM! Modern C# syntax.Using expression-bodied member for the getter improves code readability.
125-141: LGTM! Well-documented path properties.The new properties:
PluginSettingsDirectoryPath: Clear explanation of settings directory management.PluginCacheDirectoryPath: Clear explanation of cache directory management.
142-149: LGTM! Simple and effective ToString implementation.The
ToStringmethod is well-documented and returns a meaningful representation.Flow.Launcher.Plugin/PluginPair.cs (2)
3-6: LGTM! Comprehensive XML documentation.The documentation is clear, concise, and follows XML documentation best practices.
Also applies to: 8-11, 13-16, 18-22, 27-32, 44-48
34-34: LGTM! Modern C# pattern matching.Using pattern matching with
isimproves code readability and safety.Flow.Launcher.Plugin/Interfaces/IResultUpdated.cs (2)
7-16: Well-designed interface for manual result updates!The interface is well-structured with clear XML documentation and follows C# event pattern best practices.
28-44: Great implementation of event arguments with cancellation support!The
ResultUpdatedEventArgsclass is well-designed with:
- Clear separation of concerns for results, query, and cancellation
- Proper use of
CancellationTokenfor async operation cancellation- Comprehensive XML documentation
Flow.Launcher.Infrastructure/UserSettings/DataLocation.cs (1)
28-38: Well-organized directory structure!The directory structure is well-organized with:
- Clear separation of concerns (logs, cache, settings, themes)
- Consistent use of constants
- Logical grouping of related paths
Flow.Launcher/PublicAPIInstance.cs (1)
192-204:⚠️ Potential issue✅ Verification successful
Breaking change: Method return type changed from
objecttovoid.The return type change of
RemovePluginSettingsfromobjecttovoidis a breaking change that could affect dependent code. Please ensure that all callers are updated to handle the new return type.Run the following script to verify the impact:
🏁 Script executed:
#!/bin/bash # Description: Find all usages of RemovePluginSettings to ensure they handle the new return type. # Test: Search for method calls. Expect: No usages that depend on the return value. rg -A 5 'RemovePluginSettings'Length of output: 2409
Verification Confirmed: No Impact from Return Type Change in RemovePluginSettings
After verifying the codebase, the change of the
RemovePluginSettingsmethod's return type fromobjecttovoidis safe for internal usage. In particular:
- The only invocation (in
Flow.Launcher.Core/Plugin/PluginManager.cs) uses reflection (method?.Invoke(API, new object[] { plugin.AssemblyName });) without capturing any return value.- No internal caller depends on the returned value, so the breaking change does not affect the current logic.
Please confirm there are no external dependencies or documentation that expect the previous return type. If not, this change can be considered safe.
Flow.Launcher/SettingPages/ViewModels/SettingsPaneAboutViewModel.cs
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
| @@ -577,7 +577,7 @@ internal static async Task UninstallPluginAsync(PluginMetadata plugin, bool remo | |||
| throw new ArgumentException($"Plugin {plugin.Name} has been modified"); | |||
| } | |||
|
|
|||
| if (removePluginFromSettings) | |||
| if (removePluginSettings || removePluginFromSettings) | |||
There was a problem hiding this comment.
what's the difference between these two?
There was a problem hiding this comment.
removePluginFromSettings: When one plugin is deleted, this flag will be true and we will remove it from Settings.Plugins and AllPlugins so that FL will not have this plugin. Additionally, we need to clean the cache folder of this plugin.
removePluginSettings: When one plugin is deleted, FL will ask users if they want to remove the settings of this plugin. Users can choose no and FL will keep its settings for next usage.
Because these two will all delete folders of plugins (one for cache folder, another for settings folder), so we need to dispose plugin if either of them is true.
There was a problem hiding this comment.
emmm I think the first one is pretty weird...but sure
| UpdateNotNetPluginDirectory(_metadatas); | ||
| } | ||
|
|
||
| private static void UpdateJsonRPCPluginDirectory(List<PluginMetadata> metadatas) |
There was a problem hiding this comment.
I think we can merge these two function.
There was a problem hiding this comment.
Okay. Since only JsonRPC v2 & dotnet plugins both use InitAsync to get this path, we do not need to update directory before their constuctors
| @@ -228,10 +261,9 @@ public static ICollection<PluginPair> ValidPluginsForQuery(Query query) | |||
| if (query is null) | |||
| return Array.Empty<PluginPair>(); | |||
|
|
|||
| if (!NonGlobalPlugins.ContainsKey(query.ActionKeyword)) | |||
| if (!NonGlobalPlugins.TryGetValue(query.ActionKeyword, out var plugin)) | |||
There was a problem hiding this comment.
Code quality improvement
| @@ -577,7 +577,7 @@ internal static async Task UninstallPluginAsync(PluginMetadata plugin, bool remo | |||
| throw new ArgumentException($"Plugin {plugin.Name} has been modified"); | |||
| } | |||
|
|
|||
| if (removePluginFromSettings) | |||
| if (removePluginSettings || removePluginFromSettings) | |||
There was a problem hiding this comment.
emmm I think the first one is pretty weird...but sure
| Metadata = metadata | ||
| }; | ||
|
|
||
| plugin.Metadata.AssemblyName = string.Empty; |
There was a problem hiding this comment.
why do we need to manually set this? isn't the default is either empty or null?
There was a problem hiding this comment.
Yeah, we can remove that
| /// When plugin is deleted, FL will ask users whether to keep its settings. | ||
| /// If users do not want to keep, this directory will be deleted. | ||
| /// </summary> | ||
| [JsonIgnore] |
There was a problem hiding this comment.
I think the simplest way to do this is by removing the JsonIgnore for both PluginSettingsDirectoryPath and PluginCacheDirectoryPath. It's not a big deal to save a little bit more information in the setting.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@taooceros Sorry, I do not think we should save them in settings. The storage usage does not matter. From my view, this data should be runtime only, and need to be initialized every time we start FL. Similar to Windows |
|
no i mean even if we save it during the setting we can still load them from runtime. its just for allow it to serialize when sending to jsonrpc plugins. |
I really have no idea how to do that. How can we send it to jsonrpc plugins? Could you please help me with it? |
Sorry I think I confused you. I am just saying we should remove the JsonIgnore attribute so when jsonrpc v2 plugins are initialized, these property will be sent to them. |
|
@taooceros Oh, I get it. Now I changed my codes and please review. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
This PR improves the handling of plugin metadata and path management while enhancing plugin uninstall/update operations. Key changes include:
- Adding three new metadata properties (AssemblyName, PluginSettingsDirectoryPath, PluginCacheDirectoryPath) for improved plugin data handling.
- Refactoring path management across the codebase by leveraging new constants and helper properties.
- Updating plugin disposal logic to ensure resource release before deletion of settings and cache directories.
Reviewed Changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Flow.Launcher.Plugin/Query.cs | Minor documentation update for query properties. |
| Flow.Launcher.Plugin/PluginPair.cs | Enhanced documentation and simplified equality check. |
| Flow.Launcher.Plugin/PluginMetadata.cs | Added new metadata properties and refactored property accessors. |
| Flow.Launcher.Plugin/PluginInitContext.cs | Added constructor documentation. |
| Flow.Launcher.Plugin/Interfaces/ISettingProvider.cs | Introduced documentation for settings panel creation. |
| Flow.Launcher.Plugin/Interfaces/IResultUpdated.cs | Improved event and arguments documentation. |
| Flow.Launcher.Plugin/ActionContext.cs | Provided documentation for the default special key state. |
| Flow.Launcher.Infrastructure/UserSettings/PluginSettings.cs | Updated settings lookup to use TryGetValue. |
| Flow.Launcher.Infrastructure/UserSettings/DataLocation.cs | Added new directory paths and constants for logs, cache, and settings. |
| Flow.Launcher.Infrastructure/Storage/PluginJsonStorage.cs | Updated storage path management using new directory constants. |
| Flow.Launcher.Infrastructure/Storage/JsonStorage.cs | Revised directory name handling via Constant.Settings. |
| Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs | Refactored file path handling and improved deserialization method. |
| Flow.Launcher.Infrastructure/Logger/Log.cs | Updated logging directory path using new constant. |
| Flow.Launcher.Infrastructure/Constant.cs | Introduced a new constant for Cache directory. |
| Flow.Launcher.Core/Plugin/PluginsLoader.cs | Updated assembly name extraction and streamlined string join usage. |
| Flow.Launcher.Core/Plugin/PluginManager.cs | Enhanced plugin disposal and updated uninstall/update logic. |
| Flow.Launcher.Core/Plugin/PluginConfig.cs | Improved ActionKeywords initialization via null-coalescing assignment. |
| Flow.Launcher.Core/Plugin/JsonRPCPluginBase.cs | Adjusted settings path usage to rely on plugin metadata. |
| Flow.Launcher.Core/Plugin/JsonRPCPlugin.cs | Updated settings file path construction to use new metadata property. |
| Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs | Clears AssemblyName for certain plugins; behavior should be verified. |
Comments suppressed due to low confidence (1)
Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs:121
- Overwriting AssemblyName for plugins matching the provided language may lead to issues if AssemblyName is expected to have a valid value later. Please verify whether clearing AssemblyName here is the intended behavior or if this is a potential bug.
metadata.AssemblyName = string.Empty;
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
This PR improves plugin metadata handling and refines path management across various modules of the application. Key changes include:
- Adding new metadata properties (AssemblyName, PluginSettingsDirectoryPath, PluginCacheDirectoryPath) to support .Net and JsonRPC plugins.
- Enhancing path management by introducing predefined directories in DataLocation and updating related storage and plugin loader implementations.
- Updating documentation and refactoring several methods to support asynchronous disposal and improved logging.
Reviewed Changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Flow.Launcher.Plugin/Query.cs | Added XML documentation tags and adjusted the placement of attributes. |
| Flow.Launcher.Plugin/PluginPair.cs | Added documentation for properties and refactored equality comparisons. |
| Flow.Launcher.Plugin/PluginMetadata.cs | Introduced new metadata properties and updated getter syntax; added XML comments for clarity. |
| Flow.Launcher.Plugin/PluginInitContext.cs | Added XML documentation for constructors. |
| Flow.Launcher.Plugin/Interfaces/ISettingProvider.cs | Added XML documentation for the interface and its method. |
| Flow.Launcher.Plugin/Interfaces/IResultUpdated.cs | Added XML documentation for the event, delegate, and event arguments. |
| Flow.Launcher.Plugin/ActionContext.cs | Added documentation for a default special key state object. |
| Flow.Launcher.Infrastructure/UserSettings/PluginSettings.cs | Updated dictionary access from index operator to TryGetValue. |
| Flow.Launcher.Infrastructure/UserSettings/DataLocation.cs | Introduced new fields for version logs, cache, and settings directories using constants. |
| Flow.Launcher.Infrastructure/Storage/PluginJsonStorage.cs | Updated directory path construction to use PluginSettingsDirectory. |
| Flow.Launcher.Infrastructure/Storage/JsonStorage.cs | Updated constant value reference from hard-coded string to Constant.Settings. |
| Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs | Adjusted modifiers and method signature for asynchronous deserialization. |
| Flow.Launcher.Infrastructure/Logger/Log.cs | Updated log directory construction to use the new constant for logs. |
| Flow.Launcher.Infrastructure/Constant.cs | Added a cache constant for use in path management. |
| Flow.Launcher.Core/Plugin/PluginsLoader.cs | Improved dotnet plugin assembly name assignment and standardized instantiation for executable plugins. |
| Flow.Launcher.Core/Plugin/PluginManager.cs | Refactored asynchronous plugin disposal and updated uninstallation methods to properly remove settings/cache. |
| Flow.Launcher.Core/Plugin/PluginConfig.cs | Applied null-coalescing operator shorthand for action keywords initialization. |
| Flow.Launcher.Core/Plugin/JsonRPCPluginBase.cs | Modified settings directory composition to use PluginSettingsDirectoryPath from metadata. |
| Flow.Launcher.Core/Plugin/JsonRPCPlugin.cs | Updated the settings path similarly to rely on PluginSettingsDirectoryPath. |
| Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs | Added an extra using directive and set AssemblyName to empty for specific languages. |
Comments suppressed due to low confidence (1)
Flow.Launcher.Plugin/PluginMetadata.cs:44
- Typo in XML comment: 'hash coode' should be 'hash code'.
/// Get hash coode
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. Pattern suggestions ✂️ (1)You could add these patterns to If the flagged items are 🤯 false positivesIf items relate to a ...
|
jjw24
removed
enhancement
Improve Plugin Metadata & Path Management
1. Add three properties in plugin metadata
(1)
AssemblyNameFor .Net plugins, we do not need to use reflection to get its assembly name.
(2)
PluginSettingsDirectoryPath& (3)PluginCacheDirectoryPathFor csharp & JsonRPC v2 plugins, FL can provide them with these directories so that they can store their temporary and permeant data in that place.
2. Improve path management
Add many fields in
DataLocation.csso that we do not need to usePath.Combineto construct the paths.3. Improve
Flow.Launcher.PlugindocumentsAdd docuements in
Flow.Launcher.Pluginproject4. Fix Settings & Cache Directory Removing Issue
Dispose plugin when we need to delete plugin Settings or Cache Directory.
Test
Programcan move old cache files to itsPluginCacheDirectoryPath.Future
Cacheclean feature.