X11: enable dpi scaling #890

vincentbernat · 2021-07-05T17:30:39Z

The scaling factor is derived from the active screen's dpi. This may
or may not match what is expected. Other sources could be font DPI or
Gdk/WindowScalingFactor.

Currently, only integer scaling is supported.

This reuses most of the code that was present for Wayland and the
change is limited to compute a scaling factor and putting the window
at the right position and at the right size. The code to compute the
window position in draw.c was only used for X11, so it was moved
directly in x.c.

See #884 for the discussion.

I would also be interested to support fractional scaling. From my understanding, this is not possible with Wayland, right? For X11, we can and the change would mostly be replacing scale by scale*10 and divide it again by 10 everywhere.

frebib · 2021-07-06T16:40:36Z

This seems to work on my computer, where I presume it's intended that the current DPI is rounded up to the nearest integer scale which is used to calculate the geometry. I tested 96dpi (1.0x), 144dpi (1.5x) and 192dpi (2.0x) where I got the geometry scale 1x, 2x, 2x respectively.

I'll chime in and say whilst this is better than no scaling at all, fractional scaling would be a lot more useful for the majority of people (myself included), so hopefully this improvement can lead to that next!

For X11, we can and the change would mostly be replacing scale by scale*10 and divide it again by 10 everywhere.

I'm not sure this would work great to be honest, see cases like 1.25x (120dpi), it would be slightly off. I think it'd be better to pass around the full scale value as a float and clamp/round it for wayland but not for X11.

Either way, this is a great improvement and a good start to solving #754 fully. Thanks for the hard work @vincentbernat and @fwsmit

codecov-commenter · 2021-07-06T21:07:38Z

Codecov Report

Merging #890 (45ea305) into master (6d41d1c) will increase coverage by 0.26%.
The diff coverage is 46.29%.

@@            Coverage Diff             @@
##           master     #890      +/-   ##
==========================================
+ Coverage   59.91%   60.18%   +0.26%     
==========================================
  Files          39       39              
  Lines        6137     6181      +44     
==========================================
+ Hits         3677     3720      +43     
- Misses       2460     2461       +1

Flag	Coverage Δ
unittests	`60.18% <46.29%> (+0.26%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/wayland/wl.c	`0.00% <0.00%> (ø)`
src/x11/x.c	`2.21% <0.00%> (-0.08%)`	⬇️
test/settings_data.c	`96.90% <ø> (ø)`
src/draw.c	`0.91% <2.94%> (+0.02%)`	⬆️
src/icon.c	`69.18% <69.23%> (+0.73%)`	⬆️
test/option_parser.c	`98.82% <95.83%> (-0.23%)`	⬇️
src/option_parser.c	`83.87% <100.00%> (+0.08%)`	⬆️
src/utils.c	`90.80% <100.00%> (+0.80%)`	⬆️
test/setting.c	`97.14% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d41d1c...45ea305. Read the comment docs.

fwsmit · 2021-07-06T22:29:15Z

Thank you for the PR

The scaling factor is derived from the active screen's dpi. This may
or may not match what is expected. Other sources could be font DPI or Gdk/WindowScalingFactor.

That could be an option. It's mostly that the screen dpi detection was already there. I believe advantages of screen dpi include (correct me if I'm wrong)

It can be updated on the fly
It's built into an (universal) X11 extension
It is automatically assigned (???)

I don't know about the other options, but I'm open to change it. Depending on which one we choose, an option in the dunst settings to overwrite the scale value may be useful.

Currently, only integer scaling is supported.

This reuses most of the code that was present for Wayland and the
change is limited to compute a scaling factor and putting the window
at the right position and at the right size. The code to compute the
window position in draw.c was only used for X11, so it was moved
directly in x.c.

Yes seems good to me.

I would also be interested to support fractional scaling. From my understanding, this is not possible with Wayland, right? For X11, we can and the change would mostly be replacing scale by scale*10 and divide it again by 10 everywhere.

Why not change scale to a float? I'm trying to think of disadvantages. I can come up with

Sizes may not end up at whole pixels. You can either round, making thinks potentially look not aligned. Or you can alias, which results in smooth edges.

The advantage of the scale*10 way is that when the scale is a multiple 10, everything will be exact, where as with a float the scale of 1 may still result in distortion (depending on if you round).

fwsmit · 2021-07-06T22:32:43Z

To explain the Wayland situation a bit more: the scale factor can only be an integer. The nearest integer above the scale factor is chosen, and then the image is scaled down by interpolation. I understand that non-integer scaling is very useful and also very possible for dunst, so let's look at the best way to implement it.

fwsmit

Looks good overall just a small change needed. If you want you can implement instant changing when the DPI changes by calling wake_up() when a dpi change event is emitted (note that there's a bug that icons still don't scale instantly).

src/x11/x.c

vincentbernat · 2021-07-07T04:51:50Z

❦ 6 July 2021 15:29 -07, fwSmit:

> The scaling factor is derived from the active screen's dpi. This may > or may not match what is expected. Other sources could be font DPI or > Gdk/WindowScalingFactor. That could be an option. It's mostly that the screen dpi detection was already there. I believe advantages of screen dpi include (correct me if I'm wrong) - It can be updated on the fly - It's built into an (universal) X11 extension - It is automatically assigned (???) I don't know about the other options, but I'm open to change it. Depending on which one we choose, an option in the dunst settings to overwrite the scale value may be useful.

I just mention this to document it somewhere. GTK applications are not reacting directy to XRandR DPI, the watch for XSETTINGS changes, but watching XRandR DPI is not unknown territory as Chromium does that too. I don't think we need to do something now. If the issue popup, you can search for it and find this PR ;-)

> I would also be interested to support fractional scaling. From my > understanding, this is not possible with Wayland, right? For X11, we > can and the change would mostly be replacing `scale` by `scale*10` > and divide it again by 10 everywhere. Why not change `scale` to a float? I'm trying to think of disadvantages. I can come up with - Sizes may not end up at whole pixels. You can either round, making thinks potentially look not aligned. Or you can alias, which results in smooth edges. The advantage of the `scale*10` way is that when the scale is a multiple 10, everything will be exact, where as with a float the scale of 1 may still result in distortion (depending on if you round).

Yes, that was the main reason. But we can also round everything and it should be fine. As it would handle 1.25x case better, I'll do that.

fwsmit · 2021-07-08T17:28:38Z

Yes, that was the main reason. But we can also round everything and it should be fine. As it would handle 1.25x case better, I'll do that.

Ok you can go ahead and make scale a float. Let's see what happens.

vincentbernat · 2021-07-08T18:26:06Z

Done. Tested on X11 with 1, 1.25, 1.5 and 2. No issue detected! This is quite a big change due to the need to add rounding everywhere. This is not strictly necessary but it may result as an off-by-one error if we don't do that. Maybe we could live with that if you want to reduce the changeset.

fwsmit · 2021-07-08T19:02:04Z

I've taken a quick look and it looks good. If I understand it correctly the + 0.5 is because C rounds toward zero, so yes that can stay.
Could you add a setting for overriding the scale? It should also work for wayland.

frebib · 2021-07-08T19:02:54Z

Just tested this and it seems to work a treat on X11 with 1.0x and 1.5x scale! Thanks so much

fwsmit

For me the notification became blurry from this patch and it looked like it was (half a) pixel below where it should be.

Before

After

Note also that a separator seems to appear in the bottom image, but I have the color set to my background color. It will probably be fixed when you address the comments

src/draw.c

src/wayland/wl.c

frebib · 2021-07-09T15:00:36Z

I can confirm that I also noticed the text aliasing at 1.0x scale at closer inspection with 66a7efa, but it appears to be back to "normal" with the latest revision (7048e74) and the scaling seems to still work as expected 👍🏻

src/x11/x.c

fwsmit · 2021-07-11T13:51:52Z

docs/dunst.5.pod

+=item B<scale> (default: 0)
+
+Specifies a scale factor for dimensions to adapt notifications to the
+screen DPI. This does not apply to text. If 0 is specified, the scale


Screen dpi or wayland scale factor

That's the source of auto-detection, not really the purpose. I have pushed another tentative to not mention DPI. Otherwise, please propose the whole text.

It's documentation for the dpi scaling at the same time :)
The text should also be scaled with the scale factor. If that's not the case then something is wrong.

I would suggest this as text:

Specifies a scale factor for dimensions to adapt notifications to HiDPI screens. This scales the notification geometry and it's contents. It is not recommended to use a fractional scaling factor, as this may result in things being one pixel off. Try to use a whole number scaling factor and adjust the font size and other sizes as needed. If 0 is specified, the scale factor is auto-detected.

Unfortunately, on X11, it does not scale text. Text is handled using Xft/DPI (from XSETTINGS, with a fallback to X resource database). I don't think this is very important, unless you use the scale settings, in this case, you don't control the whole scale.

Weird I thought we controlled text size directly. The scale variable is kind of useless on X11 then. On wayland it also doesn't work currently. It doesn't scale the window dimensions, so the notification is cut off.

We could make it an X11 only setting. Wayland does not really need it because there is only one way to get the scaling, so we are unlikely to be wrong. And since it does not scale text, we can just have a 3-state setting: integer-scaling, integer-scaling++, fractional-scaling. I am not good with names.

Yeah but if the text size doesn't change from this scale variable it's pretty much useless. Is there a way to draw larger text independent from the X dpi?

I don't know. But if there is, I wouldn't expect Dunst to override Xft/DPI (or Xft.DPI). Text scaling is pretty much the only thing that's easy to do on Linux without much surprise.

fwsmit · 2021-07-11T13:53:25Z

I will test it later today, but other then the little documentation thing it looks very good

fwsmit

The fractional scaling does work, but there are a couple of artifacts introduced by the rounding. It's fine to have those for a first effort, but I would like to keep the default to not use fractional scaling. So could you make x_get_scale only return rounded scale factors?

vincentbernat · 2021-07-11T17:41:23Z

What kind of artifacts? Having to force a fractional scaling through settings only means dunst has to be restarted when the DPI change (for example, when you connect external screens). Maybe I can add -1 to autodetect and keep fractional scaling?

vincentbernat · 2021-07-11T17:53:20Z

If the artifacts are due to the scaling factor using non fractional values, like 1.04 because user did not set Xft.DPI (which may be possible if you don't care about HiDPI), maybe we could also whitelist 1.5 and 1.25.

Linux is kind of a hell for people using HiDPI displays. The general mechanism is to get text size from Xft/DPI and interface scale from Gdk/WindowScalingFactor (which is not fractional) and to listen for changes on XSETTINGS. However, many apps fallback to various mechanisms, like monitor DPI settings or more usually Xft.DPI. So, the best we can do when using HiDPI display is to set XRandR DPI, Xft.DPI, Xft/DPI, and Gdk/WindowScalingFactor to consistent values. Most apps then just work fine or don't handle DPI at all. Having to mangle each app is cumbersome. That's why I would like to default to something working out of the box.

fwsmit · 2021-07-11T18:31:49Z

What kind of artifacts? Having to force a fractional scaling through settings only means dunst has to be restarted when the DPI change (for example, when you connect external screens). Maybe I can add -1 to autodetect and keep fractional scaling?

I've noticed two things:
The border is visible at the place of the separator. In the below pic I set the border to green and the separator to red to show the effect.

scale: 1.25x

Progress bar border isn't placed correctly. Note the space at the left:

scale: 1x

The second one is a regression, because the border was placed in a non-whole pixel pixel position. Cairo strokes are specified by the center of the stroke, so that's why a non-whole pixel position was needed.
Note that the border of the progress bar is drawn differently than the border of the notification. The progress bar border is drawn with a stroke and the other one is just a rounded rectangle.

All in all, it's a little more complicated than rounding everything. I'm currently feeling like we will be better off not rounding and letting anti-aliassing do it's work, but I'm curious what you think

If the artifacts are due to the scaling factor using non fractional values, like 1.04 because user did not set Xft.DPI (which may be possible if you don't care about HiDPI), maybe we could also whitelist 1.5 and 1.25.

I don't think it's a problem on 1.04 scale, but it is on 1.25 as seen above. Whitelisting certain values doesn't seem like a good solution to me. I think it's better to improve the scaling just a bit more.

Linux is kind of a hell for people using HiDPI displays. The general mechanism is to get text size from Xft/DPI and interface scale from Gdk/WindowScalingFactor (which is not fractional) and to listen for changes on XSETTINGS. However, many apps fallback to various mechanisms, like monitor DPI settings or more usually Xft.DPI. So, the best we can do when using HiDPI display is to set XRandR DPI, Xft.DPI, Xft/DPI, and Gdk/WindowScalingFactor to consistent values. Most apps then just work fine or don't handle DPI at all. Having to mangle each app is cumbersome. That's why I would like to default to something working out of the box.

Agreed, I'm trying to make dunst as good out of the box as possible. One should need to do no configuration to make it work well.

vincentbernat · 2021-07-11T20:18:57Z

I am not familiar with Cairo at all, so I don't know what other software do. It's a bit odd the regression at 1x, since nothing has really changed. We multiply by 1 or divide by 1. Maybe using a float instead of a double is the reason? Cairo is using double. Conversion from float to double may introduce this kind of things?

As for fractional scaling, that's mostly why Gnome was really reluctant to support it for anything else than text. But if you have a HiDPI screen at 1.5x scale, not scaling the interface does not give a very good result. That's why I think recent versions of Gnome started supporting fractional scaling.

I think that whitelisting known ratio (1.25 and 1.5) by default and using integer ratio otherwise would be a good compromise. Setup matching one of those two ratios have most likely configured their environment for it (by opposition to people using 1x scale with no Xft.DPI set and where dunst is using computed DPI from XRandR) and are willing to live with the little imperfections. This would make the setup work out of the box for most people in the way they like it. As long as we find the regression at 1x with Cairo.

fwsmit · 2021-07-11T21:04:52Z

We could round the scale to multiples of 0.25. But again, I haven't tested what happens at scales like 1.04, so let's test that first.

I've explained the regression for 1x above already. It has to do with the line needing to be on a non-whole pixel for certain line widths. If the line width is 3 for example, you have to specify the location of the line by it's middle. So if the left of the line has to be on pixel 40, the middle is on pixel 41.5.

vincentbernat · 2021-07-12T05:44:02Z

What I don't understand is that it shouldn't be a regression as we didn't change that. We still pass the same values as before to Cairo, except instead of being int, they are floats (but rounded). From what I understand, the problem should already be present in master?

fwsmit · 2021-07-12T08:41:22Z

src/draw.c

-                // TODO draw_rect instead of cairo_rectangle resulted in blurry lines. Why?
-                cairo_rectangle(c, (frame_x + half_frame_width) * scale, (frame_y + half_frame_width) * scale, (progress_width - frame_width) * scale, progress_height * scale);
-                cairo_set_line_width(c, frame_width * scale);
+                draw_rect(c,


The regression is here. Before it was using floats directly to cairo_rectangle

Oh, good catch! Should we convert draw_rect to floats?

I have converted it to double, but this is not enough because of rounding.

Unrelated, but I also noticed I have used float while most of the existing code and Cairo are using double. So, I have converted everything to double (and use of round() introduced a conversion to double as well).

I have converted it to double, but this is not enough because of rounding.

Yeah it's better then to leave the old cairo_rectangle line that was there. You can go ahead and leave the TODO out though.

Unrelated, but I also noticed I have used float while most of the existing code and Cairo are using double. So, I have converted everything to double (and use of round() introduced a conversion to double as well).

👍

vincentbernat · 2021-08-10T07:15:51Z

Sorry, I am a bit low on time currently. Dunno exactly when I will get more time.

fwsmit · 2021-08-10T07:23:21Z

Okay, no problem. Maybe someone else wants to finish it, or I can do it some time.

vincentbernat · 2021-08-10T07:26:02Z

Let me do it quickly.

The scaling factor is derived from the active screen's dpi. This may or may not match what is expected. Other sources could be font DPI or Gdk/WindowScalingFactor. Currently, only integer scaling is supported. This reuses most of the code that was present for Wayland and the change is limited to compute a scaling factor and putting the window at the right position and at the right size. The code to compute the window position in `draw.c` was only used for X11, so it was moved directly in `x.c`.

On X11, we can support fractional scaling. Let's convert `scale` to a float number and round it automatically on multiplication. When scale is used, we `round()` the result.

vincentbernat · 2021-08-10T07:40:15Z

OK, it's rebased.

vincentbernat · 2021-08-10T07:46:03Z

Hold on, it's crashing, I didn't test correctly...

vincentbernat · 2021-08-10T07:49:56Z

OK, all good for me.

vincentbernat · 2021-08-10T08:03:04Z

And I have added tests too.

fwsmit

Thank you for the quick reply and fix! It works very well for me now. There's two things remaining as far as I can tell:

The scale variable doesn't change the window size in wayland. I would suggest making scale a X11 only option, since sway on wayland has a better way to set scale anyways.
The text size on X11 doesn't change with a change in scale. Is that expected behaviour? It's not a dealbreaker for me, so it can be merged with this quirk.

vincentbernat · 2021-08-10T11:25:05Z

❦ 10 August 2021 03:01 -07, fwSmit:

- The scale variable doesn't change the window size in wayland. I would suggest making `scale` a X11 only option, since sway on wayland has a better way to set scale anyways.

Is it only a matter of adding a "(X11 only)" in the description?

- The text size on X11 doesn't change with a change in `scale`. Is that expected behaviour? It's not a dealbreaker for me, so it can be merged with this quirk.

Yes, this is expected. We discussed a bit about this, and that's how scaling generally work for X11. I don't think it will surprise most people. And people should just use `scale=0` since it will match the size of the text.

fwsmit · 2021-08-10T11:38:17Z

Is it only a matter of adding a "(X11 only)" in the description?

Yes and removing the setting.scale from wl_get_scale().

Yes, this is expected. We discussed a bit about this, and that's how scaling generally work for X11. I don't think it will surprise most people. And people should just use scale=0 since it will match the size of the text.

Okay that's fine by me. If people get confused we can always add a note in the documentation

vincentbernat · 2021-08-10T11:49:49Z

OK, updated.

fwsmit

Thank you, I only have one small comment

fwsmit · 2021-08-10T21:34:31Z

docs/dunst.5.pod

@@ -106,6 +106,15 @@ the notification on the left border of the screen while a horizontal offset of

 =back

+=item B<scale> (default: 0)


Could you add (X11 only) here as well?

The default value is 0 and it makes dunst auto-detect the value from screen DPI. X11 only.

fwsmit · 2021-08-11T06:40:49Z

Thank you for your work!

svenstaro · 2021-09-06T11:25:15Z

A release with this would be great! :)

fwsmit · 2021-09-06T13:36:48Z

I agree a release is due. What do you think @tsipinakis?

fwsmit requested changes Jul 6, 2021

View reviewed changes

src/x11/x.c Show resolved Hide resolved

src/x11/x.c Outdated Show resolved Hide resolved

vincentbernat requested a review from fwsmit July 8, 2021 16:17

fwsmit reviewed Jul 8, 2021

View reviewed changes

src/draw.c Outdated Show resolved Hide resolved

src/draw.c Show resolved Hide resolved

src/draw.c Outdated Show resolved Hide resolved

src/wayland/wl.c Outdated Show resolved Hide resolved

vincentbernat force-pushed the fix/x11-scaling branch 2 times, most recently from 3d91183 to 071e4f9 Compare July 8, 2021 21:32

vincentbernat requested a review from fwsmit July 8, 2021 21:34

vincentbernat force-pushed the fix/x11-scaling branch from 071e4f9 to 7048e74 Compare July 8, 2021 21:36

fwsmit reviewed Jul 11, 2021

View reviewed changes

src/x11/x.c Show resolved Hide resolved

fwsmit reviewed Jul 11, 2021

View reviewed changes

vincentbernat force-pushed the fix/x11-scaling branch from e58e1c0 to edb7f70 Compare July 11, 2021 14:27

fwsmit reviewed Jul 11, 2021

View reviewed changes

vincentbernat force-pushed the fix/x11-scaling branch from edb7f70 to f01c134 Compare July 11, 2021 17:36

fwsmit reviewed Jul 12, 2021

View reviewed changes

vincentbernat closed this Aug 10, 2021

vincentbernat reopened this Aug 10, 2021

vincentbernat force-pushed the fix/x11-scaling branch from 19adc75 to 9c72d0e Compare August 10, 2021 07:25

fwsmit and others added 2 commits August 10, 2021 09:26

X11: Support for fractional scaling

f12cc85

On X11, we can support fractional scaling. Let's convert `scale` to a float number and round it automatically on multiplication. When scale is used, we `round()` the result.

vincentbernat force-pushed the fix/x11-scaling branch from 9c72d0e to 77f0f0e Compare August 10, 2021 07:39

vincentbernat force-pushed the fix/x11-scaling branch from 77f0f0e to 996890f Compare August 10, 2021 07:48

vincentbernat force-pushed the fix/x11-scaling branch from 996890f to ea6d042 Compare August 10, 2021 08:02

vincentbernat force-pushed the fix/x11-scaling branch from ea6d042 to 8d358a8 Compare August 10, 2021 08:08

fwsmit reviewed Aug 10, 2021

View reviewed changes

vincentbernat force-pushed the fix/x11-scaling branch from 8d358a8 to 611796a Compare August 10, 2021 11:49

fwsmit approved these changes Aug 10, 2021

View reviewed changes

settings: allow to specify scale factor in settings

45ea305

The default value is 0 and it makes dunst auto-detect the value from screen DPI. X11 only.

vincentbernat force-pushed the fix/x11-scaling branch from 611796a to 45ea305 Compare August 11, 2021 06:36

fwsmit merged commit 97b48b1 into dunst-project:master Aug 11, 2021

fwsmit mentioned this pull request Nov 3, 2021

Provide an option to apply Xorg scaling factor to the window geometry? #754

Closed

fwsmit mentioned this pull request Nov 14, 2021

Notifications are really big despite no conifg changes #976

Closed

		@@ -106,6 +106,15 @@ the notification on the left border of the screen while a horizontal offset of

		=back

		=item B<scale> (default: 0)

X11: enable dpi scaling #890

X11: enable dpi scaling #890

Conversation

vincentbernat commented Jul 5, 2021

frebib commented Jul 6, 2021

codecov-commenter commented Jul 6, 2021 • edited Loading

Codecov Report

fwsmit commented Jul 6, 2021

fwsmit commented Jul 6, 2021

fwsmit left a comment

Choose a reason for hiding this comment

vincentbernat commented Jul 7, 2021 via email

fwsmit commented Jul 8, 2021

vincentbernat commented Jul 8, 2021

fwsmit commented Jul 8, 2021

frebib commented Jul 8, 2021

fwsmit left a comment • edited Loading

Choose a reason for hiding this comment

frebib commented Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwsmit commented Jul 11, 2021

fwsmit left a comment

Choose a reason for hiding this comment

vincentbernat commented Jul 11, 2021

vincentbernat commented Jul 11, 2021

fwsmit commented Jul 11, 2021

vincentbernat commented Jul 11, 2021

fwsmit commented Jul 11, 2021

vincentbernat commented Jul 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentbernat commented Aug 10, 2021

fwsmit commented Aug 10, 2021

vincentbernat commented Aug 10, 2021

vincentbernat commented Aug 10, 2021

vincentbernat commented Aug 10, 2021

vincentbernat commented Aug 10, 2021

vincentbernat commented Aug 10, 2021

fwsmit left a comment

Choose a reason for hiding this comment

vincentbernat commented Aug 10, 2021 via email

fwsmit commented Aug 10, 2021

vincentbernat commented Aug 10, 2021

fwsmit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwsmit commented Aug 11, 2021

svenstaro commented Sep 6, 2021

fwsmit commented Sep 6, 2021

codecov-commenter commented Jul 6, 2021 •

edited

Loading

fwsmit left a comment •

edited

Loading

frebib commented Jul 9, 2021 •

edited

Loading