Fix Dialog Owner & Improve Window Exiting & Improve WindowsThumbnailProvider & Fix Warnings #3467
Conversation
There was a problem hiding this comment.
Copilot reviewed 18 out of 21 changed files in this pull request and generated no comments.
Files not reviewed (3)
- Flow.Launcher/SettingPages/Views/SettingsPanePlugins.xaml: Language not supported
- Plugins/Flow.Launcher.Plugin.Explorer/Flow.Launcher.Plugin.Explorer.csproj: Language not supported
- Plugins/Flow.Launcher.Plugin.Explorer/Views/ExplorerSettings.xaml: Language not supported
Comments suppressed due to low confidence (1)
Flow.Launcher/SettingPages/ViewModels/SettingsPanePluginsViewModel.cs:114
- Ensure that the UI binding now passes a Button instance to this command since the method signature requires a parameter. If the binding is not updated accordingly, the command may fail at runtime.
private async Task OpenHelperAsync(Button button)
This comment has been minimized.
This comment has been minimized.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. If the flagged items are 🤯 false positivesIf items relate to a ...
|
|
Tested and it works well. |
There was a problem hiding this comment.
Copilot reviewed 18 out of 21 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
- Flow.Launcher/SettingPages/Views/SettingsPanePlugins.xaml: Language not supported
- Plugins/Flow.Launcher.Plugin.Explorer/Flow.Launcher.Plugin.Explorer.csproj: Language not supported
- Plugins/Flow.Launcher.Plugin.Explorer/Views/ExplorerSettings.xaml: Language not supported
|
🥷 Code experts: no user but you matched threshold 10 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:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughThis set of changes primarily refactors command handling in the Explorer plugin to use CommunityToolkit.Mvvm's Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsViewModel
participant MVVMToolkit
participant UI
User->>UI: Clicks "Add Link" button
UI->>SettingsViewModel: Invokes AddLinkCommand (via [RelayCommand])
SettingsViewModel->>MVVMToolkit: [RelayCommand] attribute triggers AddLink method
SettingsViewModel->>UI: Updates UI based on logic
sequenceDiagram
participant User
participant SettingsPanePluginsViewModel
participant Button
participant ContentDialog
User->>Button: Clicks "Open Helper"
Button->>SettingsPanePluginsViewModel: Executes OpenHelperCommand with Button as parameter
SettingsPanePluginsViewModel->>ContentDialog: Sets Owner to Window.GetWindow(button)
ContentDialog->>User: Displays dialog
Possibly related PRs
Suggested labels
Suggested reviewers
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:
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 (8)
Plugins/Flow.Launcher.Plugin.WebSearch/SearchSourceSetting.xaml.cs (2)
31-31: Warning suppression for async void methodSuppressing the VSTHRD100 warning is appropriate for event handlers and initialization methods in UI code. Consider adding a more specific justification comment to explain why async void is necessary here.
-[System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "VSTHRD100:Avoid async void methods", Justification = "<Pending>")] +[System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "VSTHRD100:Avoid async void methods", Justification = "Required for UI initialization method")]
128-128: Warning suppression for async void event handlerSuppressing the VSTHRD100 warning is appropriate for event handlers and initialization methods in UI code. Consider adding a more specific justification comment to explain why async void is necessary here.
-[System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "VSTHRD100:Avoid async void methods", Justification = "<Pending>")] +[System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "VSTHRD100:Avoid async void methods", Justification = "Required for UI event handler")]Flow.Launcher/Msg.xaml.cs (1)
62-62: Add a specific justification for the warning suppression.The warning suppression for the async void method is appropriate since this is a public API that would require significant refactoring to change, but the justification should be more specific than "".
-[System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "VSTHRD100:Avoid async void methods", Justification = "<Pending>")] +[System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "VSTHRD100:Avoid async void methods", Justification = "Public API that would require significant refactoring to change to Task-returning method")]Plugins/Flow.Launcher.Plugin.Program/Views/ProgramSetting.xaml.cs (5)
149-149: Add a specific justification for the warning suppression.The suppression is appropriate, but the justification should be more specific about why this method needs to remain as async void.
-[System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "VSTHRD100:Avoid async void methods", Justification = "<Pending>")] +[System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "VSTHRD100:Avoid async void methods", Justification = "Called from multiple event handlers; refactoring would require extensive changes")]
187-187: Add a specific justification for the warning suppression.Similar to the previous suppression, provide a clear justification for why this method uses async void.
-[System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "VSTHRD100:Avoid async void methods", Justification = "<Pending>")] +[System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "VSTHRD100:Avoid async void methods", Justification = "Called from event handlers that cannot await Task-returning methods")]
282-282: Add a specific justification for the warning suppression.For event handlers, the justification should mention that these are UI event handlers where async void is appropriate.
-[System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "VSTHRD100:Avoid async void methods", Justification = "<Pending>")] +[System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "VSTHRD100:Avoid async void methods", Justification = "UI event handler that cannot return Task")]
290-290: Add a specific justification for the warning suppression.For event handlers, the justification should mention that these are UI event handlers where async void is appropriate.
-[System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "VSTHRD100:Avoid async void methods", Justification = "<Pending>")] +[System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "VSTHRD100:Avoid async void methods", Justification = "UI event handler that cannot return Task")]
149-156: Consider refactoring this method to return Task.Although suppressing the warning is a valid approach, a better practice would be to refactor non-event-handler methods to return Task when possible. This improves exception handling and testability.
-[System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "VSTHRD100:Avoid async void methods", Justification = "<Pending>")] -private async void ReIndexing() +private async Task ReIndexingAsync() { ViewRefresh(); indexingPanel.Visibility = Visibility.Visible; await Main.IndexProgramsAsync(); indexingPanel.Visibility = Visibility.Hidden; }You would also need to update the calling methods to await this method, which is why this is suggested as an optional refactor for a future PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
Flow.Launcher.Infrastructure/Image/ImageLoader.cs(1 hunks)Flow.Launcher.Infrastructure/Image/ThumbnailReader.cs(3 hunks)Flow.Launcher/Msg.xaml.cs(1 hunks)Flow.Launcher/ReportWindow.xaml.cs(0 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPanePluginsViewModel.cs(1 hunks)Flow.Launcher/SettingPages/Views/SettingsPanePlugins.xaml(1 hunks)Flow.Launcher/SettingPages/Views/SettingsPaneTheme.xaml.cs(1 hunks)Flow.Launcher/ViewModel/RelayCommand.cs(0 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Flow.Launcher.Plugin.Explorer.csproj(1 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Helper/ShellContextMenu.cs(3 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Helper/SortOptionTranslationHelper.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Search/EnvironmentVariables.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/ActionKeywordModel.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/RelayCommand.cs(0 hunks)Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/SettingsViewModel.cs(6 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Views/ActionKeywordSetting.xaml.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Views/ExplorerSettings.xaml(3 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Views/ExplorerSettings.xaml.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Program/Views/ProgramSetting.xaml.cs(3 hunks)Plugins/Flow.Launcher.Plugin.Sys/Main.cs(1 hunks)Plugins/Flow.Launcher.Plugin.WebSearch/SearchSourceSetting.xaml.cs(2 hunks)
💤 Files with no reviewable changes (3)
- Flow.Launcher/ReportWindow.xaml.cs
- Flow.Launcher/ViewModel/RelayCommand.cs
- Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/RelayCommand.cs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (32)
Flow.Launcher.Infrastructure/Image/ThumbnailReader.cs (4)
15-15: Documentation improvement.Using the shorter reference to
SIIGBFinstead of the fully qualified name improves readability while maintaining clarity.
34-36: Good practice: Named constants for error codes.Using named constants for HRESULT values makes the code more readable and maintainable. This change supports better error handling by clearly identifying specific error conditions.
84-88: Enhanced exception handling improves robustness.The updated exception handling now properly accounts for both extraction failures and path not found errors when in ThumbnailOnly mode. The fallback to IconOnly is appropriate in both cases.
95-99: Good addition of general exception handler.Adding a catch-all exception handler prevents unhandled exceptions from propagating up the call stack. Wrapping in an InvalidOperationException with a descriptive message provides better context about the failure.
Flow.Launcher.Infrastructure/Image/ImageLoader.cs (1)
174-175: Improved log message clarity.Removing the redundant class name prefix from log messages makes them cleaner and more concise while maintaining all necessary information. Since the class name is already passed as the first parameter to
Log.Exception, there's no need to include it again in the message.Flow.Launcher/SettingPages/Views/SettingsPanePlugins.xaml (1)
60-60: Good improvement for dialog ownershipAdding the CommandParameter binding to pass the button itself to the OpenHelperCommand ensures the correct dialog owner window can be determined. This fixes potential dialog parenting issues that could cause focus/z-order problems.
Plugins/Flow.Launcher.Plugin.Sys/Main.cs (1)
365-365: Good UX improvement for application exitHiding the main window before closing it provides a smoother visual transition during application shutdown. This prevents users from seeing partial UI destruction which could appear jarring.
Flow.Launcher/SettingPages/ViewModels/SettingsPanePluginsViewModel.cs (1)
114-114: Great fix for dialog owner resolutionUsing the Button parameter and Window.GetWindow() to determine the dialog owner is the correct approach. This ensures the dialog is properly owned by its parent window instead of using the application's main window, which might not be the correct context for the dialog.
This change:
- Improves focus management between the dialog and parent window
- Ensures the dialog follows the parent window when moved
- Prevents z-order issues when multiple windows are open
Also applies to: 118-118
Plugins/Flow.Launcher.Plugin.Explorer/Flow.Launcher.Plugin.Explorer.csproj (1)
48-48: Good addition to support modern MVVM patterns.Adding the CommunityToolkit.Mvvm package is a positive change that enables using source-generated commands via the
[RelayCommand]attribute, which reduces boilerplate code compared to manual RelayCommand implementations.Flow.Launcher/SettingPages/Views/SettingsPaneTheme.xaml.cs (1)
1-5: Harmless formatting change.The code only contains minor ordering changes to using directives and the addition of a BOM character. These changes don't affect functionality.
Plugins/Flow.Launcher.Plugin.Explorer/Search/EnvironmentVariables.cs (2)
50-50: Good nullability annotation improvement.Adding the null-forgiving operator ensures the compiler that
special.Valuewon't be null, improving null safety analysis without changing runtime behavior.
64-64: Good nullability annotation improvement.Adding the null-forgiving operator ensures the compiler that the result of
special.Key.ToString()won't be null, improving null safety analysis.Plugins/Flow.Launcher.Plugin.Explorer/Helper/SortOptionTranslationHelper.cs (1)
19-19: Good nullability annotation improvement.Adding the null-forgiving operator to
enumNameproperly informs the compiler that this value won't be null whenSplit()is called, which aligns with the null check done in line 16 usingArgumentNullException.ThrowIfNull(API).Plugins/Flow.Launcher.Plugin.Explorer/Views/ExplorerSettings.xaml (3)
248-248: Updated binding to match source-generated command name.This change correctly updates the Command binding to use the new property name generated by CommunityToolkit.Mvvm's
[RelayCommand]attribute.
275-275: Updated binding to match source-generated command name.This change correctly updates the Command binding to use the new property name generated by CommunityToolkit.Mvvm's
[RelayCommand]attribute.
302-302: Updated binding to match source-generated command name.This change correctly updates the Command binding to use the new property name generated by CommunityToolkit.Mvvm's
[RelayCommand]attribute.Plugins/Flow.Launcher.Plugin.Explorer/Views/ActionKeywordSetting.xaml.cs (3)
88-88: Code formatting improvement with added blank line.The addition of a blank line after the method improves readability by creating clearer visual separation between methods.
98-98: Code formatting improvement with added blank line.The addition of a blank line after the method improves readability by creating clearer visual separation between methods.
104-104: Code formatting improvement with added blank line.The addition of a blank line after the method improves readability by creating clearer visual separation between methods.
Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/ActionKeywordModel.cs (3)
4-5: Enabling nullable reference types improves type safety.Enabling nullable reference types helps catch potential null reference exceptions at compile time by making nullability explicit in the type system.
10-10: Appropriate use of null-forgiving operator.Using the null-forgiving operator with the static field is appropriate here since the field is initialized through the
Initmethod before use.
12-12: Correctly marking event as nullable.The PropertyChanged event is correctly marked as nullable to align with the nullable reference type system.
Plugins/Flow.Launcher.Plugin.Explorer/Views/ExplorerSettings.xaml.cs (1)
1-6: Improved organization of using directives.The using directives have been reordered for better organization, with specific plugin namespaces now explicitly ordered.
Plugins/Flow.Launcher.Plugin.Explorer/Helper/ShellContextMenu.cs (1)
1538-1538: Updated to use modern thread ID retrieval.Replaced the deprecated
AppDomain.GetCurrentThreadId()with the recommendedEnvironment.CurrentManagedThreadId. This is a good modernization of the code.Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/SettingsViewModel.cs (8)
11-16: LGTM! Good refactoring to use CommunityToolkit.Mvvm.The changes properly set up the class for using the MVVM toolkit's source generation features. Converting the class to
partialand adding the necessary import forCommunityToolkit.Mvvm.Inputare correct steps for enabling the[RelayCommand]attribute pattern.Also applies to: 20-20
262-298: Refactoring to [RelayCommand] pattern looks good.The
EditActionKeywordmethod has been properly annotated with the[RelayCommand]attribute. This will generate a publicEditActionKeywordCommandproperty via source generation, eliminating the need for manual command property implementation.
318-347: Refactoring to [RelayCommand] pattern looks good.The
EditLinkmethod has been properly annotated with the[RelayCommand]attribute. The implementation itself is well-structured, handling both quick access links and excluded paths in a consistent way.
355-378: Refactoring to [RelayCommand] pattern looks good.The
AddLinkmethod has been properly annotated with the[RelayCommand]attribute. The implementation correctly handles adding different types of links based on the command parameter.
380-397: Refactoring to [RelayCommand] pattern looks good.The
RemoveLinkmethod has been properly annotated with the[RelayCommand]attribute. The implementation correctly handles both link types and callsSave()to persist the changes.
443-451: Refactoring to [RelayCommand] pattern looks good.The
OpenFileEditorPathmethod has been properly annotated with the[RelayCommand]attribute. This will generate a publicOpenFileEditorPathCommandproperty that can be bound to in XAML.
453-461: Refactoring to [RelayCommand] pattern looks good.The
OpenFolderEditorPathmethod has been properly annotated with the[RelayCommand]attribute. This will generate a publicOpenFolderEditorPathCommandproperty.
463-471: Refactoring to [RelayCommand] pattern looks good.The
OpenShellPathmethod has been properly annotated with the[RelayCommand]attribute. This will generate a publicOpenShellPathCommandproperty.
Fix dialog owner
Follow on with #3466. Use
Window.GetWindowto get owner.Improve window exiting
First hide main window and then exiting window for better display effect.
Improve WindowsThumbnailProvider
Fix possible task exception with WindowsThumbnailProvider.
Fix warnings
Lots of tweaks for fixing warnings.