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

[GTK] use DP scaling for window size and position. #1713

Merged
merged 4 commits into from
Apr 16, 2021
Merged

[GTK] use DP scaling for window size and position. #1713

merged 4 commits into from
Apr 16, 2021

Conversation

jneem
Copy link
Collaborator

@jneem jneem commented Apr 10, 2021

I was playing around with subwindows and noticed that window sizes and positions are not scaled.

@HoNile
Copy link
Contributor

HoNile commented Apr 11, 2021

There is a similar issue on windows so I guess this is correct but the doc sring for this various method in druid_shell/src/window.rs say this:

set_position:

Sets the position of the window in pixels, relative to the origin of the virtual screen.

get_position:

Returns the position of the top left corner of the window in pixels, relative to the origin of the virtual screen.

set_size:

Set the window's size in display points.
...

get_size:

Gets the window size, in pixels.

I may miss understood but to me it look like the doc doesn't match your implementation.

@cmyr
Copy link
Member

cmyr commented Apr 11, 2021

@HoNile Good catch. I think I disagree with the docs; window size and position should be set in display points. For instance, on a 4k display running at 1920x1080, setting the position to (0, 0) and the size to (960, 1080) should result in a window that occupies the left half of the screen.

Am I overlooking anything?

@jneem
Copy link
Collaborator Author

jneem commented Apr 11, 2021

Ok, right, I vaguely remember something about this now. One point in favor of pixels is that these dimensions really need to be integer numbers of pixels. The disadvantage being, of course, that if everything else is in display points then this becomes somewhat of a footgun. In particular, I ran into this because the sub_window example doesn't work correctly with scaling.

Either way, these four should definitely be in the same units, but the current docs for set_size say it's in display points.

@cmyr
Copy link
Member

cmyr commented Apr 12, 2021

How does this work on various platforms? on macOS, for instance, you basically never use device pixels in any AppKit API (that I'm aware of, I'm sure there might be some when dealing with devices directly)

@jneem
Copy link
Collaborator Author

jneem commented Apr 12, 2021

I'm... not sure. I just tried to find out; on windows, it looks like the behavior depends on whether your application claims to support hidpi mode. The GTK docs aren't clear, but at least on my (X11) system it seems to use device pixels. Wayland has some built-in support for scaling, but I couldn't easily figure out whether the positioning extension takes it into account.

But I think the summary is that the other platforms all support specifying sizes and positions in device pixels, so druid-shell should be using them internally and doing its own scaling.

@jneem
Copy link
Collaborator Author

jneem commented Apr 14, 2021

I went digging a little; this was implemented and discussed a little bit here. One potential issue with using display points here is the presence of multiple monitors with different DPI.

@cmyr
Copy link
Member

cmyr commented Apr 15, 2021

I went digging a little; this was implemented and discussed a little bit here. One potential issue with using display points here is the presence of multiple monitors with different DPI.

Would those problems be any better if we were using device pixels? I think we're on the same page: we should be surfacing API that uses display points, and druid-shell should handle scaling as needed.

From my understanding, this means:

  • merge this PR
  • follow-up with something that fixes the currently incorrect docs

Am I missing anything?

@jneem
Copy link
Collaborator Author

jneem commented Apr 15, 2021

I went digging a little; this was implemented and discussed a little bit here. One potential issue with using display points here is the presence of multiple monitors with different DPI.

Would those problems be any better if we were using device pixels? I think we're on the same page: we should be surfacing API that uses display points, and druid-shell should handle scaling as needed.

I'm worried about the rounding, particularly in the case of multiple monitors. For example, if you try to put it on the second monitor, but the scaling calculation rounds down, you could end up positioning your window in the "last pixel" of the first monitor instead. Also, what if you want to make the window the same size as a monitor, but due to rounding you get it off by one?

Because of rounding, I think it is reasonable for druid-shell to expose size and position in pixels. But it's definitely a footgun, and so maybe we could expose them with clunkier names? What about this as the API:

// These are the "low level" versions in pixels
fn set_size_in_pixels(&self, Size);
fn get_size_in_pixels(&self) -> Size;
fn set_absolute_position_in_pixels(&self, Size);
fn get_absolute_position_in_pixels(&self) -> Size;

// These are the "high level" versions in display points.
fn set_size(&self, Size);
fn get_size(&self) -> Size;
// These are in relative coordinates because
// - it's usually what you want, at least for subwindows, and
// - it's portable to wayland, unlike absolute positions.
fn set_position(&self, relative_to: WindowHandle, offset: Vec2);
fn get_position(&self, relative_to: WindowHandle) -> Vec2;

@cmyr
Copy link
Member

cmyr commented Apr 15, 2021

I went digging a little; this was implemented and discussed a little bit here. One potential issue with using display points here is the presence of multiple monitors with different DPI.

Would those problems be any better if we were using device pixels? I think we're on the same page: we should be surfacing API that uses display points, and druid-shell should handle scaling as needed.

I'm worried about the rounding, particularly in the case of multiple monitors. For example, if you try to put it on the second monitor, but the scaling calculation rounds down, you could end up positioning your window in the "last pixel" of the first monitor instead.

Hmm. I would expect the positioning of a window on a screen to be a (screen + position) pair, not a position on a contigious plane? If a screen position moves, we shouldn't tear the window. As an additional data point, I don't believe macOS allows windows to span multiple monitors; a window may be visible on multiple monitors during drag, but when you release the mouse the window is assigned to whatever display currenty contains the mouse, and it is not drawn on the other monitor, period.

Also, what if you want to make the window the same size as a monitor, but due to rounding you get it off by one?

In this case I would also expect you to be position the window at (0, 0), and then using the size that we report as the size of the monitor, in display points, which should have been calculated with the same rounding behaviour that will be used to go in the other direction. If this problem ever actually manifested I would probably suggest some special-case for values that neighbour the known height/width?

Basically I'm not currently convinced. It feels to me like we're overcomplicating things in a way that does not produce any benefits that are clear to me, and has some clear downsides: a more verbose API with more options, that will be easier to misuse.

That said: I do think that your API (burying the pixel methods behind verbose names) is a good choice if this is what we ultimately want to do.

@jneem
Copy link
Collaborator Author

jneem commented Apr 15, 2021

I'm worried about the rounding, particularly in the case of multiple monitors. For example, if you try to put it on the second monitor, but the scaling calculation rounds down, you could end up positioning your window in the "last pixel" of the first monitor instead.

Hmm. I would expect the positioning of a window on a screen to be a (screen + position) pair, not a position on a contigious plane?

That makes sense to me, although it does complicate the (presumably common case) of just wanting an offset relative to a window, because then you need to figure out what screen your window's on.

If a screen position moves, we shouldn't tear the window. As an additional data point, I don't believe macOS allows windows to span multiple monitors; a window may be visible on multiple monitors during drag, but when you release the mouse the window is assigned to whatever display currenty contains the mouse, and it is not drawn on the other monitor, period.

I'll open an issue about this -- it sounds like it's worth thinking about the window positioning API a bit more carefully.

Also, what if you want to make the window the same size as a monitor, but due to rounding you get it off by one?

In this case I would also expect you to be position the window at (0, 0), and then using the size that we report as the size of the monitor, in display points, which should have been calculated with the same rounding behaviour that will be used to go in the other direction. If this problem ever actually manifested I would probably suggest some special-case for values that neighbour the known height/width?

Basically I'm not currently convinced. It feels to me like we're overcomplicating things in a way that does not produce any benefits that are clear to me, and has some clear downsides: a more verbose API with more options, that will be easier to misuse.

Ok, I agree that I was overcomplicating it. If we round-to-nearest-pixel, then (for all sane scaling factors) we should losslessly round-trip from px -> dp -> px, and so the off-by-one that I was worrying about shouldn't happen. (Currently, at least the windows backend rounds down instead of to-nearest, but I'll fix that.)

So I'll update this PR to do rounding properly, work in DP on all backends, and update the docs.

@cmyr
Copy link
Member

cmyr commented Apr 16, 2021

Sounds good, thanks for digging into this!

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Thanks!

@jneem
Copy link
Collaborator Author

jneem commented Apr 16, 2021

Thanks! Just a heads-up that I also converted content_insets to dp -- it was the only thing left in pixel units and I didn't want it to be lonely. Doing a changelog now and then I'll push this afternoon.

@jneem jneem merged commit 5958963 into linebender:master Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants