-
Notifications
You must be signed in to change notification settings - Fork 219
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
Fix: Send notifies directly using python-dbus replacing notify-send #1156
Conversation
Interesting PR, it replaces command line tool call I have found the API doc here: https://specifications.freedesktop.org/notification-spec/latest/ar01s09.html The source code of of https://gitlab.gnome.org/GNOME/libnotify I think the code could probably need a small refactoring to be fail safe in case of DBus errors like eg. this code from
Also we need to check if |
OK, the PR code uses the same DBus interface and we could use this PR as basis for the improved notification plugin of BiT (as written above after improving the code and checking the unit tests).
https://gitlab.gnome.org/GNOME/libnotify/-/blob/master/libnotify/notify.c#L567 |
If backintime runs longer than the notification daemon (or starts before it), it may miss to detect it properly. This change will add a small overhead by resolving the notify interface for each notification, however it will make backintime send notifications when possible (again) and will still be faster than calling notify-send (which had to do the same).
Because sending notifications is not the main feature, backintime should continue to work when that fails. Idea from bit-team#1156 (comment)
Yeah, you're right. I forgot to mention that as it seemed obvious to me (because I learned using that interface at that time), sorry for not explaining it further in my PR. I added the exception handling because that really was a good point. About the unit tests: I cannot run them on my system as they throw a bunch of errors like |
Thanks a lot for your contribution! We are currently stabilizing and fixing BiT before adding new
This is a known "bug" causes by a change in Python 3.10 (see #1245).
Yes, 3.10 had a breaking change. |
Thank you from me also for your contribution!
Of course I can search myself. But if you have a very good tutorial or ressource about python and dbus in mind please let me know. "Learning" DBus is on my ToDo list also.
Please let me know if you need further assistance with the unittesting. You can open a new Issue and mention me (via @buhtzz ) or you can use the bit-dev mailing list. |
I figured out how to update/sync the fork with current upstream state. There are some other code parts distinguishing behaviour if they are run on Travis (via
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did some manual tests and also some refactoring. I would say this 3-years old PR is ready to merge. 🧓
This allows to configure the APP_NAME, so notification servers / user can better distinguish Back In Time notifications from other script's notifications and removes the dependency on notify-send.
Probably it also makes sending notifications a little bit faster.
The change was tested on Debian Unstable with KDE's Plasma 5. Plasma reflected the app name correctly.