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

Implement dynamic height and change vertical padding #1342

Merged
merged 10 commits into from
May 6, 2024

Conversation

bynect
Copy link
Member

@bynect bynect commented Apr 20, 2024

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:

  • implement dynamic height (min/max)
  • generalize implementation of vertical alignment to work without icon and with hide_text
  • add functional-test for the changes
  • add a test suite for min height
  • refactored functional-tests and added them to the makefile
  • added height to the docs and fixed some indentation on them

@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

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

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

Files with missing lines Patch % Lines
src/draw.c 42.59% 31 Missing ⚠️
src/settings.c 50.00% 2 Missing ⚠️

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

Additional details and impacted files
@@            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 Coverage Δ
unittests 66.08% <66.66%> (+0.13%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bynect
Copy link
Member Author

bynect commented Apr 20, 2024

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

@bynect bynect requested review from fwsmit and zappolowski April 20, 2024 14:36
@bynect
Copy link
Member Author

bynect commented Apr 20, 2024

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

@bynect
Copy link
Member Author

bynect commented Apr 21, 2024

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
Copy link
Member Author

bynect commented Apr 24, 2024

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

@bynect
Copy link
Member Author

bynect commented Apr 29, 2024

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

@fwsmit could you test this

@fwsmit
Copy link
Member

fwsmit commented Apr 30, 2024

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

@bynect
Copy link
Member Author

bynect commented Apr 30, 2024

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

Copy link
Member

@zappolowski zappolowski left a comment

Choose a reason for hiding this comment

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

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

docs/dunst.5.pod Show resolved Hide resolved
@bynect
Copy link
Member Author

bynect commented May 1, 2024

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
Copy link
Member

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
Copy link
Member Author

bynect commented May 1, 2024

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 bynect force-pushed the dynamic-height branch from 8225c09 to 4ac7596 Compare May 1, 2024 22:22
@bynect
Copy link
Member Author

bynect commented May 1, 2024

Ok I should have fixed the problem above

img-1714602201

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

@bynect
Copy link
Member Author

bynect commented May 3, 2024

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
Copy link
Member

fwsmit commented May 6, 2024

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
Copy link
Member Author

bynect commented May 6, 2024

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
Copy link
Member Author

bynect commented May 6, 2024

since this seems to work well I'll merge

@bynect bynect merged commit 20033b8 into dunst-project:master May 6, 2024
21 checks passed
Krismix1 added a commit to Krismix1/dotfiles that referenced this pull request Dec 10, 2024
https://dunst-project.org/changelog/
BREAKING: Implement dynamic height (changes the the height setting, see
manual) (dunst-project/dunst#1342)
rarescosma added a commit to rarescosma/env.dotfiles that referenced this pull request Dec 15, 2024
reyniersbram added a commit to reyniersbram/dotfiles that referenced this pull request Dec 16, 2024
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.

4 participants