-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
windows: Properly handle DPI
#9456
Conversation
let _ = SetWindowPos( | ||
self.hwnd, | ||
None, | ||
rect.left, | ||
rect.top, | ||
width, | ||
height, | ||
SWP_NOZORDER | SWP_NOACTIVATE, | ||
) | ||
.inspect_err(|_| { | ||
log::error!( | ||
"unable to set window position after dpi has changed: {}", | ||
std::io::Error::last_os_error() | ||
) | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of ignoring the result and inspecting the error I'd suggest just using .log_err()
or something. If you want to add context to the error you could do something like this:
let _ = SetWindowPos( | |
self.hwnd, | |
None, | |
rect.left, | |
rect.top, | |
width, | |
height, | |
SWP_NOZORDER | SWP_NOACTIVATE, | |
) | |
.inspect_err(|_| { | |
log::error!( | |
"unable to set window position after dpi has changed: {}", | |
std::io::Error::last_os_error() | |
) | |
}); | |
SetWindowPos( | |
self.hwnd, | |
None, | |
rect.left, | |
rect.top, | |
width, | |
height, | |
SWP_NOZORDER | SWP_NOACTIVATE, | |
) | |
.context("unable to set window position after dpi has changed") | |
.log_err(); | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some ffi functions return just the error code, no usefull infomation. So I use inspect_err
to pull error message from windows by calling last_os_error
, .log_err()
will just log the err code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The windows crate actually wrap the last error message in their functions. I tested the above snippet I suggested and it works as expected. the os error message is printed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks for the tips!
/// the same monitor | ||
pub fn new_logical(x: i32, y: i32, scale_factor: f32) -> Self { | ||
Point { | ||
x: Pixels(x as f32 / scale_factor), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can use the px()
helper for these.
scale_factor: Cell<f32>, | ||
} | ||
|
||
impl WindowMetrics { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that, rather than creating this type and these new helpers, we should just be consistently using the existing GPUI types, Pixels, GlobalPixels, ScaledPixels, DevicePixels, Size, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using these aliases might make things clearer, I guess. It was only after reading the macOS impls that I understood the things realy goes here. Should we remove the type aliases and keep this struct? Any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless there's a compelling code reason to do otherwise, I think the best solution would be to take all of these type aliases and the WindowMetrics struct out, and to just have origin: Point<GlobalPixels>
, size: Size<GlobalPixels>
, and scale_factor: f32
(with appropriate Cell<>
wrapping). As that's the only data you actually need to store. The logical size and bounds can be recomputed on the fly from those values.
Once you have those inlined, I think you could just make a fn logical_point(x, y, scale_factor) -> Point<Pixels>
helper that you use when you need it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
screenshots and description incoming ## title bar when window is maximized | before | after | | --- | --- | | ![image](https://github.com/zed-industries/zed/assets/1284289/075a943d-54db-4b71-9fa0-15f823255182) | ![image](https://github.com/zed-industries/zed/assets/1284289/39a1d381-fcfd-4651-aab4-231a8ec3bd99) | ## ~~caption buttons at 200%~~ ~~buttons are now properly responsive at different scales~~ ~~closes #9438~~ ~~proper scale factor handling in follow up PR (possibly #9440)~~ <details> <summary>out of date image</summary> ![scale-factor](https://github.com/zed-industries/zed/assets/1284289/299d37b8-0d2e-4f2e-81db-2fff6fc59a62) </details> should be fixed by #9456 Release Notes: - N/A
As I mentioned before, there are the following issues with how GPUI handles scale factors greater than 1.0: 1. The title bar buttons do not function correctly, with minimizing button performing maximization and maximizing button performing closure. 2. As discussed in zed-industries#8809, setting a scale factor greater than 1.0 causes GPUI's drawing content to be pushed off the screen. This PR introduces `LogicalSize` and `PhysicalSize` to differentiate between coordinate systems for proper GPUI rendering, and now scale factors above 1.5 are working correctly. `Zed` with a scale factor equals 1.5, and change between different scale factors: https://github.com/zed-industries/zed/assets/14981363/3348536d-8bd3-41dd-82f6-052723312a5b Release Notes: - N/A
screenshots and description incoming ## title bar when window is maximized | before | after | | --- | --- | | ![image](https://github.com/zed-industries/zed/assets/1284289/075a943d-54db-4b71-9fa0-15f823255182) | ![image](https://github.com/zed-industries/zed/assets/1284289/39a1d381-fcfd-4651-aab4-231a8ec3bd99) | ## ~~caption buttons at 200%~~ ~~buttons are now properly responsive at different scales~~ ~~closes zed-industries#9438~~ ~~proper scale factor handling in follow up PR (possibly zed-industries#9440)~~ <details> <summary>out of date image</summary> ![scale-factor](https://github.com/zed-industries/zed/assets/1284289/299d37b8-0d2e-4f2e-81db-2fff6fc59a62) </details> should be fixed by zed-industries#9456 Release Notes: - N/A
…ustries#9637) In zed-industries#9456 I forgot to handle this... Release Notes: - N/A
As I mentioned before, there are the following issues with how GPUI handles scale factors greater than 1.0:
This PR introduces
LogicalSize
andPhysicalSize
to differentiate between coordinate systems for proper GPUI rendering, and now scale factors above 1.5 are working correctly.Zed
with a scale factor equals 1.5, and change between different scale factors:10.mp4
Release Notes: