Use Flow.Launcher.Localization to improve code quality #3765
Conversation
|
🥷 Code experts: jjw24 Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
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. |
1 similar comment
|
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 replaces custom localization and enum-binding implementations with the Flow.Launcher.Localization package, removes obsolete converters and project references, and updates XAML and code to use the new Localize API.
- Swap out
LocalizationConverterandEnumBindingSourceforFlow.Launcher.Localizationattributes and helpers - Remove unnecessary project references and add package references (
MemoryPack,NLog,Flow.Launcher.Localization) - Update all enum bindings in XAML and translation calls in code to use
Display/Valueand theLocalizestatic class
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Plugins/Flow.Launcher.Plugin.Program/Main.cs | Compute cache directory via parent folders and move cache files |
| Plugins/Flow.Launcher.Plugin.Program/Flow.Launcher.Plugin.Program.csproj | Remove Infrastructure reference; add MemoryPack and NLog |
| Plugins/Flow.Launcher.Plugin.Calculator/Views/CalculatorSettings.xaml.cs | Change constructor to accept Settings and instantiate VM |
| Plugins/Flow.Launcher.Plugin.Calculator/Views/CalculatorSettings.xaml | Replace enum-binding resource with AllDecimalSeparator binding |
| Plugins/Flow.Launcher.Plugin.Calculator/ViewModels/SettingsViewModel.cs | Introduce AllDecimalSeparator list and SelectedDecimalSeparator property |
| Plugins/Flow.Launcher.Plugin.Calculator/Main.cs | Rename constants, switch to Localize calls, make some methods static |
| Plugins/Flow.Launcher.Plugin.Calculator/Flow.Launcher.Plugin.Calculator.csproj | Remove Core reference; add Flow.Launcher.Localization |
| Plugins/Flow.Launcher.Plugin.Calculator/DecimalSeparator.cs | Replace TypeConverter attributes with EnumLocalize attrs |
| Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/CustomBrowserSetting.xaml | Update enum binding to AllBrowserTypes with Display/Value |
| Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs | Implement OnPropertyChanged; add AllBrowserTypes list |
| Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs | Replace _context field with Context property; use Localize |
| Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs | Update logging calls to use Context |
| Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj | Remove Infrastructure reference; add Flow.Launcher.Localization |
| Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs | Use Context instead of _context for logging and stopwatch |
| Plugins/Flow.Launcher.Plugin.BrowserBookmark/Commands/BookmarkLoader.cs | Use Context for FuzzySearch |
| Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs | Use Context for logging and stopwatch |
| Flow.Launcher.Infrastructure/UI/EnumBindingSource.cs | Remove obsolete enum-binding extension |
| Flow.Launcher.Core/Resource/LocalizationConverter.cs | Remove obsolete localization converter |
📝 Walkthrough""" WalkthroughThis set of changes removes obsolete localization and enum binding infrastructure, replacing it with a new localization package. Plugins are updated to use new localization attributes and context access patterns. XAML bindings are refactored to use view model-driven collections for localized display. Project files are updated to reference the new localization package, and code is refactored for consistency and maintainability. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Plugin
participant Localize
participant ViewModel
User->>Plugin: Open settings panel
Plugin->>ViewModel: Instantiate (with settings)
ViewModel->>Localize: Get localized enum values (e.g., DecimalSeparatorLocalized.GetValues())
Localize-->>ViewModel: Return list of localized values
ViewModel-->>Plugin: Provide AllDecimalSeparator/AllBrowserTypes collection
Plugin->>User: Display ComboBox with localized values
User->>Plugin: Select value
Plugin->>ViewModel: Update SelectedValue property
ViewModel->>Plugin: Notify property changed (if value changed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (1)
243-248: Consider defensive programming for cache file operations.The cache file migration logic looks correct, but consider adding validation to ensure the derived cache directory path is valid before attempting file operations.
The file move operations correctly use the dynamically derived cache directory. You might consider logging when cache files are successfully migrated for better observability:
MoveFile(oldWin32CacheFile, newWin32CacheFile); + if (!File.Exists(oldWin32CacheFile) && File.Exists(newWin32CacheFile)) + Context.API.LogInfo(ClassName, "Successfully migrated Win32 cache file"); var oldUWPCacheFile = Path.Combine(cacheDirectory, $"{UwpCacheName}.cache"); var newUWPCacheFile = Path.Combine(pluginCacheDirectory, $"{UwpCacheName}.cache"); MoveFile(oldUWPCacheFile, newUWPCacheFile); + if (!File.Exists(oldUWPCacheFile) && File.Exists(newUWPCacheFile)) + Context.API.LogInfo(ClassName, "Successfully migrated UWP cache file");Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (1)
29-29: Consider initialization safety for static Context.The static
Contextproperty withnull!default could lead to null reference exceptions if accessed before initialization. Consider adding a validation check or using a different initialization pattern.-internal static PluginInitContext Context { get; set; } = null!; +internal static PluginInitContext Context { get; set; } = null!;Alternatively, consider adding a property that throws a descriptive exception if accessed before initialization:
+private static PluginInitContext? _context; +internal static PluginInitContext Context +{ + get => _context ?? throw new InvalidOperationException("Plugin not initialized"); + set => _context = value; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
Flow.Launcher.Core/Resource/LocalizationConverter.cs(0 hunks)Flow.Launcher.Infrastructure/UI/EnumBindingSource.cs(0 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs(4 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Commands/BookmarkLoader.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs(5 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs(4 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs(7 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs(3 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/CustomBrowserSetting.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.Calculator/DecimalSeparator.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Calculator/Flow.Launcher.Plugin.Calculator.csproj(1 hunks)Plugins/Flow.Launcher.Plugin.Calculator/Main.cs(6 hunks)Plugins/Flow.Launcher.Plugin.Calculator/ViewModels/SettingsViewModel.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Calculator/Views/CalculatorSettings.xaml(2 hunks)Plugins/Flow.Launcher.Plugin.Calculator/Views/CalculatorSettings.xaml.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Program/Flow.Launcher.Plugin.Program.csproj(1 hunks)Plugins/Flow.Launcher.Plugin.Program/Main.cs(1 hunks)
💤 Files with no reviewable changes (2)
- Flow.Launcher.Core/Resource/LocalizationConverter.cs
- Flow.Launcher.Infrastructure/UI/EnumBindingSource.cs
🧰 Additional context used
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs
[warning] 61-61:
favicons is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (26)
Plugins/Flow.Launcher.Plugin.Program/Flow.Launcher.Plugin.Program.csproj (1)
66-66: Verify package versions for security and currency.The added package references use specific versions that may not be the latest. Please ensure these versions are secure and up-to-date.
What are the latest stable versions of MemoryPack and NLog NuGet packages, and are there any known security vulnerabilities in versions 1.21.4 and 4.7.10 respectively?Also applies to: 72-72
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj (1)
98-98: Ensure localization package version consistencyConfirm that
Flow.Launcher.Localizationversion 0.0.3 is used uniformly across all plugin projects to prevent version conflicts.Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs (1)
30-32: Consistent context usage in logging callsAll exception logging now correctly uses
Main.Context.API.LogExceptioninstead of the old_contextfield, aligning with the new static property. No issues detected.Also applies to: 42-43, 52-53, 64-65
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Commands/BookmarkLoader.cs (1)
12-12: Context property used for fuzzy searchThe calls to
Main.Context.API.FuzzySearchhave been updated correctly for both bookmark name and URL. Functionality remains intact.Also applies to: 16-16
Plugins/Flow.Launcher.Plugin.Calculator/Flow.Launcher.Plugin.Calculator.csproj (1)
65-65: Add Localization package dependencyThe new reference to
Flow.Launcher.Localizationv0.0.3 matches the refactor goal. Verify alignment with other plugins to maintain consistency.Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs (1)
52-53: Update context references consistentlyAll instances of
Main._contexthave been replaced withMain.Context, including exception logging and stopwatch logging calls. This change is consistent with the refactoring plan and introduces no functional regressions.Also applies to: 87-90, 101-102, 114-115, 189-190
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs (1)
48-48: LGTM! Consistent refactoring from static field to property.The changes consistently update all API access points from
Main._context.APItoMain.Context.API, which aligns with the broader refactor in the plugin. The functionality remains unchanged while improving code organization.Also applies to: 61-61, 128-128, 193-193
Plugins/Flow.Launcher.Plugin.Calculator/Views/CalculatorSettings.xaml.cs (1)
15-21: LGTM! Good encapsulation improvement.The constructor change improves encapsulation by accepting the raw
Settingsobject and handlingSettingsViewModelinstantiation internally. This simplifies the interface for callers and gives the view control over its own ViewModel creation.Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/CustomBrowserSetting.xaml (1)
144-147: LGTM! Modernized ComboBox binding approach.The change from enum binding source to collection-based binding with
DisplayMemberPathandSelectedValuePathis a good improvement. This approach provides better localization support and follows MVVM best practices.Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (3)
22-22: LGTM! Context field to property refactor.The change from static field
_contextto static propertyContextimproves encapsulation and provides a cleaner API. The usage is consistently updated throughout the file.Also applies to: 32-32, 45-45
87-87: LGTM! Consistent Context.API usage.All API access points have been consistently updated to use the new
Contextproperty, maintaining the same functionality while aligning with the refactored architecture.Also applies to: 111-111, 220-220, 227-227, 229-229
195-195: LGTM! Localization refactor to static methods.The transition from
Context.API.GetTranslationtoLocalizestatic methods aligns perfectly with the PR objectives of adopting Flow.Launcher.Localization to improve code quality. This approach provides better type safety and consistency.Also applies to: 200-200, 214-215
Plugins/Flow.Launcher.Plugin.Calculator/Views/CalculatorSettings.xaml (2)
9-9: LGTM! Improved design-time experience.Adding the design-time DataContext for
SettingsViewModelwill improve the development experience by providing IntelliSense and design-time data binding in the XAML designer.
40-43: LGTM! Modernized ComboBox binding pattern.The change from enum binding source to view model collection binding follows MVVM best practices and aligns with the new localization approach. Using
DisplayMemberPathandSelectedValuePathprovides better flexibility and localization support.Plugins/Flow.Launcher.Plugin.Calculator/DecimalSeparator.cs (2)
1-16: LGTM! Clean migration to new localization system.The migration from the old
LocalizationConverterapproach to the newEnumLocalizeattributes is well-implemented:
- Proper use of
[EnumLocalize]on the enum- Correct
[EnumLocalizeKey]attributes withnameof()for compile-time safety- Clean removal of obsolete dependencies
8-8: ```shell
#!/bin/bash1. Search .resx files for the full localization keys (typo and correct spelling)
echo "Searching for 'flowlauncher_plugin_calculator_decimal_seperator' in .resx:"
rg -i "flowlauncher_plugin_calculator_decimal_seperator" -g '*.resx'echo -e "\nSearching for 'flowlauncher_plugin_calculator_decimal_separator' in .resx:"
rg -i "flowlauncher_plugin_calculator_decimal_separator" -g '*.resx'2. Check if any alias or using directive maps a Resources class to Localize
echo -e "\nSearching for 'using .*Localize' in the plugin code:"
rg -n "using .*Localize" -t csecho -e "\nSearching for assignment alias 'Localize =' in the code:"
rg -n "Localize *= *" -t cs3. Show the top of DecimalSeparator.cs to see namespace/imports
echo -e "\nHeader of DecimalSeparator.cs:"
sed -n '1,30p' Plugins/Flow.Launcher.Plugin.Calculator/DecimalSeparator.cs</details> <details> <summary>Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs (3)</summary> `54-62`: **LGTM! Proper enum localization implementation.** The `BrowserType` enum correctly uses the new localization attributes: - `[EnumLocalize]` on the enum declaration - `[EnumLocalizeValue]` with string literals for each member This provides proper localization support for the browser type selection. --- `38-38`: **LGTM! Clean approach for UI binding.** The `AllBrowserTypes` property provides a localized collection that can be easily bound to UI controls, replacing the need for enum-based XAML bindings. This aligns well with the removal of `EnumBindingSource`. --- `17-35`: **LGTM! Improved property change notification.** The property setters now properly implement change detection before firing `OnPropertyChanged()`, which prevents unnecessary notifications and improves performance. </details> <details> <summary>Plugins/Flow.Launcher.Plugin.Calculator/ViewModels/SettingsViewModel.cs (3)</summary> `18-18`: **LGTM! Clean collection initialization for UI binding.** The `AllDecimalSeparator` property provides a localized collection that replaces the need for enum-based XAML binding, consistent with the new localization approach. --- `20-31`: **LGTM! Proper two-way data binding implementation.** The `SelectedDecimalSeparator` property correctly implements: - Direct access to the underlying Settings property - Change detection before notification - Proper `OnPropertyChanged()` call for UI updates This enables clean two-way binding in the UI. --- `11-11`: **LGTM! Proper localization initialization.** The call to `DecimalSeparatorLocalized.UpdateLabels(AllDecimalSeparator)` ensures the localized labels are updated at initialization time, which is necessary for proper display. </details> <details> <summary>Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (4)</summary> `26-27`: **LGTM! Cleaner constant definitions.** The decimal separator constants are now more readable and consistent with their usage throughout the class. --- `141-184`: **LGTM! Good refactoring to static methods.** Converting these utility methods to static is appropriate since they don't depend on instance state. The `ChangeDecimalSeparator` method also has improved null safety. --- `198-198`: **LGTM! Simplified settings panel creation.** Passing the `Settings` object directly to the constructor is cleaner and aligns with the updated `CalculatorSettings` constructor signature. --- `72-76`: ```shell #!/bin/bash # Search for any Localize class definitions in the C# sources echo "Searching for Localize class definitions:" rg "class Localize" -n . # Look for the auto‐generated designer file that may contain the Localize class echo -e "\nLooking for Localize.Designer.cs:" fd Localize.Designer.cs # List all .resx files to locate where the resources are defined echo -e "\nListing all .resx files:" find . -type f -name "*.resx" # Search .resx files for the specific resource names echo -e "\nSearching .resx for ‘flowlauncher_plugin_calculator_not_a_number’:" grep -Rn "<data name=\"flowlauncher_plugin_calculator_not_a_number\"" --include="*.resx" echo -e "\nSearching .resx for ‘flowlauncher_plugin_calculator_expression_not_complete’:" grep -Rn "<data name=\"flowlauncher_plugin_calculator_expression_not_complete\"" --include="*.resx"
|
@jjw24 I found analyzers cannot work with CI build here?!! 
I can confirm the build of Calculator project works fine for me with Visual Studio. 
|
Hm, not sure what's going without researching myself. |
|
Actually are you missing the project reference? That's the usual error with this. Edit: nvm I see it's already added. |
|
As you can see, GitHub Action also works well here... It seems to be the problem related to AppVeyor |
|
@Jack251970 LGTM. Please review. |
| return; | ||
| Settings.SortOption = value; | ||
| OnPropertyChanged(nameof(SelectedEverythingSortOption)); | ||
| OnPropertyChanged(nameof(FastSortWarningVisibility)); |
There was a problem hiding this comment.
May be we should update them in on language change too?
There was a problem hiding this comment.
May be we should update them in on language change too?
No, I do not think so because when we are editing something in setting panel, the language will not change which means for translations that are get in SettingViewModel will be refreshed during setting panel construction. So we only need to update the AllEverythingSortOptions translations which cache translations in SettingViewModel instance.
|
let's merge it before the decimal pr |
Oh, I am unable to do that😢 AppVeyor CI fails to build with Flow.Launcher.Localization package
|
Do you have this warning on your Visual studio? |
Nope |
Could you try to change the required analyzer version of Flow.Launcher.Localization to 4.12.0? |
how can I do that? |
CHANGES
LocalizationConverter.cs&EnumBindingSource.csTEST