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

dbus/dunstctl: add history command (#307) #970

Merged
merged 13 commits into from
Nov 29, 2021
Merged

Conversation

nikp123
Copy link
Contributor

@nikp123 nikp123 commented Nov 9, 2021

The current implementation tries to retrieve notifications stored in the
history buffer (which means they've been either discarded or hidden by
the user) and does so using the existing D-Bus system. However the
output of the dbus-send command is (in my opinion) super ugly and
unusable for any practical scripting application.

This is VERY much still a work in progress.

Any additional suggestions or ideas are welcome.

The current implementation tries to retrieve notifications stored in the
history buffer (which means they've been either discarded or hidden by
the user) and does so using the existing D-Bus system. However the
output of the dbus-send command is (in my opinion) super ugly and
unusable for any practical scripting application.

This is VERY much still a work in progress.
@fwsmit
Copy link
Member

fwsmit commented Nov 10, 2021

Maybe you could use busctl? After some playing around I came up with this command: busctl --user --json=pretty call org.freedesktop.Notifications /org/freedesktop/Notifications org.dunstproject.cmd0 NotificationListAll.
Btw, don't forget to add the method name to the introspection_xml and methods_fdn. And add the new command in the dunstctl --help output and man page.

@nikp123
Copy link
Contributor Author

nikp123 commented Nov 10, 2021

I like the idea. I've been also playing around the idea that instead of arrays i use a dbus dictionary instead

ie this: https://stackoverflow.com/questions/8846671/how-to-use-a-variant-dictionary-asv-in-dbus-send#38573254

@fwsmit
Copy link
Member

fwsmit commented Nov 10, 2021

I like the idea. I've been also playing around the idea that instead of arrays i use a dbus dictionary instead

ie this: https://stackoverflow.com/questions/8846671/how-to-use-a-variant-dictionary-asv-in-dbus-send#38573254

A dictionary seems good, since it would create a stable interface. You can add or remove items without breaking all scripts.

The JSON output is quite messy but usable. I'm way happier with this
version compared to the last one.

Changes:
Added some basic error-checking to the "dunstctl history" command.
Added ID, timestamp, timeout and progress to the "history" command.
Used dictionary-like structure instead of arrays.
@nikp123
Copy link
Contributor Author

nikp123 commented Nov 10, 2021

I like the idea. I've been also playing around the idea that instead of arrays i use a dbus dictionary instead
ie this: https://stackoverflow.com/questions/8846671/how-to-use-a-variant-dictionary-asv-in-dbus-send#38573254

A dictionary seems good, since it would create a stable interface. You can add or remove items without breaking all scripts.

Implemented :) after an annoyingly long session of googling

@nikp123
Copy link
Contributor Author

nikp123 commented Nov 10, 2021

it's done

@fwsmit
Copy link
Member

fwsmit commented Nov 12, 2021

Thanks for working on this. If you want you can add the script you're gonna use this feature with to the contrib directory

@nikp123
Copy link
Contributor Author

nikp123 commented Nov 12, 2021

Thanks for working on this. If you want you can add the script you're gonna use this feature with to the contrib directory

sure

nikp123 added a commit to nikp123/dunst that referenced this pull request Nov 12, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 14, 2021

Codecov Report

Merging #970 (675993a) into master (e4ada4e) will decrease coverage by 0.08%.
The diff coverage is 55.08%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #970      +/-   ##
==========================================
- Coverage   60.37%   60.29%   -0.09%     
==========================================
  Files          44       44              
  Lines        6655     6772     +117     
==========================================
+ Hits         4018     4083      +65     
- Misses       2637     2689      +52     
Flag Coverage Δ
unittests 60.29% <55.08%> (-0.09%) ⬇️

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

Impacted Files Coverage Δ
src/dbus.c 52.17% <0.00%> (-7.66%) ⬇️
src/queues.c 91.44% <88.88%> (-0.19%) ⬇️
test/queues.c 98.86% <100.00%> (+0.09%) ⬆️

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 e4ada4e...675993a. Read the comment docs.

@fwsmit
Copy link
Member

fwsmit commented Nov 14, 2021

Could you add tests for queues_history_pop_by_id and dbus_cb_dunst_NotificationListHistory? We try to keep most of the code tested.

@nikp123
Copy link
Contributor Author

nikp123 commented Nov 14, 2021

sure, just not immediately

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.

I have a few more comments on the code :)

@fwsmit
Copy link
Member

fwsmit commented Nov 14, 2021

sure, just not immediately

That's okay. Take your time 👍

@nikp123
Copy link
Contributor Author

nikp123 commented Nov 20, 2021

seems that queues_update breaks after 5 messages in waiting. is it supposed to do that?

@nikp123
Copy link
Contributor Author

nikp123 commented Nov 20, 2021

let me describe the breakage in more detail.

If i put into waiting more than 5 messages with skip_display on, ONLY the first five get to remain in the history after a queues_update(STATUS_NORMAL);

@nikp123
Copy link
Contributor Author

nikp123 commented Nov 20, 2021

oops seems that i have messed up the function prefixes when using sed, one sec

@nikp123 nikp123 force-pushed the master branch 2 times, most recently from b26a06a to 8c8a1e7 Compare November 20, 2021 16:45
@nikp123
Copy link
Contributor Author

nikp123 commented Nov 20, 2021

here you go; added some tests

unfortunately i think adding the dbus tests would create way more code than the dbus functions themselves, at which point you would most likely debug the tests themselves than the actual code

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.

There are still 2 small things that need to be done

@fwsmit
Copy link
Member

fwsmit commented Nov 23, 2021

Thanks for adding the tests! It's fine if you don't add the dbus tests.

@fwsmit
Copy link
Member

fwsmit commented Nov 23, 2021

It seems like a conflict was caused by me merging another PR

@nikp123
Copy link
Contributor Author

nikp123 commented Nov 23, 2021

here we go

@fwsmit fwsmit merged commit 06534a0 into dunst-project:master Nov 29, 2021
@fwsmit
Copy link
Member

fwsmit commented Nov 29, 2021

Merged it!

@fwsmit
Copy link
Member

fwsmit commented Nov 29, 2021

I'm thinking that we don't need a separate command for history-pop-id. history-pop can be used with an argument instead. I'll go ahead and change that.

@nikp123
Copy link
Contributor Author

nikp123 commented Dec 2, 2021

thanks

fwsmit pushed a commit to WhitePeter/dunst that referenced this pull request Dec 6, 2021
…#970)

* dbus/dunstctl: add history command (dunst-project#307)

The current implementation tries to retrieve notifications stored in the
history buffer (which means they've been either discarded or hidden by
the user) and does so using the existing D-Bus system. The output 
format is JSON, so it can be easily processed by scripts.

* dbus: check string validity before sending reply

* dbus/dunstctl: use dictionaries instead

* dbus: change whitespace to blank string

* dbus: change history callback function name

* dbus/dunstctl: add history-pop-id functionality (dunst-project#970)

* contrib: add notification-history.sh (dunst-project#970)

* dbus/dunstctl: change i32 to u32 for history-pop-id

* queues_history_pop_by_id: remove old notification from history when restore (bug)

* test/queues: add notification_skip_display_redisplayed_by_random_id

* test: add queue_get_history

* dunstctl: history-pop-id change to uint
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