dunst-project / dunst

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

Implement dynamic height and change vertical padding #1342

Closed bynect closed 2 months ago

bynect commented 2 months ago

Implement dynamic height for notification, namely a way to define a minimum height.

I also changed the behavior of vertical padding to work even when there is no icon.


Summary:

codecov-commenter commented 2 months ago

Codecov Report

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

Project coverage is 66.08%. Comparing base (a6e1d4e) to head (2b407f2). Report is 9 commits behind head on master.

Files Patch % Lines
src/draw.c 42.59% 31 Missing :warning:
src/settings.c 50.00% 2 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 #1342 +/- ## ========================================== + Coverage 65.95% 66.08% +0.13% ========================================== Files 50 50 Lines 8209 8247 +38 Branches 962 958 -4 ========================================== + Hits 5414 5450 +36 - Misses 2795 2797 +2 ``` | [Flag](https://app.codecov.io/gh/dunst-project/dunst/pull/1342/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/1342/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project) | `66.08% <66.66%> (+0.13%)` | :arrow_up: | 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 2 months ago

Everything seems to work. Maybe I will add some functional tests

bynect commented 2 months ago

I noticed that there are some differences from the old behavior so it's not ready yet

bynect commented 2 months ago

Everything is working fine now. I also added test and functional tests.

For a quick check on what this implements use ./test.sh vertical_align

I also noticed some indentation error in the docs and fixed (sorry for not making another pr :d)

bynect commented 2 months ago

I think it's best to merge this before #1337

bynect commented 2 months ago

For a quick check on what this implements use ./test.sh vertical_align

@fwsmit could you test this

fwsmit commented 2 months ago

Sorry, I cannot test at the moment. Maybe someone else can do it?

bynect commented 2 months ago

For a quick check on what this implements use ./test.sh vertical_align

@fwsmit could you test this

@zappolowski could you see this if you have the time

bynect commented 2 months ago

Code seems good, but the notifications look different than current master with too much padding on the top and too little at the bottom.

Current master: old

This branch: new

I'm not sure this is the intended behavior (except if vertical alignment is bottom I guess) so I will check again. did you test with the default dunstrc?

zappolowski commented 2 months ago

did you test with the default dunstrc?

I've tested it with the built-in default config (./dunst -conf /dev/null) but it looks the same as using the repository's dunstrc (I didn't check whether the built-in defaults and the one from dunstrc differ).

bynect commented 2 months ago

Now it should work as intended.

The only corner case that I am not sure how to handle is when the max height is too small to fit everything

initially it was img-1714600820

then I made it overlap but not go outside the border img-1714600854

but this is just an hack. the real problem is that there is not enough room in this case. I would leave it as it this...

bynect commented 2 months ago

Ok I should have fixed the problem above

img-1714602201

edit: just for reference this is the old behavior pre-pr img-1714603435

bynect commented 2 months ago

note for the future: maybe we could save the output of functional tests and compare it automatically in the ci to see if we break the drawing (which is not very covered)

fwsmit commented 2 months ago

How would you compare the output of the functional tests? Pixel for pixel comparison of the images might not work so well, because the font rendering might change slightly with different versions of pango or a font

bynect commented 2 months ago

How would you compare the output of the functional tests? Pixel for pixel comparison of the images might not work so well, because the font rendering might change slightly with different versions of pango or a font

I didn't think about that. Maybe with specific pango and cairo versions

bynect commented 2 months ago

since this seems to work well I'll merge