Merge lp:~gnumdk/libdbusmenu-qt/fix-dropdown-firefox into lp:libdbusmenu-qt

[Cédric Bellegarde]

Fix issue with Firefox dropdown menus:
https://bugs.launchpad.net/plasma-widget-menubar/+bug/1027784

https://bugs.launchpad.net/unity-2d/+bug/1010466

[Cédric Bellegarde (gnumdk)]

ping? Kubuntu 13.04 will be released soon with this bug :-(

[Mathieu Trudel-Lapierre (cyphermox)]

I think it's fine... Why hasn't Jenkins picked it up?

[Mathieu Trudel-Lapierre (cyphermox)]

Or perhaps this simply isn't under CI :)

[Charles Kerr (charlesk)]

Looks good, this same fix has been used in other places, such as https://code.launchpad.net/~chrisccoulson/unity-2d/lp1010466/+merge/110075.

[Charles Kerr (charlesk)]

Approving to trigger CI as instructed by alesage :)

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/libdbusmenu-qt-autolanding/4/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/libdbusmenu-qt-raring-amd64-autolanding/1/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/libdbusmenu-qt-raring-armhf-autolanding/1/console

review: Needs Fixing (continuous-integration)

[Charles Kerr (charlesk)]

My previous comment was smoking crack, we don't want to add an X11 dependency here or the package will fail when X isn't available.

Cédric, is there a more portable way of achieving the same ends?

[lp:~gnumdk/libdbusmenu-qt/fix-dropdown-firefox]

241. By Cédric Bellegarde on 2013-04-17

Check for X11 availability

[Cédric Bellegarde (gnumdk)]

Is last commit ok ?

[Charles Kerr (charlesk)]

Well if we're going with that route, it would be better to test for #if defined(Q_WS_X11) ... CI is still going to fail with this new revision.

However since Mir and Wayland are gaining steam, I'd prefer if there was a fix that wasn't dependent on QX11Info. The Qt documentation for QX11AppInfo.appTime() only says "Returns the X11 time", so I'm not sure what the patch is doing.

Is there a portable alternative to using QX11AppInfo.appTime()?

[Charles Kerr (charlesk)]

For the benefit of other reviewers, this patch is based on a fix for a similar old Unity 2D issue tracked in bug #1010466 which had an excellent summary writeup by Chris Coulson @ https://bugs.launchpad.net/unity-2d/+bug/1010466/comments/22

[Charles Kerr (charlesk)]

...actually, even if we use some portable monotonically increasing timestamp, that still won't work because it'll disagree with the X11 time that gdk is passing along to FF elsewhere. So in the absence of a better solution we should try to use the X11 timestamp when available and keep the old behavior the rest of the time.

Cédric, please add the #ifdef wrappers and we'll run it through CI again, something like:

#if defined(Q_WS_X11)
#include <QX11Info>
#endif

...

#if defined(Q_WS_X11)
        // https://bugs.launchpad.net/libdbusmenu-qt/+bug/1027784
 uint timestamp = QX11Info::appTime();
#else
        timestamp = QDateTime::currentDateTime().toTime_t();
#endif

[lp:~gnumdk/libdbusmenu-qt/fix-dropdown-firefox]

242. By Cédric Bellegarde on 2013-04-18

Check for Q_WS_X11

[Joseph Yasi (joe-yasi)]

That change will build, but the same wrong timestamp problem with happen when used with QT5. I've updated the patch to use the new QPA X11 features of QT5:
https://bugs.launchpad.net/ubuntu/+source/libdbusmenu-qt/+bug/1035755/+attachment/3621848/+files/fix-x-apptime.patch

QT5.1 will add QX11Info again.

[Cédric Bellegarde (gnumdk)]

Any plan to commit last patch version ? :-)

I receive a lot of bug reports like this:

https://bugs.kde.org/show_bug.cgi?id=318837

[Cédric Bellegarde (gnumdk)]

Charles Kerr?

[Albert Astals Cid (aacid)]

So we have this and https://code.launchpad.net/~joe-yasi/libdbusmenu-qt/fix-x-timestamp/+merge/174573

Which is better?

[Albert Astals Cid (aacid)]

dropping in favor of https://code.launchpad.net/~joe-yasi/libdbusmenu-qt/fix-x-timestamp/+merge/174573

review: Disapprove

Unmerged revisions

242. By Cédric Bellegarde on 2013-04-18

Check for Q_WS_X11

241. By Cédric Bellegarde on 2013-04-17

Check for X11 availability

240. By Cédric Bellegarde on 2013-03-12

Fix dropdown menus in Firefox

=== modified file 'src/dbusmenuimporter.cpp'
--- src/dbusmenuimporter.cpp	2012-03-29 12:45:27 +0000
+++ src/dbusmenuimporter.cpp	2013-04-18 12:31:26 +0000
@@ -35,6 +35,10 @@
 #include <QToolButton>
 #include <QWidgetAction>
 
+#ifdef Q_WS_X11
+#include <QX11Info>
+#endif
+
 // Local
 #include "dbusmenutypes_p.h"
 #include "dbusmenushortcut_p.h"
@@ -276,7 +280,12 @@
     void sendEvent(int id, const QString &eventId)
     {
         QVariant empty = QVariant::fromValue(QDBusVariant(QString()));
-        uint timestamp = QDateTime::currentDateTime().toTime_t();
+        uint timestamp;
+        #ifdef Q_WS_X11
+        timestamp = QX11Info::appTime();
+        #else
+        timestamp = QDateTime::currentDateTime().toTime_t();
+        #endif
         m_interface->asyncCall("Event", id, eventId, empty, timestamp);
     }
 };