Fix incorrect window sizes for non-resizable windows on Wayland #7103
Conversation
GoldsteinE
force-pushed
the
window-logical-size
branch
from
24e322e to
52bc8a7
Compare
GoldsteinE
changed the title
|
Okay, I think this is ready. I wasn’t able to run all the tests locally (because some tests failed even on main branch for some reason), but it shouldn’t break anything. |
GoldsteinE
changed the title
GoldsteinE
changed the title
|
Currently checking on my device.... |
|
all good here just some pixels in some images - i dont think thats because of you |
crates/egui-winit/src/lib.rs
Outdated
| )); | ||
| } | ||
|
|
||
| #[expect(clippy::disallowed_types)] |
There was a problem hiding this comment.
i'd like to see also a reason here this would be cool
| #[expect(clippy::disallowed_types)] | |
| #[expect(clippy::disallowed_types, reason = "aaaa")] |
There was a problem hiding this comment.
I added lint reasons to all the expects.
crates/egui-winit/src/lib.rs
Outdated
| window_attributes = window_attributes.with_max_inner_size(PhysicalSize::new( | ||
| pixels_per_point * size.x, | ||
| pixels_per_point * size.y, | ||
| window_attributes = window_attributes.with_max_inner_size(dpi::LogicalSize::new( |
There was a problem hiding this comment.
a direct import would be better i think as we imported all the other dpi:: also
There was a problem hiding this comment.
The reason I didn't do a direct import is to avoid allowing the lint there too. I can do it though if you prefer it.
There was a problem hiding this comment.
ah okay :-)
maybe we should then think about removing the lint or that disallowed type?
but @emilk should decide
|
@bircni Hi! Could you please direct me to my next steps here? This is my first time contributing to egui and I’m a bit confused. I’m happy to add lint reasons and do a direct import if you prefer it, but I’m not sure if this PR is waiting on me doing that or on emilk making a decision about this lint in general. Thanks for review either way |
|
@GoldsteinE we are still waiting on comments from @emilk - but adding a reason would always be great! If emilk decides to reallow the types we can do that afterwards :-) |
Using physical window sizes leads to all kinds of fun stuff: winit always uses scale factor 1.0 on start to convert it back to logical pixels and uses these logical pixels to set min/max size for non-resizeable windows. You're supposed to adjust size after getting a scale change event if you're using physical sizes, but adjusting min/max sizes doesn't seem to work on sway, so the window is stuck with an incorrect size. The scale factor we guessed might also be wrong even if there's only a single display since it doesn't take fractional scale into account. TL;DR: winit actually wants logical sizes in these methods (since Wayland in general operates mostly on logical sizes) and converting them back and forth is lossy.
GoldsteinE
force-pushed
the
window-logical-size
branch
from
52bc8a7 to
3ddf288
Compare
|
Sure, I’ve done that. |
emilk
added
bug
|
This works beautifully on my mac ❤️ |
|
Preview available at https://egui-pr-preview.github.io/pr/7103-window-logical-size |
crates/egui-winit/src/lib.rs
Outdated
| )); | ||
| } | ||
|
|
||
| #[expect(clippy::disallowed_types, reason = "zoom factor is manually accounted for")] |
|
Thanks for fixing this! |
|
Thanks for reviewing! |
3 participants
Using physical window sizes leads to all kinds of fun stuff: winit always uses scale factor 1.0 on start to convert it back to logical pixels and uses these logical pixels to set min/max size for non-resizeable windows. You're supposed to adjust size after getting a scale change event if you're using physical sizes, but adjusting min/max sizes doesn't seem to work on sway, so the window is stuck with an incorrect size.
The scale factor we guessed might also be wrong even if there's only a single display since it doesn't take fractional scale into account.
TL;DR: winit actually wants logical sizes in these methods (since Wayland in general operates mostly on logical sizes) and converting them back and forth is lossy.