[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
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

[RFC] DBus add NotificationListDisplayed #1348

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

zappolowski
Copy link
Member
@zappolowski zappolowski commented May 1, 2024

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)

  • add tests
  • add subcommand to dunstctl
  • update completions if needed

This will be removed for the displayed queue in the future.
This is analog to the already present `NotificationListHistory` and
allows e.g. for selectively closing some notifications.
@codecov-commenter
Copy link
codecov-commenter commented May 1, 2024

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

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

Additional details and impacted files
@@            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 Coverage Δ
unittests 65.93% <62.50%> (-0.02%) ⬇️

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
bynect commented May 1, 2024

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
Copy link
Member
bynect commented May 2, 2024

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

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
Copy link
Member
bynect commented May 3, 2024

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

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
Copy link
Member
bynect commented May 3, 2024

But creating another method is alright

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

@fwsmit
Copy link
Member
fwsmit commented Jul 2, 2024

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

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: dunstctl close notifications based on conditions
4 participants