Skip to content

Conversation

@npwoods
Copy link

@npwoods npwoods commented Jul 19, 2025

This might not be the best fix, and it should probably be extended to cover cases where a window is repositioned beyond Window::set_fullscreen()

Feel free to reject this if there is a better way to go!

…ow::set_fullscreen()` call

This might not be the best fix, and it should probably be extended to cover cases where a window is repositioned beyond `Window::set_fullscreen()`
@kchibisov
Copy link
Member

@npwoods have you seen this one #4119 ?

@npwoods
Copy link
Author

npwoods commented Aug 12, 2025

@kchibisov indeed, I have not. At first glance this seems to be a similar, but not identical problems.

These are two different scenarios whose only commonality is they both involve suppressing the window size DPI adjustment.

And ironically, I just upgraded to Windows 11 yesterday and have confirmed that the problem my PR addresses is still happening on Windows 11. So #4119 didn't help that scenario.

let position: (i32, i32) = monitor.position().into();
let size: (u32, u32) = monitor.size().into();

window_state.lock().unwrap().currently_repositioning = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a bunch of SetWindowPos calls, wouldn't it be the problem for all of them, if so we would need to probably wrap it, somehow.

Copy link
Member

@kchibisov kchibisov Aug 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like, I wonder why fullscreen is special here, if maximize is probably the same, though, I'm not familiar with windows much? I'd assume that the point if you change position to other monitor?

@npwoods
Copy link
Author

npwoods commented Aug 23, 2025

When I worked on this PR, I was not aware of #4119; it was not clear to me why we were doing a bunch of adjustments for WM_DPICHANGED; it was just clear to me that is was bad when we were manually positioning the window with SetWindowPos()

After reading the chatter #4119; it sounds like to me that the reason for handling WM_DPICHANGED seems to be to work around buggy behavior when dragging windows from one monitor to another on Windows 10 and earlier. The fix on #4119 looks like it won't handle the situation described in this thread. It sounds like the proper fix would be to only trigger that behavior when somebody is:

  • Dragging and dropping the window
  • The OS is Windows 10 and earlier

It would be nice to understand the original reason for handling WM_DPICHANGED

@kchibisov
Copy link
Member

The OS is Windows 10 and earlier

It seems like other PR is taking care of this, right?

And for this, I'd assume that fullscreen is kind of as a drag/drop of window, well, we exclude it from the logic when we move the window, I also wonder what is the behavior when we change DPI of the monitor in settings, I'd assume that the window will get resized here as well?

If that's the case, we can probably detect the drag and only apply logic when the drag actually happens, as you say?

@kchibisov
Copy link
Member

It would be nice to understand the original reason for handling WM_DPICHANGED

#1130
#997 (comment)

That's what I've found.

@npwoods
Copy link
Author

npwoods commented Aug 24, 2025

Reading through that thread, I see lots of chatter, and even a few test cases, but if there is a description of the human visible behavior they are trying to work around somewhere, I have not been able to find it.

I have a crazy idea - stop resizing windows in response to WM_DPICHANGED window events. If nobody can discern why this behavior is necessary, maybe we should just get rid of it? That would eliminate the need for my PR :-)

@kchibisov
Copy link
Member

I think what they wanted to do is preserve physical size on screen, to some extent, I guess.

@npwoods
Copy link
Author

npwoods commented Aug 25, 2025

I suppose that I understand that part, but if there is a list of confirmed scenarios where this does happen and does not happen, I have not seen it.

The PR that I filed describes a scenario where this logic interferes with a legitimate operation, so arguably there is at least one scenario where the current logic detracts from that goal.

@madsmtm madsmtm added the DS - win32 Affects the Win32/Windows backend label Sep 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DS - win32 Affects the Win32/Windows backend

Development

Successfully merging this pull request may close these issues.

3 participants