Skip to content
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

[WIP] Dpi scaling, resolution changes and multimonitor support #293

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

fredizzimo
Copy link
Contributor

This pull request started by me trying to fix #92. I found out that DWMWA_EXTENDED_FRAME_BOUNDS gives the actual visible window size and position in pixels and the GetWindowRect function gives the size including the invisible part. So to position the window correctly, we need to offset the window by the difference between the two values. This seems to work for all different windows I have tried, including firefox and chrome, and both works without any special cases and additionally the problem in #207 is fixed.

But in order to do that, we need to enable per monitor DPI awareness, since GetWindowRect returns virtual pixels instead of real pixels, so otherwise the two values are not comparable. That means that we need to convert points to pixels in a few places, so I added the utility function points_to_pixels. I also made all size configuration variables use points in pixels, so that they are consistent across monitors and scalings, and are properly relative to the font sizes. For the fonts I changed the size to be negative, which means that you specify the actual font size instead of the bounding rect, which makes the fonts sizes consistent with other programs like notepad. These two changes means that you might have to change your configuration a little bit to make it look like before, but I believe that this way is better and more natural.

One side effect of enabling the per monitor awareness is also that I no longer get the (unreported?) bug where the font of windows titlebars become very small.

This dpi awareness naturally led me to implement support for changing the scaling on the fly. And since changing resolution is closely related I added support for that too. In all cases, when there are some changes, the display arrays are re-created, which avoids the need of too much special case handling. This design naturally led me to support display connects and disconnects as well. For disconnects, if a workspace belongs to a display that's no longer available, it's migrated to the new primary display. If a new display is added, then nothing happens to the workspaces.

There was one complication when implementing this, the taskbar does not react immediately to the new dpi or resolution, so I had to detect taskbar size and position changes, and react to that in the same way as well. And this had the nice side effect that the workspace now correctly updates when resizing or moving the taskbar.

Resolution changes are a little bit more complicated in relation to the taskbar, since many query functions on the taskbar temporarily returns errors, so those errors are now ignored. They don't really matter, since the taskbar position event is sent very quickly after, and at that point the taskbar is valid again, so everything refreshes correctly.

Note that the pull request is still WIP. I need to go through the code, and possibly clean up some things first, this is the first time I write Rust, and I learned a few things when doing this, so I need to clean up the earlier commits. But feel free to give suggestions, I'm still very new to Rust.

Additionally there's still at least one known bug, if you have moved the workspace to another montior, then it will flash between the screens when the monitor size changes. I know the reason, but I haven't fixed it yet, the focused_grid_id field of the old display is not cleared.

It also most likely does not handle unplugging all of the monitors, and I'm not actually sure what to do in that case. Perhaps create a virtual display? Or exit the workmode?

Finally #90 reports that the monitor ids are not stable, so I think the comparison has to use device ids instead. I have personally not run into that issue, but I believe it's there.

fix #90
fix #92
fix #207

This makes sure everything is up to date in case the resolution, scaling
or set of monitors changes.
DPI, Resolution, Monitor changes and Windows monitor layout changes
The taskbar is invalid for a short while, so errors related to that are
ignored.
@fredizzimo fredizzimo marked this pull request as draft August 4, 2021 21:51
@TimUntersberger
Copy link
Owner

TimUntersberger commented Aug 4, 2021

Accidently closed sorry. I will try to look over this PR in the coming days.

Just reading your description already makes me excited to get this merged!

Thank you for your work!

Copy link
Owner

@TimUntersberger TimUntersberger left a comment

Choose a reason for hiding this comment

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

Overall looks already very good!

@@ -29,3 +32,13 @@ pub fn scale_color(color: i32, factor: f64) -> i32 {

rgb_to_hex((red, green, blue))
}

pub fn points_to_pixels<T: PrimInt + AsPrimitive<i64> + FromPrimitive>(points: T, display: &Display) -> T {
Copy link
Owner

Choose a reason for hiding this comment

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

Moving this function into the display struct should be possible right?

@@ -246,6 +258,19 @@ impl Window {
nullable_to_result(GetWindowRect(self.id.into(), &mut temp)).map(|_| temp.into())
}
}
pub fn get_extended_rect(&self) -> WinResult<Rectangle> {
unsafe {
let mut frame_rect: RECT = mem::zeroed();
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
let mut frame_rect: RECT = mem::zeroed();
let mut frame_rect = RECT::default();

@@ -64,6 +65,8 @@ impl TileGrid {
0
},
);
let padding = util::points_to_pixels(padding, &display);
let maging = util::points_to_pixels(margin, &display);
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
let maging = util::points_to_pixels(margin, &display);
let margin = util::points_to_pixels(margin, &display);

@ramirezmike
Copy link
Collaborator

this is amazing 🥳

I'm gonna test this out locally tomorrow, I'm also pretty excited

@fredizzimo
Copy link
Contributor Author

Thanks for the positive feedback!

But just to let you know, I might not have time to fix the issues and return to this until the beginning of next week. Until then, the code should be stable enough to test and try to break and find more issues than the ones I mentioned in the descriptions. But the actual final fixes before this can be merged probably has to wait, unless you want to take over and fix them yourself.

@TimUntersberger
Copy link
Owner

TimUntersberger commented Aug 5, 2021

It also most likely does not handle unplugging all of the monitors, and I'm not actually sure what to do in that case. Perhaps create a virtual display? Or exit the workmode?

my vote is for exiting work mode

But just to let you know, I might not have time to fix the issues and return to this until the beginning of next week

No worries!

Edit:

Just so we don't forget:

  • remove rules for firefox/chromium
  • document above change in changelog
  • update default config
  • handle unplugging all monitors
  • if you have moved the workspace to another montior, then it will flash between the screens when the monitor size changes. I know the reason, but I haven't fixed it yet, the focused_grid_id field of the old display is not cleared.

@NotSpaulding
Copy link

Has any progress been made on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants