dunst-project / dunst

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

[RFC] DBus add NotificationListDisplayed #1348

Open zappolowski opened 5 months ago

zappolowski commented 5 months ago

A possible implementation to fix #1242.

This way one could e.g. use (fish syntax):

for id in (busctl --user call org.freedesktop.Notifications /org/freedesktop/Notifications org.dunstproject.cmd0 NotificationListDisplayed -j | jq -r '.data[][] | select(.appname.data == "Spotify") | .id.data')
    busctl --user call org.freedesktop.Notifications /org/freedesktop/Notifications org.freedesktop.Notifications CloseNotification u $id
end

To close all currently displayed notifications generated by Spotify.

TODO (if accepted)

codecov-commenter commented 5 months ago

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

Codecov Report

Attention: Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 65.93%. Comparing base (f556044) to head (3b29cae). Report is 51 commits behind head on master.

Files with missing lines Patch % Lines
src/dbus.c 62.50% 3 Missing :warning:

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1348 +/- ## ========================================== - Coverage 65.95% 65.93% -0.02% ========================================== Files 50 50 Lines 8209 8213 +4 Branches 962 962 ========================================== + Hits 5414 5415 +1 - Misses 2795 2798 +3 ``` | [Flag](https://app.codecov.io/gh/dunst-project/dunst/pull/1348/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/dunst-project/dunst/pull/1348/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project) | `65.93% <62.50%> (-0.02%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bynect commented 5 months ago

I like the idea.

I was wondering if there was a nice way to call closeNotification from dunstctl or the like. Maybe we can add an optional argument to dunstctl close?

edit: I actually made it in #1351

bynect commented 5 months ago

I suggest to implement a more generic NotificationList that takes a bool parameter for displayed. This way we can query all the current notifications (otherwise the one neither displayed nor in history would be inaccessible). this would allow for example autocompletion of dunstctl close with the id (and surely other nice things).

zappolowski commented 5 months ago

I don't think, that NotificationList bool would make good API as the meaning of the parameter is not immediately clear. Given that NotificationListHistory already exists and this one adds NotificationListDisplayed adding another NotificationListQueued or NotificationListWaiting seems preferable from my side.

Maybe for the future one could think about implementing NotificationList [waiting/queued|displayed|history], but at the moment NotificationListHistory is already published and thus it might be used. Changing it now would be a breaking change.

bynect commented 5 months ago

I don't think, that NotificationList bool would make good API as the meaning of the parameter is not immediately clear.

Well the parameter name can be specified in the xml. Maybe for a clearer name something like NotificationListCurrent to differentiate from the history one?

But creating another method is alright. So we would just need to combine the two where needed

zappolowski commented 5 months ago

Maybe for a clearer name something like NotificationListCurrent to differentiate from the history one?

TBH, I don't get this proposal. NotificationListDisplayed states that it's listing notifications displayed. For NotificationListCurrent I would expect the union of displayed and waiting notifications being returned.

I also don't understand why there would be a need to differentiate from NotificationListHistory. We then would end up with NotificationListDisplayed, NotificationListWaiting, and NotificationListHistory, which form a kind of self-describing API stating what they're about to return.

But creating another method is alright. So we would just need to combine the two where needed

For both use cases described in #1242 and #1346 just the displayed notifications are of interest and thus NotificationListWaiting (to use the term used in the code already) would not be strictly needed (though, I'd add it anyhow as it's just a couple of lines).

bynect commented 5 months ago

But creating another method is alright

It was just an idea. You can go ahead with displayed and waiting as you said

fwsmit commented 3 months ago

NotificationListWaiting would be useful if you want to have shell completion for dunstctl close, since I believe queued notifications can already be closed