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 (1)
🪧 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.
This comment has been minimized.
This comment has been minimized.
| { | ||
| File.Delete(sourcePath); | ||
| } | ||
| catch (Exception) |
There was a problem hiding this comment.
the old cache files are not cleaned. next time it wil retry it again
There was a problem hiding this comment.
no I mean why the delete will fail. If it fail shouldn't next time it will also failed?
There was a problem hiding this comment.
no I mean why the delete will fail.
it will fail if user happens to open this file.
If it fail shouldn't next time it will also failed?
not exactly. if last time user happens to open this file, it will success next time if user closes this file. no matter how many times it has failed before, if it succeeds once, the file will be deleted permanently since we will never create file in the old path.
| /// 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 we can not ignore these, as that would help jsonrpc plugin (V2) to retrive these information.
However, I am not sure whether we save it to somewhere.
There was a problem hiding this comment.
sorry, I do not think so. from my view, this data should be runtime only, and need to be initialized every time we start FL. perhaps in the future, we will change the plugin settings path to other place and we should not use the original paths
There was a problem hiding this comment.
Oh, I know your meaning. I find that JsonRPC plugins cannot get metadata instance which means they cannot get these paths.
Could we pass this parameter to the plugins in function like InitAsync (Dotnet plugins has this but JsonRPC not, why?)
Additionally, I have changed the codes to make sure metadata of these JsonRPC plugins is correct before creating JsonRPCPlugin instance.
There was a problem hiding this comment.
Similar to
Flow.Launcher/Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs
Lines 26 to 27 in dbf51a3
I believe we should also expose PluginMetadata to JsonRPC plugins.
I guess we can add here:
However, I’m not very familiar with this part of the codes and might not be able to contribute directly at the moment. 😢
There was a problem hiding this comment.
That's what I do to v2. For v1, it's kindly not in the protocol so I don't think we can do that.
There was a problem hiding this comment.
So provide this feature only to JsonRPC v2 plugins?
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (1)
196-236: Enhance error handling in the file move operation.While the current implementation with silent error handling is designed to retry on next plugin start, it would be beneficial to:
- Log specific error types for better debugging.
- Handle specific error cases differently (e.g., file in use vs. permission denied).
Apply this diff to improve error handling:
static void MoveFile(string sourcePath, string destinationPath) { if (!File.Exists(sourcePath)) { return; } if (File.Exists(destinationPath)) { try { File.Delete(sourcePath); } - catch (Exception) + catch (UnauthorizedAccessException e) { - // Ignore, we will handle next time we start the plugin + Log.Error($"|Flow.Launcher.Plugin.Program.Main|Access denied while deleting source file: {e.Message}"); + } + catch (IOException e) when ((e.HResult & 0x0000FFFF) == 32) + { + Log.Error("|Flow.Launcher.Plugin.Program.Main|Source file is in use"); + } + catch (Exception e) + { + Log.Error($"|Flow.Launcher.Plugin.Program.Main|Failed to delete source file: {e.Message}"); } return; } var destinationDirectory = Path.GetDirectoryName(destinationPath); if (!Directory.Exists(destinationDirectory) && (!string.IsNullOrEmpty(destinationDirectory))) { try { Directory.CreateDirectory(destinationDirectory); } - catch (Exception) + catch (UnauthorizedAccessException e) { - // Ignore, we will handle next time we start the plugin + Log.Error($"|Flow.Launcher.Plugin.Program.Main|Access denied while creating directory: {e.Message}"); + return; + } + catch (Exception e) + { + Log.Error($"|Flow.Launcher.Plugin.Program.Main|Failed to create directory: {e.Message}"); + return; } } try { File.Move(sourcePath, destinationPath); } - catch (Exception) + catch (UnauthorizedAccessException e) { - // Ignore, we will handle next time we start the plugin + Log.Error($"|Flow.Launcher.Plugin.Program.Main|Access denied while moving file: {e.Message}"); + } + catch (IOException e) when ((e.HResult & 0x0000FFFF) == 32) + { + Log.Error("|Flow.Launcher.Plugin.Program.Main|File is in use"); + } + catch (Exception e) + { + Log.Error($"|Flow.Launcher.Plugin.Program.Main|Failed to move file: {e.Message}"); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Core/Plugin/PluginManager.cs(4 hunks)Flow.Launcher.Plugin/PluginMetadata.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Program/Main.cs(2 hunks)
🔇 Additional comments (7)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (2)
194-194: LGTM!Good practice to validate the directory before performing file operations.
238-248: LGTM!The implementation correctly:
- Moves cache files from old to new locations.
- Initializes storage with the new plugin cache directory paths.
Flow.Launcher.Core/Plugin/PluginManager.cs (2)
154-183: LGTM!The implementation correctly:
- Separates .NET and non-.NET plugins.
- Uses consistent path patterns.
- Updates paths at appropriate times during plugin loading.
573-610: LGTM!The implementation correctly:
- Handles .NET plugin settings removal.
- Provides appropriate error handling with user notifications.
- Uses the new plugin metadata properties for path management.
Flow.Launcher.Plugin/PluginMetadata.cs (3)
7-62: LGTM!The XML documentation is clear, descriptive, and follows standard conventions.
63-84: LGTM!The implementation correctly:
- Adds AssemblyName property for .NET plugins.
- Updates related paths in PluginDirectory setter.
125-141: LGTM!The implementation correctly:
- Adds properties for settings and cache directories.
- Provides clear documentation about their purpose and behavior.
- Marks properties as JsonIgnore since they are runtime-only.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher.Core/Plugin/PluginManager.cs (1)
259-259: Improved dictionary access safety with TryGetValue.Using
TryGetValueinstead of direct indexing eliminates potentialKeyNotFoundExceptionwhen the action keyword doesn't exist.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs(2 hunks)Flow.Launcher.Core/Plugin/PluginManager.cs(4 hunks)Flow.Launcher.Core/Plugin/PluginsLoader.cs(4 hunks)Flow.Launcher.Infrastructure/Storage/JsonStorage.cs(1 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)
🚧 Files skipped from review as they are similar to previous changes (7)
- Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs
- Flow.Launcher.Infrastructure/Storage/JsonStorage.cs
- Flow.Launcher/SettingPages/ViewModels/SettingsPaneAboutViewModel.cs
- Flow.Launcher/PublicAPIInstance.cs
- Flow.Launcher/Languages/en.xaml
- Flow.Launcher.Core/Plugin/PluginsLoader.cs
- Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (6)
Flow.Launcher.Core/Plugin/PluginManager.cs (6)
38-38: Modernized syntax using C# target-typed new expression.This is a minor readability improvement using C#'s target-typed new expression.
157-162: Good separation of plugin directory updates based on plugin type.Excellent approach to update paths at the right time - JSON RPC plugin directories before loading plugins, and .NET plugin directories after loading (when assembly name is available).
164-186: Clean implementation of plugin directory management.The code clearly distinguishes between .NET and non-.NET plugins, setting appropriate paths based on either assembly name or plugin name. This implementation supports the PR objective of providing dedicated locations for plugins to store their settings and cache files.
577-582: Eliminated reflection in favor of direct property access.Using the
AssemblyNameproperty directly rather than obtaining it through reflection is a good improvement that aligns with the PR objectives.
584-595: Enhanced error handling for plugin settings directory deletion.Using the new
PluginSettingsDirectoryPathproperty simplifies the code while the added try-catch block with proper error logging and user notification improves robustness.
600-611: Added proper cleanup of plugin cache directory.This new block correctly handles the deletion of plugin cache files when a plugin is removed, with appropriate error handling and user notification.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher.Core/Plugin/PluginManager.cs (1)
573-573: Consider using Task return type instead of async void.Using
async voidis generally discouraged except for event handlers as it can lead to unhandled exceptions and difficult debugging.- internal static async void UninstallPlugin(PluginMetadata plugin, bool removePluginFromSettings, bool removePluginSettings, bool checkModified) + internal static async Task UninstallPlugin(PluginMetadata plugin, bool removePluginFromSettings, bool removePluginSettings, bool checkModified)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Core/Plugin/PluginManager.cs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (9)
Flow.Launcher.Core/Plugin/PluginManager.cs (9)
38-38: Modern syntax improvement.Using target-typed new expression (C# 9.0+) for list initialization.
75-89: Good refactoring of plugin disposal logic.This refactoring properly handles both
IDisposableandIAsyncDisposableinterfaces, improving code organization and maintainability.
162-167: Improved plugin directory management with clear sequencing.The code correctly updates JSON RPC plugin directories before loading plugins (when they're needed) and .NET plugin directories after loading (when assembly names are available).
169-190: Well-implemented plugin directory path management.These new methods establish proper path management using the new metadata properties, which aligns with the PR objectives. The separation by plugin language type ensures correct path construction for different plugin types.
264-264: Improved dictionary lookup pattern.Changed from direct check to
TryGetValuepattern for better readability.
580-590: Good approach to plugin disposal before removal.Disposing plugins before deletion ensures they release file handles, which helps with successfully removing directories.
592-613: Well-structured plugin settings directory handling.The code properly uses the new metadata properties instead of constructing paths manually, which aligns with the PR objectives. The exception handling with user-friendly error messages is also commendable.
617-628: Good implementation of cache directory removal.Similar to settings directory handling, this properly uses the new metadata properties and includes appropriate error handling with user feedback.
598-598: Reference to previous discussion on AssemblyName usage.This line demonstrates how the new AssemblyName property replaces the previous reflection-based approach, as mentioned in the past review comments. Good implementation.
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.
@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 ...
|
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.