Remove explicit DBUS notificator code? #573

1. 
   laanwj commented at 2:22 pm on April 4, 2022: member

   Back in the day I implemented DBUS support in notificator.cpp because the default Qt notifications used a deprecated notification through X11, which didn’t show up in Ubuntu’s GUI anymore. However I just found in the Qt 5.5 release notes that

   On supported desktops, the xcb plugin now uses the D-Bus based org.kde.StatusNotifier protocol for system tray icons, and org.freedesktop.Notifications for notifications.

   (for comparison, we require Qt 5.11.3)

   From this I understand that Qt will already automatically do the right thing with regard to notifications. Unless we do something custom not supported by this, it may be possible to get rid of our custom code for this.

   It would remove the (direct) dependency on QtDbus. This will need to be tested carefully, of course.

   (something similar may be true for MacOS notifications; I did not look into this)

2. laanwj added the label Linux on Apr 4, 2022
3. 
   laanwj commented at 2:33 pm on April 4, 2022: member

   The appropriate code is in src/platformsupport/themes/genericunix/dbustray/qdbustrayicon.cpp:

   0void QDBusTrayIcon::showMessage(const QString &title, const QString &msg, const QIcon &icon,
   1                                QPlatformSystemTrayIcon::MessageIcon iconType, int msecs)
   

   to compare with our code in Notificator::notifyDBus. On first glance it’s pretty similar.

   Edit: it looks like we want to pass in the icon, see #574.

4. laanwj referenced this in commit 9723b29c13 on Apr 4, 2022
5. 
   fanquake commented at 8:30 am on April 5, 2022: member

   Concept ACK
6. laanwj referenced this in commit c5e43fdd22 on Apr 6, 2022
7. 
   hebasto commented at 7:14 pm on April 9, 2022: member

   Commenting here as #575 has been closed. @laanwj #575 (comment):

   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.

   Moreover, our code requires (needlessly, I guess) existence of an icon. The following patch:

    0--- a/src/qt/notificator.cpp
    1+++ b/src/qt/notificator.cpp
    2@@ -37,7 +37,7 @@ Notificator::Notificator(const QString &_programName, QSystemTrayIcon *_trayIcon
    3     ,interface(nullptr)
    4 #endif
    5 {
    6-    if(_trayIcon && _trayIcon->supportsMessages())
    7+    if(QSystemTrayIcon::supportsMessages())
    8     {
    9         mode = QSystemTray;
   10     }
   

   should works for such cases.

   #575 (comment):

   I am not sure this is working as expected.

   The same behavior could be observed on the master branch after configuring --without-qtdbus. The reason of such behavior is that the Notificator constructor prioritize DBus over QSystemTrayIcon. I don’t know why this design has been chosen.

8. 
   laanwj commented at 5:54 am on April 14, 2022: member

   Moreover, our code requires (needlessly, I guess) existence of an icon. The following patch:

   Unfortunately, that would crash, as sending the notification itself also needs an icon:

   0trayIcon->showMessage(title, text, sicon, millisTimeout);
   

   I don’t know why this design has been chosen.

   Because dbus notifications are more reliable. The reason for introducing them was that Qt’s notifications were ignored on Ubuntu’s UI. I don’t think this is still the case, but as long as we have the dbus code we may as well use it.

9. laanwj closed this on Jun 14, 2022

   ---

10. bitcoin-core locked this on Jun 14, 2023

Contributors
laanwj fanquake hebasto

Labels
Linux