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

Fix scaling on GTK & X11 #1389

Merged
merged 1 commit into from
Nov 17, 2020
Merged

Fix scaling on GTK & X11 #1389

merged 1 commit into from
Nov 17, 2020

Conversation

dhardy
Copy link
Contributor

@dhardy dhardy commented Nov 11, 2020

WIP: fixes #939

The first commit fixes scaling on the GTK backend, per my testing.

The second commit attempts to get the scaling factor when using the X11 backend. Unfortunately it doesn't work:

     Running `/home/dhardy/.cache/cargo/debug/examples/calc`
INFO  [druid_shell::platform::x11::application] X server supports Present version 1.0
INFO  [druid_shell::platform::x11::application] X server supports XFIXES version 5.0
WARN  [druid_shell::platform::x11::application] Application::get_locale is currently unimplemented for X11 platforms. (defaulting to en-US)
DEBUG [druid::localization] available locales [en-US, fr-CA, de-DE], current en-US
DEBUG [druid::localization] resolved: [en-US]
WARN  [druid_shell::platform::x11::window] WindowBuilder::resizable is currently unimplemented for X11 platforms.
[druid-shell/src/platform/x11/window.rs:233] scale = Scale {
    x: 1.0004923682914821,
    y: 1.0,
}

(Scale factor is actually 1.1667.) Any idea what's wrong here? @crsaracco

let scale = Scale::new(
FACTOR * screen.width_in_pixels as f64 / screen.width_in_millimeters as f64,
FACTOR * screen.height_in_pixels as f64 / screen.height_in_millimeters as f64,
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't rely on this information, for two reasons. First, I have no idea where it comes from and whether its reliable. On my system, xrandr and xdpyinfo report two different physical sizes for the same screen. Second, I think it's a bad idea to default to non-integer scaling. I'd argue for the xft-based solution that @crsaracco linked in #939.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll try implementing the xft method.

Second, I think it's a bad idea to default to non-integer scaling.

You're joking, right? 1080p screens are common across 13" - 17" laptops now (165 - 130 DPI). 27" 1440p screens are also common (109 DPI). Besides which, many older users like slightly larger text. This is a huge demand for scale factors between 1 and 2.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wasn't actually joking, but I'm definitely happy to discuss it further. To be clear, I'm not arguing against supporting fractional scaling, just about defaulting to it. My main reason is that it causes anti-aliasing artifacts (like #1088).

Besides which, many older users like slightly larger text.

Is there a reason to prefer scaling everything over just increasing the font size? (This is a genuine question -- I like tiny text so I'm very much not in the target audience)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that it causes anti-aliasing artifacts

Which is why I took a rather different approach to scaling with KAS (e.g. here). But that's off topic...

Is there a reason to prefer scaling everything over just increasing the font size?

Good question. Actually, I don't know much about how Druid likes to handle scaling, but at least the GTK backend currently is happy to use fractional scaling. This whole topic should probably get a new issue since this PR is just about fixing the currently-missing scaling support on X11 and broken support on GTK.

Copy link
Contributor

@psychon psychon Nov 16, 2020

Choose a reason for hiding this comment

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

First, I have no idea where it comes from and whether its reliable. On my system, xrandr and xdpyinfo report two different physical sizes for the same screen.

The data reported by xdpyinfo / the connection setup will always report 96 DPI. IIRC Keith Packard wrote that in a blog post somewhen, but I am not sure. But this can definitely not report data per RandR-output and there is also no way for the X11 server to update the information (well, RandR has something about that, but then you can just use the "real" info from RandR).

Edit: Related: https://wiki.archlinux.org/index.php/HiDPI#X_Server and https://wiki.archlinux.org/index.php/Xorg#Display_size_and_DPI

Copy link
Member

Choose a reason for hiding this comment

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

I'm happy to see this merged as an improvement, and then revisit the larger question of how we want to 'properly' do scaling? I'm not spending much time on linux (although I'm happy to play around in a vm as needed) so I trust @jneem and @dhardy to get this merged when there's consensus, and I'm happy to break any ties if needed. 👼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps we should also refer to the work of our peers: https://docs.rs/winit/0.23.0/winit/dpi/index.html#how-is-the-scale-factor-calculated

I'm in two minds whether it's worth bothering much here.

Copy link
Contributor Author

@dhardy dhardy Nov 17, 2020

Choose a reason for hiding this comment

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

Winit uses Xlib::open from x11-dl to access Xft.dpi. The RustAudio code uses the x11 crate. I think we can't access X resources with x11rb, so an extra dependency would be necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

At least from a C point of view: You are already linking against libcairo.so even when using x11rb. libcairo.so links against libX11. So your extra dependency would only be a Rust wrapper for libX11.

Also: Due to this issue and psychon/x11rb#550, I noticed that Xresource handling is missing in x11rb and opened psychon/x11rb#551 about that. It might be that the next release of x11rb will provide a way to query Xft.dpi. Technically, it is really easy. It's just that I'd have to hand-write a parser for the necessary file format (easy but annoying) and be reasonable sure that it behaves as expected (this seems to be the hard case - Xresources seem to be quite powerful and I do not really understand all the rules (yet)).
X11-the-protocol-wise, Xresources are just a property on the root window containing a string, plus some fallbacks in case there is no such properties. Thus, only the parsing and the actual query matching are necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

I certainly do not claim to know what I am doing, but I just added an example to psychon/x11rb#550 (comment) that draws a 2 cm line by querying information from RandR. It works for me, so it must obviously work everywhere. ;-)
This does not do anything with Xft configuration and has zero user configurability. It just detects values and uses them.
Also, I screwed up the calculation of DPI three times and then gave up. Thus, the code works in dots per cm instead. This also makes it a lot easier to calculate the length of a 2 cm line. This should just be a matter of adding the right scaling factor.

Also: Urgh for X11 (and thus x11rb) mixing u16, i16 and u32 in the involved APIs.

@jneem jneem mentioned this pull request Nov 12, 2020
druid-shell/src/platform/x11/window.rs Outdated Show resolved Hide resolved
let scale = Scale::new(
FACTOR * screen.width_in_pixels as f64 / screen.width_in_millimeters as f64,
FACTOR * screen.height_in_pixels as f64 / screen.height_in_millimeters as f64,
);
Copy link
Member

Choose a reason for hiding this comment

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

I'm happy to see this merged as an improvement, and then revisit the larger question of how we want to 'properly' do scaling? I'm not spending much time on linux (although I'm happy to play around in a vm as needed) so I trust @jneem and @dhardy to get this merged when there's consensus, and I'm happy to break any ties if needed. 👼

@dhardy
Copy link
Contributor Author

dhardy commented Nov 17, 2020

Right, I removed the X11 stuff since it may make sense to wait for the next x11rb release as mentioned by @psychon.

The GTK fix there shouldn't be any problem merging I think.

@jneem
Copy link
Collaborator

jneem commented Nov 17, 2020

Sounds good, let's get the gtk fix in first.

@jneem jneem merged commit 93741e6 into linebender:master Nov 17, 2020
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.

X11 shell does not handle DPI scaling.
4 participants