Review Request 126691: Add DBusMenuShortcut type overload for QDBusArgument

[Dāvis Mosāns]

> On Jan. 10, 2016, 3:06 p.m., David Edmundson wrote:
> > That sounds like you're saying Qt has broken API. 
> > If that's true, they'll want to know about it.
> 
> David Kahles wrote:
>     I talked to them (see https://codereview.qt-project.org/#/c/144823/) and it seems like we shouldn't have done it like it was before, it worked by pure luck.
>     According to them, this change will do the right thing, but additional, we shouldn't inherit containers at all.
>     (I think the problem is, that QT containers don't have a virtual destructor)
>     
>     Btw., libdbusmenu-qt suffers from the same problem, but I'm  not sure whether this is our responsibility (I'm confused because there's kind of a fork in plasma).
> 
> David Edmundson wrote:
>     Right so:
>      - libdbusmenu-qt is on launchpad somewhere (though not a canonical project or anything)
>      - I /needed/ a patch that fixed a crash but it had to break API, so I copied a tiny part of it (client only) into our code, whilst the original is in review. AFAIK it still is.
> 
> David Kahles wrote:
>     Ok thanks, that's what i've assumed.
>     @D?vis Mos?ns: It would be great if you could additional try to get this change upstream (https://launchpad.net/libdbusmenu-qt)

Submited https://code.launchpad.net/~davispuh/libdbusmenu-qt/libdbusmenu-qt/+merge/285423


- Dāvis


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126691/#review90839
-----------------------------------------------------------


On Feb. 9, 2016, 12:56 a.m., Dāvis Mosāns wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126691/
> -----------------------------------------------------------
> 
> (Updated Feb. 9, 2016, 12:56 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> -------
> 
> Add DBusMenuShortcut type overload for QDBusArgument.
> I don't know if there's a better way to fix this than this kinda code duplication.
> 
> 
> This is needed because otherwise it wouldn't compile with latest Qt dev branch. It's probably because QList<T> overload was removed in http://code.qt.io/cgit/qt/qtbase.git/commit/src/dbus/qdbusargument.h?h=dev&id=5f542f3cca13f2da58b82aee2efbaffefeee00a7
> 
> and Container<T> doesn't work...
> 
> /usr/include/QtDBus/qdbusargument.h:244:29: note: candidate: template<template<class> class Container, class T> const QDBusArgument& operator>>(const QDBusArgument&, Container<T>&)
>  inline const QDBusArgument &operator>>(const QDBusArgument &arg, Container<T> &list)
>                              ^
> /usr/include/QtDBus/qdbusargument.h:244:29: note:   template argument deduction/substitution failed:
> /mnt/KDE/kde/workspace/plasma-workspace/dataengines/statusnotifieritem/libdbusmenuqt/dbusmenuimporter.cpp:261:16: note:   can't deduce a template for ‘Container<T>’ from non-template type ‘DBusMenuShortcut’
>          arg >> dmShortcut;
>                 ^
> In file included from /usr/include/QtDBus/qdbuspendingreply.h:39:0,
>                  from /usr/include/QtDBus/qdbusreply.h:44,
>                  from /usr/include/QtDBus/QDBusReply:1,
>                  from /mnt/KDE/kde/workspace/plasma-workspace/dataengines/statusnotifieritem/libdbusmenuqt/dbusmenuimporter.cpp:27
> 
> 
> Diffs
> -----
> 
>   dataengines/statusnotifieritem/libdbusmenuqt/dbusmenutypes_p.h 4950a22279c09fb93c68fe3d38ff600279e856ca 
>   dataengines/statusnotifieritem/libdbusmenuqt/dbusmenutypes_p.cpp e98c4b93bc8532367ee96138ce72a54f44ac05ca 
> 
> Diff: https://git.reviewboard.kde.org/r/126691/diff/
> 
> 
> Testing
> -------
> 
> Compiles
> 
> 
> Thanks,
> 
> Dāvis Mosāns
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160208/37c17b3b/attachment-0001.html>