Fix Vertical Window Positioning with Multiple Monitors #3537
Conversation
Co-authored-by: onesounds <[email protected]>
@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 ...
|
|
🥷 Code experts: onesounds Jack251970, onesounds have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: 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. |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses multi-monitor window positioning issues by improving how window positions are calculated and adjusted.
- Refactored window position assignment in SettingWindow.xaml.cs using new helper methods.
- Enhanced resolution and DPI change detection in MainWindow.xaml.cs to adjust window placement dynamically.
- Updated Settings.cs to store previous screen dimensions and DPI data needed for accurate repositioning.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Flow.Launcher/SettingWindow.xaml.cs | Introduces SetWindowPosition and AdjustWindowPosition methods to clamp window bounds. |
| Flow.Launcher/MainWindow.xaml.cs | Implements resolution change detection and DPI adjustments, and refactors custom alignment logic. |
| Flow.Launcher.Infrastructure/UserSettings/Settings.cs | Adds properties to track previous screen width, height, and DPI values. |
📝 WalkthroughWalkthroughThis update introduces DPI and screen resolution awareness to window positioning logic. New properties are added to store previous screen dimensions and DPI. Main and settings windows now adjust their positions when display settings change, ensuring windows remain visible and correctly placed on multi-monitor setups with varying resolutions and DPI. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainWindow
participant Settings
User->>MainWindow: Launch application
MainWindow->>Settings: Retrieve previous screen size and DPI
MainWindow->>MainWindow: Get current screen size and DPI
MainWindow->>MainWindow: Compare previous and current values
alt Screen/DPI changed
MainWindow->>MainWindow: AdjustPositionForResolutionChange()
MainWindow->>MainWindow: Scale and clamp window position
else No change
MainWindow->>MainWindow: Set position from saved settings
end
MainWindow->>User: Show window at correct position
Assessment against linked issues
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ 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: 3
🧹 Nitpick comments (1)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
296-299: Consider annotating new fields or initializing with explicit defaultsThe four new properties will serialize automatically, but:
- Marking them with
[JsonInclude](or[JsonPropertyName]) makes intent explicit.- Initializing to
SystemParameters.VirtualScreenWidth/Height(or96for DPI) would avoid first-run “zero” values that can cause divide-by-zero or scaling to(0,0)before the first successful save.Not critical, but helps robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs(1 hunks)Flow.Launcher/MainWindow.xaml.cs(3 hunks)Flow.Launcher/SettingWindow.xaml.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
| private void SetWindowPosition(double top, double left) | ||
| { | ||
| // Ensure window does not exceed screen boundaries | ||
| top = Math.Max(top, SystemParameters.VirtualScreenTop); | ||
| left = Math.Max(left, SystemParameters.VirtualScreenLeft); | ||
| top = Math.Min(top, SystemParameters.VirtualScreenHeight - ActualHeight); | ||
| left = Math.Min(left, SystemParameters.VirtualScreenWidth - ActualWidth); | ||
|
|
||
| Top = top; | ||
| Left = left; | ||
| } | ||
|
|
||
| private void AdjustWindowPosition(ref double top, ref double left) | ||
| { | ||
| // Adjust window position if it exceeds screen boundaries | ||
| top = Math.Max(top, SystemParameters.VirtualScreenTop); | ||
| left = Math.Max(left, SystemParameters.VirtualScreenLeft); | ||
| top = Math.Min(top, SystemParameters.VirtualScreenHeight - ActualHeight); | ||
| left = Math.Min(left, SystemParameters.VirtualScreenWidth - ActualWidth); | ||
| } |
There was a problem hiding this comment.
Clamp calculations use width/height instead of absolute screen bounds – may push window off-screen
SystemParameters.VirtualScreenHeight/Width return the size of the virtual desktop, not the bottom/right coordinates.
To keep the window fully on screen you must add the origin (VirtualScreenTop/Left) when computing the maximum allowed Top/Left.
Current logic allows the window to slip below / to the right of the virtual desktop when monitors start at a negative coordinate (very common with mixed-resolution or vertical layouts).
-top = Math.Min(top, SystemParameters.VirtualScreenHeight - ActualHeight);
-left = Math.Min(left, SystemParameters.VirtualScreenWidth - ActualWidth);
+var maxTop = SystemParameters.VirtualScreenTop + SystemParameters.VirtualScreenHeight - ActualHeight;
+var maxLeft = SystemParameters.VirtualScreenLeft + SystemParameters.VirtualScreenWidth - ActualWidth;
+
+top = Math.Min(top, maxTop);
+left = Math.Min(left, maxLeft);Apply the same fix in AdjustWindowPosition.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private void SetWindowPosition(double top, double left) | |
| { | |
| // Ensure window does not exceed screen boundaries | |
| top = Math.Max(top, SystemParameters.VirtualScreenTop); | |
| left = Math.Max(left, SystemParameters.VirtualScreenLeft); | |
| top = Math.Min(top, SystemParameters.VirtualScreenHeight - ActualHeight); | |
| left = Math.Min(left, SystemParameters.VirtualScreenWidth - ActualWidth); | |
| Top = top; | |
| Left = left; | |
| } | |
| private void AdjustWindowPosition(ref double top, ref double left) | |
| { | |
| // Adjust window position if it exceeds screen boundaries | |
| top = Math.Max(top, SystemParameters.VirtualScreenTop); | |
| left = Math.Max(left, SystemParameters.VirtualScreenLeft); | |
| top = Math.Min(top, SystemParameters.VirtualScreenHeight - ActualHeight); | |
| left = Math.Min(left, SystemParameters.VirtualScreenWidth - ActualWidth); | |
| } | |
| private void SetWindowPosition(double top, double left) | |
| { | |
| // Ensure window does not exceed screen boundaries | |
| top = Math.Max(top, SystemParameters.VirtualScreenTop); | |
| left = Math.Max(left, SystemParameters.VirtualScreenLeft); | |
| var maxTop = SystemParameters.VirtualScreenTop + SystemParameters.VirtualScreenHeight - ActualHeight; | |
| var maxLeft = SystemParameters.VirtualScreenLeft + SystemParameters.VirtualScreenWidth - ActualWidth; | |
| top = Math.Min(top, maxTop); | |
| left = Math.Min(left, maxLeft); | |
| Top = top; | |
| Left = left; | |
| } | |
| private void AdjustWindowPosition(ref double top, ref double left) | |
| { | |
| // Adjust window position if it exceeds screen boundaries | |
| top = Math.Max(top, SystemParameters.VirtualScreenTop); | |
| left = Math.Max(left, SystemParameters.VirtualScreenLeft); | |
| var maxTop = SystemParameters.VirtualScreenTop + SystemParameters.VirtualScreenHeight - ActualHeight; | |
| var maxLeft = SystemParameters.VirtualScreenLeft + SystemParameters.VirtualScreenWidth - ActualWidth; | |
| top = Math.Min(top, maxTop); | |
| left = Math.Min(left, maxLeft); | |
| } |
| var widthRatio = screenWidth / _settings.PreviousScreenWidth; | ||
| var heightRatio = screenHeight / _settings.PreviousScreenHeight; | ||
| var dpiXRatio = currentDpiX / previousDpiX; | ||
| var dpiYRatio = currentDpiY / previousDpiY; | ||
|
|
||
| var newLeft = previousLeft * widthRatio * dpiXRatio; | ||
| var newTop = previousTop * heightRatio * dpiYRatio; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Scaling uses freshly-overwritten values ⇒ ratios are always 1
Inside AdjustPositionForResolutionChange you divide by _settings.PreviousScreenWidth/Height, but these fields were just overwritten with the current size a few lines earlier, so widthRatio and heightRatio are always 1.
Either pass the captured previousScreenWidth/Height/Dpi into the method, or compute the ratios before writing the new values.
-var widthRatio = screenWidth / _settings.PreviousScreenWidth;
-var heightRatio = screenHeight / _settings.PreviousScreenHeight;
-var dpiXRatio = currentDpiX / previousDpiX;
-var dpiYRatio = currentDpiY / previousDpiY;
+var widthRatio = screenWidth / previousScreenWidth;
+var heightRatio = screenHeight / previousScreenHeight;
+var dpiXRatio = currentDpiX / previousDpiX;
+var dpiYRatio = currentDpiY / previousDpiY;Fixing this will finally position the window correctly after display configuration changes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var widthRatio = screenWidth / _settings.PreviousScreenWidth; | |
| var heightRatio = screenHeight / _settings.PreviousScreenHeight; | |
| var dpiXRatio = currentDpiX / previousDpiX; | |
| var dpiYRatio = currentDpiY / previousDpiY; | |
| var newLeft = previousLeft * widthRatio * dpiXRatio; | |
| var newTop = previousTop * heightRatio * dpiYRatio; | |
| // compute ratios using the captured "previous" values, not the overwritten settings | |
| var widthRatio = screenWidth / previousScreenWidth; | |
| var heightRatio = screenHeight / previousScreenHeight; | |
| var dpiXRatio = currentDpiX / previousDpiX; | |
| var dpiYRatio = currentDpiY / previousDpiY; | |
| var newLeft = previousLeft * widthRatio * dpiXRatio; | |
| var newTop = previousTop * heightRatio * dpiYRatio; |
| var previousScreenWidth = _settings.PreviousScreenWidth; | ||
| var previousScreenHeight = _settings.PreviousScreenHeight; | ||
| GetDpi(out var previousDpiX, out var previousDpiY); | ||
|
|
||
| _settings.PreviousScreenWidth = SystemParameters.VirtualScreenWidth; | ||
| _settings.PreviousScreenHeight = SystemParameters.VirtualScreenHeight; | ||
| GetDpi(out var currentDpiX, out var currentDpiY); | ||
|
|
There was a problem hiding this comment.
Previous DPI/screen data are never read from settings and therefore always equal current values
previousDpiX/Y are obtained via GetDpi, which returns current DPI, not the saved values.
As a result the “resolution/DPI change” branch is never taken and scaling never occurs.
-var previousScreenWidth = _settings.PreviousScreenWidth;
-var previousScreenHeight = _settings.PreviousScreenHeight;
-GetDpi(out var previousDpiX, out var previousDpiY);
+var previousScreenWidth = _settings.PreviousScreenWidth;
+var previousScreenHeight = _settings.PreviousScreenHeight;
+var previousDpiX = _settings.PreviousDpiX;
+var previousDpiY = _settings.PreviousDpiY;Remember to store the current values after the position logic runs:
_settings.PreviousScreenWidth = SystemParameters.VirtualScreenWidth;
_settings.PreviousScreenHeight = SystemParameters.VirtualScreenHeight;
_settings.PreviousDpiX = currentDpiX;
_settings.PreviousDpiY = currentDpiY;Without this, the new properties added to Settings are never populated.
Jack251970
changed the title
2 participants
From #2756.
Resolve #2320.
More information is available in #2756.