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

1. 
   hebasto commented at 4:21 pm on April 13, 2022: member

   qDBusRegisterMetaType returns:

   • int in Qt 5
   • QMetaType in Qt 6
2. 
   hebasto commented at 4:21 pm on April 13, 2022: member

   Friendly ping @laanwj :)
3. qt: Do not assume `qDBusRegisterMetaType` return type

   `qDBusRegisterMetaType` returns:
    - `int` in Qt 5
    - `QMetaType` in Qt 6

   6cf4dc7f64
4. hebasto force-pushed on Apr 13, 2022
5. hebasto added the label Qt 6 on Apr 13, 2022
6. 
   laanwj commented at 10:46 pm on April 15, 2022: member

   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

   0hints["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 bitcoin-core/gui#575, but will still allow deleting this code.

7. 
   hebasto commented at 9:49 am on April 16, 2022: member

   @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

   0hints["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

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

   

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

8. 
   laanwj commented at 10:07 am on April 16, 2022: member

   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.

9. 
   laanwj commented at 10:13 am on April 16, 2022: member

   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 6cf4dc7f64b42cbbff6a2ce7616ee625a87a29f5

10. 
    hebasto commented at 10:19 am on April 16, 2022: member

    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.

11. 
    hebasto commented at 10:22 am on April 16, 2022: member

    @luke-jr @jarolrod @prusnak

    Mind looking into this PR?

12. 
    prusnak commented at 4:41 pm on April 16, 2022: contributor

    Concept ACK
13. w0xlt approved
14. 
    w0xlt commented at 2:20 pm on April 19, 2022: contributor

    tACK https://github.com/bitcoin-core/gui/pull/584/commits/6cf4dc7f64b42cbbff6a2ce7616ee625a87a29f5 on Ubuntu 21.10, Qt 5.15.2.
15. hebasto merged this on Apr 19, 2022
16. hebasto closed this on Apr 19, 2022

    ---

17. hebasto deleted the branch on Apr 19, 2022
18. sidhujag referenced this in commit b314a358a7 on Apr 19, 2022
19. bitcoin-core locked this on Apr 19, 2023

Contributors
hebasto laanwj prusnak w0xlt

Labels
Qt 6