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

[Martin Flöser]

graesslin added a comment.


  I need to point out that this creates a functional difference to Wayland and according to the latest rules of Plasma such changes are no longer allowed unless the implementation is done first for Wayland.
  
  Personally I am not really comfortable with such a large change to X11 nowadays. The code will be pretty much untested till the release and recent changes showed me that this is not a good idea. Especially I am no longer able to test the code (that's why I sent a mail to frameworks to step down as kglobalaccel maintainer, but so far nobody else stepped up).
  
  So personally I am rather inclined to give you a -1 on it as I just think it's too dangerous. The kglobalaccel code is not good, but it works mostly. Proper fix will be in Wayland.

INLINE COMMENTS

> kglobalaccel_x11.cpp:278-287
> -	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);

The shift handling code has shown regressions whenever it was touched. Also on Wayland I needed several tries to get it right. I would prefer if it were not touched any more.

This is not as simple as it looks. There are besties out there like Alt+Shift+Backtab as a global shortcut and a generic implementation breaks quickly there. It is quite likely that this change would break Alt+(Shift)+Tab in KWin.

REPOSITORY
  R268 KGlobalAccel

REVISION DETAIL
  https://phabricator.kde.org/D6234

To: dfaure, graesslin
Cc: #frameworks
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170616/f5b131f1/attachment.html>