Fluent Window #3271
Fluent Window #3271
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. |
📝 WalkthroughWalkthroughThe pull request introduces enhancements to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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: 0
🧹 Nitpick comments (4)
Flow.Launcher.Infrastructure/Win32Helper.cs (1)
24-31: Consider adding error handling for DWM API calls.The
SetMicaForWindowmethod should handle potential DWM API failures:public static void SetMicaForWindow(Window window, bool enableMica) { var windowHelper = new WindowInteropHelper(window); windowHelper.EnsureHandle(); DwmSystemBackdropType backdropType = enableMica ? DwmSystemBackdropType.DWMSBT_MICA : DwmSystemBackdropType.DWMSBT_NONE; - DwmSetWindowAttribute(windowHelper.Handle, DWMWA_SYSTEMBACKDROP_TYPE, ref backdropType, sizeof(int)); + var result = DwmSetWindowAttribute(windowHelper.Handle, DWMWA_SYSTEMBACKDROP_TYPE, ref backdropType, sizeof(int)); + if (result != 0) + { + Log.Error($"Failed to set window backdrop type. Error code: {result}"); + } }Flow.Launcher/ViewModel/MainViewModel.cs (1)
412-413: Translate Korean comments to English.The comment "텍스트 초기화 즉시 적용" should be in English for consistency.
-// 텍스트 초기화 즉시 적용 +// Immediately apply text initializationFlow.Launcher.Core/Resource/Theme.cs (2)
230-246: Remove commented-out code.The
BGColormethod is commented out and should be removed if no longer needed.-//public SolidColorBrush BGColor(string colorBgKey) -//{ -// var defaultBG = Application.Current.TryFindResource("DefaultBG"); -// var darkBG = Application.Current.TryFindResource("DarkBG"); -// -// if (colorBgKey == "") -// { -// var resource = Application.Current.TryFindResource("BlurMode"); -// -// if (resource is string) -// return (SolidColorBrush)resource; -// -// return new SolidColorBrush(Colors.DarkGray); -// } -// -// return null; -//}
139-169: Consider adding version check for Windows 11 features.The
SetBlurForWindowmethod should verify Windows version before using Windows 11-specific features.public void SetBlurForWindow() { + // Check for Windows 11 (build 22000 or higher) + if (!OperatingSystem.IsWindowsVersionAtLeast(10, 0, 22000)) + { + Log.Debug("Skipping blur effect - requires Windows 11"); + return; + } + var dict = GetThemeResourceDictionary(Settings.Theme); if (dict == null) return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Flow.Launcher.Core/Resource/Theme.cs(3 hunks)Flow.Launcher.Infrastructure/Win32Helper.cs(1 hunks)Flow.Launcher/MainWindow.xaml(2 hunks)Flow.Launcher/MainWindow.xaml.cs(2 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs(1 hunks)Flow.Launcher/Themes/BlurBlack.xaml(1 hunks)Flow.Launcher/Themes/Win11Light.xaml(2 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (14)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs (2)
46-68: LGTM! Good handling of shadow effects.The property correctly prevents combining drop shadow with blur effects, which could cause visual artifacts. The user feedback through message box is appropriate.
40-42:❓ Verification inconclusive
LGTM! Verify theme changes are applied correctly.
The addition of
ThemeManager.Instance.RefreshFrame()ensures window effects are updated when themes change. This is essential for the new blur and translucency features.Run this script to verify theme changes are handled correctly:
🏁 Script executed:
#!/bin/bash # Description: Check theme change handling in ThemeManager ast-grep --pattern $'class ThemeManager { $$$ public void ChangeTheme($_) { $$$ } $$$ public void RefreshFrame() { $$$ } $$$ }'Length of output: 150
LGTM! However, the automated ast-grep check didn't yield any output, so please manually verify that
ThemeManager.Instance.RefreshFrame()correctly updates the window frame after theme changes (and hence applies the new blur/translucency effects).Flow.Launcher/Themes/BlurBlack.xaml (1)
13-14: LGTM! Good implementation of blur mode.The addition of
BlurModewith "Auto" value allows for flexible blur effects while maintaining compatibility. This aligns well with the PR's objective of enhancing window translucency.Flow.Launcher/Themes/Win11Light.xaml (1)
4-4: LGTM! Consistent blur implementation.The changes properly enable blur effects for the Windows 11 light theme, maintaining consistency with other blur-enabled themes.
Also applies to: 15-16
Flow.Launcher/MainWindow.xaml.cs (2)
184-187: LGTM! Good use of async theme refresh.The asynchronous theme frame refresh with background priority is a good practice for maintaining UI responsiveness while applying theme changes.
63-63:❓ Verification inconclusive
Verify impact of removing paste handler.
The removal of the paste handler could affect how pasted content is processed in the QueryTextBox. Please ensure this doesn't impact user experience.
Run this script to check for other paste handling mechanisms:
🏁 Script executed:
#!/bin/bash # Description: Check for paste handling in the codebase rg -A 5 "OnPaste|DataObject.AddPastingHandler|Paste"Length of output: 3901
Confirm Paste Handling Remains Intentional
The code in
Flow.Launcher/MainWindow.xaml.csstill attaches the paste handler viaDataObject.AddPastingHandler(QueryTextBox, OnPaste);and the corresponding
OnPastemethod is present. Please verify that this behavior is intentional and that paste operations continue to work as expected. If modifications to the paste handling logic were expected, please update the implementation accordingly.Flow.Launcher/MainWindow.xaml (2)
18-19: LGTM! The window property changes enable DWM-based blur effects.The changes to
AllowsTransparencyandBackgroundare necessary for implementing DWM-based blur effects:
- Setting
AllowsTransparency="False"is required as DWM handles the transparency- The semi-transparent background color
#E12E2E2Eprovides a good base for the blur effect
33-33: LGTM! The window initialization changes support proper window chrome.The addition of
SourceInitializedevent handler andSingleBorderWindowstyle:
- Enables proper initialization of DWM effects
- Provides standard window chrome with proper borders
Also applies to: 37-37
Flow.Launcher.Infrastructure/Win32Helper.cs (2)
10-17: LGTM! Well-defined DWM backdrop type enumeration.The
DwmSystemBackdropTypeenum properly defines all available backdrop types for Windows 11, including Mica and Acrylic effects.
21-22: LGTM! Correct P/Invoke declaration for DWM API.The
DwmSetWindowAttributeP/Invoke declaration:
- Uses correct parameter types
- Properly imports from "dwmapi.dll"
Flow.Launcher/ViewModel/MainViewModel.cs (2)
412-417: LGTM! Improved responsiveness for empty query mode.The immediate query text clearing with
Task.Yield()ensures responsive UI updates.
419-436: LGTM! Simplified query mode handling.The switch statement for
LastQueryModehas been streamlined:
- Removed unnecessary delay
- Consolidated action keyword cases
- Cleaner state management
Flow.Launcher.Core/Resource/Theme.cs (2)
71-77: LGTM! Well-structured DWM window attribute enumeration.The
DWMWINDOWATTRIBUTEenum properly defines the required attributes for window effects.
89-104: LGTM! Clean implementation of DWM API methods.The
Methodsclass provides a clean wrapper around DWM API calls with proper parameter handling.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Flow.Launcher.Infrastructure/NativeMethods.txt (2)
19-20: Consistency Check for EnumWindows EntryThe
EnumWindowsentry is marked with a change (~) which suggests that a formatting (newline) adjustment may have been applied. Please verify that this formatting change is intentional and does not affect any downstream parsing or processing that depends on the file's structure.
21-22: Addition of Native Methods for Translucency EffectsThe new entries
DwmSetWindowAttributeandDWM_SYSTEMBACKDROP_TYPEhave been added, which are essential for enabling the blur-enabled themes on Windows 11. Ensure that these native method names exactly match their usage in the corresponding implementation files (for example, inTheme.csandWin32Helper.cs). Additionally, consider adding a brief comment or documentation reference here to indicate their role in the new translucency effects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher.Infrastructure/NativeMethods.txt(1 hunks)Flow.Launcher.Infrastructure/Win32Helper.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher.Infrastructure/Win32Helper.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
|
It looks so nice on Windows 11.👍And I improve code quality for it with PInvoke. (But it seems that you did not use |
|
I remember previously we have a lot of issue with fluent window? Things changed? |
This comment has been minimized.
This comment has been minimized.
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: 1
🔭 Outside diff range comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
1073-1204: 🛠️ Refactor suggestionConsider isolating concurrency logic in QueryResultsAsync.
The method coordinates multiple asynchronous plugin queries with cancellation tokens and progress bar management. This is correct in principle but quite involved. Splitting the gathering, rendering, and cancellation segments may reduce complexity. You might wrap the token logic in a helper or aim for more granular tasks withConfigureAwait(false).
🧹 Nitpick comments (6)
Flow.Launcher/MainWindow.xaml.cs (5)
34-59: Ensure clarity on new private fields initialization.
The addition of read-only fields for_settings,_theme,_viewModel, and other resources is consistent with dependency injection and code style. However, double-check that each field’s lifecycle aligns with the application’s lifetime. If any field might need reinitialization or disposal later, relaxing read-only or adding explicit teardown may be necessary.
85-92: Validate window procedure hook usage.
ConnectingWndProcinOnSourceInitializedis a valid approach for advanced resizing or alt-tab–hiding logic. Ensure that the hook is removed or accounted for on window re-creation or disposal if the application flow changes in the future.
250-272: Beware of potential flicker in OnDeactivated.
Usingawait Task.Delay(100)before hiding the window can introduce flicker if quickly activated-deactivated. You might prefer a more robust approach (like setting a flag to throttle frequent calls).
740-857: Consider extracting WindowAnimation logic into a separate utility or behavior.
With numerous storyboards and animations, the method is growing large. Moving animation logic into its own class or behavior could simplify maintenance and testing, and allow more customizing of transitions.
859-937: Manage race conditions for concurrent ClockPanel fade in/out.
The fade logic relies on_isClockPanelAnimatingto prevent overlap. Rapid updates toContextMenu.Visibilityor typed characters could theoretically introduce race conditions if asynchronous tasks overlap. You can keep using the_isClockPanelAnimatingflag, but you may also considerStoryboardcompletion callbacks or a specialized animation library to finalize transitions safely.Would you like assistance refining this fade logic to handle quick consecutive events more gracefully?
Flow.Launcher/ViewModel/MainViewModel.cs (1)
1269-1306: Minor readability improvement for topmost logic.
ContextMenuTopMosttoggles topmost on or off, with separateAddOrUpdateorRemovecalls. If you have more topmost states or conditions in the future, consider a single “toggleTopMost(Result)” method for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher/MainWindow.xaml.cs(9 hunks)Flow.Launcher/SettingWindow.xaml.cs(3 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(19 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/SettingWindow.xaml.cs
🔇 Additional comments (6)
Flow.Launcher/MainWindow.xaml.cs (4)
63-77: Constructor DI looks good.
Switching to IoC-based construction is typically beneficial for testability and maintainability. Good job ensuring fields_settings,_theme, and_viewModelare all injected. Please verify that exceptions are handled if the services are unavailable.
229-237: [Duplicate] Potential resource leak in OnClosing.
This reproduces a previously raised concern. Bypassing normal close withEnvironment.Exit(0)may lead to leaked resources and incomplete cleanup.
1011-1014: PreviewDragOver event looks fine.
Explicitly markinge.Handled = trueprevents dropping onto theQueryTextBox. This is consistent with disallowing invalid drag-and-drop.
94-227:✅ Verification successful
Unsubscribe or manage event handlers in OnLoaded for safety.
The code eagerly subscribes to multiple property descriptors (ContextMenu,History) and sets upQueryTextBox.TextChanged. If the window could be reloaded, consider unsubscribing or using a short-lived scope to prevent potential memory leaks or repeated event calls.Would you like to verify that OnLoaded is called only once per app lifetime? If not, unsubscribing is advisable. You can confirm by searching for subsequent calls of
InitializeComponent()or navigation events:
🏁 Script executed:
#!/bin/bash # Searching for additional calls to InitializeComponent or OnLoaded rg -A 5 "InitializeComponent|OnLoaded"Length of output: 28780
MainWindow OnLoaded is triggered only once at startup.
Our search confirms that the MainWindow’s Loaded event is set only in MainWindow.xaml (viaLoaded="OnLoaded") and that there are no subsequent triggers (e.g., from navigation or re-callingInitializeComponent()) that would cause OnLoaded to execute again. Given that the event subscriptions (such as those on_viewModel,_settings, and UI element changes) are registered only once, the current setup does not risk multiple registrations or memory leaks under normal circumstances. If you are confident that the MainWindow will not be unloaded and reloaded during the app’s lifetime, then it is safe to leave the subscriptions as-is.Flow.Launcher/ViewModel/MainViewModel.cs (2)
243-251: ReloadPluginDataAsync notification approach is acceptable.
UsingNotification.Show(...)to give feedback aligns well with a user-friendly experience. The method also properly hides the main view, reloading data, and then notifies success. No immediate concerns here.
386-394:❓ Verification inconclusive
Verify concurrency in OpenResultAsync.
The method awaits anExecuteAsync(...)call that might do I/O or plugin logic. Ensure the UI remains responsive and consider try-catch for plugin exceptions. Also verify that multiple calls toOpenResultAsyncwon't introduce race conditions if the user quickly selects multiple results.
🏁 Script executed:
#!/bin/bash # Searching for all references to OpenResultAsync to see if multiple calls are triggered quickly: rg -A 5 "OpenResultAsync"Length of output: 488
Action Required: Review Concurrency and Exception Handling in OpenResultAsync
Please verify the following in the
Flow.Launcher/ViewModel/MainViewModel.csfile (around the current asynchronous snippet):
- UI Responsiveness: The asynchronous call to
result.ExecuteAsync(...)is awaited with.ConfigureAwait(false), which prevents resuming on the UI thread. Confirm that this behavior is intentional and that any subsequent UI updates are marshaled back correctly.- Exception Handling: There is no try-catch block around the
ExecuteAsynccall. Consider wrapping the call in a try-catch to gracefully handle any I/O or plugin-related exceptions.- Race Conditions: Since
OpenResultAsynccan be triggered rapidly (as seen with the check for a non-nullindex), ensure that multiple concurrent calls do not lead to unsafe modifications of shared state (for example, updatingresults.SelectedIndexwithout proper synchronization).Please review these areas to ensure that rapid, consecutive invocations don’t introduce race conditions or unhandled exceptions that could impact the UI stability.
|
Getting the following exception in version 1.19.5.10163 when opening the Hotkeys section in the settings window: Log: 2025-03-17.txt |
Many thanks for you report! This is already fixed in #3354 and #3357. |
This comment has been minimized.
This comment has been minimized.
|
@fiso64 You can download the installer from here once the 
|
This comment has been minimized.
This comment has been minimized.
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: 0
♻️ Duplicate comments (1)
Flow.Launcher/MainWindow.xaml.cs (1)
229-237:⚠️ Potential issuePotential memory leak in application closing.
The
OnClosingmethod cancels the close event and then forcefully exits the application usingEnvironment.Exit(0). This approach bypasses normal cleanup routines, which might lead to resource leaks.private async void OnClosing(object sender, CancelEventArgs e) { _notifyIcon.Visible = false; App.API.SaveAppAllSettings(); - e.Cancel = true; await PluginManager.DisposePluginsAsync(); Notification.Uninstall(); - Environment.Exit(0); + // Allow normal closing process to continue }Alternatively, if direct application termination is required:
private async void OnClosing(object sender, CancelEventArgs e) { + // Cancel the event to handle cleanup properly + e.Cancel = true; + _notifyIcon.Visible = false; App.API.SaveAppAllSettings(); - e.Cancel = true; await PluginManager.DisposePluginsAsync(); Notification.Uninstall(); + + // Use Application.Current.Shutdown() instead of Environment.Exit + Application.Current.Shutdown(); - Environment.Exit(0); }
🧹 Nitpick comments (3)
Flow.Launcher/MainWindow.xaml.cs (3)
59-59: Translate non-English comments to English.There's a Korean comment that should be translated to English for consistency and better collaboration with international contributors.
- private bool _isClockPanelAnimating = false; // 애니메이션 실행 중인지 여부 + private bool _isClockPanelAnimating = false; // Indicates whether animation is in progress
848-852: Consider extracting condition to a well-named method.The clock visibility condition check is well-structured but could be extracted into a separate method for improved readability and maintainability.
+ private bool ShouldShowClock() + { + return QueryTextBox.Text.Length == 0 && + ContextMenu.Visibility != Visibility.Visible && + History.Visibility != Visibility.Visible; + } private void UpdateClockPanelVisibility() { // ... - bool shouldShowClock = QueryTextBox.Text.Length == 0 && - ContextMenu.Visibility != Visibility.Visible && - History.Visibility != Visibility.Visible; + bool shouldShowClock = ShouldShowClock(); // ... }
846-846: Extract magic number as a named constant.The animation duration calculation uses a magic number that should be extracted as a named constant for better maintainability.
+ private const double FADE_ANIMATION_DURATION_FACTOR = 2.0 / 3.0; private void UpdateClockPanelVisibility() { // ... - var animationDuration = TimeSpan.FromMilliseconds(animationLength * 2 / 3); + var animationDuration = TimeSpan.FromMilliseconds(animationLength * FADE_ANIMATION_DURATION_FACTOR); // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/MainWindow.xaml.cs(9 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
Flow.Launcher/MainWindow.xaml.cs (3)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1) (1)
Settings(16:391)Flow.Launcher/ViewModel/MainViewModel.cs (10) (10)
MainViewModel(30:1566)MainViewModel(56:173)Show(1370:1391)Hide(1395:1473)ResetPreview(909:926)Task(535:546)Task(1073:1204)SelectedIsFromQueryResults(1336:1340)Result(1268:1306)Result(1308:1334)Flow.Launcher/SettingWindow.xaml.cs (4) (4)
OnLoaded(34:44)WindowLeft(143:150)WindowTop(152:159)InitializePosition(110:126)
🔇 Additional comments (6)
Flow.Launcher/MainWindow.xaml.cs (6)
65-70: Nice improvement with dependency injection.The constructor has been refactored to use proper dependency injection, with fields marked as readonly. This is a good practice that improves maintainability and testability.
94-139: Improved window initialization with async handling.The
OnLoadedmethod now properly uses async/await pattern to handle the frame refresh, which will prevent UI blocking during initialization. The method also better organizes the startup sequence with clearer conditional blocks.
740-831: Improved animation properties for better visual effects.The animation configuration has been enhanced with appropriate
FillBehavior.HoldEndsettings and better initialization of animation properties. The code now correctly uses styling information to determine visual elements' properties.
833-911: Well-implemented clock panel visibility management.The new
UpdateClockPanelVisibilitymethod provides a robust way to control the clock panel's visibility based on multiple conditions. The implementation correctly uses animations for smooth transitions and properly handles edge cases.However, consider replacing the direct Dispatcher invocation with a more efficient approach:
- Application.Current.Dispatcher.Invoke(() => + Dispatcher.BeginInvoke(DispatcherPriority.Render, new Action(() => { ClockPanel.Visibility = Visibility.Visible; // Other code... ClockPanel.BeginAnimation(OpacityProperty, fadeIn); - }, DispatcherPriority.Render); + }));
913-943: Good utility methods for style property extraction.The
GetOpacityFromStyleandGetThicknessFromStylemethods are well-implemented utilities that safely extract values from styles with proper fallbacks. This prevents potential null reference issues and improves code robustness.
574-581: Method properly updated to return Task.The
PositionResetAsyncmethod now correctly returns a Task, which is a better practice for async methods than using void returns. This change improves error handling and allows callers to await the operation.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
1393-1394: 🛠️ Refactor suggestionConsider converting async void Hide() to async Task HideAsync().
Using
async voidcan obscure exceptions and hamper chaining. Changing the signature topublic async Task HideAsync()and adjusting call sites is recommended so callers can properly await or handle errors.Apply the following diff as a starting point:
-public async void Hide() +public async Task HideAsync() { lastHistoryIndex = 1; if (ExternalPreviewVisible) { CloseExternalPreview(); } // ... rest of method ... }Flow.Launcher/MainWindow.xaml.cs (1)
229-237:⚠️ Potential issuePotential memory leak in application closing.
The
OnClosingmethod cancels the close event and then forcefully exits the application usingEnvironment.Exit(0). This approach bypasses normal cleanup routines, which might lead to resource leaks.Consider allowing the normal window closing process:
private async void OnClosing(object sender, CancelEventArgs e) { _notifyIcon.Visible = false; App.API.SaveAppAllSettings(); - e.Cancel = true; await PluginManager.DisposePluginsAsync(); Notification.Uninstall(); - Environment.Exit(0); + // Allow normal closing process to continue }Or if direct application termination is required:
private async void OnClosing(object sender, CancelEventArgs e) { + // Cancel the event to handle cleanup properly + e.Cancel = true; + _notifyIcon.Visible = false; App.API.SaveAppAllSettings(); - e.Cancel = true; await PluginManager.DisposePluginsAsync(); Notification.Uninstall(); + + // Use Application.Current.Shutdown() instead of Environment.Exit + Application.Current.Shutdown(); - Environment.Exit(0); }
🧹 Nitpick comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
1459-1459: Consider explaining the purpose of the delay.The 50ms delay before updating window visibility properties isn't clearly explained. Consider adding a comment to clarify its purpose.
-await Task.Delay(50); +// Allow time for visual transitions to complete before changing window visibility +await Task.Delay(50);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher/MainWindow.xaml.cs(9 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(19 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
Flow.Launcher/MainWindow.xaml.cs (2)
Flow.Launcher/ViewModel/MainViewModel.cs (6) (6)
MainViewModel(30:1559)MainViewModel(56:173)Show(1370:1391)Hide(1395:1465)Task(535:546)Task(1073:1204)Flow.Launcher/WelcomeWindow.xaml.cs (2) (2)
WelcomeWindow(13:102)WelcomeWindow(26:31)
Flow.Launcher/ViewModel/MainViewModel.cs (2)
Flow.Launcher/Msg.xaml.cs (1) (1)
Show(63:91)Flow.Launcher/PublicAPIInstance.cs (8) (8)
Task(94:94)Task(165:165)Task(167:168)Task(170:171)Task(347:347)GetTranslation(158:158)ShowMsg(99:100)ShowMsg(102:105)
🔇 Additional comments (42)
Flow.Launcher/ViewModel/MainViewModel.cs (25)
179-180: Improved error handling and task management.The change to use
TaskScheduler.Defaultfor the continuation is a good practice that helps ensure tasks run on the thread pool, avoiding UI thread blocking.
182-182: Good refactoring to use async/await pattern.Renaming
updateActiontoUpdateActionAsyncfollows the naming convention for asynchronous methods, improving code readability and maintainability.
210-211: Ensure consistent error handling approach.This error handling code in
continueActionlogs the exception in release mode but re-schedules the same task without addressing the root cause, which could lead to repeated failures.Consider implementing a more robust error recovery mechanism like retries with exponential backoff or a circuit breaker pattern:
Log.Error($"Error happen in task dealing with viewupdate for results. {t.Exception}"); -_resultsViewUpdateTask = - Task.Run(UpdateActionAsync).ContinueWith(continueAction, CancellationToken.None, TaskContinuationOptions.OnlyOnFaulted, TaskScheduler.Default); +// Implement retry with delay or circuit breaker +_resultsViewUpdateTask = Task.Delay(TimeSpan.FromSeconds(5)) + .ContinueWith(_ => Task.Run(UpdateActionAsync) + .ContinueWith(continueAction, CancellationToken.None, + TaskContinuationOptions.OnlyOnFaulted, TaskScheduler.Default));
249-250: Consider removing the translator comments.The code now uses
App.API.GetTranslationinstead of the removed_translatorfield, making the code cleaner and more consistent.
267-273: Improved async method calls by using Task return types.The change from calling
QueryResultsdirectly to awaitingQueryResultsAsyncwith discard operator allows for better error propagation and asynchronous operation.
279-279: Consistent use of async/await pattern.The change from calling
QueryResultsdirectly to awaitingQueryResultsAsyncwith discard operator allows for better error propagation and asynchronous operation.
893-893: Method visibility adjusted appropriately.Changing
SwitchExternalPreviewto static makes sense since it doesn't use any instance members, improving code clarity.
1070-1073: Improved method signature from void to Task.Changing
QueryResultstoQueryResultsAsyncwith a Task return type allows for proper error propagation and better asynchronous operation chaining.
1093-1093: Consistent token usage.Using
_updateSource.Tokenconsistently for cancellation improves code reliability and helps prevent race conditions.
1101-1101: Consistent cancellation token checking.Using
_updateSource.Tokeninstead of a local variable makes the code more consistent and easier to maintain.
1131-1133: Improved cancellation handling.The code now properly checks for cancellation after the delay, which is important for responsive UI and resource management.
1136-1143: Enhanced progress bar visibility logic.The improvement in handling the progress bar visibility with proper cancellation token checking makes the UI more responsive.
1149-1149: Consistent async method naming.Renaming
QueryTasktoQueryTaskAsyncfollows the naming convention for asynchronous methods, improving code readability.
1163-1164: Consistent cancellation token checking.The code now properly checks for cancellation before proceeding, which is important for responsive UI and resource management.
1169-1169: Consistent cancellation token checking.Using
_updateSource.Tokenconsistently throughout the code improves reliability and prevents race conditions.
1176-1176: Consistent async method naming.Renaming
QueryTasktoQueryTaskAsyncfollows the naming convention for asynchronous methods, improving code readability.
1183-1183: Consistent token usage.Using
_updateSource.Tokenconsistently for cancellation improves code reliability and helps prevent race conditions.
1185-1185: Proper cancellation token checking.Using
_updateSource.Token.ThrowIfCancellationRequested()is the correct way to handle cancellation in asynchronous operations.
1199-1199: Consistent token usage in TryWrite call.Using
_updateSource.Tokenconsistently throughout the code improves reliability.
1371-1391: Improved window visibility management.The Show method has been enhanced with better handling of DWM window cloak and UI element visibility, providing a smoother user experience.
1409-1418: Improved LastQueryMode handling.The code now more directly handles the LastQueryMode.Empty case, with explicit UI updates and a frame delay to ensure changes take effect.
1420-1426: Simplified LastQueryMode logic.Consolidating the logic for LastQueryMode.Preserved and LastQueryMode.Selected improves code readability and maintainability.
1437-1457: Enhanced window hiding with DWM cloaking.The implementation of DWM cloaking for window visibility provides a more seamless user experience. The careful handling of UI element opacity and visibility states helps ensure smooth transitions.
1467-1468: Good use of pragma to suppress compiler warnings.The pragmas to suppress compiler warnings are appropriately scoped and only applied where necessary.
1475-1475: Improved GameMode integration.The code now properly considers GameModeStatus for hotkey ignoring, which enhances the user experience.
Flow.Launcher/MainWindow.xaml.cs (17)
35-37: Good use of dependency injection.Converting to readonly fields and using dependency injection improves code maintainability and testability.
38-44: Improved code organization with clear comments.The addition of comments that group related fields enhances code readability and maintainability.
67-69: Robust dependency injection implementation.Using
Ioc.Default.GetRequiredServiceensures that required dependencies are properly injected and will throw appropriate exceptions if missing.
85-91: Proper window initialization.The
OnSourceInitializedhandler correctly sets up window hooks and applies necessary window styles.
94-102: Improved first launch experience.The code now properly handles the first launch scenario by setting
_settings.FirstLaunchto false and displaying the welcome window.
105-114: Enhanced startup behavior.The code now consistently handles window visibility based on the
HideOnStartupsetting, providing a better user experience.
135-136: Async theme refreshing.Using
awaitwith the asyncRefreshFrameAsyncmethod ensures that the operation completes before continuing, preventing race conditions.
150-173: Improved MainWindow visibility handling.The code now properly manages the window's visual state including sound effects, position updates, and animations, enhancing the user experience.
215-226: Enhanced UI element visibility coordination.Adding event handlers for TextChanged, ContextMenu visibility, and History visibility ensures consistent UI updates.
250-272: Improved window deactivation handling.The code now properly saves window position and handles animations with appropriate delays, preventing UI flicker.
371-382: Good use of pragma directives.The pragmas to suppress compiler warnings are appropriately scoped and only applied where necessary.
574-574: Improved method signature.Changing
PositionResettoPositionResetAsyncwith Task return type follows async naming conventions and allows for proper error handling.
740-831: Enhanced window animation.The animation code now properly calculates opacity and margin values from styles, making the animations more consistent with the theme.
833-911: Well-implemented ClockPanel visibility management.The
UpdateClockPanelVisibilitymethod implements smooth fade-in and fade-out animations based on application state, providing a better user experience.
914-928: Useful utility method for style properties.The
GetOpacityFromStylemethod cleanly extracts opacity values from styles, improving code reusability and reducing duplication.
930-944: Useful utility method for thickness properties.The
GetThicknessFromStylemethod cleanly extracts margin values from styles, improving code reusability and reducing duplication.
986-989: Proper drag handling.Setting
e.Handled = truein the drag event ensures that the textbox properly handles drag operations.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. Forbidden patterns 🙅 (2)In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves. These forbidden patterns matched content: s.b. workaround(s)Reject duplicate wordsIf the flagged items are 🤯 false positivesIf items relate to a ...
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Flow.Launcher/MainWindow.xaml.cs (1)
230-238:⚠️ Potential issuePotential memory leak in application closing.
The
OnClosingmethod cancels the close event and then forcefully exits the application usingEnvironment.Exit(0). This approach bypasses normal cleanup routines, which might lead to resource leaks.private async void OnClosing(object sender, CancelEventArgs e) { _notifyIcon.Visible = false; App.API.SaveAppAllSettings(); - e.Cancel = true; await PluginManager.DisposePluginsAsync(); Notification.Uninstall(); - Environment.Exit(0); + // Allow normal closing process to continue }Alternatively, if direct application termination is required:
private async void OnClosing(object sender, CancelEventArgs e) { + // Cancel the event to handle cleanup properly + e.Cancel = true; + _notifyIcon.Visible = false; App.API.SaveAppAllSettings(); - e.Cancel = true; await PluginManager.DisposePluginsAsync(); Notification.Uninstall(); + + // Use Application.Current.Shutdown() instead of Environment.Exit + Application.Current.Shutdown(); - Environment.Exit(0); }
🧹 Nitpick comments (6)
Flow.Launcher.Infrastructure/Win32Helper.cs (3)
65-87: Complete XML documentation needed for DWMSetCornerPreferenceForWindow.The method has incomplete XML documentation. While the parameter descriptions are present, a method description is missing.
/// <summary> -/// +/// Sets the corner preference (roundness) for the specified window. /// </summary> /// <param name="window"></param> /// <param name="cornerType">DoNotRound, Round, RoundSmall, Default</param> /// <returns></returns>
281-296: Robust DPI conversion with fallback handling.The method properly handles the case when no presentation source is available by creating a temporary HwndSource. Consider adding a comment explaining the fallback case.
else { + // Create a temporary source when no presentation source is available using var src = new HwndSource(new HwndSourceParameters()); matrix = src.CompositionTarget.TransformFromDevice; }
302-304: Consider using constants for all window message codes.The code defines constants for WM_ENTERSIZEMOVE and WM_EXITSIZEMOVE but not for other window messages that might be used elsewhere. Consider a more systematic approach to defining message constants.
Consider creating a separate internal class or enum for window messages to centralize these definitions:
public static class WindowMessages { public const int WM_ENTERSIZEMOVE = (int)PInvoke.WM_ENTERSIZEMOVE; public const int WM_EXITSIZEMOVE = (int)PInvoke.WM_EXITSIZEMOVE; // Add other window messages as needed }This would make it easier to maintain and extend window message handling.
Flow.Launcher/MainWindow.xaml.cs (3)
59-59: Translate comment to English for code consistency.There's a Korean comment that should be translated to English for better maintainability across international contributors.
- private bool _isClockPanelAnimating = false; // 애니메이션 실행 중인지 여부 + private bool _isClockPanelAnimating = false; // Whether animation is currently running
402-407: Consider using constants for shadow margin values.The hardcoded shadow margin value (32) should be defined as a constant at the class level for better maintainability.
+ private const int ShadowMargin = 32; ... - shadowMargin = 32; + shadowMargin = ShadowMargin;
834-912: Well-structured clock panel visibility management with smooth animations.The new
UpdateClockPanelVisibilitymethod effectively manages the clock panel's visibility state with fade animations. The code correctly handles various conditions and edge cases.However, the booleans for visibility checks would benefit from better naming.
- bool shouldShowClock = QueryTextBox.Text.Length == 0 && - ContextMenu.Visibility != Visibility.Visible && - History.Visibility != Visibility.Visible; + bool isQueryEmpty = QueryTextBox.Text.Length == 0; + bool isContextMenuHidden = ContextMenu.Visibility != Visibility.Visible; + bool isHistoryHidden = History.Visibility != Visibility.Visible; + bool shouldShowClock = isQueryEmpty && isContextMenuHidden && isHistoryHidden;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Infrastructure/Win32Helper.cs(1 hunks)Flow.Launcher/MainWindow.xaml.cs(9 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(22 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/ViewModel/MainViewModel.cs
🧰 Additional context used
🧬 Code Definitions (1)
Flow.Launcher/MainWindow.xaml.cs (4)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1) (1)
Settings(16-391)Flow.Launcher/ViewModel/MainViewModel.cs (7) (7)
MainViewModel(30-1545)MainViewModel(56-173)Show(1356-1377)Hide(1381-1452)ResetPreview(895-912)Result(1254-1292)Result(1294-1320)Flow.Launcher/SettingWindow.xaml.cs (4) (4)
OnLoaded(34-44)WindowLeft(143-150)WindowTop(152-159)InitializePosition(110-126)Flow.Launcher/App.xaml.cs (2) (2)
App(28-253)App(35-88)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Report (PR)
🔇 Additional comments (24)
Flow.Launcher.Infrastructure/Win32Helper.cs (12)
19-24: Good implementation of OS version checking.The method properly checks both the OS platform and build number to ensure compatibility with Windows 11 (build 22000+) for backdrop effects.
26-35: Effective use of PInvoke for window cloaking.The implementation correctly uses unsafe code and marshal sizing for proper interop. The boolean parameter provides a clean API for toggling the cloak state.
37-52: Good implementation of backdrop management.The method effectively converts the application-specific backdrop types to the appropriate DWM constants with a clean switch expression.
54-63: Proper implementation of dark mode toggle.The implementation correctly converts the boolean parameter to the integer value expected by the Windows API.
93-102: Efficient implementation of wallpaper path retrieval.The method efficiently uses stack allocation with
stackallocandMemoryMarshalto avoid unnecessary heap allocations while retrieving the wallpaper path.
108-121: Good window foreground management functions.The overloaded methods provide flexibility for both Window objects and raw handles, which will improve code reusability.
131-141: Effective implementation of Alt+Tab hiding.The method correctly manipulates window style flags to hide the window from Alt+Tab without affecting its normal visibility.
147-157: Proper implementation of Alt+Tab showing.The implementation correctly removes the TOOLWINDOW style and adds the APPWINDOW style to ensure the window appears in Alt+Tab.
163-172: Good implementation of control box disabling.The implementation properly modifies the window style to remove the system menu while keeping other style elements intact.
174-195: Improved error handling in window style methods.Both methods properly check for PInvoke errors and throw exceptions with detailed error information when necessary.
201-268: Comprehensive fullscreen detection logic.The implementation handles various edge cases for different window classes and performs proper bounds checking against monitor information. The code is well-structured with early returns for quick resolution of common cases.
309-317: Good implementation of window handle retrieval.The method provides flexibility with the
ensureparameter to optionally create a handle if one doesn't exist.Flow.Launcher/MainWindow.xaml.cs (12)
34-43: Good improvement for code organization and dependency injection patterns.The code has been restructured to use dependency injection and clearly organize fields by category. Making fields like
_settings,_theme, and_viewModelreadonly is a good practice that prevents accidental modification.
65-77: Good use of dependency injection for better testability and maintainability.The constructor now properly uses dependency injection to obtain services, which is a significant improvement over direct instantiation. This approach makes the class more testable and decouples it from concrete implementations.
94-142: Async window loading with proper initialization sequence.The OnLoaded event handler has been properly converted to async to support awaiting the theme refresh operation. The initialization sequence is well-structured, with clear comments indicating each step.
216-227: Good implementation of dynamic UI element visibility management.Adding event handlers to monitor text changes and control visibility provides a more responsive user experience. The use of DependencyPropertyDescriptor for tracking visibility changes is an elegant approach.
251-273: Improved window deactivation handling with animation delay.Adding a delay before hiding the window when deactivated prevents flickering and provides a smoother user experience. Using
Task.Delaywith an async method is the correct approach for this scenario.
575-582: Proper async method signature for PositionReset.Changing
PositionResetfromasync voidtoasync Taskis a good practice. This allows callers to await the completion of the method and handle exceptions properly.
915-929: Good utility method for extracting style properties.The
GetOpacityFromStylemethod is a clean implementation that safely extracts style properties with fallback values.
931-945: Good utility method for extracting thickness from styles.The
GetThicknessFromStylemethod follows the same pattern asGetOpacityFromStyle, providing a consistent approach to style property extraction.
783-795: Good use of new style property extraction methods.The code now properly extracts opacity and thickness values from styles, making it more robust when dealing with different themes.
741-832: Enhanced window animation with proper property values.The WindowAnimation method now properly manages animation properties including FillBehavior settings. Using the new utility methods to extract style properties makes the code more resilient to theme changes.
987-991: Prevent default drag behavior for better user experience.Adding the PreviewDragOver handler ensures that the QueryTextBox handles drag operations consistently with the application's expected behavior.
694-739:❓ Verification inconclusive
Verify the animation completion event handlers for potential memory leaks.
The InitProgressbarAnimation method creates animations and wires up event handlers but doesn't handle cleanup. This could potentially lead to memory leaks if the window is recreated multiple times without removing old event handlers.
🏁 Script executed:
#!/bin/bash # Look for other event handler cleanup in the codebase rg -A 5 -B 5 "\.Completed \+=" --type csLength of output: 3158
Action: Confirm Proper Cleanup of ProgressBar Animation Resources
After reviewing the code, no explicit
Completedevent handlers are wired up in theInitProgressbarAnimationmethod. Instead, the animation is set up using style triggers with aBeginStoryboard/StopStoryboardmechanism, and the animations are frozen withda.Freeze()andda1.Freeze(). This pattern is in line with similar animations elsewhere in the code (e.g., fade-in/fade-out animations in other parts of the file) where explicit cleanup isn’t needed.However, please verify that the storyboard’s registered name (i.e.,
"ProgressBarAnimation", registered viaprogressStyle.RegisterName) is either unregistered or managed appropriately when the ProgressBar control or its container is disposed. This precaution will ensure that if the window is recreated multiple times, no stale references remain causing memory retention.
What's the PR
Details
Added properties for handling background colors and borders. Removed the existing windowborderstyle-related properties to make them obsolete.Blur theme can't use Corner radius.Todo List
Sysplugin theme change codes.Test Cases