qt: Remove dbus notification code

laanwj commented at 4:31 pm on April 6, 2022: member

As of Qt 5.5, Qt will automatically use Freedesktop notifications when available and relevant to the platform. There is no more need to roll our own, so remove the dbus notification code.

This change also removes any mention of the QtDbus library from the build system. We do not need to link against it explicitly anymore. I have however not removed Qt dbus flags from the depends system, because it needs to be available internally for this to work.

The feature to set a custom QIcon for notifications is also removed, as it was only available for DBUS and besides, isn’t used at all!

This change only affects Linux. Closes #573.

Known limitation

So there’s one expected case where this fails to show a notification, where the old code does. If there is no recognized tray (QSystemTrayIcon::isSystemTrayAvailable), there is no tray icon, and no notification will be shown (as Qt’s notifications go through the tray icon).

I’m not sure this is a blocking problem. It may actually be an advantage as it avoids the problem underlying #575. It won’t try to connect to the notification service at all if there is likely no support.

qt: Remove dbus notification code

As of Qt 5.5, Qt will automatically use Freedesktop notifications when
available and relevant to the platform. There is no more need to roll
our own, so remove the dbus notification code.

This change also removes any mention of the QtDbus library from the
build system. We do not need to link against it explicitly anymore.
I have however not removed Qt dbus flags from the depends
system, because it needs to be available internally for this to work.

The feature to set a custom QIcon for notifications is also removed, as
it was only available for DBUS and besides, isn't used at all!

b26f6343f4

laanwj added the label Linux on Apr 6, 2022

laanwj added this to the milestone 24.0 on Apr 6, 2022

fanquake commented at 4:32 pm on April 6, 2022: member

Concept ACK

laanwj commented at 4:51 pm on April 6, 2022: member

The following patch may be useful for testing. It adds menu items to the “help” menu that launch notifications of every type:

 0commit 9ccd2147f0c581f56f9bbfa714bae31f993d5d96 (HEAD -> master)
 1Author: laanwj <126646+laanwj@users.noreply.github.com>
 2Date:   Wed Apr 6 18:44:02 2022 +0200
 3
 4    notification testing
 5
 6diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
 7index 85e3c23085243be8300197fd5c5b5b518df2cf9d..53ebc50fad475bc5ca1f677a2b8bfa04a02a2e4b 100644
 8--- a/src/qt/bitcoingui.cpp
 9+++ b/src/qt/bitcoingui.cpp
10@@ -525,6 +525,25 @@ void BitcoinGUI::createMenuBar()
11     help->addSeparator();
12     help->addAction(aboutAction);
13     help->addAction(aboutQtAction);
14+
15+    QAction *notifyAction1 = new QAction("Example notification (Information)", this);
16+    help->addAction(notifyAction1);
17+    connect(notifyAction1, &QAction::triggered, [this] {
18+            notificator->notify(Notificator::Information, "Example title", "Example message");
19+        }
20+    );
21+    QAction *notifyAction2 = new QAction("Example notification (Warning)", this);
22+    help->addAction(notifyAction2);
23+    connect(notifyAction2, &QAction::triggered, [this] {
24+            notificator->notify(Notificator::Warning, "Example title", "Example message");
25+        }
26+    );
27+    QAction *notifyAction3 = new QAction("Example notification (Critical)", this);
28+    help->addAction(notifyAction3);
29+    connect(notifyAction3, &QAction::triggered, [this] {
30+            notificator->notify(Notificator::Critical, "Example title", "Example message");
31+        }
32+    );
33 }

laanwj commented at 5:34 pm on April 6, 2022: member

Tested on:

Debian Testing, Qt 5.15.2+dfsg-9, GNOME, native compile: seems to work

Edit: okay, this is weird, I tested with the old code and get a completely different kind of notification. I am not sure this is working as expected. screenshot-oldnotification

DrahtBot commented at 6:17 pm on April 6, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#152 (GUI: Initialise DBus notifications in another thread by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in build-aux/m4/bitcoin_qt.m4:350 in b26f6343f4

346@@ -369,7 +347,7 @@ dnl _BITCOIN_QT_FIND_LIBS
347 dnl ---------------------
348 dnl
349 dnl Outputs: All necessary QT_* variables are set.
350-dnl Outputs: have_qt_test and have_qt_dbus are set (if applicable) to yes|no.
351+dnl Outputs: have_qt_test are set (if applicable) to yes|no.

jonatack commented at 8:31 pm on April 6, 2022:

0dnl Outputs: have_qt_test is set (if applicable) to yes|no.

jonatack commented at 8:38 pm on April 6, 2022: contributor

Concept ACK. Debug build is clean. Tested on same config as #575 (comment) with the suggested patch, tried various actions and read documentation but didn’t figure out how to see notifications (other than a pop-up dialog box with the critical example, pointers welcome).

laanwj commented at 2:25 pm on April 8, 2022: member

tried various actions and read documentation but didn’t figure out how to see notifications (other than a pop-up dialog box with the critical example, pointers welcome).

That happens if it doesn’t detect a system tray. Normal notifications are ignored but it will popup for critical ones.

I’m going to close this for now, might want to revisit for Qt6.

laanwj closed this on Apr 8, 2022

laanwj removed this from the milestone 24.0 on Apr 8, 2022

jonatack commented at 2:34 pm on April 8, 2022: contributor

Thanks! Yes, I now have some nice new notifications, note taken to figure out the system tray part.

laanwj commented at 2:38 pm on April 8, 2022: member

Thanks! Yes, I now have some nice new notifications, note taken to figure out the system tray part.

I expect this to give too much complaints from people that are currently using notifications without a system tray. There’s no way to do what within Qt, so my dbus code is still the only viable option.

It’s not like we’re going to get rid of the MacOS custom notification code any time soon (which is even worse, requiring ObjC compiler), so I guess I’m ok with that.

laanwj commented at 2:40 pm on April 8, 2022: member

But thanks for testing anyway and making me realize that :smile:

bitcoin-core locked this on Apr 8, 2023

qt: Remove dbus notification code #575

Known limitation

Conflicts