Introduce Dependency Injection for Better Development Experience #3175
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive refactoring of message display and dependency management across the Flow Launcher application. The primary changes involve replacing direct Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
This comment has been minimized.
This comment has been minimized.
|
I was thinking about unifying the access to Very earlier I was thinking about integrating dependency injection in flow, but that seems to be pretty complicated. Maybe we can do that gradually specifically for the PublicAPI? I am not sure. What do others think about global state access? |
|
@taooceros I agree that dependency injection is the most elegant way to access |
|
I mean it can serve as a transitional solution for unifying the call of api functions. (here I just do for functions related to |
This comment has been minimized.
This comment has been minimized.
Jack251970
changed the title
|
🥷 Code experts: Yusyuriv, jjw24 Yusyuriv, jjw24 have most 🧠 knowledge in the files. See details
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
Now I implement dependency injection model for this, and can you please take a look of this? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
Flow.Launcher.Core/Plugin/PluginsLoader.cs (1)
Line range hint
123-127: Consider using constructor injection instead of service locator pattern.The use of
Ioc.Default.GetRequiredServiceis a service locator anti-pattern that makes the code harder to test and maintain. Consider:
- Making the class non-static
- Injecting IPublicAPI through the constructor
public static class PluginsLoader { + private readonly IPublicAPI _api; + + public PluginsLoader(IPublicAPI api) + { + _api = api; + } public static IEnumerable<PluginPair> DotNetPlugins(...) { // ... - Ioc.Default.GetRequiredService<IPublicAPI>().ShowMsgBox(...); + _api.ShowMsgBox(...);
🧹 Nitpick comments (19)
Flow.Launcher.Core/Resource/Internationalization.cs (2)
128-128: Consider constructor injection instead of service locator pattern.While the change aligns with the PR's objective to unify API access, using
Ioc.Defaultdirectly is a service locator anti-pattern. Consider injectingIPublicAPIthrough the constructor for better testability and maintainability.Example refactor:
public class Internationalization { public Settings Settings { get; set; } + private readonly IPublicAPI _api; private const string Folder = "Languages"; // ... other fields ... - public Internationalization() + public Internationalization(IPublicAPI api) { + _api = api; AddFlowLauncherLanguageDirectory(); } // ... other methods ... public bool PromptShouldUsePinyin(string languageCodeToSet) { // ... existing code ... - if (Ioc.Default.GetRequiredService<IPublicAPI>().ShowMsgBox(text, string.Empty, MessageBoxButton.YesNo) == MessageBoxResult.No) + if (_api.ShowMsgBox(text, string.Empty, MessageBoxButton.YesNo) == MessageBoxResult.No) return false; // ... existing code ... } }
Line range hint
114-131: Consider separating UI interaction from business logic.The method currently mixes business logic (determining if Pinyin should be used) with UI concerns (showing dialog). Consider extracting the UI interaction to improve testability and maintainability.
Example approach:
+public interface IUserPrompt +{ + bool ConfirmPinyinUsage(Language language); +} public class Internationalization { private readonly IPublicAPI _api; + private readonly IUserPrompt _userPrompt; - public Internationalization(IPublicAPI api) + public Internationalization(IPublicAPI api, IUserPrompt userPrompt) { _api = api; + _userPrompt = userPrompt; } public bool PromptShouldUsePinyin(string languageCodeToSet) { var languageToSet = GetLanguageByLanguageCode(languageCodeToSet); if (Settings.ShouldUsePinyin) return false; if (languageToSet != AvailableLanguages.Chinese && languageToSet != AvailableLanguages.Chinese_TW) return false; - string text = languageToSet == AvailableLanguages.Chinese ? "是否启用拼音搜索？" : "是否啓用拼音搜索？"; - if (_api.ShowMsgBox(text, string.Empty, MessageBoxButton.YesNo) == MessageBoxResult.No) - return false; - - return true; + return _userPrompt.ConfirmPinyinUsage(languageToSet); } }Flow.Launcher.Infrastructure/UserSettings/Settings.cs (2)
16-26: Consider constructor injection for better dependency management.While the current implementation works, it adds another initialization requirement that needs to be managed. Given the discussion about unifying API access and potentially moving towards dependency injection, consider:
- Using constructor injection instead of manual initialization
- Making the class immutable where possible
- Using an interface for the storage dependency
This would better align with the goal of improving the development experience and make future DI adoption easier.
Example approach:
- public class Settings : BaseModel, IHotkeySettings + public class Settings : BaseModel, IHotkeySettings, ISettings { - private FlowLauncherJsonStorage<Settings> _storage; + private readonly ISettingsStorage<Settings> _storage; - public void Initialize(FlowLauncherJsonStorage<Settings> storage) - { - _storage = storage; - } + public Settings(ISettingsStorage<Settings> storage) + { + _storage = storage ?? throw new ArgumentNullException(nameof(storage)); + }
Line range hint
1-590: Consider breaking down the Settings class into smaller, focused components.The Settings class has grown to handle multiple concerns, making it harder to maintain and test. Consider splitting it into smaller, focused classes:
- HotkeySettings (hotkey-related properties)
- UISettings (window size, fonts, themes)
- BrowserSettings (browser configurations)
- ExplorerSettings (explorer configurations)
- PluginSettings (already partially separated)
This would:
- Improve maintainability
- Make the code more testable
- Reduce the cognitive load when making changes
- Make it easier to implement proper dependency injection
Flow.Launcher/ViewModel/MainViewModel.cs (1)
62-68: Consider future refactoring to constructor injection.While using
Ioc.Default.GetRequiredServiceworks as a transitional solution, it introduces service location which can make the code harder to test and maintain. Consider gradually moving towards constructor injection in future refactors.A more maintainable approach would be:
-public MainViewModel() +public MainViewModel(Settings settings) { _queryTextBeforeLeaveResults = ""; _queryText = ""; _lastQuery = new Query(); - Settings = Ioc.Default.GetRequiredService<Settings>(); + Settings = settings;Flow.Launcher/App.xaml.cs (6)
89-90: Avoid using static instances when employing dependency injectionIn lines 89-90, you assign the
StringMatcher.Instancestatic property to the instance retrieved from the IoC container:StringMatcher.Instance = stringMatcher;This practice introduces a global state, which undermines the benefits of dependency injection and can lead to harder-to-test code. Consider refactoring the code to inject
StringMatcherwhere needed rather than relying on the static instance.
98-98: Refactor to eliminate the staticApp.APIpropertyWhile transitioning to dependency injection, retaining the static
App.APIproperty may lead to inconsistencies and reduced testability. Instead, injectIPublicAPIinto classes that require it, promoting better modularity and adherence to dependency injection principles.
106-107: Inject dependencies intoMainWindowinstead of passing_settingsdirectlyIn lines 106-107,
_settingsis passed directly toMainWindow's constructor:var window = new MainWindow(_settings, mainVM);To maintain consistency with the dependency injection approach, consider injecting
SettingsintoMainWindowvia the IoC container. This enhances testability and aligns with best practices.
114-114: RefactorHotKeyMapperto use dependency injectionCurrently,
HotKeyMapperis initialized statically:HotKeyMapper.Initialize(mainVM);Consider refactoring
HotKeyMapperto accept dependencies through constructor injection. This change would improve modularity and make the codebase more maintainable.
163-167: OptimizeUpdaterby injectingIPublicAPIIn the
AutoUpdatesmethod,APIis passed explicitly toUpdateAppAsync:await Ioc.Default.GetRequiredService<Updater>().UpdateAppAsync(API);Since
Updateris registered in the IoC container, consider injectingIPublicAPIintoUpdaterdirectly. This reduces dependencies between components and enhances code readability.
210-210: Avoid overusingIoc.Defaultfor resolving servicesIn line 210,
MainViewModelis resolved directly from the service locator:Ioc.Default.GetRequiredService<MainViewModel>().Show();Frequent use of the service locator pattern can lead to code that's harder to maintain and test. Consider injecting
MainViewModelwhere needed instead of resolving it directly.Flow.Launcher/ActionKeywords.xaml.cs (1)
47-47: InjectIPublicAPIinstead of accessingApp.APIdirectlyIn line 47,
App.API.ShowMsgBox(msg);is used to display a message box:App.API.ShowMsgBox(msg);To adhere to dependency injection principles and improve testability, consider injecting
IPublicAPIinto theActionKeywordsclass. This approach reduces coupling to theAppclass and enhances modularity.Flow.Launcher.Core/ExternalPlugins/Environments/PythonEnvironment.cs (1)
31-31: LGTM! Consider adding error handling.The change from
MessageBoxEx.ShowtoAPI.ShowMsgBoxaligns with the PR objectives. The lambda syntax is appropriate for the callback scenario.Consider wrapping the folder removal in a try-catch block to handle potential IO exceptions:
- FilesFolders.RemoveFolderIfExists(InstallPath, (s) => API.ShowMsgBox(s)); + try { + FilesFolders.RemoveFolderIfExists(InstallPath, (s) => API.ShowMsgBox(s)); + } catch (System.IO.IOException ex) { + API.ShowMsgBox($"Failed to remove folder: {ex.Message}"); + }Flow.Launcher/PriorityChangeWindow.xaml.cs (1)
47-47: LGTM! Consider extracting the message preparation.The change aligns with the PR objectives.
Consider extracting the message preparation to improve readability:
- string msg = translater.GetTranslation("invalidPriority"); - App.API.ShowMsgBox(msg); + App.API.ShowMsgBox(translater.GetTranslation("invalidPriority"));Flow.Launcher/CustomShortcutSetting.xaml.cs (1)
52-52: LGTM! Consider consolidating validation logic.The change aligns with the PR objectives.
Consider consolidating the validation logic into a separate method:
+ private bool ValidateShortcut(string key, string value) + { + if (string.IsNullOrEmpty(key) || string.IsNullOrEmpty(value)) + { + App.API.ShowMsgBox(InternationalizationManager.Instance.GetTranslation("emptyShortcut")); + return false; + } + if (((update && originalKey != key) || !update) && _hotkeyVm.DoesShortcutExist(key)) + { + App.API.ShowMsgBox(InternationalizationManager.Instance.GetTranslation("duplicateShortcut")); + return false; + } + return true; + }Flow.Launcher/CustomQueryHotkeySetting.xaml.cs (1)
66-66: LGTM! Error handling is maintained.The change from
MessageBoxEx.ShowtoApp.API.ShowMsgBoxmaintains the existing error handling while aligning with the PR objectives.Consider adding a descriptive error message that includes the invalid hotkey details:
- App.API.ShowMsgBox(InternationalizationManager.Instance.GetTranslation("invalidPluginHotkey")); + var message = $"{InternationalizationManager.Instance.GetTranslation("invalidPluginHotkey")} (Action: {item.ActionKeyword}, Hotkey: {item.Hotkey})"; + App.API.ShowMsgBox(message);Flow.Launcher/SettingPages/ViewModels/SettingsPaneAboutViewModel.cs (1)
Line range hint
65-72: LGTM! Consider extracting message strings to constants.The migration from
MessageBoxEx.ShowtoApp.API.ShowMsgBoxis implemented correctly. The internationalization and message box parameters are properly maintained.Consider extracting the translation keys to constants at the class level for better maintainability and reusability:
+ private const string ClearLogFolderMessageKey = "clearlogfolderMessage"; + private const string ClearLogFolderTitleKey = "clearlogfolder"; private void AskClearLogFolderConfirmation() { var confirmResult = App.API.ShowMsgBox( - InternationalizationManager.Instance.GetTranslation("clearlogfolderMessage"), - InternationalizationManager.Instance.GetTranslation("clearlogfolder"), + InternationalizationManager.Instance.GetTranslation(ClearLogFolderMessageKey), + InternationalizationManager.Instance.GetTranslation(ClearLogFolderTitleKey), MessageBoxButton.YesNo );Flow.Launcher/SettingPages/ViewModels/SettingsPaneHotkeyViewModel.cs (2)
45-47: Extract common message to reduce duplication.The "pleaseSelectAnItem" message is used multiple times. Consider extracting it to a constant or helper method.
+ private const string PleaseSelectItemKey = "pleaseSelectAnItem"; + private void ShowPleaseSelectItemMessage() => + App.API.ShowMsgBox(InternationalizationManager.Instance.GetTranslation(PleaseSelectItemKey)); private void CustomHotkeyDelete() { var item = SelectedCustomPluginHotkey; if (item is null) { - App.API.ShowMsgBox(InternationalizationManager.Instance.GetTranslation("pleaseSelectAnItem")); + ShowPleaseSelectItemMessage(); return; }Also applies to: 70-72, 91-93, 115-117
Line range hint
49-56: Consider extracting confirmation dialog pattern.Both delete operations follow the same confirmation pattern. Consider extracting this to a reusable method.
+ private bool ConfirmDeletion<T>(T item, string warningKey, string itemDisplay) + { + var result = App.API.ShowMsgBox( + string.Format( + InternationalizationManager.Instance.GetTranslation(warningKey), + itemDisplay + ), + InternationalizationManager.Instance.GetTranslation("delete"), + MessageBoxButton.YesNo + ); + return result == MessageBoxResult.Yes; + } private void CustomHotkeyDelete() { var item = SelectedCustomPluginHotkey; if (item is null) { ShowPleaseSelectItemMessage(); return; } - var result = App.API.ShowMsgBox(/*...*/); - if (result is MessageBoxResult.Yes) + if (ConfirmDeletion(item, "deleteCustomHotkeyWarning", item.Hotkey)) { Settings.CustomPluginHotkeys.Remove(item); HotKeyMapper.RemoveHotkey(item.Hotkey); } }Also applies to: 95-102
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
Flow.Launcher.Core/Configuration/Portable.cs(7 hunks)Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs(5 hunks)Flow.Launcher.Core/ExternalPlugins/Environments/PythonEnvironment.cs(1 hunks)Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptEnvironment.cs(1 hunks)Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptV2Environment.cs(1 hunks)Flow.Launcher.Core/Plugin/PluginManager.cs(1 hunks)Flow.Launcher.Core/Plugin/PluginsLoader.cs(2 hunks)Flow.Launcher.Core/Resource/Internationalization.cs(2 hunks)Flow.Launcher.Core/Resource/Theme.cs(4 hunks)Flow.Launcher.Core/Updater.cs(4 hunks)Flow.Launcher.Infrastructure/Flow.Launcher.Infrastructure.csproj(1 hunks)Flow.Launcher.Infrastructure/PinyinAlphabet.cs(2 hunks)Flow.Launcher.Infrastructure/StringMatcher.cs(2 hunks)Flow.Launcher.Infrastructure/UserSettings/Settings.cs(2 hunks)Flow.Launcher/ActionKeywords.xaml.cs(1 hunks)Flow.Launcher/App.xaml.cs(5 hunks)Flow.Launcher/CustomQueryHotkeySetting.xaml.cs(1 hunks)Flow.Launcher/CustomShortcutSetting.xaml.cs(1 hunks)Flow.Launcher/Flow.Launcher.csproj(2 hunks)Flow.Launcher/Helper/HotKeyMapper.cs(1 hunks)Flow.Launcher/MessageBoxEx.xaml(1 hunks)Flow.Launcher/MessageBoxEx.xaml.cs(1 hunks)Flow.Launcher/PriorityChangeWindow.xaml.cs(2 hunks)Flow.Launcher/PublicAPIInstance.cs(1 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneAboutViewModel.cs(1 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneHotkeyViewModel.cs(4 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneProxyViewModel.cs(1 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs(1 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(2 hunks)Flow.Launcher/ViewModel/SettingWindowViewModel.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Flow.Launcher/MessageBoxEx.xaml.cs
🧰 Additional context used
📓 Learnings (1)
Flow.Launcher/Flow.Launcher.csproj (1)
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-11-12T03:58:56.325Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (20)
Flow.Launcher.Core/Resource/Internationalization.cs (1)
14-14: LGTM! Using CommunityToolkit.Mvvm for dependency injection.The addition of CommunityToolkit.Mvvm's dependency injection aligns well with the PR's objective to improve API access patterns.
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
7-7: LGTM!The addition of the Storage namespace import is appropriate for the new storage functionality.
Flow.Launcher/PublicAPIInstance.cs (1)
36-36: Good use of abstraction by changing_alphabettoIAlphabetSwitching the type from
PinyinAlphabetto theIAlphabetinterface enhances flexibility and allows for different implementations, promoting a more extensible design.Flow.Launcher/ViewModel/MainViewModel.cs (2)
28-28: LGTM! Using statement aligns with DI changes.The addition of CommunityToolkit.Mvvm.DependencyInjection enables proper dependency injection support.
68-68: Verify DI container configuration.Ensure that Settings is properly registered in the DI container before MainViewModel is instantiated.
Run this script to verify the DI setup:
✅ Verification successful
DI container configuration for Settings is properly set up
The Settings class is correctly registered as a singleton in the DI container before MainViewModel is instantiated, ensuring proper dependency management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for proper DI registration of Settings # Look for container configuration and Settings registration rg -A 5 'AddSingleton<Settings>|services\.Add.*Settings'Length of output: 58
Script:
#!/bin/bash # First, find Settings class definition ast-grep --pattern 'class Settings' # Then look for service collection setup and registration patterns rg -l "IServiceCollection|ServiceCollection|BuildServiceProvider" # Also search for any Settings usage in startup/configuration files rg -l "Settings" "*Program.cs" "*Startup.cs"Length of output: 34619
Script:
#!/bin/bash # Look for DI configuration in App.xaml.cs and service registration patterns rg -A 10 "CreateHost|Host\.CreateDefault|ConfigureServices|AddSingleton|\.Services\." "Flow.Launcher/App.xaml.cs" # Search for service registration using ast-grep to find method calls ast-grep --pattern 'AddSingleton<$_>($_)' # Look for Settings usage in App initialization rg -A 5 "new Settings\(\)|GetService<Settings>|GetRequiredService<Settings>" "Flow.Launcher/App.xaml.cs"Length of output: 1522
Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptV2Environment.cs (1)
28-28: Confirmed refactoring to centralized message handlingThe update to use
API.ShowMsgBox(s)aligns with the project's shift towards centralized message handling viaIPublicAPI. This enhances consistency across the codebase.Flow.Launcher/PriorityChangeWindow.xaml.cs (1)
27-27: LGTM! Error handling is maintained.The change from
MessageBoxEx.ShowtoApp.API.ShowMsgBoxmaintains the existing error handling while aligning with the PR objectives.Flow.Launcher/CustomShortcutSetting.xaml.cs (1)
46-46: LGTM! Validation is maintained.The change from
MessageBoxEx.ShowtoApp.API.ShowMsgBoxmaintains the existing validation while aligning with the PR objectives.Flow.Launcher/MessageBoxEx.xaml (1)
2-2: LGTM! Namespace changes align with project reorganization.The MessageBoxEx class has been correctly moved from Flow.Launcher.Core to Flow.Launcher namespace, aligning with the PR objectives.
Let's verify that all references to MessageBoxEx have been updated:
Also applies to: 6-6
✅ Verification successful
✅ Namespace changes are properly implemented
All references to MessageBoxEx have been updated to use the new
Flow.Launchernamespace. No remaining references to the oldFlow.Launcher.Corenamespace were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to Flow.Launcher.Core.MessageBoxEx rg "Flow\.Launcher\.Core\.MessageBoxEx"Length of output: 41
Script:
#!/bin/bash # Search for MessageBoxEx references in all files rg "MessageBoxEx" -A 2 # Specifically look for XAML references fd -e xaml -x grep -l "MessageBoxEx" {} \;Length of output: 1685
Flow.Launcher.Core/Updater.cs (2)
59-59: LGTM! Consistent API usage for message boxes.The changes to use API.ShowMsgBox are consistent with the project-wide changes and maintain the same functionality.
Also applies to: 74-76, 89-89
26-26: Consider thread safety implications of singleton API instance.While using dependency injection is good, retrieving the API instance at field initialization might cause issues if the IoC container isn't set up when the class is instantiated.
Consider injecting IPublicAPI through the constructor instead:
-private readonly IPublicAPI API = Ioc.Default.GetRequiredService<IPublicAPI>(); +private readonly IPublicAPI API; + +public Updater(IPublicAPI api) +{ + API = api; +}Flow.Launcher.Core/Configuration/Portable.cs (1)
19-19: LGTM! Well-structured dependency injection.The changes properly implement dependency injection by using a private readonly field initialized through the constructor.
Flow.Launcher.Core/Resource/Theme.cs (2)
14-14: LGTM: Dependency injection setup looks good.The addition of CommunityToolkit.Mvvm.DependencyInjection and initialization of IPublicAPI through dependency injection aligns with the PR objectives of improving the development experience.
Also applies to: 27-27
114-114: LGTM: Message box calls properly migrated.The MessageBoxEx.Show calls have been consistently replaced with API.ShowMsgBox, maintaining the same functionality while using the new centralized message handling approach.
Also applies to: 124-124
Flow.Launcher.Core/Plugin/PluginManager.cs (1)
522-522: LGTM: Consistent message handling approach.The change to use API.ShowMsgBox via lambda expression maintains consistency with the PR's objective of centralizing message handling.
Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptEnvironment.cs (1)
28-28: LGTM: Consistent message handling approach.The change to use API.ShowMsgBox via lambda expression maintains consistency with the PR's objective of centralizing message handling.
Flow.Launcher/SettingPages/ViewModels/SettingsPaneProxyViewModel.cs (1)
25-25: LGTM! Change aligns with unified API access pattern.The replacement of MessageBoxEx.Show with App.API.ShowMsgBox is consistent with the PR objective of unifying API access.
Let's verify error handling in proxy testing scenarios:
✅ Verification successful
Error handling verification successful
The proxy-related error handling is comprehensive and properly maintained:
- Input validation for server and port
- Connection failure handling
- All error messages are properly internationalized
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other proxy-related error handling patterns rg -A 2 "proxyConnectFailed|serverCantBeEmpty|portCantBeEmpty" --type csLength of output: 1129
Flow.Launcher/Helper/HotKeyMapper.cs (1)
49-49: LGTM! Consistent error handling pattern.The change to App.API.ShowMsgBox maintains consistent error handling across the application.
Let's verify the consistency of error handling in hotkey-related scenarios:
✅ Verification successful
Verified: Message box usage is consistent across the codebase
The change from MessageBoxEx.Show to App.API.ShowMsgBox in HotKeyMapper.cs aligns with the application-wide pattern where all UI components use the API abstraction for showing message boxes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar hotkey registration error handling patterns rg "registerHotkeyFailed|HotkeyManager.Current" --type cs -A 2Length of output: 1387
Script:
#!/bin/bash # Search for MessageBoxEx.Show and App.API.ShowMsgBox usage rg "MessageBoxEx\.Show|App\.API\.ShowMsgBox" --type cs -A 2Length of output: 5488
Flow.Launcher.Infrastructure/Flow.Launcher.Infrastructure.csproj (1)
57-57: LGTM! Addition of MVVM toolkit enhances development experience.The CommunityToolkit.Mvvm package addition supports the MVVM pattern implementation across the project.
Flow.Launcher/Flow.Launcher.csproj (1)
91-92: Review framework and package version compatibility.The project targets .NET 7 but uses .NET 8 packages (Microsoft.Extensions.DependencyInjection and Microsoft.Extensions.Hosting version 8.0.1). This version mismatch might cause compatibility issues.
Consider downgrading to version 7.x.x or upgrading the target framework to .NET 8.
Let's verify the impact:
✅ Verification successful
Version mismatch is safe for current DI usage patterns
The codebase uses only basic DI features (Host builder and singleton registration) that are stable between .NET 7 and 8. While aligning versions with the target framework is recommended, the current implementation doesn't use any .NET 8 specific features that would cause compatibility issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for DI usage patterns that might be affected rg "IServiceCollection|IServiceProvider|IHost" --type cs -A 2Length of output: 1080
Script:
#!/bin/bash # Search for DI setup and usage patterns rg "using Microsoft\.Extensions\.(DependencyInjection|Hosting)" --type cs -A 3 # Search for common DI patterns rg "AddSingleton|AddScoped|AddTransient|BuildServiceProvider|CreateDefaultBuilder|ConfigureServices" --type cs -A 3Length of output: 1767
Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs
Show resolved
Hide resolved
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 (1)
Flow.Launcher.Infrastructure/StringMatcher.cs(3 hunks)
🔇 Additional comments (2)
Flow.Launcher.Infrastructure/StringMatcher.cs (2)
11-11: LGTM!The use of target-typed
new()expression improves code conciseness while maintaining readability.
Line range hint
254-268: LGTM!The implementation of
CalculateClosestSpaceIndexis correct and the change to static is appropriate as it doesn't use instance state.
This comment has been minimized.
This comment has been minimized.
This reverts commit ff110b3.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Flow.Launcher.Infrastructure/StringMatcher.cs (2)
17-26: 🛠️ Refactor suggestionConsider using constructor injection instead of service locator pattern.
The current implementation uses the service locator pattern which is generally considered an anti-pattern. While the test constructor provides a workaround, it would be better to use constructor injection consistently.
-public StringMatcher() -{ - _alphabet = Ioc.Default.GetRequiredService<IAlphabet>(); -} - -// This is a workaround to allow unit tests to set the instance -public StringMatcher(IAlphabet alphabet) -{ - _alphabet = alphabet; -} +public StringMatcher(IAlphabet alphabet) +{ + _alphabet = alphabet ?? throw new ArgumentNullException(nameof(alphabet)); +}
256-257:⚠️ Potential issueFix numeric character comparison in IsAcronymNumber.
The current implementation incorrectly compares character codes with integers.
-private static bool IsAcronymNumber(string stringToCompare, int compareStringIndex) - => stringToCompare[compareStringIndex] >= 0 && stringToCompare[compareStringIndex] <= 9; +private static bool IsAcronymNumber(string stringToCompare, int compareStringIndex) + => char.IsDigit(stringToCompare[compareStringIndex]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher.Infrastructure/StringMatcher.cs(3 hunks)Flow.Launcher.Test/FuzzyMatcherTest.cs(11 hunks)
🔇 Additional comments (2)
Flow.Launcher.Infrastructure/StringMatcher.cs (2)
251-254: LGTM! Good static method refactoring.The change to make IsAcronymChar static is appropriate as it doesn't depend on instance state.
Line range hint
260-274: LGTM! Good static method refactoring.The change to make CalculateClosestSpaceIndex static is appropriate as it doesn't depend on instance state.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Flow.Launcher/App.xaml.cs (1)
163-167: 🛠️ Refactor suggestionMake update checks cancellable.
The update check task should be cancellable when the app exits to prevent potential memory leaks or hanging operations.
Apply this diff to make update checks cancellable:
+ private readonly CancellationTokenSource _updateCancellation = new(); + private void AutoUpdates() { _ = Task.Run(async () => { if (_settings.AutoUpdates) { // check update every 5 hours var timer = new PeriodicTimer(TimeSpan.FromHours(5)); - await Ioc.Default.GetRequiredService<Updater>().UpdateAppAsync(); + await Ioc.Default.GetRequiredService<Updater>().UpdateAppAsync(_updateCancellation.Token); - while (await timer.WaitForNextTickAsync()) + while (await timer.WaitForNextTickAsync(_updateCancellation.Token)) // check updates on startup - await Ioc.Default.GetRequiredService<Updater>().UpdateAppAsync(); + await Ioc.Default.GetRequiredService<Updater>().UpdateAppAsync(_updateCancellation.Token); } - }); + }, _updateCancellation.Token); } public void Dispose() { if (!_disposed) { + _updateCancellation.Cancel(); API.SaveAppAllSettings(); _disposed = true; } }
🧹 Nitpick comments (1)
Flow.Launcher/App.xaml.cs (1)
96-97: Track technical debt for static instance cleanup.The TODOs indicate a plan to remove static instances of
InternationalizationManagerandThemeManager. Consider creating issues to track this technical debt.Would you like me to create issues to track the removal of these static instances? This will help ensure the cleanup tasks aren't forgotten.
Also applies to: 117-118
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Flow.Launcher.Core/Resource/Theme.cs(12 hunks)Flow.Launcher.Core/Updater.cs(5 hunks)Flow.Launcher/App.xaml.cs(5 hunks)Flow.Launcher/Helper/HotKeyMapper.cs(3 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Views/ActionKeywordSetting.xaml.cs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- Plugins/Flow.Launcher.Plugin.Explorer/Views/ActionKeywordSetting.xaml.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Flow.Launcher/Helper/HotKeyMapper.cs
- Flow.Launcher.Core/Resource/Theme.cs
🔇 Additional comments (5)
Flow.Launcher/App.xaml.cs (3)
30-30: LGTM! Good use of interface abstraction.The change from
PublicAPIInstancetoIPublicAPIimproves loose coupling and aligns with dependency injection principles.
35-63: Improve error handling and code organization in constructor.While the DI setup is well-structured, consider:
- Adding error handling for settings and DI container initialization
- Moving DI container setup to a separate method for better organization
Apply this diff to improve the constructor:
public App() { try { // Initialize settings InitializeSettings(); // Configure and build DI container var host = ConfigureDependencyInjection(); Ioc.Default.ConfigureServices(host.Services); // Initialize the public API and Settings first API = Ioc.Default.GetRequiredService<IPublicAPI>(); _settings.Initialize(); } catch (Exception ex) { MessageBox.Show($"Failed to initialize application: {ex.Message}", "Critical Error", MessageBoxButton.OK, MessageBoxImage.Error); throw; } } private void InitializeSettings() { var storage = new FlowLauncherJsonStorage<Settings>(); _settings = storage.Load(); _settings.SetStorage(storage); _settings.WMPInstalled = WindowsMediaPlayerHelper.IsWindowsMediaPlayerInstalled(); } private IHost ConfigureDependencyInjection() { return Host.CreateDefaultBuilder() .UseContentRoot(AppContext.BaseDirectory) .ConfigureServices(services => services .AddSingleton(_ => _settings) .AddSingleton(sp => new Updater(sp.GetRequiredService<IPublicAPI>(), Launcher.Properties.Settings.Default.GithubRepo)) .AddSingleton<Portable>() .AddSingleton<SettingWindowViewModel>() .AddSingleton<IAlphabet, PinyinAlphabet>() .AddSingleton<StringMatcher>() .AddSingleton<Internationalization>() .AddSingleton<IPublicAPI, PublicAPIInstance>() .AddSingleton<MainViewModel>() .AddSingleton<Theme>() ).Build(); }
210-210: LGTM! Good use of dependency injection.The change correctly uses DI to get the MainViewModel instance.
Flow.Launcher.Core/Updater.cs (2)
25-28: LGTM! Thread-safe property implementation.The
init-only property ensures thread safety by preventing modifications after initialization, and the private field follows C# naming conventions.
37-104: LGTM! Consistent use of dependency injection.The changes effectively utilize the injected
_apiinstance throughout the method, maintaining consistent error handling and translation support while removing the need for passing the API as a parameter.
This comment has been minimized.
This comment has been minimized.
|
@Jack251970 I make a few more service to be injected via constructor when possible. Please take a brief look. I think the status of this pr is fine to merge. It's hard to get such kind of refactor to work, very nice work! |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
Flow.Launcher.Infrastructure/StringMatcher.cs (3)
1-23: 🛠️ Refactor suggestionConsider using constructor injection instead of service locator pattern.
While the changes introduce dependency injection, using
Ioc.Default(service locator pattern) is generally considered an anti-pattern. The constructor injection forSettingsis a step in the right direction, but the service locator is still used elsewhere.Apply this diff to improve the dependency injection approach:
-public StringMatcher(IAlphabet alphabet, Settings settings) +public StringMatcher(IAlphabet alphabet, Settings settings, IServiceProvider serviceProvider) { - _alphabet = alphabet; + _alphabet = alphabet ?? throw new ArgumentNullException(nameof(alphabet)); + _settings = settings ?? throw new ArgumentNullException(nameof(settings)); UserSettingSearchPrecision = settings.QuerySearchPrecision; }
25-28: 🛠️ Refactor suggestionReplace service locator with constructor injection.
The static
FuzzySearchmethod uses service locator pattern which makes testing and dependency tracking harder.Apply this diff to improve the implementation:
-public static MatchResult FuzzySearch(string query, string stringToCompare) -{ - return Ioc.Default.GetRequiredService<StringMatcher>().FuzzyMatch(query, stringToCompare); -} +public MatchResult FuzzySearch(string query, string stringToCompare) +{ + return FuzzyMatch(query, stringToCompare); +}
246-252:⚠️ Potential issueFix numeric character comparison in IsAcronymNumber.
The current implementation incorrectly compares characters with numeric values. The comparison
stringToCompare[compareStringIndex] >= 0 && stringToCompare[compareStringIndex] <= 9compares character codes with integers.Apply this diff to fix the numeric check:
-private static bool IsAcronymNumber(string stringToCompare, int compareStringIndex) - => stringToCompare[compareStringIndex] >= 0 && stringToCompare[compareStringIndex] <= 9; +private static bool IsAcronymNumber(string stringToCompare, int compareStringIndex) + => char.IsDigit(stringToCompare[compareStringIndex]);Flow.Launcher/App.xaml.cs (1)
161-165: 🛠️ Refactor suggestionMake update checks cancellable.
The update check task should be cancellable when the app exits to prevent potential memory leaks or hanging operations.
Flow.Launcher/PublicAPIInstance.cs (3)
40-44: 🛠️ Refactor suggestionConsider using constructor injection over service location for better testability.
Using
Ioc.Default.GetRequiredService<T>()within the constructor introduces a dependency on the service locator pattern, which can make the code harder to test and less maintainable. Prefer using constructor injection to explicitly declare dependencies.Apply this diff to improve testability:
-public PublicAPIInstance() +public PublicAPIInstance( + SettingWindowViewModel settingsVM, + MainViewModel mainVM) { - _settingsVM = Ioc.Default.GetRequiredService<SettingWindowViewModel>(); - _mainVM = Ioc.Default.GetRequiredService<MainViewModel>(); + _settingsVM = settingsVM; + _mainVM = mainVM; GlobalHotkey.hookedKeyboardCallback = KListener_hookedKeyboardCallback; WebRequest.RegisterPrefix("data", new DataWebRequestFactory()); }
88-88:⚠️ Potential issueAdd proper async/await handling for image loader save.
Similar to the update check, discarding the Task could hide potential errors.
Apply this diff to properly handle async operations:
-_ = ImageLoader.Save(); +try +{ + await ImageLoader.Save(); +} +catch (Exception ex) +{ + LogException(nameof(PublicAPIInstance), "Failed to save image loader", ex); +}
80-81:⚠️ Potential issueAdd proper error handling for update check.
The current implementation discards the Task with
_, which could hide potential errors. Additionally, using service locator here introduces the same testability issues as in the constructor.Apply this diff to improve error handling and testability:
-// Must use Ioc.Default.GetRequiredService<Updater>() to avoid circular dependency -public void CheckForNewUpdate() => _ = Ioc.Default.GetRequiredService<Updater>().UpdateAppAsync(false); +private readonly Updater _updater; + +public PublicAPIInstance( + SettingWindowViewModel settingsVM, + MainViewModel mainVM, + Updater updater) +{ + _updater = updater; + // ... existing constructor code ... +} + +public async Task CheckForNewUpdate() +{ + try + { + await _updater.UpdateAppAsync(false); + } + catch (Exception ex) + { + LogException(nameof(PublicAPIInstance), "Failed to check for updates", ex); + ShowMsgError("Update Check Failed", ex.Message); + } +}
🧹 Nitpick comments (4)
Flow.Launcher.Infrastructure/StringMatcher.cs (1)
12-12: Use explicit type declaration for better readability.The use of
new()target-typed new expression can make the code less readable, especially for complex types.Apply this diff to improve readability:
-private readonly MatchOption _defaultMatchOption = new(); +private readonly MatchOption _defaultMatchOption = new MatchOption();Flow.Launcher/ViewModel/SettingWindowViewModel.cs (2)
1-1: Remove unused import.After moving to constructor injection, the
CommunityToolkit.Mvvm.DependencyInjectionimport is no longer needed.-using CommunityToolkit.Mvvm.DependencyInjection;
7-47: Great architectural improvements!The changes successfully:
- Replace service locator with proper dependency injection
- Avoid cyclic dependencies through constructor injection
- Improve testability by making dependencies explicit
- Maintain existing functionality while improving the architecture
This aligns well with the PR objectives of implementing dependency injection for better development experience.
Flow.Launcher/App.xaml.cs (1)
94-95: Track TODO comments for future cleanup.The TODO comments indicate plans to remove static instances of
InternationalizationManagerandThemeManager. These should be tracked to ensure they're addressed.Would you like me to create issues to track the removal of these static instances?
Also applies to: 115-116
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Flow.Launcher.Infrastructure/StringMatcher.cs(2 hunks)Flow.Launcher/App.xaml.cs(5 hunks)Flow.Launcher/PublicAPIInstance.cs(6 hunks)Flow.Launcher/SettingWindow.xaml.cs(5 hunks)Flow.Launcher/ViewModel/SettingWindowViewModel.cs(1 hunks)
🔇 Additional comments (9)
Flow.Launcher/SettingWindow.xaml.cs (5)
21-22: LGTM! Fields are properly marked as readonly.The new fields for injected dependencies follow good practices by being marked as readonly, preventing accidental modifications after initialization.
27-35: Consider the trade-offs of service locator pattern.While using
Ioc.Default.GetRequiredService(service locator pattern) makes dependencies implicit and complicates testing, I understand from the PR objectives that this is an intentional transitional solution to maintain compatibility withApp.API. However, consider planning a future refactor to:
- Make dependencies explicit through constructor injection
- Improve testability
- Make class dependencies clear to consumers
134-147: LGTM! Appropriate use of static method.The
IsPositionValidmethod has been correctly marked as static since it doesn't access any instance members.
154-154: LGTM! Removed unnecessary this keyword.Removing the
thiskeyword improves code clarity as it's not needed for disambiguation here.Also applies to: 163-163
169-169: LGTM! Correctly using injected services.The
PaneDatainitialization properly uses the injected services_updaterand_portable.Flow.Launcher/ViewModel/SettingWindowViewModel.cs (2)
11-14: LGTM! Good use of constructor injection.The constructor now properly accepts dependencies through injection, which is a significant improvement over the service locator pattern.
21-21: LGTM! Clean property implementations.The properties correctly delegate to the injected settings instance, maintaining functionality while leveraging dependency injection.
Also applies to: 26-27, 32-33, 38-39, 44-45
Flow.Launcher/App.xaml.cs (2)
30-30: Good use of interface abstraction!Changing the API property type from
PublicAPIInstancetoIPublicAPIimproves abstraction and testability.
206-209: Good use of dependency injection for second instance handling!The change consistently follows the DI pattern by using
Ioc.Default.GetRequiredService<MainViewModel>().
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Infrastructure/StringMatcher.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (4)
Flow.Launcher.Infrastructure/StringMatcher.cs (4)
1-1: Consider using constructor injection instead of service locator pattern.While the change introduces dependency injection, using
Ioc.Default(service locator pattern) is generally considered an anti-pattern as it makes dependencies implicit and harder to track.Apply this diff to improve the implementation:
-using CommunityToolkit.Mvvm.DependencyInjection; public StringMatcher(IAlphabet alphabet, Settings settings) { _alphabet = alphabet ?? throw new ArgumentNullException(nameof(alphabet)); - UserSettingSearchPrecision = settings.QuerySearchPrecision; + UserSettingSearchPrecision = settings?.QuerySearchPrecision ?? throw new ArgumentNullException(nameof(settings)); }Also applies to: 18-21
30-33: Refactor static method to use dependency injection properly.The static
FuzzySearchmethod uses service locator pattern which makes testing and dependency tracking harder.Apply this diff to improve the implementation:
-public static MatchResult FuzzySearch(string query, string stringToCompare) -{ - return Ioc.Default.GetRequiredService<StringMatcher>().FuzzyMatch(query, stringToCompare); -} +public MatchResult FuzzySearch(string query, string stringToCompare) +{ + return FuzzyMatch(query, stringToCompare); +}
256-257: Fix numeric character comparison in IsAcronymNumber.The current implementation incorrectly compares characters with numeric values.
Apply this diff to fix the numeric check:
-private static bool IsAcronymNumber(string stringToCompare, int compareStringIndex) - => stringToCompare[compareStringIndex] >= 0 && stringToCompare[compareStringIndex] <= 9; +private static bool IsAcronymNumber(string stringToCompare, int compareStringIndex) + => char.IsDigit(stringToCompare[compareStringIndex]);
251-254: LGTM: Method visibility changes improve code organization.The conversion of helper methods to static improves code organization by clearly indicating these methods don't depend on instance state.
Also applies to: 260-261
|
@taooceros Hi, I fix unit test build issue and remove usage of _settingsVM._settings, could you please check if this works for you? |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
Flow.Launcher/PublicAPIInstance.cs (3)
42-47: 🛠️ Refactor suggestionConsider using constructor injection over service location.
Using
Ioc.Default.GetRequiredService<T>()within the constructor introduces a dependency on the service locator pattern, which can make the code harder to test and less maintainable.Apply this diff to use constructor injection:
-public PublicAPIInstance() +public PublicAPIInstance( + Settings settings, + SettingWindowViewModel settingsVM, + MainViewModel mainVM) { - _settings = Ioc.Default.GetRequiredService<Settings>(); - _settingsVM = Ioc.Default.GetRequiredService<SettingWindowViewModel>(); - _mainVM = Ioc.Default.GetRequiredService<MainViewModel>(); + _settings = settings; + _settingsVM = settingsVM; + _mainVM = mainVM; GlobalHotkey.hookedKeyboardCallback = KListener_hookedKeyboardCallback; WebRequest.RegisterPrefix("data", new DataWebRequestFactory()); }
83-84:⚠️ Potential issueAdd proper error handling for update check.
The current implementation discards the Task with
_, which could hide potential errors.Apply this diff to properly handle async operations:
-public void CheckForNewUpdate() => _ = Ioc.Default.GetRequiredService<Updater>().UpdateAppAsync(false); +public async Task CheckForNewUpdate() +{ + try + { + await Ioc.Default.GetRequiredService<Updater>().UpdateAppAsync(false); + } + catch (Exception ex) + { + LogException(nameof(PublicAPIInstance), "Failed to check for updates", ex); + ShowMsgError("Update Check Failed", ex.Message); + } +}
91-91:⚠️ Potential issueAdd proper error handling for image loader save.
Similar to the update check, discarding the Task could hide potential errors.
Apply this diff to properly handle async operations:
-_ = ImageLoader.Save(); +try +{ + await ImageLoader.Save(); +} +catch (Exception ex) +{ + LogException(nameof(PublicAPIInstance), "Failed to save image loader", ex); +}
🧹 Nitpick comments (1)
Flow.Launcher/PublicAPIInstance.cs (1)
253-254: Avoid direct access to internal settings field.Accessing
_settingsdirectly bypasses any encapsulation provided bySettings. Consider using properties or methods to access settings.Apply this diff to use proper encapsulation:
-var explorerInfo = _settings.CustomExplorer; +var explorerInfo = _settingsVM.Settings.CustomExplorer; -var browserInfo = _settings.CustomBrowser; +var browserInfo = _settingsVM.Settings.CustomBrowser;Also applies to: 274-275
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher/PublicAPIInstance.cs(5 hunks)Flow.Launcher/ViewModel/SettingWindowViewModel.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: Report (PR)
🔇 Additional comments (5)
Flow.Launcher/ViewModel/SettingWindowViewModel.cs (4)
6-8: LGTM! Good use of encapsulation and immutability.The changes demonstrate good practices:
- Using
partialclass for potential extensions- Private readonly field for better encapsulation
- Immutable field that can only be set in constructor
10-13: LGTM! Good use of constructor injection.The constructor now properly follows dependency injection pattern, which:
- Improves testability
- Makes dependencies explicit
- Avoids service locator anti-pattern
15-21: LGTM! Clear documentation and straightforward implementation.The Save method is well-documented and follows single responsibility principle.
23-45: LGTM! Clean property delegation pattern.The properties provide a clean abstraction over settings while avoiding state duplication.
Flow.Launcher/PublicAPIInstance.cs (1)
28-37: LGTM! Good separation of concerns with DI.The addition of
CommunityToolkit.Mvvm.DependencyInjectionand the newSettingsfield properly separates configuration concerns.
@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 (2)
Flow.Launcher/PublicAPIInstance.cs (2)
81-82: 🛠️ Refactor suggestionService locator pattern introduces testability concerns.
Using
Ioc.Default.GetRequiredService<Updater>()makes the code harder to test and maintain. While the comment explains it's to avoid circular dependency, consider restructuring to use constructor injection.Consider this alternative approach:
-// Must use Ioc.Default.GetRequiredService<Updater>() to avoid circular dependency -public void CheckForNewUpdate() => _ = Ioc.Default.GetRequiredService<Updater>().UpdateAppAsync(false); +private readonly Lazy<Updater> _updater; + +public PublicAPIInstance(Settings settings, MainViewModel mainVM, Func<Updater> updaterFactory) +{ + _updater = new Lazy<Updater>(updaterFactory); + // ... other initialization +} + +public void CheckForNewUpdate() => _ = _updater.Value.UpdateAppAsync(false);
88-89:⚠️ Potential issueImprove async operation handling in SaveAppAllSettings.
The method discards the Task from
ImageLoader.Save()with_, which could hide potential errors.Apply this diff to properly handle the async operation:
public void SaveAppAllSettings() { PluginManager.Save(); _mainVM.Save(); _settings.Save(); - _ = ImageLoader.Save(); + try + { + await ImageLoader.Save(); + } + catch (Exception ex) + { + LogException(nameof(PublicAPIInstance), "Failed to save image loader", ex); + } }
🧹 Nitpick comments (2)
Flow.Launcher/PublicAPIInstance.cs (2)
28-30: Consider the implications of direct Settings dependency.The change from
SettingWindowViewModeltoSettingsreduces a layer of abstraction. While this aligns with the PR's objective of improving dependency injection, ensure that UI-specific settings logic isn't leaked into this class.Also applies to: 36-36
41-47: Constructor changes look good but consider constructor injection for all dependencies.The removal of
PinyinAlphabetand switch toSettingssimplifies the constructor. However, the use ofIoc.Default.GetRequiredServiceelsewhere in the class suggests not all dependencies are constructor-injected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/PublicAPIInstance.cs(5 hunks)
🔇 Additional comments (2)
Flow.Launcher/PublicAPIInstance.cs (2)
109-109: LGTM: Settings window opening simplified.The removal of the
_settingsVMparameter inSingletonWindowOpener.Open<SettingWindow>()aligns with the dependency injection improvements.
251-251: Direct access to settings properties is appropriate here.The direct access to
_settings.CustomExplorerand_settings.CustomBrowseris cleaner than the previous approach through_settingsVM. This aligns with the PR's objective of improving the development experience.Also applies to: 272-272
Introduce dependency injection for accessing the
APIproperty in anywhere inside the project.This can improve development experience. E.g. We can put the
MessgaeBoxExtoFlow.Launcherproject and we can access the design page like this: