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 multiple pause levels #1193

Merged
merged 13 commits into from
Nov 9, 2023
Merged

Conversation

matejdro
Copy link
Contributor

@matejdro matejdro commented Aug 16, 2023

This fixes #865, specifically by introducing multiple pause levels (as outlined in #865 (comment)).

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

fwsmit commented Sep 6, 2023

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.

dunstctl Outdated Show resolved Hide resolved
src/dbus.c Outdated Show resolved Hide resolved
@fwsmit
Copy link
Member

fwsmit commented Sep 6, 2023

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

@matejdro matejdro marked this pull request as ready for review September 27, 2023 06:59
@matejdro
Copy link
Contributor Author

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

@roobre
Copy link

roobre commented Sep 30, 2023

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! ❤️

Copy link
Member

@fwsmit fwsmit left a comment

Choose a reason for hiding this comment

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

Thanks for your improvements! The code looks good, but there are a few small documentation and naming improvements needed before this can be merged. Could you add an entry for override_pause_level in the man page as well?

dunstrc Outdated Show resolved Hide resolved
dunstctl Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2023

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%.

❗ 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%) ⬆️

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%) ⬆️
src/queues.c 92.04% <100.00%> (+0.02%) ⬆️
src/rules.c 80.68% <100.00%> (+0.27%) ⬆️
test/dunst.c 100.00% <100.00%> (ø)
test/queues.c 99.01% <100.00%> (+0.03%) ⬆️
test/dbus.c 97.89% <92.30%> (+0.08%) ⬆️
src/dbus.c 56.92% <71.42%> (+0.26%) ⬆️
src/dunst.c 12.29% <40.00%> (+1.55%) ⬆️

... and 1 file with indirect coverage changes

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

@matejdro matejdro requested a review from fwsmit October 20, 2023 17:07
docs/dunst.5.pod Outdated Show resolved Hide resolved
@fwsmit
Copy link
Member

fwsmit commented Nov 9, 2023

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

@fwsmit fwsmit merged commit c867291 into dunst-project:master Nov 9, 2023
18 checks passed
@liskin
Copy link
Contributor

liskin commented Nov 9, 2023

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: #1230

liskin added a commit to liskin/dotfiles that referenced this pull request Nov 9, 2023
@matejdro matejdro deleted the pause-levels branch November 9, 2023 18:48
@igorline
Copy link

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

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

@igorline
Copy link

yeah, tried various combinations and still get notifications bypassing

@matejdro
Copy link
Contributor Author

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

@igorline
Copy link

I installed it from master branch

@matejdro
Copy link
Contributor Author

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?

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.

Feature Request: Do Not Disturb mode
6 participants