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

qt: Remove dbus notification code #575

Closed
wants to merge 1 commit into from

Conversation

laanwj
Copy link
Member

@laanwj laanwj commented Apr 6, 2022

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.

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!
@laanwj laanwj added the Linux label Apr 6, 2022
@laanwj laanwj added this to the 24.0 milestone Apr 6, 2022
@fanquake
Copy link
Member

fanquake commented Apr 6, 2022

Concept ACK

@laanwj
Copy link
Member Author

laanwj commented Apr 6, 2022

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

commit 9ccd2147f0c581f56f9bbfa714bae31f993d5d96 (HEAD -> master)
Author: laanwj <126646+laanwj@users.noreply.github.com>
Date:   Wed Apr 6 18:44:02 2022 +0200

    notification testing

diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
index 85e3c23085243be8300197fd5c5b5b518df2cf9d..53ebc50fad475bc5ca1f677a2b8bfa04a02a2e4b 100644
--- a/src/qt/bitcoingui.cpp
+++ b/src/qt/bitcoingui.cpp
@@ -525,6 +525,25 @@ void BitcoinGUI::createMenuBar()
     help->addSeparator();
     help->addAction(aboutAction);
     help->addAction(aboutQtAction);
+
+    QAction *notifyAction1 = new QAction("Example notification (Information)", this);
+    help->addAction(notifyAction1);
+    connect(notifyAction1, &QAction::triggered, [this] {
+            notificator->notify(Notificator::Information, "Example title", "Example message");
+        }
+    );
+    QAction *notifyAction2 = new QAction("Example notification (Warning)", this);
+    help->addAction(notifyAction2);
+    connect(notifyAction2, &QAction::triggered, [this] {
+            notificator->notify(Notificator::Warning, "Example title", "Example message");
+        }
+    );
+    QAction *notifyAction3 = new QAction("Example notification (Critical)", this);
+    help->addAction(notifyAction3);
+    connect(notifyAction3, &QAction::triggered, [this] {
+            notificator->notify(Notificator::Critical, "Example title", "Example message");
+        }
+    );
 }

@laanwj
Copy link
Member Author

laanwj commented Apr 6, 2022

Tested on:

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

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
Copy link
Contributor

DrahtBot commented Apr 6, 2022

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.

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@@ -369,7 +347,7 @@ dnl _BITCOIN_QT_FIND_LIBS
dnl ---------------------
dnl
dnl Outputs: All necessary QT_* variables are set.
dnl Outputs: have_qt_test and have_qt_dbus are set (if applicable) to yes|no.
dnl Outputs: have_qt_test are set (if applicable) to yes|no.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dnl Outputs: have_qt_test are set (if applicable) to yes|no.
dnl Outputs: have_qt_test is set (if applicable) to yes|no.

@laanwj
Copy link
Member Author

laanwj commented Apr 8, 2022

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 laanwj closed this Apr 8, 2022
@laanwj laanwj removed this from the 24.0 milestone Apr 8, 2022
@jonatack
Copy link
Member

jonatack commented Apr 8, 2022

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

@laanwj
Copy link
Member Author

laanwj commented Apr 8, 2022

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

laanwj commented Apr 8, 2022

But thanks for testing anyway and making me realize that 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove explicit DBUS notificator code?
4 participants