Remove TranslationConverter & project reference in Explorer plugin #3595
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
This PR removes the old TranslationConverter class and project references from the Explorer plugin, replacing XAML bindings with a new LocalizedDescription property that uses DI for translations, and updates package references to include Droplex.
- Replace
TranslationConverterusage in XAML with directLocalizedDescriptionbindings - Add
LocalizedDescriptionproperties in ViewModel and settings models to callGetTranslationvia DI - Remove project references to Core/Infrastructure in Explorer plugin and add Droplex NuGet package
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Plugins/Flow.Launcher.Plugin.Explorer/Views/ExplorerSettings.xaml | Removed converter namespace and replaced binding with LocalizedDescription |
| Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/ActionKeywordModel.cs | Added LocalizedDescription property using Main.Context.API |
| Plugins/Flow.Launcher.Plugin.Explorer/Flow.Launcher.Plugin.Explorer.csproj | Removed Core/Infrastructure project refs, added Droplex package |
| Flow.Launcher/SettingPages/Views/SettingsPaneHotkey.xaml | Switched from TranslationConverter binding to LocalizedDescription |
| Flow.Launcher/Resources/SettingWindowStyle.xaml | Removed TranslationConverter resource entry |
| Flow.Launcher.Infrastructure/UserSettings/CustomShortcutModel.cs | Added LocalizedDescription with static DI resolution |
| Flow.Launcher.Core/Resource/TranslationConverter.cs | Deleted obsolete converter class |
Comments suppressed due to low confidence (3)
Flow.Launcher.Infrastructure/UserSettings/CustomShortcutModel.cs:61
- [nitpick] Private static fields should follow naming conventions (e.g.
_apiors_api) to improve readability and maintain consistency with the codebase style.
private static IPublicAPI api = null;
Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/ActionKeywordModel.cs:27
- Instead of calling the API via
Main.Context, consider injectingIPublicAPIinto your ViewModel (e.g. via constructor) to follow DI patterns consistently and improve testability.
public string LocalizedDescription => Main.Context.API.GetTranslation(Description);
Flow.Launcher.Infrastructure/UserSettings/CustomShortcutModel.cs:58
- [nitpick] The
LocalizedDescriptionlogic is duplicated in multiple models; consider extracting a shared base class or helper method to reduce duplication and centralize translation logic.
public string LocalizedDescription => API.GetTranslation(Description);
|
🥷 Code experts: onesounds Jack251970, onesounds have 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: 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. |
📝 WalkthroughWalkthroughThis change removes the Changes
Sequence Diagram(s)sequenceDiagram
participant View (XAML)
participant Model
participant API
View->>Model: Bind to LocalizedDescription
Model->>API: GetTranslation(Description)
API-->>Model: Localized string
Model-->>View: LocalizedDescription value
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Flow.Launcher.Core/Resource/TranslationConverter.cs(0 hunks)Flow.Launcher.Infrastructure/UserSettings/CustomShortcutModel.cs(2 hunks)Flow.Launcher/Resources/SettingWindowStyle.xaml(0 hunks)Flow.Launcher/SettingPages/Views/SettingsPaneHotkey.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Flow.Launcher.Plugin.Explorer.csproj(1 hunks)Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/ActionKeywordModel.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Views/ExplorerSettings.xaml(2 hunks)
💤 Files with no reviewable changes (2)
- Flow.Launcher/Resources/SettingWindowStyle.xaml
- Flow.Launcher.Core/Resource/TranslationConverter.cs
🧰 Additional context used
🧬 Code Graph Analysis (1)
Flow.Launcher.Infrastructure/UserSettings/CustomShortcutModel.cs (2)
Flow.Launcher/PublicAPIInstance.cs (1)
GetTranslation(240-240)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
GetTranslation(141-141)
🪛 GitHub Actions: Check Spelling
Plugins/Flow.Launcher.Plugin.Explorer/Views/ExplorerSettings.xaml
[warning] 489-489: Spell check warning: previewpanel is not a recognized word. (unrecognized-spelling)
[warning] 496-496: Spell check warning: previewpanel is not a recognized word. (unrecognized-spelling)
[warning] 500-500: Spell check warning: previewpanel is not a recognized word. (unrecognized-spelling)
[warning] 504-504: Spell check warning: previewpanel is not a recognized word. (unrecognized-spelling)
[warning] 717-717: Spell check warning: lbx is not a recognized word. (unrecognized-spelling)
[warning] 774-774: Spell check warning: lbx is not a recognized word. (unrecognized-spelling)
[warning] 777-777: Spell check warning: lbx is not a recognized word. (unrecognized-spelling)
[warning] 778-778: Spell check warning: Lbx is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (6)
Flow.Launcher/SettingPages/Views/SettingsPaneHotkey.xaml (1)
427-427: LGTM! Clean refactoring from converter to property-based localization.The change from using
TranslationConverterto directly binding toLocalizedDescriptionaligns perfectly with the PR objectives to remove the converter and use dependency injection. This simplifies the XAML binding and centralizes localization logic in the model.Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/ActionKeywordModel.cs (1)
27-27:❓ Verification inconclusive
Verify API availability for localization.
The
LocalizedDescriptionproperty implementation looks good and aligns with the refactoring goals. However, ensure thatMain.Context.APIis properly initialized before this property is accessed to avoid potential null reference exceptions.Run the following script to verify that Main.Context.API is properly initialized:
🏁 Script executed:
#!/bin/bash # Description: Check how Main.Context.API is initialized and used # Expected: Find proper initialization and null checks where appropriate # Search for Main.Context.API usage patterns rg -A 3 -B 3 "Main\.Context\.API" --type cs # Search for Main.Context initialization rg -A 5 "Main\.Context\s*=" --type csLength of output: 21601
Verify initialization of Main.Context.API
The
LocalizedDescriptionimplementation is correct, but I wasn’t able to locate whereMain.Context(and itsAPIproperty) is assigned before use. Please confirm thatMain.Context.APIis initialized in your plugin’s startup (e.g., in theMainclass constructor orProgram.cs) so that accessingLocalizedDescriptionat runtime cannot produce a null reference.
- Locate the assignment of
Main.Context(and itsAPIfield/property) in the plugin initialization code.- Ensure that
Context.APIis non-null before any UI bindings or view-models accessLocalizedDescription.Plugins/Flow.Launcher.Plugin.Explorer/Flow.Launcher.Plugin.Explorer.csproj (1)
49-49:✅ Verification successful
Verify Droplex package version and security.
The addition of Droplex v1.7.0 aligns with the PR objectives to retain this package for downloading Everything. Please verify this is the latest stable version and check for any security advisories.
🌐 Web query:
What is the latest version of the Droplex NuGet package and are there any known security vulnerabilities?💡 Result:
Latest Version of Droplex NuGet Package
The latest version of the Droplex NuGet package is 1.7.0, which was released on August 3, 2023. This version targets .NET Core 3.1 and is compatible with that framework or higher[1][2].
Known Security Vulnerabilities
There are no publicly reported security vulnerabilities associated with the Droplex package version 1.7.0 on major sources like NuGet or its GitHub repository as of the current date[1][2].
Key points on vulnerability checking:
- NuGet and tools like
dotnet list package --vulnerablepull vulnerability data primarily from sources like the GitHub Advisory Database, covering reported vulnerabilities[6][9].- If a vulnerability exists, it usually shows up as a warning in Visual Studio or via the .NET CLI, and there is currently no such notice for Droplex 1.7.0[1][2].
- No security advisories or warnings for Droplex appear in available vulnerability management and advisory resources[1][2][9].
- Keep in mind that an absence of reported vulnerabilities does not guarantee there are none, only that none have been documented in the main public databases.
Summary Table
Package Latest Version Release Date Known Vulnerabilities Droplex 1.7.0 2023-08-03 None reported Recommendation:
To stay secure, always monitor for new advisories by regularly updating your packages, reviewing upstream security feeds, and using automated tools for vulnerability scanning[6][9].If you’re using Droplex in a sensitive or production environment, routinely check for updates or notices from its maintainers and repositories.
Citations:
- 1: https://www.nuget.org/packages/Droplex
- 2: https://github.com/jjw24/Droplex
- 3: https://www.nuget.org/packages/mimekit/
- 4: https://learn.microsoft.com/en-us/nuget/quickstart/install-and-use-a-package-in-visual-studio
- 5: https://docs.inedo.com/docs/proget-feeds-nuget
- 6: https://blog.inedo.com/nuget/vulnerabilities/
- 7: https://learn.microsoft.com/en-us/nuget/release-notes/nuget-6.8
- 8: https://www.nuget.org/packages/microsoft.io.recyclablememorystream/
- 9: https://learn.microsoft.com/en-us/nuget/concepts/auditing-packages
- 10: https://jfrog.com/blog/impala-stealer-malicious-nuget-package-payload/
Droplex package version and security verified
Droplex v1.7.0 (released Aug 3, 2023) is the latest stable version and has no publicly reported vulnerabilities. You can safely proceed with this dependency.
• File: Plugins/Flow.Launcher.Plugin.Explorer/Flow.Launcher.Plugin.Explorer.csproj (line 49)
Plugins/Flow.Launcher.Plugin.Explorer/Views/ExplorerSettings.xaml (2)
111-111: LGTM! Consistent refactoring to property-based localization.The change from using
DescriptionwithTranslationConverterto directly binding toLocalizedDescriptionis consistent with the refactoring approach used throughout the codebase. This simplifies the binding and eliminates the need for the converter.
506-506: Good cleanup of unnecessary whitespace.Minor but good cleanup removing the empty line for better code formatting.
Flow.Launcher.Infrastructure/UserSettings/CustomShortcutModel.cs (1)
4-5: LGTM: Appropriate dependencies for the new localization approach.The added using statements correctly support the dependency injection pattern for accessing the translation API.
@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 ...
|
Since we have used DI for api instance, there is no need to keep translation converter.
Also removed all project references in Explorer project.
We should keep Droplex NuGet package since it is used in Explorer plugin for Everything download.