dunst-project / dunst

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

Add multiple pause levels #1193

Closed matejdro closed 8 months ago

matejdro commented 11 months ago

This fixes #865, specifically by introducing multiple pause levels (as outlined in https://github.com/dunst-project/dunst/issues/865#issuecomment-1378273947).

Instead of pause being a toggle, it is now a number and notification rules can set override_pause_level which determines maximum pause level at which the notification is displayed.

Old is-paused and set-paused commands are deprecated, but they still function to maintain backwards compatibility (set-paused true sets paused level to 100, which is an arbitrary number, we could pick any other if desired).

PR is not finished yet, I still need to update tests, documentation, scripts etc. but I only edited the core for now to get feedback on my approach. Does this look like something we would want in dunst?

fwsmit commented 10 months ago

Your suggestion for adding a pause level seems like a good approach. I would be careful with adding too many pause levels, as it could make it overwhelming for people. I'd suggest adding a maximum of 10 pause levels, since I cannot think of a situation that requires more pause levels. The limit could always be raised later.

fwsmit commented 10 months ago

Your approach is good and the implementation seems mostly good. Could take a look at the build errors, my comments and the documentation?

matejdro commented 10 months ago

Thanks for the review. Added several commits which should address things.

roobre commented 9 months ago

Hi there! Just wanted drop by and thank you both for your work on this feature! My search engine led me here when searching how to mute/pause certain notifications only.

I'm runing dunst compiled from this branch, with two simple set-pause-level schedules using systemd user timers and so far it's working great.

If I could make a small suggestion, from the example dunstrc config it was not immediately clear to me that override_pause_level can (and probably should?) go inside rules. Perhaps it might be worth adding it to the list of things that can be overridden in the default config file. The new option is also not present in the man page.

Thanks again for your work on this! ❤️

codecov-commenter commented 9 months ago

Codecov Report

Merging #1193 (68f7f22) into master (7619e59) will increase coverage by 0.23%. Report is 22 commits behind head on master. The diff coverage is 85.71%.

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

@@            Coverage Diff             @@
##           master    #1193      +/-   ##
==========================================
+ Coverage   66.03%   66.26%   +0.23%     
==========================================
  Files          46       46              
  Lines        7595     7656      +61     
==========================================
+ Hits         5015     5073      +58     
- Misses       2580     2583       +3     
Flag Coverage Δ
unittests 66.26% <85.71%> (+0.23%) :arrow_up:

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

Files Coverage Δ
src/notification.c 60.70% <100.00%> (+0.09%) :arrow_up:
src/queues.c 92.04% <100.00%> (+0.02%) :arrow_up:
src/rules.c 80.68% <100.00%> (+0.27%) :arrow_up:
test/dunst.c 100.00% <100.00%> (ø)
test/queues.c 99.01% <100.00%> (+0.03%) :arrow_up:
test/dbus.c 97.89% <92.30%> (+0.08%) :arrow_up:
src/dbus.c 56.92% <71.42%> (+0.26%) :arrow_up:
src/dunst.c 12.29% <40.00%> (+1.55%) :arrow_up:

... and 1 file with indirect coverage changes

:mega: Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

fwsmit commented 8 months ago

Thank you for this new feature. I will merge this as soon as the tests pass

liskin commented 8 months ago

Wow, thanks a lot for implementing this! I wanted this for a long time, just never managed to find the time to do it myself. Tested it now, seems to work well, just needs a little fix for a bashism in dunstctl: https://github.com/dunst-project/dunst/pull/1230

igorline commented 5 months ago

Cannot get this to work properly

have this in my dunstrc now

[Spotify]
    appname = Spotify
    background = "#FF0000"
    override_pause_level = 30

when I try to set pause level to 100 messages still go through with expected red background

dunstctl set-pause-level 100

matejdro commented 5 months ago

Huh. If you remove override pause level, do messages still go through?

igorline commented 5 months ago

yeah, tried various combinations and still get notifications bypassing

matejdro commented 5 months ago

Which version do you have? This was just released in Dunst v10.0.

igorline commented 5 months ago

I installed it from master branch

matejdro commented 5 months ago

Are you setting override pause level to a higher value somewhere else (default configuration)?

I'm actually not sure what would be a good way to debug this. Maybe we should have added some more LOG_D statements for this?