Replace original MessageBox with MessageBoxEx & Improve MessageBoxEx display style & Add new public api for using MessageBoxEx #3093
Conversation
This comment has been minimized.
This comment has been minimized.
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve a significant update to the message box functionality across various classes in the Flow Launcher application. The standard Changes
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🔇 Additional comments (1)Plugins/Flow.Launcher.Plugin.WebSearch/Main.cs (1)
Reconsider making The modification of
Consider these alternatives: -internal static PluginInitContext _context;
+private readonly PluginInitContext _context;If sharing context is necessary, consider:
Let's verify the usage pattern that necessitated this change: 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (20)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneProxyViewModel.cs (1)
Line range hint
24-52: Consider modernizing the proxy testing implementation.While the current implementation works, there are several improvements that could enhance the code:
- Use HttpClient instead of deprecated HttpWebRequest
- Implement async/await pattern to prevent UI blocking
- Add timeout configuration to prevent long waits
Here's a suggested modern implementation:
- [RelayCommand] - private void OnTestProxyClicked() - { - var message = TestProxy(); - MessageBoxEx.Show(InternationalizationManager.Instance.GetTranslation(message)); - } - - private string TestProxy() + [RelayCommand] + private async Task OnTestProxyClickedAsync() + { + var message = await TestProxyAsync(); + MessageBoxEx.Show(InternationalizationManager.Instance.GetTranslation(message)); + } + + private async Task<string> TestProxyAsync() { if (string.IsNullOrEmpty(Settings.Proxy.Server)) return "serverCantBeEmpty"; if (Settings.Proxy.Port <= 0) return "portCantBeEmpty"; - HttpWebRequest request = (HttpWebRequest)WebRequest.Create(_updater.GitHubRepository); + using var handler = new HttpClientHandler(); + var proxy = new WebProxy(Settings.Proxy.Server, Settings.Proxy.Port); - if (string.IsNullOrEmpty(Settings.Proxy.UserName) || string.IsNullOrEmpty(Settings.Proxy.Password)) - { - request.Proxy = new WebProxy(Settings.Proxy.Server, Settings.Proxy.Port); - } - else + if (!string.IsNullOrEmpty(Settings.Proxy.UserName) && !string.IsNullOrEmpty(Settings.Proxy.Password)) { - request.Proxy = new WebProxy(Settings.Proxy.Server, Settings.Proxy.Port) - { - Credentials = new NetworkCredential(Settings.Proxy.UserName, Settings.Proxy.Password) - }; + proxy.Credentials = new NetworkCredential(Settings.Proxy.UserName, Settings.Proxy.Password); } + + handler.Proxy = proxy; + using var client = new HttpClient(handler) { Timeout = TimeSpan.FromSeconds(10) }; try { - var response = (HttpWebResponse)request.GetResponse(); - return response.StatusCode switch - { - HttpStatusCode.OK => "proxyIsCorrect", - _ => "proxyConnectFailed" - }; + var response = await client.GetAsync(_updater.GitHubRepository); + return response.StatusCode == HttpStatusCode.OK ? "proxyIsCorrect" : "proxyConnectFailed"; } - catch + catch (Exception) { return "proxyConnectFailed"; } }Flow.Launcher/PriorityChangeWindow.xaml.cs (2)
27-27: Consider specifying MessageBoxButton parameterWhile the code works, explicitly specifying the MessageBoxButton parameter would improve clarity and maintainability.
-MessageBoxEx.Show(translater.GetTranslation("cannotFindSpecifiedPlugin")); +MessageBoxEx.Show(translater.GetTranslation("cannotFindSpecifiedPlugin"), MessageBoxButton.OK);
46-48: Simplify message display and specify MessageBoxButtonThe code can be simplified by removing the intermediate variable and adding the MessageBoxButton parameter for clarity.
- string msg = translater.GetTranslation("invalidPriority"); - MessageBoxEx.Show(msg); + MessageBoxEx.Show(translater.GetTranslation("invalidPriority"), MessageBoxButton.OK);Flow.Launcher/CustomShortcutSetting.xaml.cs (2)
Line range hint
13-15: Consider using consistent string initializationFor consistency, consider using String.Empty for all string initializations. Also, property names could be more descriptive.
- public string Key { get; set; } = String.Empty; - public string Value { get; set; } = String.Empty; - private string originalKey { get; } = null; + public string ShortcutKey { get; set; } = String.Empty; + public string ShortcutValue { get; set; } = String.Empty; + private string originalKey { get; } = String.Empty;
Line range hint
71-76: Consider extracting window manipulation logicThe test button click handler directly manipulates the main window multiple times. Consider extracting this logic into a dedicated method in the main window class or a window service.
- private void BtnTestShortcut_OnClick(object sender, RoutedEventArgs e) - { - App.API.ChangeQuery(tbExpand.Text); - Application.Current.MainWindow.Show(); - Application.Current.MainWindow.Opacity = 1; - Application.Current.MainWindow.Focus(); - } + private void BtnTestShortcut_OnClick(object sender, RoutedEventArgs e) + { + App.API.ChangeQuery(tbExpand.Text); + ShowAndFocusMainWindow(); + } + + private static void ShowAndFocusMainWindow() + { + var mainWindow = Application.Current.MainWindow; + mainWindow.Show(); + mainWindow.Opacity = 1; + mainWindow.Focus(); + }Flow.Launcher/Helper/HotKeyMapper.cs (1)
Line range hint
44-50: Consider enhancing error handling for better user guidanceThe current error handling could be improved to:
- Log the specific exception for debugging
- Provide users with guidance on how to resolve hotkey conflicts
Consider this enhancement:
- catch (Exception) + catch (Exception ex) { + Log.Error(ex, $"Failed to register hotkey: {hotkeyStr}"); string errorMsg = string.Format(InternationalizationManager.Instance.GetTranslation("registerHotkeyFailed"), hotkeyStr); + errorMsg += Environment.NewLine + InternationalizationManager.Instance.GetTranslation("hotkeyResolutionHint"); string errorMsgTitle = InternationalizationManager.Instance.GetTranslation("MessageBoxTitle"); MessageBoxEx.Show(errorMsg, errorMsgTitle); }Flow.Launcher.Core/Updater.cs (3)
20-20: Optimize usage of static importThe static import of MessageBoxEx is not being fully utilized since the code still uses the fully qualified
MessageBoxEx.Show. Consider either removing the static import or updating the calls to use justShow().- using static Flow.Launcher.Core.MessageBoxEx;
57-57: Specify MessageBoxType for clarityConsider explicitly specifying the message box type for consistent visual appearance across the application.
- MessageBoxEx.Show(api.GetTranslation("update_flowlauncher_already_on_latest")); + MessageBoxEx.Show(api.GetTranslation("update_flowlauncher_already_on_latest"), string.Empty, MessageBoxType.Information);
Line range hint
41-97: Consider standardizing notification approachThe class currently mixes two different approaches for user notifications:
api.ShowMsg()for progress updatesMessageBoxEx.Show()for user interactionsConsider standardizing the notification approach for better consistency:
- Use
MessageBoxExfor user interactions requiring responses- Use
api.ShowMsgfor progress/status updates onlyFlow.Launcher.Core/MessageBoxEx.xaml (1)
Line range hint
40-62: Consider adding accessibility improvements to the close buttonWhile the close button implementation is solid, consider these accessibility enhancements:
<Button Grid.Column="1" Click="Button_Cancel" + ToolTip="Close" + AutomationProperties.Name="Close" Style="{StaticResource TitleBarCloseButtonStyle}">Flow.Launcher.Core/MessageBoxEx.xaml.cs (1)
Line range hint
36-36: Consider thread-safety improvements for static msgBox field.The static
msgBoxfield could cause issues in multi-window scenarios where multiple message boxes might be shown simultaneously. Consider:
- Making the field instance-based instead of static
- Adding thread synchronization if static field must be maintained
Flow.Launcher.Core/Resource/Internationalization.cs (1)
14-14: Consider using a regular import instead of a static import.While the static import works, using a regular import (
using Flow.Launcher.Core) would make it more explicit whereMessageBoxExcomes from, improving code readability.-using static Flow.Launcher.Core.MessageBoxEx; +using Flow.Launcher.Core;Flow.Launcher.Core/Configuration/Portable.cs (4)
44-45: Consider specifying MessageBoxType explicitlyWhile the default OK button is appropriate here, it's better to be explicit about the MessageBoxType for clarity and maintainability.
- MessageBoxEx.Show("Flow Launcher needs to restart to finish disabling portable mode, " + - "after the restart your portable data profile will be deleted and roaming data profile kept"); + MessageBoxEx.Show("Flow Launcher needs to restart to finish disabling portable mode, " + + "after the restart your portable data profile will be deleted and roaming data profile kept", + string.Empty, MessageBoxType.OK);
68-69: Consider specifying MessageBoxType explicitlyFor consistency with the previous suggestion and clarity of intent, specify the MessageBoxType explicitly.
- MessageBoxEx.Show("Flow Launcher needs to restart to finish enabling portable mode, " + - "after the restart your roaming data profile will be deleted and portable data profile kept"); + MessageBoxEx.Show("Flow Launcher needs to restart to finish enabling portable mode, " + + "after the restart your roaming data profile will be deleted and portable data profile kept", + string.Empty, MessageBoxType.OK);
178-179: Consider specifying MessageBoxType explicitlyFor consistency with other message boxes in the file, specify the MessageBoxType explicitly.
- MessageBoxEx.Show("Flow Launcher has detected you disabled portable mode, " + - "the relevant shortcuts and uninstaller entry have been created"); + MessageBoxEx.Show("Flow Launcher has detected you disabled portable mode, " + + "the relevant shortcuts and uninstaller entry have been created", + string.Empty, MessageBoxType.OK);
190-192: LGTM with minor enhancement suggestionThe message formatting using string.Format and Environment.NewLine is well done. Consider adding explicit MessageBoxType for consistency.
- MessageBoxEx.Show(string.Format("Flow Launcher detected your user data exists both in {0} and " + - "{1}. {2}{2}Please delete {1} in order to proceed. No changes have occurred.", - DataLocation.PortableDataPath, DataLocation.RoamingDataPath, Environment.NewLine)); + MessageBoxEx.Show(string.Format("Flow Launcher detected your user data exists both in {0} and " + + "{1}. {2}{2}Please delete {1} in order to proceed. No changes have occurred.", + DataLocation.PortableDataPath, DataLocation.RoamingDataPath, Environment.NewLine), + string.Empty, MessageBoxType.OK);Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs (2)
Line range hint
315-397: Consider refactoring font-related properties to reduce duplication.The font-related properties (SelectedQueryBoxFont, SelectedResultFont, SelectedResultSubFont) share similar logic for font existence checking and fallback behavior. Consider extracting this common logic into a helper method.
Here's a suggested approach:
+ private FontFamily GetFontFamilyWithFallback(string fontName) + { + var fontExists = Fonts.SystemFontFamilies.Any( + fontFamily => + fontFamily.FamilyNames.Values != null && + fontFamily.FamilyNames.Values.Contains(fontName) + ); + return fontExists ? new FontFamily(fontName) : new FontFamily(DefaultFont); + } public FontFamily SelectedQueryBoxFont { - get - { - var fontExists = Fonts.SystemFontFamilies.Any( - fontFamily => - fontFamily.FamilyNames.Values != null && - fontFamily.FamilyNames.Values.Contains(Settings.QueryBoxFont) - ); - return fontExists switch - { - true => new FontFamily(Settings.QueryBoxFont), - _ => new FontFamily(DefaultFont) - }; - } + get => GetFontFamilyWithFallback(Settings.QueryBoxFont); set { Settings.QueryBoxFont = value.ToString(); ThemeManager.Instance.ChangeTheme(Settings.Theme); } }
Line range hint
235-282: Consider improving PreviewResults to be more resilient.The PreviewResults property uses hardcoded paths for plugin images. Consider:
- Adding existence checks for plugin image files
- Using dependency injection for plugin paths
- Moving the sample results to a configuration file
Example approach for file existence check:
IcoPath = Path.Combine( Constant.ProgramDirectory, @"Plugins\Flow.Launcher.Plugin.Explorer\Images\explorer.png" - ) + ), + IcoPathExists = File.Exists(Path.Combine( + Constant.ProgramDirectory, + @"Plugins\Flow.Launcher.Plugin.Explorer\Images\explorer.png" + ))Flow.Launcher.Core/Resource/Theme.cs (2)
Line range hint
1-15: Add missing import for MessageBoxExThe code uses
MessageBoxExbut is missing the required import statement.Add the following import at the top of the file:
using System.Windows.Media.Effects; +using Flow.Launcher.Infrastructure.UI; using Flow.Launcher.Infrastructure;
MessageBoxEx is properly initialized, but theme loading tests are missing
Based on the verification results:
- MessageBoxEx is properly implemented as a Window class in
MessageBoxEx.xaml.cswith proper initialization and static Show methods- However, there are no dedicated test files or test methods for theme loading functionality, particularly for error scenarios
Recommendations:
- Add unit tests to verify theme loading error handling:
- Test DirectoryNotFoundException scenario
- Test XamlParseException scenario
- Test fallback to default theme behavior
🔗 Analysis chain
Line range hint
113-124: Verify MessageBoxEx initialization and theme loading testsWhile the MessageBox replacements are correct, we should ensure:
- MessageBoxEx is properly initialized before being used in these error scenarios
- Theme loading functionality has adequate test coverage for these error cases
Let's verify the test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for theme loading tests rg -l "ChangeTheme.*DirectoryNotFoundException|XamlParseException" --type-add "test:*.{cs,vb}" -t test # Check MessageBoxEx initialization ast-grep --pattern 'class $_ { $$$ MessageBoxEx.$$$ $$$ }'Length of output: 208
Script:
#!/bin/bash # Let's check for test files related to Theme fd "Theme.*Test.*\.cs$" # Check for any test methods related to theme loading rg "Test.*Theme" --type cs -A 5 # Check MessageBoxEx usage and initialization patterns rg "MessageBoxEx" --type cs -A 2 # Check if there are any existing test utilities for MessageBox rg "TestMessageBox|MockMessageBox|FakeMessageBox" --type csLength of output: 11021
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
Flow.Launcher.Core/Configuration/Portable.cs(6 hunks)Flow.Launcher.Core/MessageBoxEx.xaml(4 hunks)Flow.Launcher.Core/MessageBoxEx.xaml.cs(2 hunks)Flow.Launcher.Core/Resource/Internationalization.cs(2 hunks)Flow.Launcher.Core/Resource/Theme.cs(2 hunks)Flow.Launcher.Core/Updater.cs(4 hunks)Flow.Launcher/ActionKeywords.xaml.cs(2 hunks)Flow.Launcher/CustomQueryHotkeySetting.xaml.cs(2 hunks)Flow.Launcher/CustomShortcutSetting.xaml.cs(2 hunks)Flow.Launcher/Helper/HotKeyMapper.cs(2 hunks)Flow.Launcher/PriorityChangeWindow.xaml.cs(3 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneAboutViewModel.cs(1 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneHotkeyViewModel.cs(5 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneProxyViewModel.cs(1 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- Flow.Launcher/SettingPages/ViewModels/SettingsPaneAboutViewModel.cs
🔇 Additional comments (29)
Flow.Launcher/ActionKeywords.xaml.cs (2)
5-5: LGTM: Required namespace for MessageBoxEx
The addition of the Flow.Launcher.Core namespace is necessary for using MessageBoxEx.
47-47: Consider adding error icon for better UX
While the MessageBox replacement is correct, consider using MessageBoxEx.Show with an error icon since this is an error notification.
Let's verify the MessageBoxEx.Show signature and usage:
Flow.Launcher/SettingPages/ViewModels/SettingsPaneProxyViewModel.cs (1)
25-25: LGTM! MessageBox replacement aligns with PR objectives.
The replacement of MessageBox.Show with MessageBoxEx.Show is consistent with the PR's goal of standardizing the message box implementation across the application.
Flow.Launcher/PriorityChangeWindow.xaml.cs (1)
8-8: LGTM: Required namespace import for MessageBoxEx
The addition of the Flow.Launcher.Core namespace is necessary and properly placed with other related imports.
Flow.Launcher/CustomShortcutSetting.xaml.cs (3)
6-6: LGTM: Using directive properly added
The using directive for Flow.Launcher.Core is correctly placed and necessary for MessageBoxEx usage.
46-46: LGTM: MessageBox replacement maintains functionality
The MessageBox.Show replacement with MessageBoxEx.Show maintains the existing error handling logic while supporting the dark theme requirement.
52-52: LGTM: MessageBox replacement maintains functionality
The MessageBox.Show replacement with MessageBoxEx.Show maintains the existing error handling logic while supporting the dark theme requirement.
Flow.Launcher/Helper/HotKeyMapper.cs (2)
8-8: LGTM: Appropriate namespace change
The addition of Flow.Launcher.Core namespace aligns with the PR objective of replacing System.Windows.MessageBox with MessageBoxEx.
49-49: Verify MessageBoxEx parameters and usage pattern
The MessageBox replacement looks correct, maintaining the error handling flow and localization. Let's verify the MessageBoxEx implementation to ensure we're using it as intended.
✅ Verification successful
Let me generate additional verification to check the MessageBoxEx class definition and its Show method signature, as the previous search didn't reveal the class implementation.
MessageBoxEx usage is correct and follows the implementation pattern
The MessageBoxEx.Show usage in HotKeyMapper.cs with two parameters (errorMsg, errorMsgTitle) matches the second overload of the Show method found in Flow.Launcher.Core/MessageBoxEx.xaml.cs. The usage pattern is consistent with other occurrences throughout the codebase, particularly for showing error messages with localized content.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify MessageBoxEx implementation and usage pattern
# Look for MessageBoxEx definition and other usages to confirm parameter pattern
# Find MessageBoxEx class definition
echo "Looking for MessageBoxEx class definition:"
ast-grep --pattern 'class MessageBoxEx {
$$$
}'
# Find other MessageBoxEx.Show usages for comparison
echo "Looking for other MessageBoxEx.Show usages:"
rg "MessageBoxEx.Show" -A 2
Length of output: 9051
Script:
#!/bin/bash
# Search for MessageBoxEx class and Show method implementation
ast-grep --pattern 'class MessageBoxEx {
public static $$ Show($$$) {
$$$
}
}'
# Also search for MessageBoxType enum since it was referenced in some usages
ast-grep --pattern 'enum MessageBoxType {
$$$
}'
# Search for any other Show method overloads
rg "static.*Show.*\(" --type cs
Length of output: 1218
Flow.Launcher/CustomQueryHotkeySetting.xaml.cs (2)
9-9: LGTM: Import statement aligns with MessageBoxEx usage
The addition of the Flow.Launcher.Core namespace import is appropriate for accessing the MessageBoxEx functionality.
66-66: Verify MessageBoxEx parameters for error message
The replacement of MessageBox with MessageBoxEx aligns with the PR objectives. However, since this is an error message, we should verify if specific MessageBoxEx parameters (like icon or buttons) should be set to maintain the same user experience.
✅ Verification successful
MessageBoxEx usage is consistent with error message patterns
Looking at the codebase patterns, the simple MessageBoxEx.Show(message) form is consistently used for error notifications and validation messages. Other error messages like "emptyShortcut", "duplicateShortcut", and "cannotFindSpecifiedPlugin" follow the same pattern. Only confirmation dialogs use additional parameters for MessageBoxType.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other error message patterns using MessageBoxEx
rg "MessageBoxEx.Show.*invalidPluginHotkey" -A 2 -B 2
# Look for similar error handling patterns
ast-grep --pattern 'MessageBoxEx.Show($$$)'
Length of output: 7805
Flow.Launcher/SettingPages/ViewModels/SettingsPaneHotkeyViewModel.cs (3)
10-11: LGTM! Clean import of MessageBoxEx
The static import of MessageBoxEx is appropriate and aligns with the PR objective of replacing System.Windows.MessageBox.
46-46: LGTM! Consistent replacement of MessageBox.Show
The MessageBox replacements are consistent and maintain the same functionality while supporting the new theming capabilities.
Also applies to: 71-71, 92-92, 116-116
50-56: Verify MessageBoxType enum compatibility
The change from MessageBoxButton to MessageBoxType looks correct, but let's verify the enum values match to ensure consistent behavior.
Also applies to: 96-101
✅ Verification successful
MessageBoxType and MessageBoxResult usage is consistent and correct
The verification shows that:
MessageBoxType.YesNois correctly mapped toMessageBoxButton.YesNoinMessageBoxEx.xaml.cs- All
MessageBoxType.YesNoresults are properly handled withMessageBoxResult.Yeschecks - The specific usage in
SettingsPaneHotkeyViewModel.csfollows the same pattern as other files in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify MessageBoxType enum values match MessageBoxButton values
# and check all possible return values are handled correctly
# Search for MessageBoxType enum definition
ast-grep --pattern 'enum MessageBoxType {
$$$
}'
# Search for MessageBoxType usage patterns
rg -A 2 'MessageBoxType\.'
# Search for return value handling
rg 'if.*result.*MessageBoxResult\.'
Length of output: 4340
Flow.Launcher.Core/Updater.cs (1)
87-87: LGTM! Well-structured message box implementation
This implementation properly handles all necessary parameters (message, title, type) and correctly processes the user's response for the restart decision.
Flow.Launcher.Core/MessageBoxEx.xaml (4)
2-2: LGTM: Namespace changes are consistent
The namespace changes from Flow.Launcher to Flow.Launcher.Core align well with the architectural organization.
Also applies to: 6-6
63-67: LGTM: Grid layout improvements enhance flexibility
The switch from StackPanel to Grid with explicit row definitions provides better layout control and flexibility.
Line range hint 11-13: Ensure theme switching behavior is tested
The message box uses dynamic resources for theming (PopuBGColor, PopupTextColor, PopupButtonAreaBGColor). Consider adding theme switching tests to verify proper appearance in both light and dark themes.
98-115: Verify MaxWidth constraints for different screen sizes
Both text blocks use a MaxWidth of 400, which should be verified across different screen sizes and DPI settings.
✅ Verification successful
MaxWidth constraint is appropriate for this message box implementation
The MaxWidth of 400 for text blocks is well-designed considering:
- The parent window has a fixed width of 420px
- Text blocks have right margin of 26px
- The window is non-resizable (ResizeMode="NoResize")
- Window is always centered on screen (WindowStartupLocation="CenterScreen")
- Content height adjusts automatically (SizeToContent="Height")
The MaxWidth constraint ensures text remains within the window bounds while maintaining readability across different content lengths and screen sizes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other MaxWidth constraints in XAML files for consistency
rg -l "MaxWidth=\"400\"" --type xaml
# Check for any DPI-aware implementations
rg "SystemParameters.(Dpi|PrimaryScreen)" --type xaml
Length of output: 150
Script:
#!/bin/bash
# Search for other MaxWidth constraints in XAML files using regular ripgrep without file type filter
rg -l "MaxWidth=\"400\""
# Check for any DPI-aware implementations
rg "SystemParameters\.(Dpi|PrimaryScreen)"
# Look for any window sizing or screen-related code
rg "(Window\.(Width|Height|Size)|Screen\.(Width|Height|WorkingArea))"
Length of output: 497
Script:
#!/bin/bash
# Check the MessageBoxEx.xaml file content to understand the window layout and sizing
cat Flow.Launcher.Core/MessageBoxEx.xaml
# Look for any responsive design or window sizing related code in MessageBoxEx
rg -g "MessageBoxEx*" "(Width|Height|Size|Screen)"
Length of output: 8473
Flow.Launcher.Core/MessageBoxEx.xaml.cs (3)
40-48: LGTM! Parameter naming improvements enhance readability.
The renaming of parameters from msg/caption to messageBoxText/title makes the code more self-documenting and consistent with Windows Forms conventions.
83-94: LGTM! Improved display style for simple messages.
The new condition improves the presentation of simple messages by:
- Using messageBoxText as both description and title when appropriate
- Maintaining separate title/description display for more complex messages
52-77: Verify default case behavior for MessageBoxType.
The default case returns MessageBoxResult.No without showing any dialog. Please verify if this is the intended behavior for unknown MessageBoxType values.
✅ Verification successful
Let me search for the MessageBoxType enum definition to understand all possible values.
Let me check the actual enum definition to see all possible values.
Default case behavior for MessageBoxType is safe and appropriate.
The default case returning MessageBoxResult.No is correct because:
- The
MessageBoxTypeenum is fully defined with all possible values (ConfirmationWithYesNo, ConfirmationWithYesNoCancel, YesNo, Information, Error, Warning) - Since it's a closed enum without flags, the default case can only be hit if there's a future enum value addition
- Returning
MessageBoxResult.Nois a safe fallback that aligns with the conservative approach of not proceeding with an action when the intent is unclear
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any custom MessageBoxType values or handling across the codebase
rg -A 2 "MessageBoxType\." --type cs
Length of output: 3514
Script:
#!/bin/bash
# Search for MessageBoxType enum definition
ast-grep --pattern 'enum MessageBoxType'
Length of output: 5541
Script:
#!/bin/bash
# Get the MessageBoxType enum definition
cat Flow.Launcher.Core/MessageBoxEx.xaml.cs
Length of output: 6807
Flow.Launcher.Core/Resource/Internationalization.cs (1)
128-128: Verify MessageBoxType.YesNo behavior matches MessageBoxButton.YesNo.
The change from MessageBoxButton.YesNo to MessageBoxType.YesNo looks correct. However, let's verify that the enum values in MessageBoxType maintain the same behavior as MessageBoxButton to ensure consistent user experience.
✅ Verification successful
MessageBoxType.YesNo behavior correctly maps to MessageBoxButton.YesNo
Looking at the MessageBoxEx implementation, we can see that MessageBoxType.YesNo is properly mapped to MessageBoxButton.YesNo internally, maintaining the same behavior and user experience. The MessageBoxEx class handles the conversion in its Show method, ensuring consistent button visibility and return values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify MessageBoxType enum values and usage
# Look for MessageBoxType enum definition and other usages to confirm behavior
# Find MessageBoxType enum definition
ast-grep --pattern 'enum MessageBoxType {
$$$
}'
# Find other MessageBoxEx.Show calls with YesNo type for comparison
rg 'MessageBoxEx\.Show.*MessageBoxType\.YesNo'
Length of output: 438
Script:
#!/bin/bash
# Let's find the MessageBoxEx class implementation to understand how it handles MessageBoxType
ast-grep --pattern 'class MessageBoxEx {
$$$
}'
# Also search for any MessageBoxButton usage to compare
rg "MessageBoxButton\." -A 2
# Search for any MessageBoxType definition or references
rg "MessageBoxType\." -A 2
Length of output: 11715
Flow.Launcher.Core/Configuration/Portable.cs (2)
12-12: LGTM: Using directive change aligns with PR objective
The static import of MessageBoxEx is appropriate given the frequent usage throughout the file and aligns with the PR's goal of replacing System.Windows.MessageBox.
163-165: LGTM: Proper usage of MessageBoxType.YesNo
The MessageBoxEx implementation correctly uses MessageBoxType.YesNo and properly handles the user's response.
Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs (2)
17-18: LGTM! Good use of alias directives.
The addition of clear aliases helps disambiguate between different ThemeManager classes, improving code readability and maintainability.
52-52: LGTM! Successfully replaced MessageBox with MessageBoxEx.
The change aligns with the PR objective to support dark theme while maintaining the existing functionality.
Let's verify that this is the only MessageBox usage in this file:
✅ Verification successful
Confirmed: MessageBox has been completely replaced with MessageBoxEx
The verification shows that there is only one message box usage in the file, and it's correctly using MessageBoxEx.Show(). There are no remaining instances of the old System.Windows.MessageBox, confirming the successful migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify no remaining System.Windows.MessageBox usages
rg "MessageBox\.Show" "Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs"
Length of output: 91
Script:
#!/bin/bash
# Let's try a broader search for MessageBox usages
rg -i "messagebox" "Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs"
# Also check for any imports related to MessageBox
rg "using.*MessageBox" "Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs"
# Let's also verify MessageBoxEx usage
rg "MessageBoxEx" "Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs"
Length of output: 493
Flow.Launcher.Core/Resource/Theme.cs (2)
113-113: LGTM: MessageBox replacement for directory not found error
The replacement of MessageBox.Show with MessageBoxEx.Show is correct and maintains the existing internationalization and error handling logic.
123-123: LGTM: MessageBox replacement for XAML parse error
The replacement of MessageBox.Show with MessageBoxEx.Show is correct and maintains the existing internationalization and error handling logic.
Jack251970
changed the title
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: Yusyuriv, jjw24 Yusyuriv, VictoriousRaptor 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:
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 |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (25)
Plugins/Flow.Launcher.Plugin.Program/AddProgramSource.xaml.cs (1)
36-36: Consider improving error message clarityWhile the MessageBox replacement is correct, the "Invalid" comment suggests this is an error message. Consider making the message more descriptive for better user experience.
Example improvement:
- MessageBoxEx.Show(msg); // Invalid + MessageBoxEx.Show(msg, "Invalid Program Source", MessageBoxType.Error);Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptV2Environment.cs (1)
28-31: Consider improving error handling and async/await usage.The installation process could benefit from better error handling and async/await pattern:
- Using .Wait() on DroplexPackage.Drop could block the UI thread
- Installation steps lack try-catch error handling
Consider refactoring to:
- internal override void InstallEnvironment() + internal override async Task InstallEnvironment() { - FilesFolders.RemoveFolderIfExists(InstallPath, MessageBoxEx.Show); - DroplexPackage.Drop(App.nodejs_16_18_0, InstallPath).Wait(); - PluginsSettingsFilePath = ExecutablePath; + try { + FilesFolders.RemoveFolderIfExists(InstallPath, MessageBoxEx.Show); + await DroplexPackage.Drop(App.nodejs_16_18_0, InstallPath); + PluginsSettingsFilePath = ExecutablePath; + } + catch (Exception ex) { + MessageBoxEx.Show($"Failed to install TypeScript v2 environment: {ex.Message}"); + throw; + } }Plugins/Flow.Launcher.Plugin.WebSearch/SearchSourceViewModel.cs (1)
44-46: Consider improving error handling and user experience.The current error handling could be enhanced in several ways:
- Make the error message more user-friendly
- Add logging of the actual exception
- Provide clear next steps to the user
Consider this improvement:
catch (Exception ex) { #if DEBUG throw; #else - Flow.Launcher.Core.MessageBoxEx.Show(string.Format("Copying the selected image file to {0} has failed, changes will now be reverted", destinationFileNameFullPath)); + Log.Exception("Failed to copy image file", ex); + Flow.Launcher.Core.MessageBoxEx.Show( + $"Unable to save the custom icon. The original icon will be restored.\n\nDetails: {ex.Message}", + "Icon Save Error", + MessageBoxButton.OK, + MessageBoxImage.Warning); UpdateIconAttributes(selectedSearchSource, fullPathToOriginalImage); #endif }Plugins/Flow.Launcher.Plugin.Program/ProgramSuffixes.xaml.cs (1)
Line range hint
36-53: Consider improving code maintainabilityWhile the changes are functionally correct, consider these improvements:
- Add XML documentation to the
BtnAdd_OnClickmethod describing its validation behavior- Extract the translation keys to constants for better maintainability
+ /// <summary> + /// Validates and saves the suffix and protocol settings + /// </summary> private void BtnAdd_OnClick(object sender, RoutedEventArgs e) { + private const string SUFFIX_EMPTY_KEY = "flowlauncher_plugin_program_suffixes_cannot_empty"; + private const string PROTOCOL_EMPTY_KEY = "flowlauncher_plugin_protocols_cannot_empty"; var suffixes = tbSuffixes.Text.Split(Settings.SuffixSeparator, StringSplitOptions.RemoveEmptyEntries); var protocols = tbProtocols.Text.Split(Settings.SuffixSeparator, StringSplitOptions.RemoveEmptyEntries); if (suffixes.Length == 0 && UseCustomSuffixes) { - string warning = context.API.GetTranslation("flowlauncher_plugin_program_suffixes_cannot_empty"); + string warning = context.API.GetTranslation(SUFFIX_EMPTY_KEY); MessageBoxEx.Show(warning); return; } if (protocols.Length == 0 && UseCustomProtocols) { - string warning = context.API.GetTranslation("flowlauncher_plugin_protocols_cannot_empty"); + string warning = context.API.GetTranslation(PROTOCOL_EMPTY_KEY); MessageBoxEx.Show(warning); return; }Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingDownloadHelper.cs (1)
9-9: Consider removing unnecessary importSince we're transitioning away from System.Windows.MessageBox to MessageBoxEx, the
System.Windowsimport appears to be unused and can be removed.-using System.Windows;Plugins/Flow.Launcher.Plugin.Explorer/Views/ActionKeywordSetting.xaml.cs (1)
66-66: Consider explicitly specifying MessageBoxEx typeWhile the MessageBoxEx replacements are correct, consider explicitly specifying the message box type for better code clarity and maintainability. Since these are error messages, they should explicitly use MessageBoxButton.OK.
Example refactor:
-MessageBoxEx.Show(api.GetTranslation("plugin_explorer_globalActionKeywordInvalid")); +MessageBoxEx.Show(api.GetTranslation("plugin_explorer_globalActionKeywordInvalid"), + button: MessageBoxButton.OK);Also applies to: 69-69, 81-81
Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs (1)
41-41: Consider adding a meaningful title to the MessageBoxEx dialog.While the MessageBox replacement is correct, using an empty title (
string.Empty) might not provide the best user experience. Consider adding a translated title that provides context about the deletion action.- var result = MessageBoxEx.Show(formated, string.Empty, MessageBoxType.YesNo); + var title = _context.API.GetTranslation("flowlauncher_plugin_websearch_delete_title"); + var result = MessageBoxEx.Show(formated, title, MessageBoxType.YesNo);Plugins/Flow.Launcher.Plugin.WebSearch/SearchSourceSetting.xaml.cs (1)
59-69: Add window titles to MessageBoxEx dialogs for better UXThe MessageBoxEx calls for validation warnings should include descriptive titles to provide better context to users.
Consider updating the calls to include titles:
-MessageBoxEx.Show(warning); +MessageBoxEx.Show(warning, _api.GetTranslation("flowlauncher_plugin_websearch_validation_title"));Flow.Launcher.Core/Plugin/PluginsLoader.cs (1)
123-126: Consider improving message formattingThe current message formatting uses multiple string interpolations and Environment.NewLine calls. Consider simplifying it for better readability.
- MessageBoxEx.Show($"{errorMessage}{Environment.NewLine}{Environment.NewLine}" + - $"{errorPluginString}{Environment.NewLine}{Environment.NewLine}" + - $"Please refer to the logs for more information", "", + MessageBoxEx.Show( + $"{errorMessage}\n\n{errorPluginString}\n\nPlease refer to the logs for more information", + string.Empty, MessageBoxButton.OK, MessageBoxImage.Warning);Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (2)
104-104: Consider improving the error message and handlingWhile the change to MessageBoxEx aligns with the PR objectives, consider these improvements:
- Use a translated message key for consistency with other strings in the file
- Provide more informative error details to help users troubleshoot
- Consider logging the exception details for debugging
Here's a suggested improvement:
- MessageBoxEx.Show("Copy failed, please try later"); + string errorMessage = Context.API.GetTranslation("flowlauncher_plugin_calculator_copy_failed"); + Context.API.LogException("Failed to copy calculator result to clipboard", exception); + MessageBoxEx.Show(errorMessage);Don't forget to add the translation key to the resource files:
{ "flowlauncher_plugin_calculator_copy_failed": "Failed to copy result to clipboard. Please ensure no other program is blocking clipboard access and try again." }
Line range hint
114-117: Consider improving general error handling strategyThe current approach of silently catching all exceptions in the Query method could hide important errors from users and make debugging difficult.
Consider:
- Catching specific exceptions instead of using a general catch
- Logging exceptions for debugging
- Showing appropriate error messages to users when expression evaluation fails
- Adding unit tests for error scenarios
Would you like me to provide a detailed example of how to implement these improvements?
Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs (1)
9-12: Consider updating imports for better clarity.Since we're moving away from
System.Windows.MessageBox, we should be more specific with the imports. Replace the broadSystem.Windowsimport withSystem.Windows.MessageBoxResultif that's the only type needed from that namespace.-using System.Windows; +using System.Windows.MessageBoxResult; using System.Windows.Forms; using Flow.Launcher.Core.Resource; using static Flow.Launcher.Core.MessageBoxEx;Flow.Launcher.Plugin/SharedCommands/FilesFolders.cs (4)
24-25: LGTM! Consider improving the error message.The changes correctly implement dependency injection for the message box display. However, the error message could be more user-friendly.
Consider this improved error message:
-messageBoxExShow(string.Format("Copying path {0} has failed, it will now be deleted for consistency", targetPath)); +messageBoxExShow(string.Format("Failed to copy files to '{0}'. The incomplete copy will be removed to maintain consistency.", targetPath));Also applies to: 58-58, 66-66
79-81: Consider optimizing the directory comparison.The current implementation loads all files and directories into memory before comparison. For large directories, this could cause performance issues.
Consider this more efficient approach:
public static bool VerifyBothFolderFilesEqual(this string fromPath, string toPath, Func<string, MessageBoxResult> messageBoxExShow) { try { - var fromDir = new DirectoryInfo(fromPath); - var toDir = new DirectoryInfo(toPath); - - if (fromDir.GetFiles("*", SearchOption.AllDirectories).Length != toDir.GetFiles("*", SearchOption.AllDirectories).Length) - return false; - - if (fromDir.GetDirectories("*", SearchOption.AllDirectories).Length != toDir.GetDirectories("*", SearchOption.AllDirectories).Length) - return false; - - return true; + return Directory.EnumerateFileSystemEntries(fromPath, "*", SearchOption.AllDirectories) + .Count() == Directory.EnumerateFileSystemEntries(toPath, "*", SearchOption.AllDirectories) + .Count(); }Also applies to: 101-101
112-113: Enhance error handling with specific failure reasons.The error message could be more helpful by including the reason for the deletion failure.
Consider this improvement:
-catch (Exception) +catch (Exception ex) { #if DEBUG throw; #else - messageBoxExShow(string.Format("Not able to delete folder {0}, please go to the location and manually delete it", path)); + messageBoxExShow(string.Format("Unable to delete folder '{0}': {1}\nPlease try closing any applications using this folder and delete it manually.", + path, ex.Message)); #endif }Also applies to: 125-125
Line range hint
1-1: Overall implementation successfully replaces MessageBox.Show.The changes consistently implement the message box replacement across all methods while maintaining the existing functionality. The suggested improvements focus on:
- More user-friendly error messages
- Performance optimizations for directory operations
- Enhanced error handling with specific failure reasons
- Additional security checks for file operations
Consider extracting common error message formatting into a shared utility method to maintain consistency across all message box displays.
Plugins/Flow.Launcher.Plugin.Program/Views/ProgramSetting.xaml.cs (1)
183-183: Consider consolidating duplicate message handling.Both instances show the same translated message for program source selection. Consider extracting this into a shared helper method to improve maintainability and reduce code duplication.
+private void ShowProgramSourceSelectionMessage() +{ + string msg = context.API.GetTranslation("flowlauncher_plugin_program_pls_select_program_source"); + MessageBoxEx.Show(msg); +}Also applies to: 288-288
Plugins/Flow.Launcher.Plugin.Sys/Main.cs (3)
Line range hint
187-194: Consider adding braces for consistencyWhile the MessageBoxEx implementation is correct, consider adding braces around the if statement body for consistency with other similar blocks in the file.
if (result == MessageBoxResult.Yes) +{ Process.Start("shutdown", "/r /o /t 0"); +}
Line range hint
206-213: Consider adding braces for consistencyWhile the MessageBoxEx implementation is correct, consider adding braces around the if statement body for consistency with other similar blocks in the file.
if (result == MessageBoxResult.Yes) +{ ExitWindowsEx(EWX_LOGOFF, 0); +}
283-286: Consider using string interpolation for better readabilityWhile the MessageBoxEx implementation is correct, the error message construction could be more readable using string interpolation.
- MessageBoxEx.Show($"Error emptying recycle bin, error code: {result}\n" + - "please refer to https://msdn.microsoft.com/en-us/library/windows/desktop/aa378137", + MessageBoxEx.Show( + $""" + Error emptying recycle bin, error code: {result} + please refer to https://msdn.microsoft.com/en-us/library/windows/desktop/aa378137 + """, "Error", MessageBoxButton.OK, MessageBoxImage.Error);Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/SettingsViewModel.cs (1)
361-361: Consider adding warning icon for better UXSince this is a warning message shown when no item is selected, consider adding MessageBoxImage.Warning to make the message type more apparent to users.
- MessageBoxEx.Show(warning); + MessageBoxEx.Show(warning, string.Empty, MessageBoxButton.OK, MessageBoxImage.Warning);Plugins/Flow.Launcher.Plugin.Explorer/ContextMenu.cs (1)
178-183: LGTM! MessageBox replacement is properly implementedThe MessageBoxEx implementation maintains the same functionality while supporting the dark theme as intended. The warning icon is appropriately used for the delete confirmation dialog.
Consider enhancing the delete confirmation message
While the implementation is correct, consider making the delete confirmation message more user-friendly by including the file/folder type in the message.
- string.Format(Context.API.GetTranslation("plugin_explorer_delete_folder_link"), record.FullPath), + string.Format(Context.API.GetTranslation("plugin_explorer_delete_folder_link"), + isFile ? Context.API.GetTranslation("plugin_explorer_file") : Context.API.GetTranslation("plugin_explorer_folder"), + record.FullPath),Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs (3)
270-272: Consider extracting message box logic to a helper methodThe message box code for update confirmation is duplicated across multiple methods. Consider extracting this to a helper method to improve maintainability.
+ private bool ConfirmPluginUpdate(string pluginName, string author) + { + string message = Settings.AutoRestartAfterChanging + ? string.Format(Context.API.GetTranslation("plugin_pluginsmanager_update_prompt"), + pluginName, author, Environment.NewLine, Environment.NewLine) + : string.Format(Context.API.GetTranslation("plugin_pluginsmanager_update_prompt_no_restart"), + pluginName, author, Environment.NewLine); + + return MessageBoxEx.Show(message, + Context.API.GetTranslation("plugin_pluginsmanager_update_title"), + MessageBoxType.YesNo) == MessageBoxResult.Yes; + }
365-367: Consider consistent message box result handlingSome message box checks use
== MessageBoxResult.Nowhile others use!= MessageBoxResult.Yes. Consider standardizing this across the codebase for consistency.- if (MessageBoxEx.Show(message, - Context.API.GetTranslation("plugin_pluginsmanager_update_title"), - MessageBoxType.YesNo) == MessageBoxResult.No) + if (MessageBoxEx.Show(message, + Context.API.GetTranslation("plugin_pluginsmanager_update_title"), + MessageBoxType.YesNo) != MessageBoxResult.Yes)
479-484: Consider adding warning message to resource fileThe warning message for unknown sources is hardcoded. Consider moving it to the resource file for better internationalization support.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (25)
Flow.Launcher.Core/Configuration/Portable.cs(7 hunks)Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs(4 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/Updater.cs(4 hunks)Flow.Launcher.Plugin/SharedCommands/FilesFolders.cs(10 hunks)Plugins/Flow.Launcher.Plugin.Calculator/Main.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Explorer/ContextMenu.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingDownloadHelper.cs(3 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Search/ResultManager.cs(6 hunks)Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/SettingsViewModel.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Views/ActionKeywordSetting.xaml.cs(3 hunks)Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs(8 hunks)Plugins/Flow.Launcher.Plugin.Program/AddProgramSource.xaml.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Program/Flow.Launcher.Plugin.Program.csproj(1 hunks)Plugins/Flow.Launcher.Plugin.Program/ProgramSuffixes.xaml.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Program/Views/ProgramSetting.xaml.cs(4 hunks)Plugins/Flow.Launcher.Plugin.Sys/Flow.Launcher.Plugin.Sys.csproj(1 hunks)Plugins/Flow.Launcher.Plugin.Sys/Main.cs(6 hunks)Plugins/Flow.Launcher.Plugin.WebSearch/SearchSourceSetting.xaml.cs(5 hunks)Plugins/Flow.Launcher.Plugin.WebSearch/SearchSourceViewModel.cs(1 hunks)Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Flow.Launcher.Core/Configuration/Portable.cs
- Flow.Launcher.Core/Updater.cs
🔇 Additional comments (38)
Plugins/Flow.Launcher.Plugin.Program/AddProgramSource.xaml.cs (2)
2-2: LGTM: Required namespace import added
The Flow.Launcher.Core namespace import is correctly added to support the MessageBoxEx usage.
36-36: Verify complete MessageBox replacement
Let's ensure all System.Windows.MessageBox usages have been replaced with MessageBoxEx.
✅ Verification successful
All MessageBox.Show calls have been replaced with MessageBoxEx
Based on the search results, there are no remaining instances of System.Windows.MessageBox.Show in the codebase. All message box calls consistently use MessageBoxEx.Show. The change in AddProgramSource.xaml.cs aligns with this pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining System.Windows.MessageBox usages
rg "System\.Windows\.MessageBox\.Show"
# Search for MessageBoxEx usage pattern
rg "MessageBoxEx\.Show"
Length of output: 11504
Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptEnvironment.cs (1)
28-28: LGTM! Change aligns with PR objectives.
The addition of MessageBoxEx.Show as a callback parameter to RemoveFolderIfExists correctly implements the PR's goal of replacing System.Windows.MessageBox with the new dark-theme compatible MessageBoxEx.
Let's verify this change is consistent with other similar uses in the codebase:
✅ Verification successful
Verified: MessageBoxEx.Show is consistently used with RemoveFolderIfExists
The verification shows that all instances of RemoveFolderIfExists across the codebase correctly use MessageBoxEx.Show as the callback parameter:
- Flow.Launcher.Core/Configuration/Portable.cs
- Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptV2Environment.cs
- Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptEnvironment.cs
- Flow.Launcher.Core/ExternalPlugins/Environments/PythonEnvironment.cs
- Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs
Additionally, no remaining instances of System.Windows.MessageBox were found in the codebase, confirming complete migration to MessageBoxEx.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of MessageBoxEx.Show with RemoveFolderIfExists
# Expected: All RemoveFolderIfExists calls should use MessageBoxEx.Show as callback
# Search for RemoveFolderIfExists usage
rg -A 2 "RemoveFolderIfExists\("
# Search for any remaining System.Windows.MessageBox usage that might need updating
rg "System\.Windows\.MessageBox"
Length of output: 2725
Flow.Launcher.Core/ExternalPlugins/Environments/PythonEnvironment.cs (2)
31-31: Verify MessageBoxEx import and usage pattern
The change aligns with the PR objective to replace System.Windows.MessageBox with MessageBoxEx. However, we should verify the import and consistent usage pattern.
Line range hint 31-36: Verify MessageBoxEx compatibility across Windows versions
Given that this code handles Python version compatibility issues (specifically Windows 7), ensure that MessageBoxEx properly displays error messages across all supported Windows versions.
✅ Verification successful
Let me analyze the MessageBoxEx usage specifically to verify its compatibility across Windows versions.
Let me check for any platform-specific dependencies or OS version checks in the MessageBoxEx implementation and its base Window class.
MessageBoxEx is compatible across Windows versions
The analysis shows that MessageBoxEx is a simple WPF Window-based dialog implementation that:
- Uses standard WPF components without any platform-specific code
- Has no OS version checks or Windows API dependencies
- Doesn't implement any special handling for different Windows versions
Since it's built on standard WPF which maintains backward compatibility, the MessageBoxEx implementation will work consistently across supported Windows versions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for Windows version-specific handling in MessageBoxEx
# Look for any Windows version checks in MessageBoxEx implementation
rg "Windows|OS|Platform" --type cs
# Check for any version-specific UI handling
ast-grep --pattern 'if ($_ == $_ || $_.Contains("Windows")) {
$$$
}'
Length of output: 89227
Script:
#!/bin/bash
# Check MessageBoxEx implementation and usage
ast-grep --pattern 'class MessageBoxEx {
$$$
}'
# Check for any version-specific handling in MessageBoxEx
rg -A 5 "MessageBoxEx" Flow.Launcher.Core/MessageBoxEx.xaml.cs
Length of output: 771
Script:
#!/bin/bash
# Check for platform dependencies in MessageBoxEx
rg -l "Platform|Windows" Flow.Launcher.Core/MessageBoxEx.xaml*
# Check for any OS version checks in the file
rg "OSVersion|Environment\." Flow.Launcher.Core/MessageBoxEx.xaml*
# Check for any native Windows API calls
rg "DllImport|extern" Flow.Launcher.Core/MessageBoxEx.xaml*
Length of output: 352
Plugins/Flow.Launcher.Plugin.Sys/Flow.Launcher.Plugin.Sys.csproj (1)
40-40: LGTM! The project reference addition is necessary for MessageBoxEx integration.
The addition of Flow.Launcher.Core project reference is required to access the MessageBoxEx implementation, which aligns with the PR objective of replacing System.Windows.MessageBox.
Let's verify the project reference is valid and the project builds:
✅ Verification successful
Project reference is valid and MessageBoxEx is properly available
The verification confirms:
- Flow.Launcher.Core project exists at the specified relative path
- MessageBoxEx implementation is present in Core project (MessageBoxEx.xaml and MessageBoxEx.xaml.cs)
- Multiple components in Core already utilize MessageBoxEx, indicating it's a stable component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Flow.Launcher.Core project exists and contains MessageBoxEx
# Check if the Core project exists at the specified path
if [ -f "Flow.Launcher.Core/Flow.Launcher.Core.csproj" ]; then
echo "✓ Core project exists"
# Verify MessageBoxEx exists in the Core project
rg -l "MessageBoxEx" "Flow.Launcher.Core/"
else
echo "✗ Core project not found at relative path"
fi
Length of output: 772
Plugins/Flow.Launcher.Plugin.WebSearch/SearchSourceViewModel.cs (1)
44-44: LGTM! MessageBox replacement aligns with PR objectives.
The change from MessageBox.Show to Flow.Launcher.Core.MessageBoxEx.Show successfully implements the dark theme support as intended.
Plugins/Flow.Launcher.Plugin.Program/Flow.Launcher.Plugin.Program.csproj (1)
57-57: LGTM! Verify minimal dependency requirement.
The addition of Flow.Launcher.Core project reference is necessary to support the MessageBoxEx functionality. However, let's verify if this is the minimal required dependency.
✅ Verification successful
Project reference to Flow.Launcher.Core is justified
The dependency is minimal and necessary as evidenced by:
- MessageBoxEx is the only type imported from Flow.Launcher.Core (via using static directive)
- Multiple files in the Program plugin use MessageBoxEx.Show for displaying messages
- No other types from Flow.Launcher.Core are being used
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if MessageBoxEx is the only type used from Flow.Launcher.Core
# This helps determine if we're pulling in more dependencies than necessary
# Search for all Flow.Launcher.Core type usages in the Program plugin
rg "Flow\.Launcher\.Core\." -g "*.cs" "Plugins/Flow.Launcher.Plugin.Program"
# Search specifically for MessageBoxEx usage
rg "MessageBoxEx\." -g "*.cs" "Plugins/Flow.Launcher.Plugin.Program"
Length of output: 962
Plugins/Flow.Launcher.Plugin.Program/ProgramSuffixes.xaml.cs (2)
4-4: LGTM: Required import for MessageBoxEx
The addition of the Flow.Launcher.Core namespace import is necessary and correctly placed.
43-43: Verify MessageBoxEx behavior with localized strings
The MessageBox replacements align with the PR objectives for dark theme support. However, we should verify:
- MessageBoxEx properly handles localized strings
- The warning message display behavior remains consistent
Also applies to: 50-50
✅ Verification successful
MessageBoxEx is consistently used for localized strings across the codebase
The verification shows that MessageBoxEx.Show is used extensively throughout the codebase with both direct localized strings and GetTranslation calls, following the same pattern as the changes in ProgramSuffixes.xaml.cs. The warning message display behavior is consistent with other validation scenarios in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if MessageBoxEx is used consistently with localized strings elsewhere
ast-grep --pattern 'MessageBoxEx.Show($api.GetTranslation($_))'
# Check if MessageBoxEx maintains consistent behavior with the original MessageBox
ast-grep --pattern 'MessageBoxEx.Show($_)'
Length of output: 6567
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingDownloadHelper.cs (2)
25-28: LGTM! MessageBox replacement implemented correctly
The MessageBoxEx implementation maintains the original functionality while improving the UI consistency. The translation keys and logic flow are preserved.
56-56: Verify consistency of OpenPath usage
The modification to pass MessageBoxEx.Show to OpenPath appears correct, but we should ensure this pattern is consistently applied across the codebase.
✅ Verification successful
OpenPath usage is consistent across the codebase
All instances of FilesFolders.OpenPath correctly pass the MessageBoxEx.Show delegate as the second parameter:
EverythingDownloadHelper.cs:FilesFolders.OpenPath(installedLocation, MessageBoxEx.Show)Portable.cs:FilesFolders.OpenPath(Constant.RootDirectory, MessageBoxEx.Show)
The method signature in FilesFolders.cs confirms this is the expected usage pattern: public static void OpenPath(string fileOrFolderPath, Func<string, MessageBoxResult> messageBoxExShow)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other OpenPath calls to verify consistent usage
rg "OpenPath\(" --type cs -B 2 -A 2
Length of output: 1626
Plugins/Flow.Launcher.Plugin.Explorer/Views/ActionKeywordSetting.xaml.cs (2)
6-6: LGTM: Required namespace import added
The addition of Flow.Launcher.Core namespace is necessary for accessing the MessageBoxEx class.
Line range hint 66-81: Verify MessageBoxEx implementation
Let's ensure the MessageBoxEx class is properly implemented and handles these message scenarios correctly.
✅ Verification successful
MessageBoxEx implementation is properly configured and functional
The MessageBoxEx class is correctly implemented in Flow.Launcher.Core with:
- Full support for different message box types (Information, Warning, Error, etc.)
- Proper button handling (OK, Yes/No, Yes/No/Cancel)
- Correct image display for different message types
- Proper window styling and layout
- Proper handling of message box results
The implementation is being used correctly in the Explorer plugin's ActionKeywordSetting.xaml.cs file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify MessageBoxEx implementation and usage
# Check if MessageBoxEx exists and supports message box types
ast-grep --pattern 'class MessageBoxEx {
$$$
Show($$$) {
$$$
}
$$$
}'
# Check for other similar MessageBox replacements
rg "MessageBox\.Show" -l
Length of output: 110
Script:
#!/bin/bash
# Let's try a different approach to locate MessageBoxEx
# Find files containing MessageBoxEx
rg "MessageBoxEx" -l
# Search for MessageBoxEx class definition with a more flexible pattern
ast-grep --pattern 'class MessageBoxEx'
# Look for any files with MessageBox in their name
fd MessageBox
# Check imports/using statements for MessageBoxEx
rg "using.*MessageBox"
Length of output: 38049
Script:
#!/bin/bash
# Let's examine the MessageBoxEx implementation
# Check the actual implementation in MessageBoxEx.xaml.cs
cat "Flow.Launcher.Core/MessageBoxEx.xaml.cs"
# Check the XAML definition
cat "Flow.Launcher.Core/MessageBoxEx.xaml"
Length of output: 13128
Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs (2)
6-7: LGTM! Using directives properly added for MessageBoxEx.
The new using directives are correctly added to support the MessageBox replacement objective.
Line range hint 1-150: Verify complete replacement of MessageBox across the codebase.
Let's ensure all MessageBox instances have been replaced with MessageBoxEx consistently.
✅ Verification successful
Let me verify the actual usage pattern by checking the content of these files.
Based on the results, let me check one more aspect to ensure all MessageBox usages are properly using MessageBoxEx.
All MessageBox instances have been properly replaced with MessageBoxEx
Based on the comprehensive verification:
- All MessageBox.Show calls are using MessageBoxEx.Show
- The only remaining MessageBox references are in MessageBoxEx.xaml.cs which is the actual implementation
- The imports found are only aliasing MessageBoxImage from MessageBoxEx, not from System.Windows
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining System.Windows.MessageBox usages
# Search for remaining MessageBox usages
echo "Checking for remaining MessageBox usages..."
rg -l "System\.Windows\.MessageBox|MessageBox\.Show" --type cs
# Search for new MessageBoxEx usage pattern
echo -e "\nVerifying MessageBoxEx usage pattern..."
rg -l "MessageBoxEx\.Show" --type cs
Length of output: 2196
Script:
#!/bin/bash
# Check for any direct System.Windows.MessageBox imports or usages
echo "Checking for System.Windows.MessageBox imports..."
rg "using System\.Windows\.MessageBox|using MessageBox" --type cs
# Check actual MessageBox.Show usage patterns to ensure they're all MessageBoxEx
echo -e "\nChecking MessageBox.Show usage patterns..."
rg "MessageBox\.Show\(" -A 2 --type cs
Length of output: 674
Script:
#!/bin/bash
# Search for actual MessageBoxEx.Show usage patterns
echo "Checking MessageBoxEx.Show usage patterns..."
rg "MessageBoxEx\.Show\(" -A 2 --type cs
# Double check for any remaining System.Windows.MessageBox references
echo -e "\nChecking for any remaining MessageBox references..."
rg "new MessageBox|class MessageBox|\WMessageBox\W" --type cs
Length of output: 23875
Plugins/Flow.Launcher.Plugin.WebSearch/SearchSourceSetting.xaml.cs (2)
142-142: LGTM! Clean MessageBoxEx implementation for icon path hints
The MessageBoxEx replacement here is well-integrated with the existing conditional logic and translation system.
5-5: Verify complete migration to MessageBoxEx
The MessageBoxEx import has been correctly added. Let's verify no MessageBox usages were missed.
✅ Verification successful
Migration to MessageBoxEx is complete and properly imported
The verification confirms:
- No remaining System.Windows.MessageBox usages found in the codebase
- All files using MessageBoxEx.Show have properly imported Flow.Launcher.Core
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining System.Windows.MessageBox usages
rg -l "System\.Windows\.MessageBox" || echo "No remaining MessageBox usages found"
# Verify MessageBoxEx is properly imported everywhere it's used
rg -l "MessageBoxEx\.Show" | xargs rg "using Flow\.Launcher\.Core" || echo "MessageBoxEx import missing in some files"
Length of output: 4358
Flow.Launcher.Core/Plugin/PluginsLoader.cs (2)
6-6: LGTM! Using directives align with MessageBoxEx migration
The added using directives properly support the transition from System.Windows.MessageBox to MessageBoxEx.
Also applies to: 14-14
123-126: Verify exception handling in fire-and-forget Task
While the async Task.Run prevents UI blocking, ensure that unhandled exceptions are properly captured.
Let's check if there's a global exception handler for unobserved task exceptions:
✅ Verification successful
Fire-and-forget Task pattern is safely handled
The codebase has proper global exception handling in place:
AppDomain.CurrentDomain.UnhandledExceptionhandler for non-UI thread exceptionsDispatcherUnhandledExceptionhandler for UI thread exceptions- Both handlers use
ErrorReporting.Report()to log exceptions - UI exceptions are marked as handled to prevent application exit
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TaskScheduler.UnobservedTaskException handler
rg -l "TaskScheduler\.UnobservedTaskException"
# Search for other potential global exception handlers
ast-grep --pattern 'AppDomain.CurrentDomain.UnhandledException += $_'
Length of output: 120
Script:
#!/bin/bash
# Let's search for any exception handling patterns in the codebase
# First, check for global exception handling setup in Program/Main/App
rg -A 5 "static void Main|public class App\s*:|class Program\s*:"
# Look for any TaskScheduler or unhandled exception related code
rg -i "unhandled.*exception|taskscheduler|application_thread.*exception"
# Check the MessageBoxEx implementation for exception handling
fd -e cs MessageBoxEx.cs --exec cat {}
Length of output: 2177
Script:
#!/bin/bash
# Let's check the ErrorReporting implementation to understand the exception handling
rg -A 10 "UnhandledExceptionHandle|DispatcherUnhandledException" Flow.Launcher/Helper/ErrorReporting.cs
# Also check MessageBoxEx implementation since it wasn't found in previous search
rg -l "MessageBoxEx"
# If found, let's look at its implementation
rg -A 5 "class MessageBoxEx"
Length of output: 3064
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (1)
10-10: LGTM: Required namespace for MessageBoxEx
The addition of the Flow.Launcher.Core namespace is necessary for using MessageBoxEx.
Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs (3)
62-62: LGTM! MessageBox replacement is correctly implemented.
The change from MessageBox.Show to MessageBoxEx.Show is properly done with the correct message box type and result handling.
103-103: LGTM! Correct delegate usage.
The MessageBoxEx.Show method is correctly passed as a delegate to RemoveFolderIfExists.
12-12: Verify consistent MessageBoxEx usage across the codebase.
Let's ensure all MessageBox replacements follow consistent patterns and no instances were missed.
✅ Verification successful
All MessageBox.Show calls have been replaced with MessageBoxEx
The verification shows consistent usage of MessageBoxEx across the codebase:
- No remaining instances of System.Windows.MessageBox.Show were found
- All message box calls use the MessageBoxEx wrapper consistently
- The usage patterns are uniform, with proper parameter passing for messages, titles, and dialog types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining System.Windows.MessageBox usage
echo "Checking for remaining MessageBox.Show usage..."
rg "MessageBox\.Show" --type cs
# Search for MessageBoxEx usage patterns
echo -e "\nChecking MessageBoxEx usage patterns..."
rg "MessageBoxEx\.Show\(" -A 2 --type cs
Length of output: 23659
Plugins/Flow.Launcher.Plugin.Program/Views/ProgramSetting.xaml.cs (1)
13-14: LGTM! Clean namespace organization.
The addition of Flow.Launcher.Core namespace and static import of MessageBoxEx aligns with the PR objective of replacing System.Windows.MessageBox throughout the codebase.
Plugins/Flow.Launcher.Plugin.Explorer/Search/ResultManager.cs (3)
15-15: LGTM: Required import for MessageBoxEx
The addition of the Flow.Launcher.Core namespace import is necessary for accessing the MessageBoxEx class, aligning with the PR's objective.
127-127: LGTM: Consistent MessageBoxEx replacements
The MessageBox.Show replacements with MessageBoxEx.Show are consistent across all error handling blocks. Each usage properly maintains:
- Error message from the exception
- Translated title from Context.API
- Original error handling flow
Also applies to: 141-141, 156-156, 319-319
341-341: LGTM: Good use of dependency injection for MessageBoxEx
The modification to pass MessageBoxEx.Show as a parameter to FilesFolders.OpenFile is a good practice, allowing for flexible message box handling through dependency injection.
Let's verify the FilesFolders.OpenFile implementation accepts this new parameter:
✅ Verification successful
LGTM: FilesFolders.OpenFile implementation matches the usage
The implementation in Flow.Launcher.Plugin/SharedCommands/FilesFolders.cs correctly accepts the message box delegate parameter:
public static void OpenFile(string filePath, Func<string, MessageBoxResult> messageBoxExShow, string workingDir = "", bool asAdmin = false)This matches the usage in the review where MessageBoxEx.Show is passed as the second parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for FilesFolders.OpenFile method definition
ast-grep --pattern 'class FilesFolders {
$$$
OpenFile($$$) {
$$$
}
$$$
}'
Length of output: 87
Script:
#!/bin/bash
# Let's try a different approach to find the FilesFolders.OpenFile method
# First, let's find the file containing FilesFolders class
fd "FilesFolders.cs"
# Then search for OpenFile method with ripgrep for better context
rg "public.*OpenFile.*\(" -A 10
Length of output: 2303
Plugins/Flow.Launcher.Plugin.Sys/Main.cs (3)
10-10: LGTM: Import changes align with MessageBoxEx migration
The updated imports correctly reflect the transition from System.Windows.MessageBox to Flow.Launcher.Core.MessageBoxEx.
Also applies to: 18-18
Line range hint 147-154: LGTM: Shutdown confirmation dialog properly migrated
The MessageBoxEx implementation maintains the same functionality while supporting the new theming system.
Line range hint 167-174: LGTM: Restart confirmation dialog properly migrated
The MessageBoxEx implementation is consistent with the shutdown dialog pattern.
Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/SettingsViewModel.cs (2)
2-2: LGTM!
The added using directive for Flow.Launcher.Core is correctly placed and necessary for accessing MessageBoxEx.
361-361: Verify MessageBox replacement across the codebase
Let's verify that all MessageBox.Show calls have been replaced with MessageBoxEx.Show.
✅ Verification successful
All MessageBox.Show calls have been replaced with MessageBoxEx.Show
Based on the search results, all MessageBox.Show calls in the codebase are using MessageBoxEx.Show. There are no remaining instances of the original System.Windows.MessageBox.Show usage. The replacement has been consistently applied across all files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining System.Windows.MessageBox usage
# Search for MessageBox usage in source files
echo "Searching for remaining MessageBox.Show usage..."
rg "MessageBox\.Show" --type-add 'csharp:*.{cs,xaml}' -t csharp
# Search for System.Windows.MessageBox using declarations
echo -e "\nSearching for System.Windows.MessageBox using declarations..."
rg "using System\.Windows\.MessageBox" --type-add 'csharp:*.{cs,xaml}' -t csharp
Length of output: 394
Script:
#!/bin/bash
# Let's try a more comprehensive search for MessageBox usage
# Search for any MessageBox class usage (including MessageBoxEx)
echo "Searching for all MessageBox usage patterns..."
rg "(?:System\.Windows\.)?MessageBox(?:Ex)?\.Show" --type-add 'csharp:*.{cs,xaml}' -t csharp -A 2
# Search for MessageBox type imports/using statements
echo -e "\nSearching for MessageBox related imports..."
rg "using.*MessageBox" --type-add 'csharp:*.{cs,xaml}' -t csharp
# Search for MessageBox class declarations or extensions
echo -e "\nSearching for MessageBox class declarations..."
ast-grep --pattern 'class MessageBox$_'
Length of output: 63669
Plugins/Flow.Launcher.Plugin.Explorer/ContextMenu.cs (2)
14-15: LGTM! Using directives are properly organized
The new imports align with the PR objective to replace System.Windows.MessageBox with MessageBoxEx. The MessageBoxImage alias helps maintain code readability.
178-183: Verify consistent MessageBox usage and translations
Let's verify that:
- All MessageBox usages in the codebase have been replaced with MessageBoxEx
- The translation key "plugin_explorer_delete_folder_link" exists and is properly formatted
✅ Verification successful
MessageBox usage and translations are properly implemented
Based on the verification results:
- All MessageBox usages in the codebase consistently use MessageBoxEx
- The translation key "plugin_explorer_delete_folder_link" exists in all language XAML files
- The specific usage in ContextMenu.cs follows the correct pattern with proper imports and usage of MessageBoxEx with appropriate parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining System.Windows.MessageBox usages
echo "Checking for remaining System.Windows.MessageBox usages..."
rg "System\.Windows\.MessageBox" --type cs
# Check for the translation key usage
echo "Checking translation key usage..."
rg "plugin_explorer_delete_folder_link" --type json --type resx
Length of output: 341
Script:
#!/bin/bash
# Check for any remaining MessageBox usages with more context
echo "Checking MessageBox usages..."
rg "MessageBox" -A 2 --type cs
# Check for translation files and their content
echo "Looking for translation files..."
fd -e json -e xaml . Plugins/Flow.Launcher.Plugin.Explorer/
# If found any translation files, let's check their content
echo "Checking translation file content..."
find Plugins/Flow.Launcher.Plugin.Explorer/ -type f -name "*.json" -exec cat {} \;
Length of output: 49912
Script:
#!/bin/bash
# Check for translation key in XAML files
echo "Checking for translation key in language files..."
find Plugins/Flow.Launcher.Plugin.Explorer/Languages/ -type f -name "*.xaml" -exec grep -l "plugin_explorer_delete_folder_link" {} \;
# Check MessageBoxEx usage in the specific file
echo "Checking MessageBoxEx usage in ContextMenu.cs..."
cat "Plugins/Flow.Launcher.Plugin.Explorer/ContextMenu.cs"
Length of output: 23991
Flow.Launcher.Core/Plugin/PluginManager.cs (1)
522-522: LGTM! Verify other MessageBox usages
The update to use MessageBoxEx.Show as the message display callback aligns with the PR objective of replacing System.Windows.MessageBox with MessageBoxEx for dark theme support.
Let's verify if there are other MessageBox usages that need to be updated:
✅ Verification successful
All MessageBox usages have been migrated to MessageBoxEx
Based on the search results, all message box usages in the codebase are already using MessageBoxEx.Show(). There are no remaining instances of System.Windows.MessageBox.Show(). The changes in the PR align with the existing codebase's usage of MessageBoxEx for dark theme support.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for remaining System.Windows.MessageBox usages
rg "System\.Windows\.MessageBox" --type cs
# Search for MessageBox.Show usages that might need updating
rg "MessageBox\.Show" --type cs
# Search for potential message box related imports
rg "using.*MessageBox" --type cs
Length of output: 1545
Script:
#!/bin/bash
# Search for actual MessageBox.Show usages in the codebase
ast-grep --pattern 'MessageBox.Show($$$)'
# Search for actual MessageBoxEx.Show usages in the codebase
ast-grep --pattern 'MessageBoxEx.Show($$$)'
Length of output: 18967
Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs (2)
1-1: LGTM: Import changes align with the MessageBoxEx migration
The addition of Flow.Launcher.Core and static import of MessageBoxEx is appropriate for the MessageBox replacement changes.
Also applies to: 16-16
136-137: Verify error handling for plugin installation cancellation
The MessageBox replacement looks correct, but we should ensure proper error handling when a user cancels the installation.
✅ Verification successful
Let me check the broader context around this code to ensure we understand the full flow of the installation process.
Let me try a different pattern to find the installation method.
Let me try one more search to find the installation method with a simpler pattern.
Proper error handling exists for plugin installation cancellation
Based on the code review:
- When user clicks "No" in the MessageBox, the code returns immediately with a clean
returnstatement - For actual installation, proper error handling exists with multiple catch blocks for different scenarios
- Cleanup is properly handled:
- Downloaded files are deleted if not from local install path
- Temporary folders are created with unique GUIDs
- Installation errors are logged and shown to users
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there's proper cleanup when installation is cancelled
# Look for any cleanup code after MessageBox.Show calls
rg -A 5 "MessageBoxEx.Show.*plugin_pluginsmanager_install"
Length of output: 839
Script:
#!/bin/bash
# Get more context around the installation method
ast-grep --pattern 'async Task InstallPlugin($$$)'
Length of output: 52
Script:
#!/bin/bash
# Search for the installation method with a broader pattern
rg -B 5 -A 15 "InstallPlugin.*async"
Length of output: 38
Script:
#!/bin/bash
# Search for any method containing InstallPlugin
rg "InstallPlugin" -A 20 -B 5
Length of output: 7455
Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptV2Environment.cs
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.WebSearch/SearchSourceSetting.xaml.cs
Outdated
Show resolved
Hide resolved
Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.Program/Views/ProgramSetting.xaml.cs
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
Jack251970
changed the title
There was a problem hiding this comment.
Hey @Jack251970, could you fix the build fail https://ci.appveyor.com/project/JohnTheGr8/flow-launcher/builds/51049381. Let me know if you need help.
|
@jjw24 Sorry, I wonder why this failure will happen because it can run in VS. Will sync fork help? Or I need to do something else? |
|
See line 398 of https://ci.appveyor.com/project/JohnTheGr8/flow-launcher/builds/51049381#L398. It's complaining about 'no argument given that corresponds to the required parameter 'messageBoxExShow' of 'FilesFolders.RemoveFolderIfExists'. I can't tell why without going infront of a computer, maybe also check if you still have changes not pushed up yet? |
Plugins/Flow.Launcher.Plugin.Sys/Flow.Launcher.Plugin.Sys.csproj
Outdated
Show resolved
Hide resolved
Sorry, I will check it. Perhaps I miss something. |
This comment has been minimized.
This comment has been minimized.
…em.Windows.MessageBoxButton & MessageBoxImage Remove useless warning file
…sue & Improve close button behaviour
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Flow.Launcher.Core/MessageBoxEx.xaml.cs(1 hunks)
🔇 Additional comments (3)
Flow.Launcher.Core/MessageBoxEx.xaml.cs (3)
14-15: Consider replacing static fields with instance fields
The use of static fields msgBox and _result can lead to race conditions if multiple message boxes are shown simultaneously. Additionally, static fields can cause memory leaks if not properly cleaned up. Refactoring to instance fields can improve thread safety and resource management.
192-206: Consolidate event handling logic for button clicks
The Button_Click method uses multiple if statements to determine the result based on the button clicked. Using a switch expression can improve readability and maintainability.
Refactor the method as follows:
- if (sender == btnOk)
- _result = MessageBoxResult.OK;
- else if (sender == btnYes)
- _result = MessageBoxResult.Yes;
- else if (sender == btnNo)
- _result = MessageBoxResult.No;
- else if (sender == btnCancel)
- _result = MessageBoxResult.Cancel;
- else
- _result = MessageBoxResult.None;
+ _result = sender switch
+ {
+ var btn when btn == btnOk => MessageBoxResult.OK,
+ var btn when btn == btnYes => MessageBoxResult.Yes,
+ var btn when btn == btnNo => MessageBoxResult.No,
+ var btn when btn == btnCancel => MessageBoxResult.Cancel,
+ _ => MessageBoxResult.None
+ };180-190: 🛠️ Refactor suggestion
Ensure consistent cleanup after closing the message box
In the KeyEsc_OnPress method, msgBox is not set to null after calling Close(), unlike other event handlers. This inconsistency might lead to memory leaks or unexpected behavior.
Add msgBox = null; after Close();:
DialogResult = false;
Close();
+ msgBox = null;Likely invalid or redundant comment.
jjw24
added
the
review in progress
|
@Jack251970 could you please list in the PR description, what tests you have done so far for your changes. This will let me know what other tests I can do for this change. |
|
@jjw24 Sure, please see |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
Flow.Launcher.Plugin/SharedCommands/FilesFolders.cs (3)
24-25: Improve error message clarity in CopyAll methodWhile the messageBoxExShow parameter is correctly implemented, the error message could be more specific about what failed during the copy operation.
Consider this improvement:
-messageBoxExShow(string.Format("Copying path {0} has failed, it will now be deleted for consistency", targetPath)); +messageBoxExShow(string.Format("Failed to copy files to '{0}': {1}. The incomplete copy will be deleted for consistency", + targetPath, + ex.Message));Also applies to: 66-68
80-82: Add input validation and improve error handlingThe method could benefit from upfront parameter validation and more specific error handling.
Consider these improvements:
public static bool VerifyBothFolderFilesEqual(this string fromPath, string toPath, Func<string, MessageBoxResult> messageBoxExShow = null) { + if (string.IsNullOrEmpty(fromPath) || string.IsNullOrEmpty(toPath)) + { + return false; + } + try { var fromDir = new DirectoryInfo(fromPath); var toDir = new DirectoryInfo(toPath); if (!fromDir.Exists || !toDir.Exists) { return false; } // ... rest of the method } catch (Exception ex) { messageBoxExShow ??= MessageBox.Show; - messageBoxExShow(string.Format("Unable to verify folders and files between {0} and {1}", fromPath, toPath)); + messageBoxExShow(string.Format("Failed to compare folders '{0}' and '{1}': {2}", fromPath, toPath, ex.Message)); return false; } }Also applies to: 102-103
Line range hint
24-209: Consider platform-independent message handlingThe current implementation creates a dependency on Windows-specific MessageBox in a shared utility class. Consider using a platform-independent approach for better modularity.
Consider these alternatives:
- Create an interface for message handling that can be implemented differently for each platform
- Use a callback pattern that doesn't reference Windows-specific types
- Move platform-specific code to a separate class
Example interface approach:
public interface IMessageHandler { void ShowMessage(string message); MessageResult ShowMessageWithResult(string message); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Flow.Launcher.Plugin/SharedCommands/FilesFolders.cs(10 hunks)
🔇 Additional comments (2)
Flow.Launcher.Plugin/SharedCommands/FilesFolders.cs (2)
114-115: Add default parameter value and improve error handling
The method needs a default parameter value as requested in previous comments, and the error handling could be more specific.
Consider these improvements:
-public static void RemoveFolderIfExists(this string path, Func<string, MessageBoxResult> messageBoxExShow = null)
+public static void RemoveFolderIfExists(this string path, Func<string, MessageBoxResult> messageBoxExShow = MessageBox.Show)
{
try
{
if (Directory.Exists(path))
Directory.Delete(path, true);
}
- catch (Exception)
+ catch (Exception ex)
{
#if DEBUG
throw;
#else
- messageBoxExShow ??= MessageBox.Show;
- messageBoxExShow(string.Format("Not able to delete folder {0}, please go to the location and manually delete it", path));
+ messageBoxExShow(string.Format("Failed to delete folder '{0}': {1}. Please try deleting it manually.",
+ path,
+ ex is UnauthorizedAccessException ? "Access denied" : ex.Message));
#endif
}
}Also applies to: 127-128
188-189: Add security checks and improve error handling as previously suggested
The previous review comments about adding security checks for admin execution and improving error handling are still relevant.
Please refer to the previous review comment for the detailed implementation of security checks and error handling improvements for the OpenFile method.
Also applies to: 208-209
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
📒 Files selected for processing (2)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs(2 hunks)Flow.Launcher/PublicAPIInstance.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs
🔇 Additional comments (1)
Flow.Launcher/PublicAPIInstance.cs (1)
28-28: LGTM!
The new using directive is correctly placed and necessary for accessing the MessageBoxEx implementation.
This comment has been minimized.
This comment has been minimized.
|
@onesounds is there a reason that previously the no is on left yes on right? |
I can't remember why either. As far as I know, this order of PR is correct. I wish I could change the order freely on the code, ("no yes" or "yes no" but I remember it was probably hard to make. (I had to make several buttons.) |
@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 ...
|
|
@Jack251970 I have made the MessageBoxEx's Show method to use defaults so we can remove the other unnecessary overrides, please see my commit. Everything else looks really good. Let me know if this is OK. |
jjw24
removed
review in progress
|
@jjw24 Thank you! I have forgotton to simplify functions in the MessageBoxEx.xaml.cs. I use dark mode for my system and could not wait to experience this improvement. |
In #2774, a new message box with dark theme support is introduced.
This PR replaces all System.Windows.MessageBox in Flow.Launcher & Flow.Launcher.Core namespace with the new message box.
Also improve original MessageBoxEx of OK type:
Original:
Now:
Move Yes, OK button to the left of No, Cancel button and focus on Yes, OK by default as System.Windows.MessageBox:
Original:
Now:
Test already done:
MessageBoxEx.Show(InternationalizationManager.Instance.GetTranslation(message));. (direct call method of MessageBoxEx)var result = context.API.ShowMsgBox( context.API.GetTranslation("flowlauncher_plugin_sys_dlgtext_shutdown_computer"), context.API.GetTranslation("flowlauncher_plugin_sys_shutdown_computer"),. (api call method of MessageBoxEx)