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

Add progress bar #775

Merged
merged 2 commits into from
Nov 10, 2020
Merged

Add progress bar #775

merged 2 commits into from
Nov 10, 2020

Conversation

fwsmit
Copy link
Member

@fwsmit fwsmit commented Nov 3, 2020

This PR addresses #772. It add a progress bar at the bottom of the notification when a value is passed via a dbus hint. Settings have been added to turn off the progress bar. The old behaviour of showing a percentage in text can still be used the same way and even in conjunction with the progress bar.
A highlight colour is added and used for drawing the progress bar.

  • Bug: Icons larger than max_icon_size don't get drawn
  • Add more configuration variables
  • Make sure the border is pixel-aligned to avoid anti-aliassing
  • Let the progress bar define the width of the notification as well
  • Add tests
  • Documentation

This is what it currently looks like
notification-dunst-V2
notification-dunst-V2 2

Example usage

notify-send -h int:value:10 "Brightness" 

@fwsmit fwsmit marked this pull request as draft November 3, 2020 12:50
@codecov-io
Copy link

codecov-io commented Nov 5, 2020

Codecov Report

Merging #775 (4f10496) into master (93b52df) will decrease coverage by 0.41%.
The diff coverage is 25.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #775      +/-   ##
==========================================
- Coverage   65.94%   65.53%   -0.42%     
==========================================
  Files          29       29              
  Lines        5162     5225      +63     
==========================================
+ Hits         3404     3424      +20     
- Misses       1758     1801      +43     
Flag Coverage Δ
unittests 65.53% <25.71%> (-0.42%) ⬇️

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

Impacted Files Coverage Δ
src/draw.c 0.00% <0.00%> (ø)
src/rules.c 49.25% <33.33%> (-0.75%) ⬇️
src/notification.c 64.55% <75.00%> (+0.12%) ⬆️
src/settings.c 59.29% <77.77%> (+1.24%) ⬆️
test/dbus.c 99.11% <0.00%> (+0.44%) ⬆️

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 93b52df...4f10496. Read the comment docs.

@fwsmit fwsmit marked this pull request as ready for review November 6, 2020 13:41
Copy link
Member

@tsipinakis tsipinakis left a comment

Choose a reason for hiding this comment

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

Some code style nitpicks and you also forgot to add the new settings to the example dunstrc.

This PR was a breeze to review and couldn't even find any breaking bugs. Huge kudos! :)

src/draw.c Outdated Show resolved Hide resolved
test/functional-tests/dunstrc.progress_bar Outdated Show resolved Hide resolved
test/functional-tests/test.sh Outdated Show resolved Hide resolved
src/draw.c Show resolved Hide resolved
docs/dunst.pod Outdated Show resolved Hide resolved
@fwsmit fwsmit force-pushed the progress-bar branch 2 times, most recently from d82c8e3 to 8fa3c65 Compare November 8, 2020 14:07
@fwsmit
Copy link
Member Author

fwsmit commented Nov 8, 2020

Thank you for the review. I have addressed everything. The tests are failing, because progress values greater than 100 are being changed to 100. Is there a reason why it's better not to change them to 100?

@tsipinakis
Copy link
Member

Is there a reason why it's better not to change them to 100?

There can be cases where the progress can be >100%, think for example volume levels with pulseaudio that can go to 150+. So it's best to handle it here so %p is still accurate.

@fwsmit
Copy link
Member Author

fwsmit commented Nov 8, 2020

Good point. I have applied your suggestion

src/settings.c Outdated

settings.progress_bar = option_get_bool(
"global",
"progress_bar", "-progress_bar", false,
Copy link
Member

Choose a reason for hiding this comment

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

You can set this to true as well, the goal is to have this and dunstrc in sync at some point not make it diverge more :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I forgot that one. I changed it now

@tsipinakis tsipinakis merged commit 3d3ba4b into dunst-project:master Nov 10, 2020
@tsipinakis
Copy link
Member

Merged! Thanks for your work.

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.

3 participants