Add internal model for plugin management #3572
Conversation
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: onesounds onesounds 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 |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an internal model for plugin management to remove the dependency on the external Plugin Manager plugin, thereby preventing Flow Launcher from entering a bad state when that plugin is deleted.
- Refactored the deletion command in PluginViewModel to use an internal uninstall method.
- Updated PluginStoreItemViewModel with new async methods for installing, uninstalling, and updating plugins, and refactored plugin properties.
- Added a new user setting with corresponding UI and language translation keys for automatically restarting Flow Launcher after plugin changes.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/PluginViewModel.cs | Removed PluginManagerActionKeyword and updated delete plugin command to use the internal model. |
| Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs | Refactored plugin data handling and introduced async plugin management methods (install, uninstall, update). |
| Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml | Added a card to toggle the new auto restart setting. |
| Flow.Launcher/Languages/en.xaml | Added new translation keys for plugin management messages and auto restart functionality. |
| Flow.Launcher/Infrastructure/UserSettings/Settings.cs | Introduced a new AutoRestartAfterChanging setting. |
Comments suppressed due to low confidence (1)
Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs:26
- [nitpick] Consider renaming '_newPlugin' to 'currentPlugin' and '_oldPluginPair' to 'installedPluginPair' for clearer understanding.
_newPlugin = plugin;
📝 Walkthrough""" WalkthroughThis update introduces an "Auto Restart After Changing" option to user settings and the general settings UI, along with corresponding localization resources. It significantly refactors the plugin store item view model to support robust, asynchronous plugin install, uninstall, and update workflows, addressing potential null reference exceptions and improving error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsPane
participant Settings
User->>SettingsPane: Toggles "Auto Restart After Changing"
SettingsPane->>Settings: Updates AutoRestartAfterChanging property
sequenceDiagram
participant User
participant PluginStoreUI
participant PluginStoreItemViewModel
participant PluginManager
User->>PluginStoreUI: Clicks Install/Uninstall/Update Plugin
PluginStoreUI->>PluginStoreItemViewModel: ShowCommandQueryAsync(action)
PluginStoreItemViewModel->>User: Show confirmation dialog
User-->>PluginStoreItemViewModel: Confirms
PluginStoreItemViewModel->>PluginManager: Call async Install/Uninstall/Update method
PluginManager->>User: Show progress and handle cancellation
PluginManager->>PluginStoreItemViewModel: Return success/failure
PluginStoreItemViewModel->>User: Show result message or prompt restart
Assessment against linked issues
Suggested labels
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
This comment has been minimized.
This comment has been minimized.
![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
♻️ Duplicate comments (1)
Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs (1)
160-197: Mirror the ‘restart-on-error’ fix in Uninstall flowSame unconditional restart occurs here. Insert an early
returninsidecatch, or capture asuccessflag shared with the final block.
🧹 Nitpick comments (3)
Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs (3)
119-137: Partial download artefact not cleaned when user cancelsWhen the user presses “Cancel” in the progress box the temp zip (possibly half-written) is left on disk, and the next install run deletes only if
deleteFileis true and the filename collides. Consider explicit cleanup on cancellation:if (cts.IsCancellationRequested) { - return; + if (!newPlugin.IsFromLocalInstallPath && File.Exists(filePath)) + File.Delete(filePath); + return; }
199-254: Update flow: stale zip file not deleted after successful update
UpdatePluginAsyncdownloads to%TEMP%but never deletes the zip afterwards (unlike install). This can quickly clutter%TEMP%with large archives.@@ else { await App.API.UpdatePluginAsync(oldPlugin, newPlugin, filePath); + if (!newPlugin.IsFromLocalInstallPath && File.Exists(filePath)) + File.Delete(filePath); }
256-289: Download helper: typo and double-download logic
- Comment line 269 spells “expcetion”.
- When the progress box fails, we silently fall back to a background download without progress or user feedback. Confirm this is desired UX; otherwise surface the error.
Consider surfacing the original exception to the caller instead of a silent retry.
🧰 Tools
🪛 GitHub Check: Check Spelling
[warning] 264-264:
prgis not a recognized word. (unrecognized-spelling)🪛 GitHub Actions: Check Spelling
[warning] 264-264: Spell check warning:
prgis not a recognized word. (unrecognized-spelling)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs(1 hunks)Flow.Launcher/Languages/en.xaml(2 hunks)Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml(1 hunks)Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs(3 hunks)Flow.Launcher/ViewModel/PluginViewModel.cs(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Check Spelling
Flow.Launcher/ViewModel/PluginViewModel.cs
[warning] 17-17: Spell check warning: Ioc is not a recognized word. (unrecognized-spelling)
Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs
[warning] 19-19: Spell check warning: Ioc is not a recognized word. (unrecognized-spelling)
[warning] 264-264: Spell check warning: prg is not a recognized word. (unrecognized-spelling)
🪛 GitHub Check: Check Spelling
Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs
[warning] 19-19:
Ioc is not a recognized word. (unrecognized-spelling)
[warning] 264-264:
prg is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (7)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
179-179: Good addition of the auto-restart settingThe new
AutoRestartAfterChangingproperty with a default value offalseis a good addition that will help control the application's restart behavior after plugin changes.Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml (1)
205-214: Well-structured UI control for auto-restart settingThe toggle switch for the auto-restart setting is properly implemented with:
- Appropriate binding to the Settings.AutoRestartAfterChanging property
- Localized title and tooltip
- Consistent styling with other toggle switches
- Good positioning in the settings hierarchy
This follows the established pattern for settings controls in the application.
Flow.Launcher/ViewModel/PluginViewModel.cs (2)
1-1: Appropriate addition of Task namespaceThe addition of the System.Threading.Tasks namespace is necessary for the async implementation.
173-175: Good conversion to async uninstall methodConverting the plugin uninstall operation to an asynchronous method is a good improvement that:
- Properly uses async/await pattern
- Leverages the centralized uninstall method from PluginStoreItemViewModel
- Supports the PR objective of creating an internal model for plugin management
This change will make the plugin uninstallation process more robust.
Flow.Launcher/Languages/en.xaml (2)
134-135: Well-defined localization strings for auto-restart settingThe localization strings for the auto-restart feature are clear and descriptive, explaining both the purpose of the setting and its effects.
189-204: Comprehensive localization for plugin managementThis set of localization strings provides thorough coverage for the plugin management workflow:
- Error messages for installation/uninstallation/updating failures
- Success messages with restart instructions
- Confirmation prompts with plugin details
- Settings preservation options
- Download progress indicators
These strings will ensure a good user experience with appropriate feedback throughout the plugin management process.
Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs (1)
98-106: Generated filename may contain invalid path characters
newPlugin.Namecan legally contain:or other characters invalid on Windows, makingPath.Combinethrow.
Sanitise the name (usePath.GetInvalidFileNameChars()) before composing the filename.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an internal model for plugin management to remove dependency on the external Plugin Manager plugin, preventing unstable behavior when that plugin is removed. Key changes include:
- Removing the use of PluginManagerActionKeyword in PluginViewModel and updating the delete plugin command to an asynchronous implementation.
- Refactoring PluginStoreItemViewModel to use distinct new and existing plugin objects, and introducing asynchronous helper methods for installing, uninstalling, and updating plugins.
- Adding a new auto-restart setting in the settings pane and corresponding translation keys and configuration.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/PluginViewModel.cs | Removed PluginManagerActionKeyword and updated the delete command to an async pattern. |
| Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs | Refactored plugin variable management and added async methods for install, uninstall, and update flows. |
| Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml | Added a new settings card for the auto restart feature after plugin changes. |
| Flow.Launcher/Languages/en.xaml | Added new translation keys for plugin management error messages and prompts. |
| Flow.Launcher/Infrastructure/UserSettings/Settings.cs | Introduced a new boolean property to enable auto restart after plugin changes. |
Comments suppressed due to low confidence (2)
Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs:22
- [nitpick] Consider renaming '_oldPluginPair' to a name like 'existingPluginPair' to more clearly indicate its role compared to the '_newPlugin' variable.
private readonly PluginPair _oldPluginPair;
Flow.Launcher/ViewModel/PluginViewModel.cs:173
- Ensure that the RelayCommand attribute supports asynchronous Task-returning methods, so that exceptions from this async implementation of OpenDeletePluginWindowAsync are handled properly.
private async Task OpenDeletePluginWindowAsync()
@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
♻️ Duplicate comments (1)
Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs (1)
32-33: Version parsing remains a potential exception source.This addresses a previous review concern about
new Version(string)throwing exceptions on malformed version strings. The issue persists in the current implementation.Consider using safe version parsing to prevent UI crashes:
- public bool LabelUpdate => LabelInstalled && new Version(_newPlugin.Version) > new Version(_oldPluginPair.Metadata.Version); + public bool LabelUpdate => LabelInstalled && TryParseVersionComparison(_newPlugin.Version, _oldPluginPair.Metadata.Version); + + private static bool TryParseVersionComparison(string newVersion, string oldVersion) + { + return Version.TryParse(newVersion, out var newVer) && + Version.TryParse(oldVersion, out var oldVer) && + newVer > oldVer; + }
🧹 Nitpick comments (1)
Flow.Launcher.Core/Plugin/PluginManager.cs (1)
871-904: Fix spelling issues in parameter name and comment.The download helper method is well-designed with progress reporting and retry logic, but has spelling issues flagged by static analysis.
Apply this diff to fix the spelling issues:
- internal static async Task DownloadFileAsync(string prgBoxTitle, string downloadUrl, string filePath, CancellationTokenSource cts, bool deleteFile = true, bool showProgress = true) + internal static async Task DownloadFileAsync(string progressBoxTitle, string downloadUrl, string filePath, CancellationTokenSource cts, bool deleteFile = true, bool showProgress = true) { if (deleteFile && File.Exists(filePath)) File.Delete(filePath); if (showProgress) { var exceptionHappened = false; - await API.ShowProgressBoxAsync(prgBoxTitle, + await API.ShowProgressBoxAsync(progressBoxTitle, async (reportProgress) => { if (reportProgress == null) { - // when reportProgress is null, it means there is expcetion with the progress box + // when reportProgress is null, it means there is exception with the progress box // so we record it with exceptionHappened and return so that progress box will close instantly exceptionHappened = true; return;🧰 Tools
🪛 GitHub Check: Check Spelling
[warning] 879-879:
prgis not a recognized word. (unrecognized-spelling)🪛 GitHub Actions: Check Spelling
[warning] 879-883:
prgis not a recognized word. (unrecognized-spelling)
📜 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/ViewModel/PluginStoreItemViewModel.cs(4 hunks)Flow.Launcher/ViewModel/PluginViewModel.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/ViewModel/PluginViewModel.cs
🧰 Additional context used
🧬 Code Graph Analysis (1)
Flow.Launcher.Core/Plugin/PluginManager.cs (5)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
Settings(16-478)Plugins/Flow.Launcher.Plugin.PluginsManager/Utilities.cs (1)
UserPlugin(62-80)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (8)
GetTranslation(136-136)MessageBoxResult(366-366)InstallPlugin(511-511)LogException(276-276)ShowMsgError(85-85)RestartApp(34-34)ShowMsg(114-114)ShowMsg(123-123)Flow.Launcher.Core/Resource/Internationalization.cs (1)
GetTranslation(247-259)Flow.Launcher.Plugin/PluginMetadata.cs (1)
PluginMetadata(10-163)
🪛 GitHub Check: Check Spelling
Flow.Launcher.Core/Plugin/PluginManager.cs
[warning] 879-879:
prg is not a recognized word. (unrecognized-spelling)
🪛 GitHub Actions: Check Spelling
Flow.Launcher.Core/Plugin/PluginManager.cs
[warning] 39-53: Ioc is not a recognized word. (unrecognized-spelling)
[warning] 117-133: Reloadable is not a recognized word. (unrecognized-spelling)
[warning] 179-183: metadatas is not a recognized word. (unrecognized-spelling)
[warning] 181-185: metadatas is not a recognized word. (unrecognized-spelling)
[warning] 182-186: metadatas is not a recognized word. (unrecognized-spelling)
[warning] 184-188: metadatas is not a recognized word. (unrecognized-spelling)
[warning] 879-883: prg is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (6)
Flow.Launcher.Core/Plugin/PluginManager.cs (4)
28-28: LGTM: FlowSettings integration looks good.The static field retrieval of Settings via IoC is appropriate here since PluginManager is a static class and the settings are used for runtime behavior control.
553-624: Excellent async plugin installation with comprehensive user experience.The method provides:
- User confirmation before proceeding
- Progress reporting during download with cancellation support
- Proper error handling with early return on failure (prevents unwanted restart)
- Conditional restart based on user settings
- Clean file management for temporary downloads
626-664: Well-implemented uninstall flow with proper user choices.The method correctly:
- Confirms the uninstall action with the user
- Provides choice for keeping/removing plugin settings
- Handles errors gracefully without restarting on failure
- Follows consistent UX pattern with other plugin operations
666-722: Robust update implementation following established patterns.The update flow properly handles the two-plugin scenario (old and new) and maintains consistency with install/uninstall operations regarding user confirmation, error handling, and restart behavior.
Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs (2)
12-18: Excellent refactoring improves code clarity and separation of concerns.The split into
_newPluginand_oldPluginPairfields makes the code more readable and eliminates repeated lookups. The constructor properly initializes both fields for consistent state.
63-79: Async conversion successfully integrates with new plugin management workflow.The method correctly dispatches to the appropriate async PluginManager methods based on the action. The developer has chosen to keep the default case minimal as noted in past reviews.
Add internal model for plugin management
Let us do not rely
Plugin Managerplugin for plugin management in Flow Launcher. Flow Launcher will get into a very bad state if this plugin is deleted (plugin store page will lose ability to install/uninstall/update plugins).Resolve #3555, #3236.
Test