dunst-project / dunst

Lightweight and customizable notification daemon
https://dunst-project.org
Other
4.56k stars 340 forks source link

Refactor draw code to use `cairo_surface_set_device_scale` #1337

Open bynect opened 5 months ago

bynect commented 5 months ago

This fixes something that has been bugging me for a while. Basically now dunst is scaling itself when drawing every single shape and text (with cairo and pango). With this patch we render everything at a uniform scaling factor of 1 and scale everything afterwards using cairo_surface_set_device_scale.

This should have some advantages (apart from conciseness), like making scaling on wayland possible (like I discussed with @alebastr under #1316)

NOTE: Everything works on Xorg, however I can't test it on Wayland and my display is not Hi-Dpi, so I am not sure if the pango dpi scaling is being applied correctly. If someone can test it it would be great 👍🏻

bynect commented 5 months ago

I think that all the get_iconwidth and get*scale*icon function aren't needed anymore

bynect commented 5 months ago

I have no clue about why the test are failing 😢

bynect commented 5 months ago

I am trying to debug the use of uninitialized variables but even with libAsan I can't find it

bynect commented 5 months ago

Finally figured it out. Basically I forgot that the test didn't call draw_setup. Since I moved the initialization of the PangoContext in draw_setup it was undefined and thus buggy, but only in tests.

codecov-commenter commented 5 months ago

Codecov Report

Attention: Patch coverage is 57.52212% with 48 lines in your changes are missing coverage. Please review.

Project coverage is 65.85%. Comparing base (4384521) to head (b0620ce). Report is 15 commits behind head on master.

Files Patch % Lines
src/draw.c 57.14% 24 Missing :warning:
src/wayland/wl.c 0.00% 14 Missing :warning:
src/wayland/wl_seat.c 0.00% 4 Missing :warning:
src/x11/x.c 0.00% 3 Missing :warning:
test/draw.c 82.35% 3 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1337 +/- ## ========================================== - Coverage 65.92% 65.85% -0.08% ========================================== Files 50 50 Lines 8070 8217 +147 Branches 925 961 +36 ========================================== + Hits 5320 5411 +91 - Misses 2750 2806 +56 ``` | [Flag](https://app.codecov.io/gh/dunst-project/dunst/pull/1337/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/dunst-project/dunst/pull/1337/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project) | `65.85% <57.52%> (-0.08%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bynect commented 5 months ago

Everything things to work. I think it is ready for merge.

Things we may want to consider

bynect commented 5 months ago

@alebastr Where do you think should we place scale code so that the wayland backend can correctly get the scale needed? (to solve #1316)

Note that the pr is still not quite ready

matejdro commented 3 months ago

If someone can test it it would be great 👍🏻

I can test it on Wayland if desired, just give me instructions on what to look for

bynect commented 3 months ago

If someone can test it it would be great 👍🏻

I can test it on Wayland if desired, just give me instructions on what to look for

it would be great if you could check if the input works correctly (mouse clicks for closing etc) and if you notice any graphical discrepancy let me know. The only difference I noticed is that text is a bit less sharp but maybe it's just me?

matejdro commented 2 months ago

Only issue I have noticed is that, with scaling enabled, dunst window has bigger horizontal offset from the edge of the screen and the window is smaller. All below screenshots extend to the top right corner of the monitor.

This branch

Scaled monitor:

image

Interestingly enough, after I take a screenshot and Satty opens fullscreen, dunst window will become taller (even before Satty makes second notification):

image

Non-scaled monitor (still suffering from #1316):

image

After I turn off scaling on the other monitor, text is clearer for the non-scaled monitor and position bug disappears:

image

Dunst release

Scaled monitor:

image

Non-scaled monitor:

image

After I turn off scaling on the other monitor, text is clearer:

image

Mouse clicks seem to work fine. I also don't see any discrepancies regarding text quality between this branch and latest dunst release (but you can judge yourself from screenshots). Both monitors are 2560x1440, with 2x scaling on one of the monitors (where specified).

Compositor:

Hyprland, built from branch  at commit 918d8340afd652b011b937d29d5eea0be08467f5  (flake.lock: update).
Date: Tue Jun 25 12:06:02 2024
Tag: v0.41.2, commits: 4886
bynect commented 2 months ago

@matejdro so is the resolution still wrong even with this branch?

Also I can look into the offset problem, probably something was not scaled appropriately

matejdro commented 2 months ago

Above screenshots are from Hyprland.

On Sway, text look as bad on this branch as with the release:

image

But interestingly enough, on Hyprland, issue appears to be different. As you can see on the screenshots from the previous post, text is still worse when scaling is enabled, but it's not as jagged as on Sway. Maybe Hyprland applies some sort of font smoothing by default, not sure. This is also the case with release version of dunst, so no changes here.

bynect commented 2 months ago

Thanks very much for the feedback. I will investigate this further and make some changes 👍

bynect commented 2 months ago

@matejdro just to let you know. the problem with wayland scaling is that we check for the screen before drawing but on wayland the protocol does not allow that with follow mode. so further changes are needed to this pr for it to work on wayland. maybe it should be done in a seperate pr