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

Fix: Send notifies directly using python-dbus replacing notify-send #1156

Draft
wants to merge 10 commits into
base: dev
Choose a base branch
from

Conversation

Zocker1999NET
Copy link
Contributor

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.

@aryoda
Copy link
Contributor

aryoda commented Sep 11, 2022

Interesting PR, it replaces command line tool call notify-send by its corresponding DBus call.

I have found the API doc here:

https://specifications.freedesktop.org/notification-spec/latest/ar01s09.html

The source code of of libnotify (which is called by notify-send is here (a Gnome project!):

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 common/tools.py:

def onBattery():
    if dbus:
        try:
            bus = dbus.SystemBus()
            proxy = bus.get_object('org.freedesktop.UPower',
                                   '/org/freedesktop/UPower')
            return bool(proxy.Get('org.freedesktop.UPower',
                                  'OnBattery',
                                  dbus_interface = 'org.freedesktop.DBus.Properties'))
        except dbus.exceptions.DBusException:
            pass
    return False

Also we need to check if notify-send uses the same DBus function call on all Linux platforms otherwise we would end up with a lot of "if OS = ..." statements and would repeat the work of notify-send...

@aryoda
Copy link
Contributor

aryoda commented Sep 11, 2022

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://github.com/bit-team/backintime/pull/1156/files#diff-9cc4469f72c632f589802f261e273bd96e5f3b85b0c8feb8f8b94e14c5cdf99eR53

self.notify_interface = dbus.Interface(object=dbus.SessionBus().get_object("org.freedesktop.Notifications", "/org/freedesktop/Notifications"), dbus_interface="org.freedesktop.Notifications")
#define NOTIFY_DBUS_NAME           "org.freedesktop.Notifications"
#define NOTIFY_DBUS_CORE_INTERFACE "org.freedesktop.Notifications"
#define NOTIFY_DBUS_CORE_OBJECT    "/org/freedesktop/Notifications"
#define NOTIFY_PORTAL_DBUS_NAME           "org.freedesktop.portal.Desktop"
#define NOTIFY_PORTAL_DBUS_CORE_INTERFACE "org.freedesktop.portal.Notification"
#define NOTIFY_PORTAL_DBUS_CORE_OBJECT    "/org/freedesktop/portal/desktop"

 _proxy = g_dbus_proxy_new_for_bus_sync (G_BUS_TYPE_SESSION,
                                                G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES,
                                                NULL,
                                                NOTIFY_DBUS_NAME,
                                                NOTIFY_DBUS_CORE_OBJECT,
                                                NOTIFY_DBUS_CORE_INTERFACE,
                                                NULL,
                                                error);

https://gitlab.gnome.org/GNOME/libnotify/-/blob/master/libnotify/notify.c#L567
https://gitlab.gnome.org/GNOME/libnotify/-/blob/master/libnotify/internal.h#L27

@emtiu emtiu added the Feature requests a new feature label Sep 11, 2022
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)
@Zocker1999NET
Copy link
Contributor Author

Interesting PR, it replaces command line tool call notify-send by its corresponding DBus call.

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.
I also decided to move the "resolving the interface" part from init to the msg function because while thinking about error handling, it occurred to me that the notification daemon could be started after backintime or it could crash away (both especially as it could run for a longer time). Both may require a re-"resolving", at least as I understand the library and based on what errors I saw using that elsewhere. This may may make sending a notification run slower, but it should still also be faster than notify-send, as it had to do the same.

About the unit tests: I cannot run them on my system as they throw a bunch of errors like AttributeError: module 'collections' has no attribute 'MutableSet', and I don't understand why the code contains such references. I'm using cpython 3.10.7 on a Debian Testing system, is my python version wrong or is it something else?

@aryoda
Copy link
Contributor

aryoda commented Sep 25, 2022

I added the exception handling because that really was a good point.

Thanks a lot for your contribution! We are currently stabilizing and fixing BiT before adding new
features (or improving them) but your PR is not forgotten 🎗️

About the unit tests: I cannot run them on my system as they throw a bunch of errors like
AttributeError: module 'collections' has no attribute 'MutableSet', and I don't understand why the code contains such references.

This is a known "bug" causes by a change in Python 3.10 (see #1245).
A fix is integrated in the master branch of BiT so if you pull this upstream version into your branch the tests should work too.

I'm using cpython 3.10.7 on a Debian Testing system, is my python version wrong or is it something else?

Yes, 3.10 had a breaking change.

@buhtz
Copy link
Member

buhtz commented Sep 26, 2022

Thank you from me also for your contribution!

I learned using that interface at that time)

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.

About the unit tests

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.

@buhtz buhtz changed the base branch from main to dev January 16, 2023 09:07
@buhtz
Copy link
Member

buhtz commented May 8, 2023

My apologize. Something went wrong.

I just tried to update your Zocker1999NET:dbus-notifications to the current state of bit-team/backintime:dev.

I rolled back my last commit with

git reset --hard HEAD^ 
git push -f 

I think it is fine again.

Can you please try to sync your fork yourself?
image

@buhtz
Copy link
Member

buhtz commented Jun 5, 2023

I figured out how to update/sync the fork with current upstream state.
I also modified the TravisCI setup with apt install libdbus-glib* and pip install dbus-python. The tests on AMD64 are green. So It seems to me that we can use DBUS in testing environments. But on ppc6le the machine doesn't boot anymore. 😄 I asked the Travis support. Because of the quick and good reply of the TravisCI folks the tests on ppc64le machine now are also green.

There are some other code parts distinguishing behaviour if they are run on Travis (via ON_TRAVIS) or not. But I didn't touched them.

On the other hand we need to check it ReadTheDocs can build with dbus when the PR is merged. It is not clear yet if this works. I also tested this with ReadTheDocs. It builds even with DBUS. I see no need to use ON_TRAVIS and ON_RTD anymore.

@buhtz buhtz added this to the upcoming release (1.3.4) milestone Aug 7, 2023
@buhtz buhtz added Low relevant, but not urgent HELP-WANTED Used by 24pullrequests.com to suggest issues labels Jan 5, 2024
@buhtz buhtz changed the title Send notifies directly using python-dbus replacing notify-send Fix: Send notifies directly using python-dbus replacing notify-send Feb 4, 2024
@buhtz buhtz added the PR: Modifications requested Maintenance team requested modifications and waiting for their implementation label May 2, 2024
@buhtz buhtz marked this pull request as draft May 11, 2024 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature requests a new feature HELP-WANTED Used by 24pullrequests.com to suggest issues Low relevant, but not urgent PR: Modifications requested Maintenance team requested modifications and waiting for their implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants