Fix DPI with 0 width/hight reported by xorg #544
Conversation
|
Thanks! With the number of regressions being reported, it looks like I'll have to put out a 0.15.1 Also, I'd like a CHANGELOG entry (I really need to merge #542), though I guess you should wait until the situation with env vars is decided. |
It is now possible to override the DPI factor using the `WINIT_HIDPI_FACTOR` environment variable on X11. The changelog also has been updated to introduce all current changes made.
|
This took quite a while for such a simple change, but the PR should be good to go now. Any comments are appreciated @francesca64. |
src/window.rs
Outdated
| @@ -344,6 +344,10 @@ impl Window { | |||
| /// Returns the ratio between the backing framebuffer resolution and the | |||
| /// window size in screen pixels. This is typically one for a normal display | |||
| /// and two for a retina display. | |||
| /// | |||
| /// ## Platform-specific | |||
| /// On X11 the DPI factor can be overwritten using the `WINIT_HIDPI_FACTOR` environment | |||
src/platform/linux/x11/util/randr.rs
Outdated
| @@ -1,4 +1,5 @@ | |||
| use std::slice; | |||
| use std::str::FromStr; | |||
| use std::{slice, env}; | |||
There was a problem hiding this comment.
This is the nitpickiest of nitpicks, but I'm trying to cultivate a consistent use style.
use std::{env, slice};
use std::str::FromStr;There was a problem hiding this comment.
I love it :D. I was just following the default rustfmt import ordering.
There was a problem hiding this comment.
Thanks, but you didn't alphabetize the order of env and slice.
There was a problem hiding this comment.
Oh I didn't even notice that, sorry.
|
Oh, right, there's something important. There's nothing to stop users from specifying a DPI factor of 0.0 (or a negative DPI factor) which could cause all sorts of unexpected problems. |
|
@francesca64 Is there a problem with that? If the user wants to break the application, that should be fine, right? It might be nice to mention in the documentation and it wouldn't be an issue to just make sure it's bigger than 0, but I don't see anything wrong with allowing the user to mess stuff up if he wants to do so. |
|
They'd be semantically incorrect DPI factors, and 0.0 would result in countless divide by zeros throughout the entire software stack, whereas negative values would invert all positions and sizes (and in the case of I can think of no reason to allow this. Besides, #548 asserts that the DPI factor is greater than 0 on every physical/logical conversion, so anyone trying to break things wouldn't get very far. Adding a check here and exiting with a helpful message seems practical. |
|
So how would you go about this? Panic with an error or fix the value to 1.0 if the input is invalid? Could also do a combination and |
|
I'm partial to panicking, since I like to be loud and explicit. |
|
Loud and explicit sounds great to me, I'll put that in. |
|
Awesome, thanks! Tomorrow I'll look into the macOS regressions. I'll probably cut 0.15.1 around Friday, so if I miss anything let me know before then. |
|
We might as well go all in and use |
|
Oh yeah that would have totally been a good idea, forgot about that. @francesca64 feel free to add that yourself if you're interested in that. |
There is an issue with xorg reporting 0 width or height which leads to a division by zero with floating point numbers. This will result in a DPI factor of +inf. As a workaround to this when either width or height are 0 the DPI is now set to a fixed value (1.0).
In #543 the option of setting this as an environment variable has been discussed and I'd be happy to add this to the PR if it is desired.
This fixes #543.