Split modifier only handling into a dedicated InputEventSpy

Summary:
The functionality regarding triggering modifier only shortcuts is moved
out of Xkb - where it doesn't belong to - and is turned into an input
event spy listening for the changes it is interested in. Previously
the state got queried by asking e.g. for the pressed buttons, now it's
tracked directly.

The X11 side needs a larger change due to that as now pushing the events
into Xkb does not trigger modifier only shortcuts any more. Instead the
"normal" way through the platform API needs to be used which triggers the
processing of filters and spies.

The problem here is that our redirections only process events if they are
inited and that only happens on Wayland. We cannot call init on them as
that would create all the Wayland filters and spies and processing would
probably break. As an intermediate solution the spies are now processed
and there we know that it won't matter. A future solution would be to
remove the init checks completely and just send through both filters and
spies and ensure that on X11 only the supported ones are loaded.

Closes T5220

Test Plan: Tested on Wayland and X11

Reviewers: #kwin, #plasma

Subscribers: plasma-devel, kwin

Tags: #kwin

Maniphest Tasks: T5220

Differential Revision: https://phabricator.kde.org/D4578

CMakeLists.txt

@@ -434,6 +434,7 @@ set(kwin_KDEINIT_SRCS
     wayland_cursor_theme.cpp
     virtualkeyboard.cpp
     appmenu.cpp
+    modifier_only_shortcuts.cpp
    )
 
 if(KWIN_BUILD_TABBOX)

keyboard_input.cpp

@@ -23,7 +23,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #include "keyboard_layout.h"
 #include "keyboard_repeat.h"
 #include "abstract_client.h"
-#include "options.h"
+#include "modifier_only_shortcuts.h"
 #include "utils.h"
 #include "screenlockerwatcher.h"
 #include "toplevel.h"
@@ -38,9 +38,6 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #include <KKeyServer>
 #include <KGlobalAccel>
 // Qt
-#include <QDBusConnection>
-#include <QDBusMessage>
-#include <QDBusPendingCall>
 #include <QKeyEvent>
 #include <QTemporaryFile>
 // xkbcommon
@@ -124,13 +121,6 @@ Xkb::Xkb(InputRedirection *input)
             m_compose.state = xkb_compose_state_new(m_compose.table, XKB_COMPOSE_STATE_NO_FLAGS);
         }
     }
-
-    auto resetModOnlyShortcut = [this] {
-        m_modOnlyShortcut.modifier = Qt::NoModifier;
-    };
-    QObject::connect(m_input, &InputRedirection::pointerButtonStateChanged, resetModOnlyShortcut);
-    QObject::connect(m_input, &InputRedirection::pointerAxisChanged, resetModOnlyShortcut);
-    QObject::connect(ScreenLockerWatcher::self(), &ScreenLockerWatcher::locked, m_input, resetModOnlyShortcut);
 }
 
 Xkb::~Xkb()
@@ -289,7 +279,6 @@ void Xkb::updateKey(uint32_t key, InputRedirection::KeyboardKeyState state)
     if (!m_keymap || !m_state) {
         return;
     }
-    const auto oldMods = modifiersRelevantForGlobalShortcuts();
     xkb_state_update_key(m_state, key + 8, static_cast<xkb_key_direction>(state));
     if (state == InputRedirection::KeyboardKeyPressed) {
         const auto sym = toKeysym(key);
@@ -311,36 +300,6 @@ void Xkb::updateKey(uint32_t key, InputRedirection::KeyboardKeyState state)
     }
     updateModifiers();
     updateConsumedModifiers(key);
-    if (state == InputRedirection::KeyboardKeyPressed) {
-        m_modOnlyShortcut.pressCount++;
-        if (m_modOnlyShortcut.pressCount == 1 &&
-            !ScreenLockerWatcher::self()->isLocked() &&
-            oldMods == Qt::NoModifier &&
-            m_input->qtButtonStates() == Qt::NoButton) {
-            m_modOnlyShortcut.modifier = Qt::KeyboardModifier(int(modifiersRelevantForGlobalShortcuts()));
-        } else {
-            m_modOnlyShortcut.modifier = Qt::NoModifier;
-        }
-    } else {
-        m_modOnlyShortcut.pressCount--;
-        if (m_modOnlyShortcut.pressCount == 0 &&
-            modifiersRelevantForGlobalShortcuts() == Qt::NoModifier &&
-            !workspace()->globalShortcutsDisabled()) {
-            if (m_modOnlyShortcut.modifier != Qt::NoModifier) {
-                const auto list = options->modifierOnlyDBusShortcut(m_modOnlyShortcut.modifier);
-                if (list.size() >= 4) {
-                    auto call = QDBusMessage::createMethodCall(list.at(0), list.at(1), list.at(2), list.at(3));
-                    QVariantList args;
-                    for (int i = 4; i < list.size(); ++i) {
-                        args << list.at(i);
-                    }
-                    call.setArguments(args);
-                    QDBusConnection::sessionBus().asyncCall(call);
-                }
-            }
-        }
-        m_modOnlyShortcut.modifier = Qt::NoModifier;
-    }
 }
 
 void Xkb::updateModifiers()
@@ -610,6 +569,8 @@ void KeyboardInputRedirection::init()
     m_keyboardLayout->init();
     m_input->installInputEventSpy(m_keyboardLayout);
 
+    m_input->installInputEventSpy(new ModifierOnlyShortcuts);
+
     KeyboardRepeat *keyRepeatSpy = new KeyboardRepeat(m_xkb.data());
     connect(keyRepeatSpy, &KeyboardRepeat::keyRepeat, this,
         std::bind(&KeyboardInputRedirection::processKey, this, std::placeholders::_1, InputRedirection::KeyboardKeyAutoRepeat, std::placeholders::_2, nullptr));
@@ -689,9 +650,6 @@ void KeyboardInputRedirection::update()
 
 void KeyboardInputRedirection::processKey(uint32_t key, InputRedirection::KeyboardKeyState state, uint32_t time, LibInput::Device *device)
 {
-    if (!m_inited) {
-        return;
-    }
     QEvent::Type type;
     bool autoRepeat = false;
     switch (state) {
@@ -725,6 +683,9 @@ void KeyboardInputRedirection::processKey(uint32_t key, InputRedirection::Keyboa
     event.setModifiersRelevantForGlobalShortcuts(m_xkb->modifiersRelevantForGlobalShortcuts());
 
     m_input->processSpies(std::bind(&InputEventSpy::keyEvent, std::placeholders::_1, &event));
+    if (!m_inited) {
+        return;
+    }
     m_input->processFilters(std::bind(&InputEventFilter::keyEvent, std::placeholders::_1, &event));
 }
 

keyboard_input.h

@@ -130,10 +130,6 @@ private:
     Qt::KeyboardModifiers m_modifiers;
     Qt::KeyboardModifiers m_consumedModifiers;
     xkb_keysym_t m_keysym;
-    struct {
-        uint pressCount = 0;
-        Qt::KeyboardModifier modifier = Qt::NoModifier;
-    } m_modOnlyShortcut;
     quint32 m_currentLayout = 0;
 
     struct {

modifier_only_shortcuts.cpp

@@ -0,0 +1,99 @@
+/********************************************************************
+ KWin - the KDE window manager
+ This file is part of the KDE project.
+
+Copyright (C) 2016, 2017 Martin Gräßlin <mgraesslin@kde.org>
+
+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; either version 2 of the License, or
+(at your option) any later version.
+
+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/>.
+*********************************************************************/
+#include "modifier_only_shortcuts.h"
+#include "input_event.h"
+#include "options.h"
+#include "screenlockerwatcher.h"
+#include "workspace.h"
+
+#include <QDBusConnection>
+#include <QDBusMessage>
+#include <QDBusPendingCall>
+
+namespace KWin
+{
+
+ModifierOnlyShortcuts::ModifierOnlyShortcuts()
+    : QObject()
+    , InputEventSpy()
+{
+    connect(ScreenLockerWatcher::self(), &ScreenLockerWatcher::locked, this, &ModifierOnlyShortcuts::reset);
+}
+
+ModifierOnlyShortcuts::~ModifierOnlyShortcuts() = default;
+
+void ModifierOnlyShortcuts::keyEvent(KeyEvent *event)
+{
+    if (event->isAutoRepeat()) {
+        return;
+    }
+    if (event->type() == QEvent::KeyPress) {
+        m_pressCount++;
+        if (m_pressCount == 1 &&
+            !ScreenLockerWatcher::self()->isLocked() &&
+            m_buttonPressCount == 0 &&
+            m_cachedMods == Qt::NoModifier) {
+            m_modifier = Qt::KeyboardModifier(int(event->modifiersRelevantForGlobalShortcuts()));
+        } else {
+            m_modifier = Qt::NoModifier;
+        }
+    } else {
+        m_pressCount--;
+        if (m_pressCount == 0 &&
+            event->modifiersRelevantForGlobalShortcuts() == Qt::NoModifier &&
+            !workspace()->globalShortcutsDisabled()) {
+            if (m_modifier != Qt::NoModifier) {
+                const auto list = options->modifierOnlyDBusShortcut(m_modifier);
+                if (list.size() >= 4) {
+                    auto call = QDBusMessage::createMethodCall(list.at(0), list.at(1), list.at(2), list.at(3));
+                    QVariantList args;
+                    for (int i = 4; i < list.size(); ++i) {
+                        args << list.at(i);
+                    }
+                    call.setArguments(args);
+                    QDBusConnection::sessionBus().asyncCall(call);
+                }
+            }
+        }
+        m_modifier = Qt::NoModifier;
+    }
+    m_cachedMods = event->modifiersRelevantForGlobalShortcuts();
+}
+
+void ModifierOnlyShortcuts::pointerEvent(MouseEvent *event)
+{
+    if (event->type() == QEvent::MouseMove) {
+        return;
+    }
+    if (event->type() == QEvent::MouseButtonPress) {
+        m_buttonPressCount++;
+    } else if (event->type() == QEvent::MouseButtonRelease) {
+        m_buttonPressCount--;
+    }
+    reset();
+}
+
+void ModifierOnlyShortcuts::wheelEvent(WheelEvent *event)
+{
+    Q_UNUSED(event)
+    reset();
+}
+
+}

modifier_only_shortcuts.h

@@ -0,0 +1,55 @@
+/********************************************************************
+ KWin - the KDE window manager
+ This file is part of the KDE project.
+
+Copyright (C) 2016, 2017 Martin Gräßlin <mgraesslin@kde.org>
+
+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; either version 2 of the License, or
+(at your option) any later version.
+
+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 KWIN_MODIFIER_ONLY_SHORTCUTS_H
+#define KWIN_MODIFIER_ONLY_SHORTCUTS_H
+
+#include "input_event_spy.h"
+#include <kwin_export.h>
+
+#include <QObject>
+
+namespace KWin
+{
+
+class KWIN_EXPORT ModifierOnlyShortcuts : public QObject, public InputEventSpy
+{
+    Q_OBJECT
+public:
+    explicit ModifierOnlyShortcuts();
+    ~ModifierOnlyShortcuts() override;
+
+    void keyEvent(KeyEvent *event) override;
+    void pointerEvent(MouseEvent *event) override;
+    void wheelEvent(WheelEvent *event) override;
+
+    void reset() {
+        m_modifier = Qt::NoModifier;
+    }
+
+private:
+    uint m_pressCount = 0;
+    Qt::KeyboardModifier m_modifier = Qt::NoModifier;
+    Qt::KeyboardModifiers m_cachedMods;
+    uint m_buttonPressCount = 0;
+};
+
+}
+
+#endif

plugins/platforms/x11/standalone/x11_platform.cpp

@@ -113,7 +113,6 @@ void X11StandalonePlatform::createPlatformCursor(QObject *parent)
         m_xinputIntegration->setCursor(c);
         // we know we have xkb already
         auto xkb = input()->keyboard()->xkb();
-        m_xinputIntegration->setXkb(xkb);
         xkb->reconfigure();
     }
 #endif

plugins/platforms/x11/standalone/xinputintegration.cpp

@@ -23,8 +23,9 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #include "platform.h"
 #include "x11cursor.h"
 
-#include "keyboard_input.h"
+#include "input.h"
 #include "x11eventfilter.h"
+#include "modifier_only_shortcuts.h"
 #include <kwinglobals.h>
 
 #include <X11/extensions/XInput2.h>
@@ -46,18 +47,17 @@ public:
     bool event(xcb_generic_event_t *event) override {
         xcb_ge_generic_event_t *ge = reinterpret_cast<xcb_ge_generic_event_t *>(event);
         switch (ge->event_type) {
-        case XI_RawKeyPress:
-            if (m_xkb) {
-                m_xkb->updateKey(reinterpret_cast<xXIRawEvent*>(event)->detail - 8, InputRedirection::KeyboardKeyPressed);
-            }
+        case XI_RawKeyPress: {
+            auto re = reinterpret_cast<xXIRawEvent*>(event);
+            kwinApp()->platform()->keyboardKeyPressed(re->detail - 8, re->time);
             break;
-        case XI_RawKeyRelease:
-            if (m_xkb) {
-                m_xkb->updateKey(reinterpret_cast<xXIRawEvent*>(event)->detail - 8, InputRedirection::KeyboardKeyReleased);
-            }
+        }
+        case XI_RawKeyRelease: {
+            auto re = reinterpret_cast<xXIRawEvent*>(event);
+            kwinApp()->platform()->keyboardKeyReleased(re->detail - 8, re->time);
             break;
-        case XI_RawButtonPress:
-            if (m_xkb) {
+        }
+        case XI_RawButtonPress: {
                 auto e = reinterpret_cast<xXIRawEvent*>(event);
                 switch (e->detail) {
                 // TODO: this currently ignores left handed settings, for current usage not needed
@@ -82,8 +82,7 @@ public:
                 m_x11Cursor->schedulePoll();
             }
             break;
-        case XI_RawButtonRelease:
-            if (m_xkb) {
+        case XI_RawButtonRelease: {
                 auto e = reinterpret_cast<xXIRawEvent*>(event);
                 switch (e->detail) {
                 // TODO: this currently ignores left handed settings, for current usage not needed
@@ -122,14 +121,9 @@ public:
     void setCursor(const QPointer<X11Cursor> &cursor) {
         m_x11Cursor = cursor;
     }
-    void setXkb(Xkb *xkb) {
-        m_xkb = xkb;
-    }
 
 private:
     QPointer<X11Cursor> m_x11Cursor;
-    // TODO: QPointer
-    Xkb *m_xkb = nullptr;
 };
 
 class XKeyPressReleaseEventFilter : public X11EventFilter
@@ -142,24 +136,16 @@ public:
 
     bool event(xcb_generic_event_t *event) override {
         xcb_key_press_event_t *ke = reinterpret_cast<xcb_key_press_event_t *>(event);
-        if (m_xkb && ke->event == ke->root) {
+        if (ke->event == ke->root) {
             const uint8_t eventType = event->response_type & ~0x80;
             if (eventType == XCB_KEY_PRESS) {
-                m_xkb->updateKey(ke->detail - 8, InputRedirection::KeyboardKeyPressed);
+                kwinApp()->platform()->keyboardKeyPressed(ke->detail - 8, ke->time);
             } else {
-                m_xkb->updateKey(ke->detail - 8, InputRedirection::KeyboardKeyReleased);
+                kwinApp()->platform()->keyboardKeyReleased(ke->detail - 8, ke->time);
             }
         }
         return false;
     }
-
-    void setXkb(Xkb *xkb) {
-        m_xkb = xkb;
-    }
-
-private:
-    // TODO: QPointer
-    Xkb *m_xkb = nullptr;
 };
 
 XInputIntegration::XInputIntegration(Display *display, QObject *parent)
@@ -207,11 +193,6 @@ void XInputIntegration::setCursor(X11Cursor *cursor)
     m_x11Cursor = QPointer<X11Cursor>(cursor);
 }
 
-void XInputIntegration::setXkb(Xkb *xkb)
-{
-    m_xkb = xkb;
-}
-
 void XInputIntegration::startListening()
 {
     // this assumes KWin is the only one setting events on the root window
@@ -236,11 +217,11 @@ void XInputIntegration::startListening()
     XISelectEvents(display(), rootWindow(), evmasks, 1);
     m_xiEventFilter.reset(new XInputEventFilter(m_xiOpcode));
     m_xiEventFilter->setCursor(m_x11Cursor);
-    m_xiEventFilter->setXkb(m_xkb);
     m_keyPressFilter.reset(new XKeyPressReleaseEventFilter(XCB_KEY_PRESS));
-    m_keyPressFilter->setXkb(m_xkb);
     m_keyReleaseFilter.reset(new XKeyPressReleaseEventFilter(XCB_KEY_RELEASE));
-    m_keyReleaseFilter->setXkb(m_xkb);
+
+    // install the input event spies also relevant for X11 platform
+    input()->installInputEventSpy(new ModifierOnlyShortcuts);
 }
 
 }

plugins/platforms/x11/standalone/xinputintegration.h

@@ -31,7 +31,6 @@ namespace KWin
 class XInputEventFilter;
 class XKeyPressReleaseEventFilter;
 class X11Cursor;
-class Xkb;
 
 class XInputIntegration : public QObject
 {
@@ -47,7 +46,6 @@ public:
         return m_hasXInput;
     }
     void setCursor(X11Cursor *cursor);
-    void setXkb(Xkb *xkb);
 
 private:
     Display *display() const {
@@ -59,8 +57,6 @@ private:
     int m_majorVersion = 0;
     int m_minorVersion = 0;
     QPointer<X11Cursor> m_x11Cursor;
-    // TODO: QPointer
-    Xkb *m_xkb = nullptr;
     Display *m_x11Display;
 
     QScopedPointer<XInputEventFilter> m_xiEventFilter;

pointer_input.cpp

@@ -239,10 +239,6 @@ void PointerInputRedirection::processButton(uint32_t button, InputRedirection::P
 {
     updateButton(button, state);
 
-    if (!m_inited) {
-        return;
-    }
-
     QEvent::Type type;
     switch (state) {
     case InputRedirection::PointerButtonReleased:
@@ -262,6 +258,11 @@ void PointerInputRedirection::processButton(uint32_t button, InputRedirection::P
     event.setNativeButton(button);
 
     m_input->processSpies(std::bind(&InputEventSpy::pointerEvent, std::placeholders::_1, &event));
+
+    if (!m_inited) {
+        return;
+    }
+
     m_input->processFilters(std::bind(&InputEventFilter::pointerEvent, std::placeholders::_1, &event, button));
 }
 
@@ -273,16 +274,16 @@ void PointerInputRedirection::processAxis(InputRedirection::PointerAxis axis, qr
 
     emit m_input->pointerAxisChanged(axis, delta);
 
-    if (!m_inited) {
-        return;
-    }
-
     WheelEvent wheelEvent(m_pos, delta,
                            (axis == InputRedirection::PointerAxisHorizontal) ? Qt::Horizontal : Qt::Vertical,
                            m_qtButtons, m_input->keyboardModifiers(), time, device);
     wheelEvent.setModifiersRelevantForGlobalShortcuts(m_input->modifiersRelevantForGlobalShortcuts());
 
     m_input->processSpies(std::bind(&InputEventSpy::wheelEvent, std::placeholders::_1, &wheelEvent));
+
+    if (!m_inited) {
+        return;
+    }
     m_input->processFilters(std::bind(&InputEventFilter::wheelEvent, std::placeholders::_1, &wheelEvent));
 }