prevent incorrect shifting of window when dragging onto monitor with different DPI #4119
Conversation
|
I ran into the same issue during development of a Tauri/TAO app and also came to the conclusion that the custom positioning is not only unnecessary, but breaks on Windows 11. There is a difference in behavior between Windows 10 and 11. On Windows 11 (without the custom positioning), the Window remains at the same relative position to the cursor when moving between monitors. This is not the case on Windows 10 and this is what I think the custom positioning logic attempts to fix. I've created a similar fix for TAO but kept the logic conditionally for Windows 10 (build < 22000) just to be safe. But I'm not sure if it is worth to keep that much logic just for minimal positioning optimizations, especially now that Windows 10 will soon reach EOL. Related PR and issues (TAO seems still uses the same logic since it was forked from winit, so I think they are relevant):
|
acarl005
force-pushed
the
andy/fix-windows-dpi-changed
branch
from
3bb815d to
682b23c
Compare
|
@dgerhardt Thank you! I've switched this PR over to your implementation. |
|
I'm getting some CI failures which I think are unrelated. |
|
I'll deal with the CI, don't worry. |
|
I'm waiting for this to merge quickly. |
acarl005
force-pushed
the
andy/fix-windows-dpi-changed
branch
from
e16fa07 to
bb96618
Compare
acarl005
force-pushed
the
andy/fix-windows-dpi-changed
branch
from
bb96618 to
df52c1a
Compare
|
How come this isn't merged yet? |
|
no maintainers on windows. |
|
@kchibisov Is there anything I can do to help? We have Windows devices. Is there a QA process or something y'all would like to see to move this PR through? |
|
I'll mention that Warp has been running with this code in our fork warpdotdev#10 for about 6 months and we have more than 20K daily Windows users. |
|
If you say that the code is tested then I'm fine with merging. I'll try to take a look myself code wise once I have time. It's just mostly human resources at the end of the day. |
| // The window position needs adjustment on Windows 10. | ||
| { | ||
| let suggested_ul = | ||
| (suggested_rect.left + margin_left, suggested_rect.top + margin_top); | ||
|
|
||
| let mut conservative_rect = RECT { | ||
| left: suggested_ul.0, | ||
| top: suggested_ul.1, | ||
| right: suggested_ul.0 + new_physical_surface_size.width as i32, | ||
| bottom: suggested_ul.1 + new_physical_surface_size.height as i32, | ||
| }; | ||
|
|
||
| conservative_rect = window_flags | ||
| .adjust_rect(window, conservative_rect) | ||
| .unwrap_or(conservative_rect); | ||
|
|
||
| // If we're dragging the window, offset the window so that the cursor's | ||
| // relative horizontal position in the title bar is preserved. | ||
| if dragging_window { | ||
| let bias = { | ||
| let cursor_pos = { | ||
| let mut pos = unsafe { mem::zeroed() }; | ||
| unsafe { GetCursorPos(&mut pos) }; | ||
| pos | ||
| conservative_rect = window_flags | ||
| .adjust_rect(window, conservative_rect) | ||
| .unwrap_or(conservative_rect); | ||
|
|
||
| // If we're dragging the window, offset the window so that the cursor's | ||
| // relative horizontal position in the title bar is preserved. | ||
| if dragging_window { | ||
| let bias = { | ||
| let cursor_pos = { | ||
| let mut pos = unsafe { mem::zeroed() }; | ||
| unsafe { GetCursorPos(&mut pos) }; | ||
| pos | ||
| }; | ||
| let suggested_cursor_horizontal_ratio = | ||
| (cursor_pos.x - suggested_rect.left) as f64 | ||
| / (suggested_rect.right - suggested_rect.left) as f64; | ||
|
|
||
| (cursor_pos.x | ||
| - (suggested_cursor_horizontal_ratio | ||
| * (conservative_rect.right - conservative_rect.left) as f64) | ||
| as i32) | ||
| - conservative_rect.left | ||
| }; | ||
| let suggested_cursor_horizontal_ratio = (cursor_pos.x - suggested_rect.left) | ||
| as f64 | ||
| / (suggested_rect.right - suggested_rect.left) as f64; | ||
|
|
||
| (cursor_pos.x | ||
| - (suggested_cursor_horizontal_ratio | ||
| * (conservative_rect.right - conservative_rect.left) as f64) | ||
| as i32) | ||
| - conservative_rect.left | ||
| }; | ||
| conservative_rect.left += bias; | ||
| conservative_rect.right += bias; | ||
| } | ||
| conservative_rect.left += bias; | ||
| conservative_rect.right += bias; | ||
| } | ||
|
|
||
| // Check to see if the new window rect is on the monitor with the new DPI factor. | ||
| // If it isn't, offset the window so that it is. | ||
| let new_dpi_monitor = unsafe { MonitorFromWindow(window, MONITOR_DEFAULTTONULL) }; | ||
| let conservative_rect_monitor = | ||
| unsafe { MonitorFromRect(&conservative_rect, MONITOR_DEFAULTTONULL) }; | ||
| new_outer_rect = if conservative_rect_monitor == new_dpi_monitor { | ||
| conservative_rect | ||
| } else { | ||
| let get_monitor_rect = |monitor| { | ||
| let mut monitor_info = MONITORINFO { | ||
| cbSize: mem::size_of::<MONITORINFO>() as _, | ||
| ..unsafe { mem::zeroed() } | ||
| // Check to see if the new window rect is on the monitor with the new DPI | ||
| // factor. If it isn't, offset the window so that it is. | ||
| let new_dpi_monitor = | ||
| unsafe { MonitorFromWindow(window, MONITOR_DEFAULTTONULL) }; | ||
| let conservative_rect_monitor = | ||
| unsafe { MonitorFromRect(&conservative_rect, MONITOR_DEFAULTTONULL) }; | ||
| new_outer_rect = if conservative_rect_monitor == new_dpi_monitor { | ||
| conservative_rect | ||
| } else { | ||
| let get_monitor_rect = |monitor| { | ||
| let mut monitor_info = MONITORINFO { | ||
| cbSize: mem::size_of::<MONITORINFO>() as _, | ||
| ..unsafe { mem::zeroed() } | ||
| }; | ||
| unsafe { GetMonitorInfoW(monitor, &mut monitor_info) }; | ||
| monitor_info.rcMonitor | ||
| }; | ||
| unsafe { GetMonitorInfoW(monitor, &mut monitor_info) }; | ||
| monitor_info.rcMonitor | ||
| }; | ||
| let wrong_monitor = conservative_rect_monitor; | ||
| let wrong_monitor_rect = get_monitor_rect(wrong_monitor); | ||
| let new_monitor_rect = get_monitor_rect(new_dpi_monitor); | ||
|
|
||
| // The direction to nudge the window in to get the window onto the monitor with | ||
| // the new DPI factor. We calculate this by seeing which monitor edges are | ||
| // shared and nudging away from the wrong monitor based on those. | ||
| #[allow(clippy::bool_to_int_with_if)] | ||
| let delta_nudge_to_dpi_monitor = ( | ||
| if wrong_monitor_rect.left == new_monitor_rect.right { | ||
| -1 | ||
| } else if wrong_monitor_rect.right == new_monitor_rect.left { | ||
| 1 | ||
| } else { | ||
| 0 | ||
| }, | ||
| if wrong_monitor_rect.bottom == new_monitor_rect.top { | ||
| 1 | ||
| } else if wrong_monitor_rect.top == new_monitor_rect.bottom { | ||
| -1 | ||
| } else { | ||
| 0 | ||
| }, | ||
| ); | ||
| let wrong_monitor = conservative_rect_monitor; | ||
| let wrong_monitor_rect = get_monitor_rect(wrong_monitor); | ||
| let new_monitor_rect = get_monitor_rect(new_dpi_monitor); | ||
|
|
||
| // The direction to nudge the window in to get the window onto the monitor | ||
| // with the new DPI factor. We calculate this by seeing which monitor edges | ||
| // are shared and nudging away from the wrong monitor based on those. | ||
| #[allow(clippy::bool_to_int_with_if)] | ||
| let delta_nudge_to_dpi_monitor = ( | ||
| if wrong_monitor_rect.left == new_monitor_rect.right { | ||
| -1 | ||
| } else if wrong_monitor_rect.right == new_monitor_rect.left { | ||
| 1 | ||
| } else { | ||
| 0 | ||
| }, | ||
| if wrong_monitor_rect.bottom == new_monitor_rect.top { | ||
| 1 | ||
| } else if wrong_monitor_rect.top == new_monitor_rect.bottom { | ||
| -1 | ||
| } else { | ||
| 0 | ||
| }, | ||
| ); | ||
|
|
||
| let abort_after_iterations = new_monitor_rect.right - new_monitor_rect.left | ||
| + new_monitor_rect.bottom | ||
| - new_monitor_rect.top; | ||
| for _ in 0..abort_after_iterations { | ||
| conservative_rect.left += delta_nudge_to_dpi_monitor.0; | ||
| conservative_rect.right += delta_nudge_to_dpi_monitor.0; | ||
| conservative_rect.top += delta_nudge_to_dpi_monitor.1; | ||
| conservative_rect.bottom += delta_nudge_to_dpi_monitor.1; | ||
|
|
||
| if unsafe { MonitorFromRect(&conservative_rect, MONITOR_DEFAULTTONULL) } | ||
| == new_dpi_monitor | ||
| { | ||
| break; | ||
| let abort_after_iterations = new_monitor_rect.right - new_monitor_rect.left | ||
| + new_monitor_rect.bottom | ||
| - new_monitor_rect.top; | ||
| for _ in 0..abort_after_iterations { | ||
| conservative_rect.left += delta_nudge_to_dpi_monitor.0; | ||
| conservative_rect.right += delta_nudge_to_dpi_monitor.0; | ||
| conservative_rect.top += delta_nudge_to_dpi_monitor.1; | ||
| conservative_rect.bottom += delta_nudge_to_dpi_monitor.1; | ||
|
|
||
| if unsafe { MonitorFromRect(&conservative_rect, MONITOR_DEFAULTTONULL) } | ||
| == new_dpi_monitor | ||
| { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| conservative_rect | ||
| }; | ||
| conservative_rect | ||
| }; |
There was a problem hiding this comment.
From what I can see this hasn't changed at all and all that got added is the new_outer_rect part, could this windows10 handling be somehow extracted to a function, while you shift the code anyway, so it'll be a bit more readable?
| pub(crate) static WIN10_BUILD_VERSION: Lazy<Option<u32>> = Lazy::new(|| { | ||
| type RtlGetVersion = unsafe extern "system" fn(*mut OSVERSIONINFOW) -> NTSTATUS; | ||
| let handle = get_function!("ntdll.dll", RtlGetVersion); | ||
|
|
||
| if let Some(rtl_get_version) = handle { | ||
| unsafe { | ||
| let mut vi = OSVERSIONINFOW { | ||
| dwOSVersionInfoSize: 0, | ||
| dwMajorVersion: 0, | ||
| dwMinorVersion: 0, | ||
| dwBuildNumber: 0, | ||
| dwPlatformId: 0, | ||
| szCSDVersion: [0; 128], | ||
| }; | ||
|
|
There was a problem hiding this comment.
I'd assume that got moved just for the sake of it not being in dark_mode.rs, if so, split it to a separate commit.
| - Renamed "super" key to "meta", to match the naming in the W3C specification. | ||
| `NamedKey::Super` still exists, but it's non-functional and deprecated, `NamedKey::Meta` should be used instead. | ||
| - Move `IconExtWindows` into `WinIcon`. | ||
| - On Windows, prevent incorrect shifting when dragging window onto a monitor with different DPI. |
There was a problem hiding this comment.
I'd assume that it's talking only about Windows 11?
#4305 is not addressing the same problem. #4305 is addressing a separate need to suppress the windows size adjustment logic, specifically scenarios where |
|
Reading through this thread, it sounds like the intention of the window size adjustment problem is to address a very specific scenario where a window is being dragged from one monitor to another by the user under Windows 10. If this analysis is correct, ideally we could try to identify that very specific scenario and only apply the logic then. Detecting Windows 10 is easy... can we figure out if we're in the midst of a DnD operation? |
|
Hi @kchibisov , I implemented your suggested changes and resolved merge conflicts in my recent fork in https://github.com/moooozi/winit/tree/win32-dpi-fix I hope this gets merged. Edit: in case a new PR is needed, #4341 |
|
Thanks. applied from rebase. |
8 participants
changelogmodule if knowledge of this change could be valuable to usersUpdated documentation to reflect any user-facing changes, including notes of platform-specific behaviorCreated or updated an example program if it would help users understand this functionalityThis PR fixes this issue: #4041
The code I deleted has a validation to check if the new window bounds which Windows is suggesting to move the monitor to is actually on the monitor which triggered the
WM_DPI_CHANGEDevent. If that validation fails, it tried to move the window such that it will be on the monitor which triggered theWM_DPI_CHANGEDevent. However, this validation was buggy and was actually causing this bug. It was moving the window off of the intended monitor. It seems this is no longer necessary. For example, Chromium's DPI changed handler does not contain an analogous check.Before
https://www.loom.com/share/aefc5dc1027a42fc8f462c23621ce9a5
After
https://www.loom.com/share/81351dcb5938463c8bb60a66b2a0641b