Re: Review Request 126691: Add DBusMenuShortcut type overload for QDBusArgument
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126691/ --- (Updated Feb. 8, 2016, 11:56 p.m.) Status -- This change has been marked as submitted. Review request for Plasma. Changes --- Submitted with commit 51266c34af7d778f850f41334f5527293680419a by D?vis Mos?ns to branch master. 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 overload was removed in http://code.qt.io/cgit/qt/qtbase.git/commit/src/dbus/qdbusargument.h?h=dev=5f542f3cca13f2da58b82aee2efbaffefeee00a7 and Container doesn't work... /usr/include/QtDBus/qdbusargument.h:244:29: note: candidate: template const QDBusArgument& operator>>(const QDBusArgument&, Container&) inline const QDBusArgument >>(const QDBusArgument , Container ) ^ /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’ 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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126691: Add DBusMenuShortcut type overload for QDBusArgument
> 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 overload was removed in > http://code.qt.io/cgit/qt/qtbase.git/commit/src/dbus/qdbusargument.h?h=dev=5f542f3cca13f2da58b82aee2efbaffefeee00a7 > > and Container doesn't work... > > /usr/include/QtDBus/qdbusargument.h:244:29: note: candidate: > template const QDBusArgument& > operator>>(const QDBusArgument&, Container&) > inline const QDBusArgument >>(const QDBusArgument , > Container ) > ^ > /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’ 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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126691: Add DBusMenuShortcut type overload for QDBusArgument
> On Jan. 10, 2016, 2: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. 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 --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126691/#review90839 --- On Jan. 10, 2016, 2:59 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 Jan. 10, 2016, 2:59 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 overload was removed in > http://code.qt.io/cgit/qt/qtbase.git/commit/src/dbus/qdbusargument.h?h=dev=5f542f3cca13f2da58b82aee2efbaffefeee00a7 > > and Container doesn't work... > > /usr/include/QtDBus/qdbusargument.h:244:29: note: candidate: > template const QDBusArgument& > operator>>(const QDBusArgument&, Container&) > inline const QDBusArgument >>(const QDBusArgument , > Container ) > ^ > /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’ 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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126691: Add DBusMenuShortcut type overload for QDBusArgument
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126691/#review92129 --- Ship it! Ship It! - David Edmundson On Jan. 10, 2016, 1:59 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 Jan. 10, 2016, 1:59 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 overload was removed in > http://code.qt.io/cgit/qt/qtbase.git/commit/src/dbus/qdbusargument.h?h=dev=5f542f3cca13f2da58b82aee2efbaffefeee00a7 > > and Container doesn't work... > > /usr/include/QtDBus/qdbusargument.h:244:29: note: candidate: > template const QDBusArgument& > operator>>(const QDBusArgument&, Container&) > inline const QDBusArgument >>(const QDBusArgument , > Container ) > ^ > /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’ 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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126691: Add DBusMenuShortcut type overload for QDBusArgument
> On Jan. 10, 2016, 1: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). 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 --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126691/#review90839 --- On Jan. 10, 2016, 1:59 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 Jan. 10, 2016, 1:59 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 overload was removed in > http://code.qt.io/cgit/qt/qtbase.git/commit/src/dbus/qdbusargument.h?h=dev=5f542f3cca13f2da58b82aee2efbaffefeee00a7 > > and Container doesn't work... > > /usr/include/QtDBus/qdbusargument.h:244:29: note: candidate: > template const QDBusArgument& > operator>>(const QDBusArgument&, Container&) > inline const QDBusArgument >>(const QDBusArgument , > Container ) > ^ > /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’ 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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126691: Add DBusMenuShortcut type overload for QDBusArgument
> On Jan. 10, 2016, 2: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. 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) - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126691/#review90839 --- On Jan. 10, 2016, 2:59 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 Jan. 10, 2016, 2:59 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 overload was removed in > http://code.qt.io/cgit/qt/qtbase.git/commit/src/dbus/qdbusargument.h?h=dev=5f542f3cca13f2da58b82aee2efbaffefeee00a7 > > and Container doesn't work... > > /usr/include/QtDBus/qdbusargument.h:244:29: note: candidate: > template const QDBusArgument& > operator>>(const QDBusArgument&, Container&) > inline const QDBusArgument >>(const QDBusArgument , > Container ) > ^ > /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’ 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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126691: Add DBusMenuShortcut type overload for QDBusArgument
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126691/#review90839 --- That sounds like you're saying Qt has broken API. If that's true, they'll want to know about it. - David Edmundson On Jan. 10, 2016, 1:59 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 Jan. 10, 2016, 1:59 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 overload was removed in > http://code.qt.io/cgit/qt/qtbase.git/commit/src/dbus/qdbusargument.h?h=dev=5f542f3cca13f2da58b82aee2efbaffefeee00a7 > > and Container doesn't work... > > /usr/include/QtDBus/qdbusargument.h:244:29: note: candidate: > template const QDBusArgument& > operator>>(const QDBusArgument&, Container&) > inline const QDBusArgument >>(const QDBusArgument , > Container ) > ^ > /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’ 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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request 126691: Add DBusMenuShortcut type overload for QDBusArgument
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126691/ --- 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 overload was removed in http://code.qt.io/cgit/qt/qtbase.git/commit/src/dbus/qdbusargument.h?h=dev=5f542f3cca13f2da58b82aee2efbaffefeee00a7 and Container doesn't work... /usr/include/QtDBus/qdbusargument.h:244:29: note: candidate: template const QDBusArgument& operator>>(const QDBusArgument&, Container&) inline const QDBusArgument >>(const QDBusArgument , Container ) ^ /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’ 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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
