[kglobalaccel] src/runtime/plugins/xcb: Revert "KGlobalAccel: port to KKeyServer's new method symXModXToKeyQt, to fix numpad keys"

[Martin Flöser]

Git commit 68e35f234ca7d866bcb54442d22892450d871848 by Martin Flöser.
Committed on 25/09/2017 at 15:30.
Pushed by graesslin into branch 'master'.

Revert "KGlobalAccel: port to KKeyServer's new method symXModXToKeyQt, to fix numpad keys"

This reverts commit 2c20ddff034e4958bf0536ca91ae9e444955305d.

I'm reverting as we don't have a fix yet and the next release is too
close to risk any changes. As explained in the original review request
for this feature: touching this code base is dangerous. The code is
extremely fragile.

A month of living in master was not enough to spot the severe regressions
we had. As we are now only a few days away from next release we cannot
expect to discover regressions any fix would cause. I don't really see a
solution to this problem: any change might cause issues especially in
KWin, KScreenLocker and KGlobalAccel. All of them have the problem that
the maintainer of those components is not running X11 and won't spot the
regressions. Futhermore we don't have any test cases for the complete key
handing stack on X11. We won't find the regressions. My personal
recommendation would be to no longer change the X11 side and instead all
together work on Wayland and get it ready. There the world looks better:
we have a sane input stack which is completly unit tested. The first
regression caused by the changes here was discovered by a KWin test case
for Wayland. That's the world where we can change and improve the stack.
Let X die, long live Wayland!

If someone wants to take on the task to improve the X11 stack here I
expect some serious work on the testability of this fragile stack. If we
see that the shortcuts will still match I would be way more confident to
accept a change.

CCMAIL: kde-frameworks-devel at kde.org
BUG: 384597

M  +45   -7    src/runtime/plugins/xcb/kglobalaccel_x11.cpp

https://commits.kde.org/kglobalaccel/68e35f234ca7d866bcb54442d22892450d871848

diff --git a/src/runtime/plugins/xcb/kglobalaccel_x11.cpp b/src/runtime/plugins/xcb/kglobalaccel_x11.cpp
index d76f1d5..9b37c7b 100644
--- a/src/runtime/plugins/xcb/kglobalaccel_x11.cpp
+++ b/src/runtime/plugins/xcb/kglobalaccel_x11.cpp
@@ -122,7 +122,6 @@ bool KGlobalAccelImpl::grabKey( int keyQt, bool grab )
         // can remove shift for some keys. (all the %&* and such)
         if( !(keyQt & Qt::SHIFT) &&
             !KKeyServer::isShiftAsModifierAllowed( keyQt ) &&
-            !(keyQt & Qt::KeypadModifier) &&
             keySymX != xcb_key_symbols_get_keysym(m_keySymbols, keyCodeX, 0) &&
             keySymX == xcb_key_symbols_get_keysym(m_keySymbols, keyCodeX, 1) )
         {
@@ -244,12 +243,51 @@ bool KGlobalAccelImpl::x11KeyPress(xcb_key_press_event_t *pEvent)
     xcb_ungrab_keyboard(c, XCB_TIME_CURRENT_TIME);
     xcb_flush(c);
 
-    int keyQt;
-    if (!KKeyServer::xcbKeyPressEventToQt(pEvent, &keyQt)) {
-        qCWarning(KGLOBALACCELD) << "KKeyServer::xcbKeyPressEventToQt failed";
-        return false;
-    }
-    //qDebug() << "keyQt=" << QString::number(keyQt, 16);
+    xcb_keycode_t keyCodeX = pEvent->detail;
+    uint16_t keyModX = pEvent->state & (g_keyModMaskXAccel | KKeyServer::MODE_SWITCH);
+
+    xcb_keysym_t keySymX = xcb_key_press_lookup_keysym(m_keySymbols, pEvent, 0);
+
+	// If numlock is active and a keypad key is pressed, XOR the SHIFT state.
+	//  e.g., KP_4 => Shift+KP_Left, and Shift+KP_4 => KP_Left.
+    if (pEvent->state & KKeyServer::modXNumLock()) {
+		// If this is a keypad key,
+		if( keySymX >= XK_KP_Space && keySymX <= XK_KP_9 ) {
+			switch( keySymX ) {
+
+				// Leave the following keys unaltered
+				// FIXME: The proper solution is to see which keysyms don't change when shifted.
+				case XK_KP_Multiply:
+				case XK_KP_Add:
+				case XK_KP_Subtract:
+				case XK_KP_Divide:
+                case XK_KP_Enter:
+					break;
+
+				default:
+					keyModX ^= KKeyServer::modXShift();
+			}
+		}
+	}
+
+	int keyCodeQt;
+	int keyModQt;
+	KKeyServer::symXToKeyQt(keySymX, &keyCodeQt);
+	KKeyServer::modXToQt(keyModX, &keyModQt);
+
+	if ((keyModQt & Qt::SHIFT) && !KKeyServer::isShiftAsModifierAllowed( keyCodeQt ) ) {
+#ifdef KDEDGLOBALACCEL_TRACE
+		qCDebug(KGLOBALACCELD) << "removing shift modifier";
+#endif
+        if (keyCodeQt != Qt::Key_Tab) { // KKeySequenceWidget does not map shift+tab to backtab
+            static const int FirstLevelShift = 1;
+            keySymX = xcb_key_symbols_get_keysym(m_keySymbols, keyCodeX, FirstLevelShift);
+            KKeyServer::symXToKeyQt(keySymX, &keyCodeQt);
+        }
+		keyModQt &= ~Qt::SHIFT;
+	}
+
+	int keyQt = keyCodeQt | keyModQt;
 
 	// All that work for this hey... argh...
     if (NET::timestampCompare(pEvent->time, QX11Info::appTime()) > 0) {