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

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 ⚠️
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
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


Examples:

height = 300 # constant height of 300
Copy link
Member

Choose a reason for hiding this comment

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

As this is the old default value, I assume that a lot of people will have it in their configuration as well and thus will end up with a weird looking notification. This needs to be communicated very explicitly.

Copy link
Member Author

Choose a reason for hiding this comment

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

before the height was dynamic by default, but now we can set a minimum. what's the best way to communicate this? maybe a log info in the check settings?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not really sure about the best approach here. Maybe an entry in the CHANGELOG marking it as breaking change suffices?

@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
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
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