Fix issues related to search window maximization and Snap behavior #3606
Conversation
prlabeler
bot
added
bug
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: Jack251970 Jack251970, onesounds have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 Walkthrough""" WalkthroughThe changes refine window behavior by setting the window border corner radius to zero when blur effects are applied, and by enhancing mouse and window message handling in the main window to correctly manage resizing, maximizing, and double-click interactions, preventing window height from getting stuck. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainWindow
User->>MainWindow: MouseDown (left button)
alt Window is maximized
MainWindow->>MainWindow: Calculate mouse position ratios relative to window
MainWindow->>MainWindow: Restore window to normal state
MainWindow->>MainWindow: Adjust window position to maintain cursor relative location
MainWindow->>MainWindow: Initiate DragMove if mouse still pressed
else Window is not maximized
MainWindow->>MainWindow: DragMove immediately
end
User->>MainWindow: Window messages (WM_ENTERSIZEMOVE, WM_EXITSIZEMOVE, WM_NCLBUTTONDBLCLK, WM_SYSCOMMAND)
MainWindow->>MainWindow: Update window size, layout, and handle maximize/minimize commands accordingly
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
Flow.Launcher/MainWindow.xaml.cs (1)
538-616: 🛠️ Refactor suggestionTranslate Korean comments and improve code clarity.
The window message handling is well-implemented, but Korean comments should be translated.
else if (msg == Win32Helper.WM_EXITSIZEMOVE) { - //Prevent updating the number of results when the window height is below the height of a single result item. - //This situation occurs not only when the user manually resizes the window, but also when the window is released from a side snap, as the OS automatically adjusts the window height. - //(Without this check, releasing from a snap can cause the window height to hit the minimum, resulting in only 2 results being shown.) + // Prevent updating the number of results when the window height is below the height of a single result item. + // This situation occurs not only when the user manually resizes the window, but also when the window is released from a side snap, + // as the OS automatically adjusts the window height. Without this check, releasing from a snap can cause the window height + // to hit the minimum, resulting in only 2 results being shown. if (_initialHeight != (int)Height && Height > (_settings.WindowHeightSize + _settings.ItemHeightSize)) {Also consider extracting the magic number:
+private const int WM_COMMAND_MASK = 0xFFF0; + else if (msg == WM_SYSCOMMAND) { - int command = wParam.ToInt32() & 0xFFF0; + int command = wParam.ToInt32() & WM_COMMAND_MASK;
🧹 Nitpick comments (1)
Flow.Launcher.Core/Resource/Theme.cs (1)
674-682: Replace Korean comments with English for consistency.The implementation logic is correct, but the comments should be in English to maintain consistency with the rest of the codebase.
-// For themes with blur enabled, the window border is rendered by the system, so it's treated as a simple rectangle regardless of thickness. -//(This is to avoid issues when the window is forcibly changed to a rectangular shape during snap scenarios.) +// For themes with blur enabled, the window border is rendered by the system, so it's treated as a simple rectangle. +// This prevents shape distortion issues when the window is snapped to screen edges.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Core/Resource/Theme.cs(1 hunks)Flow.Launcher.Infrastructure/Win32Helper.cs(1 hunks)Flow.Launcher/MainWindow.xaml.cs(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: Check Spelling
Flow.Launcher/MainWindow.xaml.cs
[warning] 83-83: Ioc is not a recognized word. (unrecognized-spelling)
[warning] 84-84: Ioc is not a recognized word. (unrecognized-spelling)
[warning] 460-460: VSTHRD is not a recognized word. (unrecognized-spelling)
[warning] 520-520: VSTHRD is not a recognized word. (unrecognized-spelling)
[warning] 532-532: VSTHRD is not a recognized word. (unrecognized-spelling)
[warning] 634-634: WMP is not a recognized word. (unrecognized-spelling)
[warning] 636-636: WMP is not a recognized word. (unrecognized-spelling)
[warning] 637-637: WMP is not a recognized word. (unrecognized-spelling)
[warning] 638-638: WMP is not a recognized word. (unrecognized-spelling)
[warning] 718-718: gamemode is not a recognized word. (unrecognized-spelling)
[warning] 719-719: gamemode is not a recognized word. (unrecognized-spelling)
[warning] 722-722: gamemode is not a recognized word. (unrecognized-spelling)
[warning] 725-725: positionreset is not a recognized word. (unrecognized-spelling)
[warning] 728-728: positionreset is not a recognized word. (unrecognized-spelling)
[warning] 744-744: gamemode is not a recognized word. (unrecognized-spelling)
[warning] 745-745: positionreset is not a recognized word. (unrecognized-spelling)
[warning] 750-750: positionreset is not a recognized word. (unrecognized-spelling)
[warning] 860-860: XRatio is not a recognized word. (unrecognized-spelling)
[warning] 861-861: YRatio is not a recognized word. (unrecognized-spelling)
[warning] 1075-1075: clocksb is not a recognized word. (unrecognized-spelling)
[warning] 1076-1076: clocksb is not a recognized word. (unrecognized-spelling)
[warning] 1077-1077: iconsb is not a recognized word. (unrecognized-spelling)
[warning] 1078-1078: iconsb is not a recognized word. (unrecognized-spelling)
[warning] 1083-1083: clocksb is not a recognized word. (unrecognized-spelling)
[warning] 1084-1084: iconsb is not a recognized word. (unrecognized-spelling)
Flow.Launcher.Infrastructure/Win32Helper.cs
[warning] 43-43: Dwm is not a recognized word. (unrecognized-spelling)
[warning] 45-45: DWMWA is not a recognized word. (unrecognized-spelling)
[warning] 45-45: DWMWINDOWATTRIBUTE is not a recognized word. (unrecognized-spelling)
[warning] 55-55: DWMSBT is not a recognized word. (unrecognized-spelling)
[warning] 55-55: SYSTEMBACKDROP is not a recognized word. (unrecognized-spelling)
[warning] 56-56: DWMSBT is not a recognized word. (unrecognized-spelling)
[warning] 56-56: SYSTEMBACKDROP is not a recognized word. (unrecognized-spelling)
[warning] 57-57: DWMSBT is not a recognized word. (unrecognized-spelling)
[warning] 57-57: SYSTEMBACKDROP is not a recognized word. (unrecognized-spelling)
[warning] 60-60: Dwm is not a recognized word. (unrecognized-spelling)
[warning] 60-60: PInvoke is not a recognized word. (unrecognized-spelling)
[warning] 62-62: DWMWA is not a recognized word. (unrecognized-spelling)
[warning] 62-62: DWMWINDOWATTRIBUTE is not a recognized word. (unrecognized-spelling)
[warning] 62-62: SYSTEMBACKDROP is not a recognized word. (unrecognized-spelling)
[warning] 71-71: Dwm is not a recognized word. (unrecognized-spelling)
[warning] 71-71: PInvoke is not a recognized word. (unrecognized-spelling)
[warning] 73-73: DWMWA is not a recognized word. (unrecognized-spelling)
[warning] 73-73: DWMWINDOWATTRIBUTE is not a recognized word. (unrecognized-spelling)
[warning] 89-89: DWMWCP is not a recognized word. (unrecognized-spelling)
[warning] 90-90: DWMWCP is not a recognized word. (unrecognized-spelling)
[warning] 91-91: DWMWCP is not a recognized word. (unrecognized-spelling)
[warning] 95-95: Dwm is not a recognized word. (unrecognized-spelling)
[warning] 95-95: PInvoke is not a recognized word. (unrecognized-spelling)
[warning] 97-97: DWMWA is not a recognized word. (unrecognized-spelling)
[warning] 97-97: DWMWINDOWATTRIBUTE is not a recognized word. (unrecognized-spelling)
[warning] 108-108: PInvoke is not a recognized word. (unrecognized-spelling)
[warning] 163-163: GWL is not a recognized word. (unrecognized-spelling)
[warning] 181-181: GWL is not a recognized word. (unrecognized-spelling)
[warning] 186-186: GWL is not a recognized word. (unrecognized-spelling)
[warning] 197-197: GWL is not a recognized word. (unrecognized-spelling)
[warning] 204-204: Wnd is not a recognized word. (unrecognized-spelling)
[warning] 206-206: Wnd is not a recognized word. (unrecognized-spelling)
[warning] 214-214: Wnd is not a recognized word. (unrecognized-spelling)
[warning] 218-218: Wnd is not a recognized word. (unrecognized-spelling)
[warning] 271-271: WINTAB is not a recognized word. (unrecognized-spelling)
[warning] 284-284: Progman is not a recognized word. (unrecognized-spelling)
[warning] 285-285: WORKERW is not a recognized word. (unrecognized-spelling)
[warning] 525-525: hkl is not a recognized word. (unrecognized-spelling)
[warning] 531-531: hkl is not a recognized word. (unrecognized-spelling)
[warning] 557-557: nqo is not a recognized word. (unrecognized-spelling)
[warning] 630-630: tsf is not a recognized word. (unrecognized-spelling)
[warning] 653-653: tsf is not a recognized word. (unrecognized-spelling)
[warning] 653-653: tsf is not a recognized word. (unrecognized-spelling)
[warning] 654-654: Tsf is not a recognized word. (unrecognized-spelling)
[warning] 690-690: Noto is not a recognized word. (unrecognized-spelling)
[warning] 691-691: Noto is not a recognized word. (unrecognized-spelling)
[warning] 692-692: Noto is not a recognized word. (unrecognized-spelling)
[warning] 693-693: Noto is not a recognized word. (unrecognized-spelling)
[warning] 734-734: noto is not a recognized word. (unrecognized-spelling)
[warning] 737-737: noto is not a recognized word. (unrecognized-spelling)
[warning] 739-739: noto is not a recognized word. (unrecognized-spelling)
[warning] 759-759: noto is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| SizeToContent = SizeToContent.Height; | ||
| handled = true; | ||
| } | ||
| else if (msg == WM_SYSCOMMAND) |
There was a problem hiding this comment.
Block Maximize/Minimize by Win+Up and Win+Down Arrow
| @@ -541,7 +598,20 @@ private IntPtr WndProc(IntPtr hwnd, int msg, IntPtr wParam, IntPtr lParam, ref b | |||
|
|
|||
| handled = true; | |||
| } | |||
|
|
|||
| if (msg == WM_NCLBUTTONDBLCLK) | |||
There was a problem hiding this comment.
Block the double click in frame
| @@ -465,7 +465,52 @@ private void OnPreviewMouseMove(object sender, MouseEventArgs e) | |||
|
|
|||
| private void OnMouseDown(object sender, MouseButtonEventArgs e) | |||
| { | |||
| if (e.ChangedButton == MouseButton.Left) DragMove(); | |||
| if (e.ChangedButton == MouseButton.Left) | |||
There was a problem hiding this comment.
When the window is maximized via Snap, dragging attempts will first switch the window from Maximized to Normal state, and adjust the drag position accordingly.
| @@ -671,7 +671,15 @@ private void SetBlurForWindow(string theme, BackdropTypes backdropType) | |||
| windowBorderStyle.Setters.Remove(windowBorderStyle.Setters.OfType<Setter>().FirstOrDefault(x => x.Property.Name == "Background")); | |||
| windowBorderStyle.Setters.Add(new Setter(Border.BackgroundProperty, new SolidColorBrush(Colors.Transparent))); | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Eliminated the issue where internal border values remained visible when the window border was forcibly set to a rectangle via Snap. This behavior only applies when blur is enabled; non-blur themes are unaffected.
|
@coderabbitai Review again please, lady. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@onesounds, I'll provide a fresh comprehensive review of your window management fixes. 🏁 Script executed: Length of output: 91477 Excellent work on this comprehensive window management fix! I can see you've put significant thought into handling the complex edge cases around window snapping and maximization. Here's my detailed review: 🎯 Strengths of the Implementation1. Theme.cs - Corner Radius Fix// For themes with blur enabled, the window border is rendered by the system, so it's treated as a simple rectangle regardless of thickness.
//(This is to avoid issues when the window is forcibly changed to a rectangular shape during snap scenarios.)
var cornerRadiusSetter = windowBorderStyle.Setters.OfType<Setter>().FirstOrDefault(x => x.Property == Border.CornerRadiusProperty);
if (cornerRadiusSetter != null)
cornerRadiusSetter.Value = new CornerRadius(0);
else
windowBorderStyle.Setters.Add(new Setter(Border.CornerRadiusProperty, new CornerRadius(0)));✅ Smart approach: Setting corner radius to 0 for blur themes prevents visual artifacts when Windows forces rectangular shapes during snapping. ✅ Proper setter handling: The code correctly checks for existing setters before adding new ones. 2. MainWindow.xaml.cs - Enhanced Mouse Handlingif (WindowState == WindowState.Maximized)
{
// Calculate ratio based on maximized window dimensions
double maxWidth = this.ActualWidth;
double maxHeight = this.ActualHeight;
var mousePos = e.GetPosition(this);
double xRatio = mousePos.X / maxWidth;
double yRatio = mousePos.Y / maxHeight;
// Current monitor information
var screen = Screen.FromHandle(new WindowInteropHelper(this).Handle);
var workingArea = screen.WorkingArea;
var screenLeftTop = Win32Helper.TransformPixelsToDIP(this, workingArea.X, workingArea.Y);
// Switch to Normal state
WindowState = WindowState.Normal;✅ Excellent ratio calculation: Maintaining relative mouse position when dragging from maximized state creates a natural user experience. ✅ Multi-monitor awareness: Properly handles different monitor configurations with DPI scaling. 3. Window Message Handling//Prevent updating the number of results when the window height is below the height of a single result item.
//This situation occurs not only when the user manually resizes the window, but also when the window is released from a side snap, as the OS automatically adjusts the window height.
//(Without this check, releasing from a snap can cause the window height to hit the minimum, resulting in only 2 results being shown.)
if (_initialHeight != (int)Height && Height> (_settings.WindowHeightSize + _settings.ItemHeightSize))✅ Thoughtful safeguard: The height threshold check prevents unwanted result count recalculations during snap operations. ✅ Clear documentation: Excellent comments explaining the edge case this addresses. 🔍 Potential Improvements & Considerations1. Error Handling Enhancementtry
{
// ... existing drag logic
}
catch (InvalidOperationException)
{
// Ignored - can occur if drag operation is already in progress
}Consider adding logging for debugging purposes: catch (InvalidOperationException ex)
{
// Log for debugging but don't show to user as this is expected in some scenarios
_api.LogDebug(ClassName, $"Drag operation exception (expected): {ex.Message}");
}2. Magic Number ConstantsThe hardcoded values could be extracted to constants: private const int SHADOW_MARGIN = 32;
private const int WINDOW_BORDER_ADJUSTMENT = 14;3. Async Pattern ConsiderationThe Dispatcher.BeginInvoke(new Action(() =>
{
// ... position calculation
}), DispatcherPriority.ApplicationIdle);Consider using 🏗️ Architecture Assessment✅ Separation of Concerns: Theme-related changes in Theme.cs, window behavior in MainWindow.xaml.cs - clean separation. ✅ Defensive Programming: Multiple safeguards against edge cases (height thresholds, exception handling, etc.). ✅ Performance Conscious: Only updates when necessary (height/width change detection). 🎯 Addresses Original IssuesBased on the PR objectives, this implementation successfully addresses:
📝 Minor Suggestions
Overall, this is a well-engineered solution that demonstrates deep understanding of Windows window management complexities. The fix is comprehensive and should resolve the reported issues effectively! 🎉 |
@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 🙅 (1)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)If the flagged items are 🤯 false positivesIf items relate to a ...
|
|
Note: When the fixed size option is enabled, the snap feature doesn't work at all. Might be worth trying to enable it only while dragging. |
jjw24
changed the title
What's the PR
Details
Issue #2806 was resolved by preventing double-clicking on the window border. But when using the Snap feature with Win+Arrow keys (automatically resizing the window by dragging it to the top or sides of the monitor), several issues were identified with Flow’s behavior. The explanation is a bit complicated, but trying out various Snap scenarios easily reveals many problematic cases. No one will probably read all of this, but here’s a rough breakdown anyway:
Issue: When snapping, the window border is forcibly turned into a rectangle. This results in visible rounded corners and shadows being removed. The behavior differs between Blur and non-Blur themes.
Solution: Adjusted the border rendering logic to display correctly. This was not something that could be handled by application code alone. The window still appears as a rectangle, but we've ensured there are no remaining rounded corners or visual artifacts. Since becoming a rectangle is standard Windows behavior, we are keeping this behavior as is.
Issue: Pressing Win+Down Arrow minimizes the window, but there is no way to restore it except with Win+Up Arrow.
Solution: Prevented the window from being minimized.
Issue: While normal maximizing is blocked, OS-level forced maximization via Snap cannot be blocked.
Solution: The only way to exit this state is by dragging the title bar, but in Flow, the title area is hidden, making it non-intuitive for users. To address this, dragging from the title area while maximized now automatically restores the window to normal and allows a smooth drag transition.
Issue: When a window is unsnapped from left/right Snap, the OS automatically reduces its height. Flow interpreted this as a SizeChanged event and responded by updating the result count, often reducing it to 2.
Solution: If the window height shrinks to below the size needed to show even a single result, the result count is now preserved and not recalculated.
Test Cases
ETC
Since the maximized state can now be exited more naturally (though it might feel slightly off if the result window is already visible, it's not too distracting), allowing the WIN+Up shortcut seems acceptable. However, just in case, I’ve decided to block all maximize behaviors except those triggered by Snap.
I remember it was possible to adjust the slightly oversized window that appears when maximized, but I didn’t bother implementing that due to the hassle.
There doesn’t seem to be a reliable way to detect snap events. I initially considered estimating snap status by calculating window size, but dropped that idea because Windows 11 supports various snap layouts (such as 1:1:1 splits instead of just halves), and there may also be interactions with tools like PowerToys FancyZones.