Getting ready to Qt 6 (5/n). Do not assume qDBusRegisterMetaType return type

[hebasto]

qDBusRegisterMetaType returns:

• int in Qt 5
• QMetaType in Qt 6

[hebasto]

Friendly ping @laanwj :)

[laanwj]

I have no idea whether this is correct. But as we don't use custom icons for notifications at all, I think we could get rid of the entire icon data, as well as FreedesktopIcon? so

hints["icon_data"] =…

and everything that it depend on, including any const QIcon &icon parameters.

This is less invasive than removing dbus support completely as in #575, but will still allow deleting this code.

[hebasto]

@laanwj

I have no idea whether this is correct. But as we don't use custom icons for notifications at all, I think we could get rid of the entire icon data, as well as FreedesktopIcon? so

hints["icon_data"] =…

and everything that it depend on, including any const QIcon &icon parameters.

This is less invasive than removing dbus support completely as in #575, but will still allow deleting this code.

On my Ubuntu 22.04 with a custom testing patch:

With the following diff

--- a/src/qt/notificator.cpp
+++ b/src/qt/notificator.cpp
@@ -193,7 +193,6 @@ void Notificator::notifyDBus(Class cls, const QString &title, const QString &tex
     {
         tmpicon = icon;
     }
-    hints["icon_data"] = FreedesktopImage::toVariant(tmpicon.pixmap(FREEDESKTOP_NOTIFICATION_ICON_SIZE).toImage());
     args.append(hints);
 
     // Timeout (in msec)

As an Ubuntu user, I'd like to keep the current functionality.

[laanwj]

Doesn't the latter icon convey the same information but integrate much better into your desktop theme?

But I guess ideally you'd want notifications to have the icon of the program that emits them? Like, a bitcoin icon instead of the standard issue traffic sign? So yeah if that's what you want then keeping the custom icon functionality (and actually using it) would make sense.

[laanwj]

At some point we actually used to have different custom icons for notifications, even different ones for incoming/sent/etc transactions I don't know what happened to that. But as it's been removed my first reaction was to rip it out completely.

Anyhow code review ACK 6cf4dc7

[hebasto]

Doesn't the latter icon convey the same information but integrate much better into your desktop theme?

The same icon for message / warning / critical notification.

But I guess ideally you'd want notifications to have the icon of the program that emits them?

Probably. macOS does like that.

So yeah if that's what you want then keeping the custom icon functionality (and actually using it) would make sense.

Yes. Let's keep it for now.

[hebasto]

@luke-jr @jarolrod @prusnak

Mind looking into this PR?

[prusnak]

Concept ACK

[w0xlt]

tACK 6cf4dc7 on Ubuntu 21.10, Qt 5.15.2.