Merge lp:~aacid/unity-2d/kill_keyboardmodifiersmonitor into lp:unity-2d

[Albert Astals Cid]

[lib] Kill keyboardmodifiersmonitor, and integrate its functionality into keymonitor

This helps to prevent wrong HUD triggers on ultrafast alt+key presses

Kill keyboardmodifiersmonitor

Integrate its functionality in keymonitor, helps preventing wrong HUD triggers on ultrafast alt+key presses
UNBLOCK

[Albert Astals Cid (aacid)]

I'll try to integrate the test code from https://bugs.launchpad.net/unity-2d/+bug/968840/comments/4 next

[Albert Astals Cid (aacid)]

Branch dependency on lp:~aacid/unity-2d/fast_alt_left_press_fail_test removed as per Gerry's request

[Gerry Boland (gerboland)]

Works well, I notice a improvement in the tap detection so I get even fewer false HUD appearances. Approved!

review: Approve

=== modified file 'libunity-2d-private/src/CMakeLists.txt'
--- libunity-2d-private/src/CMakeLists.txt	2012-04-02 14:03:32 +0000
+++ libunity-2d-private/src/CMakeLists.txt	2012-04-19 08:51:21 +0000
@@ -9,7 +9,6 @@
     gconnector.cpp
     gimageutils.cpp
     gnomesessionclient.cpp
-    keyboardmodifiersmonitor.cpp
     hotmodifier.cpp
     hotkeymonitor.cpp
     hotkey.cpp
 
=== removed file 'libunity-2d-private/src/keyboardmodifiersmonitor.cpp'
--- libunity-2d-private/src/keyboardmodifiersmonitor.cpp	2012-02-15 14:45:17 +0000
+++ libunity-2d-private/src/keyboardmodifiersmonitor.cpp	1970-01-01 00:00:00 +0000
@@ -1,162 +0,0 @@
-/*
- * This file is part of unity-2d
- *
- * Copyright 2011 Canonical Ltd.
- *
- * Authors:
- * - Aurélien Gâteau <[e-mail]>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; version 3.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-
-// Self
-#include "keyboardmodifiersmonitor.h"
-
-// Local
-#include <hotmodifier.h>
-#include <debug_p.h>
-
-// Qt
-#include <QX11Info>
-
-// X11
-#include <X11/Xlib.h>
-#include <X11/XKBlib.h>
-#include <X11/extensions/XKB.h>
-
-static int sXkbBaseEventType = 0;
-
-static void setupXkb()
-{
-    int opcode, error;
-    XkbQueryExtension(QX11Info::display(), &opcode, &sXkbBaseEventType,  &error, NULL, NULL);
-    XkbSelectEvents(QX11Info::display(), XkbUseCoreKbd, XkbStateNotifyMask, XkbStateNotifyMask);
-}
-
-struct KeyboardModifiersMonitorPrivate
-{
-    KeyboardModifiersMonitorPrivate()
-    : m_modifiers(0)
-    {}
-
-    int m_modifiers;
-};
-
-KeyboardModifiersMonitor::KeyboardModifiersMonitor(QObject *parent)
-: QObject(parent)
-, d(new KeyboardModifiersMonitorPrivate)
-{
-    if (sXkbBaseEventType == 0) {
-        setupXkb();
-    }
-
-    Unity2dApplication* application = Unity2dApplication::instance();
-    if (application == NULL) {
-        /* This can happen for example when using qmlviewer to run the launcher */
-        UQ_WARNING << "The application is not an Unity2dApplication."
-                      "Modifiers will not be monitored.";
-    } else {
-        application->installX11EventFilter(this);
-    }
-}
-
-KeyboardModifiersMonitor::~KeyboardModifiersMonitor()
-{
-    delete d;
-}
-
-KeyboardModifiersMonitor* KeyboardModifiersMonitor::instance()
-{
-    static KeyboardModifiersMonitor* monitor = new KeyboardModifiersMonitor();
-    return monitor;
-}
-
-bool KeyboardModifiersMonitor::x11EventFilter(XEvent* event)
-{
-    if (event->type == sXkbBaseEventType + XkbEventCode) {
-        XkbEvent *xkbEvent = (XkbEvent*)event;
-        if (xkbEvent->any.xkb_type == XkbStateNotify) {
-            d->m_modifiers = xkbEvent->state.mods;
-            Qt::KeyboardModifiers modifiers = keyboardModifiers();
-            keyboardModifiersChanged(modifiers);
-
-            Q_FOREACH(HotModifier* hotModifier, m_hotModifiers) {
-                if (hotModifier->modifiers() & m_disabledModifiers) {
-                    /* If any of the modifiers have been disabled, the
-                     * hotModifier cannot be triggered */
-                    continue;
-                }
-                hotModifier->onModifiersChanged(modifiers);
-            }
-        }
-    }
-    return false;
-}
-
-Qt::KeyboardModifiers KeyboardModifiersMonitor::keyboardModifiers() const
-{
-    Qt::KeyboardModifiers value = 0;
-    if (d->m_modifiers & ShiftMask) {
-        value |= Qt::ShiftModifier;
-    }
-    if (d->m_modifiers & ControlMask) {
-        value |= Qt::ControlModifier;
-    }
-    if (d->m_modifiers & Mod1Mask) {
-        value |= Qt::AltModifier;
-    }
-    if (d->m_modifiers & Mod4Mask) {
-        value |= Qt::MetaModifier;
-    }
-    return value;
-}
-
-HotModifier*
-KeyboardModifiersMonitor::getHotModifierFor(Qt::KeyboardModifiers modifiers)
-{
-    Q_FOREACH(HotModifier* hotModifier, m_hotModifiers) {
-        if (hotModifier->modifiers() == modifiers) {
-            return hotModifier;
-        }
-    }
-
-    HotModifier *hotModifier = new HotModifier(modifiers, this);
-    m_hotModifiers.append(hotModifier);
-    return hotModifier;
-}
-
-void
-KeyboardModifiersMonitor::disableModifiers(Qt::KeyboardModifiers modifiers)
-{
-    m_disabledModifiers |= modifiers;
-    Q_FOREACH(HotModifier* hotModifier, m_hotModifiers) {
-        if (hotModifier->modifiers() & m_disabledModifiers) {
-            hotModifier->disable();
-        }
-    }
-}
-
-void
-KeyboardModifiersMonitor::enableModifiers(Qt::KeyboardModifiers modifiers)
-{
-    Qt::KeyboardModifiers previouslyDisabled = m_disabledModifiers;
-    m_disabledModifiers &= ~modifiers;
-    Q_FOREACH(HotModifier* hotModifier, m_hotModifiers) {
-        if (hotModifier->modifiers() & previouslyDisabled
-            && !hotModifier->modifiers() & m_disabledModifiers) {
-            hotModifier->onModifiersChanged(keyboardModifiers());
-        }
-    }
-}
-
-#include "keyboardmodifiersmonitor.moc"
 
=== removed file 'libunity-2d-private/src/keyboardmodifiersmonitor.h'
--- libunity-2d-private/src/keyboardmodifiersmonitor.h	2012-02-15 14:45:17 +0000
+++ libunity-2d-private/src/keyboardmodifiersmonitor.h	1970-01-01 00:00:00 +0000
@@ -1,69 +0,0 @@
-/*
- * This file is part of unity-2d
- *
- * Copyright 2011 Canonical Ltd.
- *
- * Authors:
- * - Aurélien Gâteau <[e-mail]>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; version 3.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-
-#ifndef KEYBOARDMODIFIERMONITOR_H
-#define KEYBOARDMODIFIERMONITOR_H
-
-// Local
-#include <unity2dapplication.h>
-
-class HotModifier;
-
-struct KeyboardModifiersMonitorPrivate;
-
-/**
- * This class monitor keyboard modifiers. It is able to track changes even if
- * the active window does not belong to the application.
- *
- * You *must* use Unity2dApplication to be able to use this class.
- *
- * In most case you don't need your own instance: use the one returned by
- * instance() instead.
- */
-class KeyboardModifiersMonitor : public QObject, protected AbstractX11EventFilter
-{
-Q_OBJECT
-public:
-    KeyboardModifiersMonitor(QObject *parent = 0);
-    ~KeyboardModifiersMonitor();
-
-    Qt::KeyboardModifiers keyboardModifiers() const;
-
-    HotModifier* getHotModifierFor(Qt::KeyboardModifiers modifiers);
-
-    void disableModifiers(Qt::KeyboardModifiers modifiers);
-    void enableModifiers(Qt::KeyboardModifiers modifiers);
-
-    static KeyboardModifiersMonitor* instance();
-
-Q_SIGNALS:
-    void keyboardModifiersChanged(Qt::KeyboardModifiers);
-
-protected:
-    bool x11EventFilter(XEvent*);
-
-private:
-    KeyboardModifiersMonitorPrivate* const d;
-    QList<HotModifier*> m_hotModifiers;
-    Qt::KeyboardModifiers m_disabledModifiers;
-};
-
-#endif /* KEYBOARDMODIFIERMONITOR_H */
 
=== modified file 'libunity-2d-private/src/keymonitor.cpp'
--- libunity-2d-private/src/keymonitor.cpp	2012-04-10 16:05:39 +0000
+++ libunity-2d-private/src/keymonitor.cpp	2012-04-19 08:51:21 +0000
@@ -27,20 +27,23 @@
 // X11
 #include <X11/Xlib.h>
 #include <X11/keysym.h>
+#include <X11/XKBlib.h>
 
 // Local
 #include <debug_p.h>
+#include <hotmodifier.h>
 
 #define INVALID_EVENT_TYPE -1
 
 static int key_press_type = INVALID_EVENT_TYPE;
 static int notify_type = INVALID_EVENT_TYPE;
-
+static int sXkbBaseEventType = 0;
 
 KeyMonitor::KeyMonitor(QObject* parent)
-: QObject(parent)
+: QObject(parent),
+  m_modifiers(Qt::NoModifier)
 {
-    if (this->registerEvents()) {
+    if (registerEvents()) {
         getModifiers();
     }
 }
@@ -57,6 +60,45 @@
     return monitor;
 }
 
+Qt::KeyboardModifiers KeyMonitor::keyboardModifiers() const
+{
+    return m_modifiers;
+}
+
+HotModifier *KeyMonitor::getHotModifierFor(Qt::KeyboardModifiers modifiers)
+{
+    Q_FOREACH(HotModifier* hotModifier, m_hotModifiers) {
+        if (hotModifier->modifiers() == modifiers) {
+            return hotModifier;
+        }
+    }
+
+    HotModifier *hotModifier = new HotModifier(modifiers, this);
+    m_hotModifiers.append(hotModifier);
+    return hotModifier;
+}
+
+void KeyMonitor::disableModifiers(Qt::KeyboardModifiers modifiers)
+{
+    m_disabledModifiers |= modifiers;
+    Q_FOREACH(HotModifier* hotModifier, m_hotModifiers) {
+        if (hotModifier->modifiers() & m_disabledModifiers) {
+            hotModifier->disable();
+        }
+    }
+}
+
+void KeyMonitor::enableModifiers(Qt::KeyboardModifiers modifiers)
+{
+    Qt::KeyboardModifiers previouslyDisabled = m_disabledModifiers;
+    m_disabledModifiers &= ~modifiers;
+    Q_FOREACH(HotModifier* hotModifier, m_hotModifiers) {
+        if (hotModifier->modifiers() & previouslyDisabled
+            && !hotModifier->modifiers() & m_disabledModifiers) {
+            hotModifier->onModifiersChanged(keyboardModifiers());
+        }
+    }
+}
 
 void KeyMonitor::getModifiers()
 {
@@ -134,6 +176,10 @@
         return false;
     }
 
+    int opcode, error;
+    XkbQueryExtension(m_display, &opcode, &sXkbBaseEventType,  &error, NULL, NULL);
+    XkbSelectEvents(m_display, XkbUseCoreKbd, XkbStateNotifyMask, XkbStateNotifyMask);
+
     /* Dispatch XEvents when there is activity on the X11 file descriptor */
     x11FileDescriptor = ConnectionNumber(m_display);
     QSocketNotifier* socketNotifier = new QSocketNotifier(x11FileDescriptor, QSocketNotifier::Read, this);
@@ -142,6 +188,23 @@
     return true;
 }
 
+static Qt::KeyboardModifiers qtModifiersFromXcbMods(int xcbModifiers)
+{
+    Qt::KeyboardModifiers value = Qt::NoModifier;
+    if (xcbModifiers & ShiftMask) {
+        value |= Qt::ShiftModifier;
+    }
+    if (xcbModifiers & ControlMask) {
+        value |= Qt::ControlModifier;
+    }
+    if (xcbModifiers & Mod1Mask) {
+        value |= Qt::AltModifier;
+    }
+    if (xcbModifiers & Mod4Mask) {
+        value |= Qt::MetaModifier;
+    }
+    return value;
+}
 
 void KeyMonitor::x11EventDispatch()
 {
@@ -158,6 +221,23 @@
         }
         else if (event.type == notify_type) {
             getModifiers();
+        } else if (event.type == sXkbBaseEventType + XkbEventCode) {
+            XkbEvent *xkbEvent = (XkbEvent*)&event;
+            if (xkbEvent->any.xkb_type == XkbStateNotify) {
+                const Qt::KeyboardModifiers prevMods = m_modifiers;
+                m_modifiers = qtModifiersFromXcbMods(xkbEvent->state.mods);
+                if (prevMods != m_modifiers) {
+                    Q_EMIT keyboardModifiersChanged(m_modifiers);
+                    Q_FOREACH(HotModifier* hotModifier, m_hotModifiers) {
+                        if (hotModifier->modifiers() & m_disabledModifiers) {
+                            /* If any of the modifiers have been disabled, the
+                            * hotModifier cannot be triggered */
+                            continue;
+                        }
+                        hotModifier->onModifiersChanged(m_modifiers);
+                    }
+                }
+            }
         }
     }
 }
 
=== modified file 'libunity-2d-private/src/keymonitor.h'
--- libunity-2d-private/src/keymonitor.h	2011-10-28 22:14:18 +0000
+++ libunity-2d-private/src/keymonitor.h	2012-04-19 08:51:21 +0000
@@ -28,12 +28,11 @@
 // X11
 #include <X11/extensions/XInput.h>
 
+class HotModifier;
+
 /**
  * This class monitors global keypresses. Whenever a non-modifier is pressed,
  * keyPressed() is emitted.
- *
- * In most case you don't need your own instance: use the one returned by
- * instance() instead.
  */
 class KeyMonitor : public QObject
 {
@@ -43,8 +42,16 @@
     static KeyMonitor* instance();
     ~KeyMonitor();
 
+    Qt::KeyboardModifiers keyboardModifiers() const;
+
+    HotModifier* getHotModifierFor(Qt::KeyboardModifiers modifiers);
+
+    void disableModifiers(Qt::KeyboardModifiers modifiers);
+    void enableModifiers(Qt::KeyboardModifiers modifiers);
+
 Q_SIGNALS:
     void keyPressed();
+    void keyboardModifiersChanged(Qt::KeyboardModifiers);
 
 private:
     KeyMonitor(QObject* parent=0);
@@ -59,6 +66,9 @@
     Display *m_display;
     QVector<XEventClass> m_eventList;
     QVector<KeyCode> m_modList;
+    Qt::KeyboardModifiers m_modifiers;
+    QList<HotModifier*> m_hotModifiers;
+    Qt::KeyboardModifiers m_disabledModifiers;
 };
 
 #endif // KEYMONITOR_H
 
=== modified file 'libunity-2d-private/tests/CMakeLists.txt'
--- libunity-2d-private/tests/CMakeLists.txt	2012-04-02 15:15:59 +0000
+++ libunity-2d-private/tests/CMakeLists.txt	2012-04-19 08:51:21 +0000
@@ -33,7 +33,7 @@
 
 libunity_2d_tests(
     gconnectortest
-    keyboardmodifiersmonitortest
+    keymonitortest
     launchermenutest
     listaggregatormodeltest
     qsortfilterproxymodeltest
@@ -47,6 +47,8 @@
 target_link_libraries(pointerbarriertest ${X11_XTest_LIB})
 
 target_link_libraries(hotkeytest ${X11_XTest_LIB})
+
+target_link_libraries(keymonitortest ${X11_XTest_LIB})
     
 # unity2dtrtest - FIXME
 #add_test(NAME unity2dtrtest_check
 
=== renamed file 'libunity-2d-private/tests/keyboardmodifiersmonitortest.cpp' => 'libunity-2d-private/tests/keymonitortest.cpp'
--- libunity-2d-private/tests/keyboardmodifiersmonitortest.cpp	2011-11-17 20:10:19 +0000
+++ libunity-2d-private/tests/keymonitortest.cpp	2012-04-19 08:51:21 +0000
@@ -22,7 +22,8 @@
 // Local
 #include <unitytestmacro.h>
 #include <debug_p.h>
-#include <keyboardmodifiersmonitor.h>
+#include <keymonitor.h>
+#include <hotmodifier.h>
 
 // Qt
 #include <QSignalSpy>
@@ -33,6 +34,8 @@
 #include <X11/Xlib.h>
 #include <X11/XKBlib.h>
 #include <X11/extensions/XKB.h>
+#include <X11/extensions/XTest.h>
+#include <X11/keysym.h>
 
 Q_DECLARE_METATYPE(Qt::KeyboardModifiers)
 
@@ -70,23 +73,45 @@
     {
         QFETCH(int, x11Mask);
         QFETCH(Qt::KeyboardModifiers, qtModifiers);
-        KeyboardModifiersMonitor monitor;
-        QSignalSpy spy(&monitor, SIGNAL(keyboardModifiersChanged(Qt::KeyboardModifiers)));
+        KeyMonitor *monitor = KeyMonitor::instance();
+        QSignalSpy spy(monitor, SIGNAL(keyboardModifiersChanged(Qt::KeyboardModifiers)));
 
         setModifierState(x11Mask, true);
         QTest::qWait(200);
-        QCOMPARE(monitor.keyboardModifiers(), qtModifiers);
+        QCOMPARE(monitor->keyboardModifiers(), qtModifiers);
         QCOMPARE(spy.count(), 1);
         QCOMPARE(spy.takeFirst().at(0).value<Qt::KeyboardModifiers>(), qtModifiers);
 
         setModifierState(x11Mask, false);
         QTest::qWait(200);
-        QCOMPARE(monitor.keyboardModifiers(), 0);
+        QCOMPARE(monitor->keyboardModifiers(), 0);
         QCOMPARE(spy.count(), 1);
         QCOMPARE(spy.takeFirst().at(0).value<Qt::KeyboardModifiers>(), 0);
     }
+
+    void testFastModifierKeyPress()
+    {
+        Display *display = XOpenDisplay(0);
+
+        HotModifier* hm = KeyMonitor::instance()->getHotModifierFor(Qt::AltModifier);
+        QSignalSpy spy(hm, SIGNAL(tapped()));
+
+        XTestGrabControl(display, 1);
+        XTestFakeKeyEvent(display, XKeysymToKeycode(display, XK_Alt_L), 1 /* PRESS */, CurrentTime);
+        XTestFakeKeyEvent(display, XKeysymToKeycode(display, XK_Left), 1 /* PRESS */, CurrentTime);
+        XFlush(display);
+
+        XTestGrabControl(display, 1);
+        XTestFakeKeyEvent(display, XKeysymToKeycode(display, XK_Alt_L), 0 /* RELEASE */, CurrentTime);
+        XTestFakeKeyEvent(display, XKeysymToKeycode(display, XK_Left), 0 /* RELEASE */, CurrentTime);
+        XFlush(display);
+
+        QTest::qWait(200);
+
+        QCOMPARE(spy.count(), 0);
+    }
 };
 
 UAPP_TEST_MAIN(KeyboardModifiersMonitorTest)
 
-#include "keyboardmodifiersmonitortest.moc"
+#include "keymonitortest.moc"
 
=== modified file 'panel/applets/appname/appnameapplet.cpp'
--- panel/applets/appname/appnameapplet.cpp	2012-04-05 09:34:40 +0000
+++ panel/applets/appname/appnameapplet.cpp	2012-04-19 08:51:21 +0000
@@ -34,7 +34,7 @@
 
 // Unity-2d
 #include <debug_p.h>
-#include <keyboardmodifiersmonitor.h>
+#include <keymonitor.h>
 #include <launcherclient.h>
 #include <hotkey.h>
 #include <hotkeymonitor.h>
@@ -271,7 +271,7 @@
 
     void setupKeyboardModifiersMonitor()
     {
-        QObject::connect(KeyboardModifiersMonitor::instance(), SIGNAL(keyboardModifiersChanged(Qt::KeyboardModifiers)),
+        QObject::connect(KeyMonitor::instance(), SIGNAL(keyboardModifiersChanged(Qt::KeyboardModifiers)),
             q, SLOT(updateWidgets()));
     }
 };
@@ -321,7 +321,7 @@
     bool isUnderMouse = rect().contains(mapFromGlobal(QCursor::pos()));
     bool isOpened = isOnSameScreen &&
         (isUnderMouse
-        || KeyboardModifiersMonitor::instance()->keyboardModifiers() == Qt::AltModifier
+        || KeyMonitor::instance()->keyboardModifiers() == Qt::AltModifier
         || d->m_menuBarWidget->isOpened()
         );
     bool showDesktopLabel = !app;
 
=== modified file 'shell/app/shellmanager.cpp'
--- shell/app/shellmanager.cpp	2012-04-06 15:16:39 +0000
+++ shell/app/shellmanager.cpp	2012-04-19 08:51:21 +0000
@@ -31,7 +31,6 @@
 #include <hotmodifier.h>
 #include <hotkeymonitor.h>
 #include <hotkey.h>
-#include <keyboardmodifiersmonitor.h>
 #include <keymonitor.h>
 #include <screeninfo.h>
 
@@ -300,7 +299,7 @@
     updateSuperKeyMonitoring();
 
     /* Super tap shows the dash, super held shows the launcher hints */
-    d->m_superHotModifier = KeyboardModifiersMonitor::instance()->getHotModifierFor(Qt::MetaModifier);
+    d->m_superHotModifier = KeyMonitor::instance()->getHotModifierFor(Qt::MetaModifier);
     connect(d->m_superHotModifier, SIGNAL(tapped()), SLOT(toggleDashRequested()));
     connect(d->m_superHotModifier, SIGNAL(heldChanged(bool)), SIGNAL(superKeyHeldChanged(bool)));
 
@@ -514,7 +513,7 @@
             d->m_hudHotKey = HotkeyMonitor::instance().getHotkeyFor(x11KeyCode, modifiers);
             connect(d->m_hudHotKey, SIGNAL(pressed()), SLOT(toggleHudRequested()));
         } else if (modifiers != Qt::NoModifier) {
-            d->m_hudHotModifier = KeyboardModifiersMonitor::instance()->getHotModifierFor(modifiers);
+            d->m_hudHotModifier = KeyMonitor::instance()->getHotModifierFor(modifiers);
             connect(d->m_hudHotModifier, SIGNAL(tapped()), SLOT(toggleHudRequested()));
         }
     } else {
@@ -527,7 +526,7 @@
 void
 ShellManager::updateSuperKeyMonitoring()
 {
-    KeyboardModifiersMonitor *modifiersMonitor = KeyboardModifiersMonitor::instance();
+    KeyMonitor *modifiersMonitor = KeyMonitor::instance();
     HotkeyMonitor& hotkeyMonitor = HotkeyMonitor::instance();
 
     QVariant value = launcher2dConfiguration().property("superKeyEnable");