D6234: KGlobalAccel: port to KKeyServer's new method symXModXToKeyQt, to fix numpad keys
dfaure created this revision. Restricted Application added a project: Frameworks. REVISION SUMMARY This commit requires KF 5.36. BUG: 183458 TEST PLAN the following global shortcuts were successfully tested: Ctrl+1, Ctrl+Num+1, Ctrl+Num+/, Ctrl+F1, Ctrl+& (implicit shift) REPOSITORY R268 KGlobalAccel BRANCH master REVISION DETAIL https://phabricator.kde.org/D6234 AFFECTED FILES src/runtime/plugins/xcb/kglobalaccel_x11.cpp To: dfaure, graesslin Cc: #frameworks
D6234: KGlobalAccel: port to KKeyServer's new method symXModXToKeyQt, to fix numpad keys
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
D6234: KGlobalAccel: port to KKeyServer's new method symXModXToKeyQt, to fix numpad keys
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. After reading through KWin code together with your KWindowSystem change this might be a no issue and just work. So only needs testing. REPOSITORY R268 KGlobalAccel REVISION DETAIL https://phabricator.kde.org/D6234 To: dfaure, graesslin Cc: #frameworks
D6234: KGlobalAccel: port to KKeyServer's new method symXModXToKeyQt, to fix numpad keys
dfaure added a comment. I don't use wayland, I use X11. I bet I'm not the only one. As long as that's the case, fixing bugs in the X11 implementation makes a lot of sense. Man it's demotivating to contribute to KDE. Users say all sorts of bad things about KDE, and then future-ex-maintainers reject your contribution. Great. INLINE COMMENTS > graesslin wrote in kglobalaccel_x11.cpp:278-287 > 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. I did not change one inch of that logic, I just moved it to KKeyServer::xcbKeyPressEventToQt. REPOSITORY R268 KGlobalAccel REVISION DETAIL https://phabricator.kde.org/D6234 To: dfaure, graesslin Cc: #frameworks
D6234: KGlobalAccel: port to KKeyServer's new method symXModXToKeyQt, to fix numpad keys
graesslin added a comment. > Man it's demotivating to contribute to KDE. Users say all sorts of bad things about KDE, and then future-ex-maintainers reject your contribution. Great. Yes sure, and you can imagine how many angry mails and bug reports I get when things break. I made the experience that touching our X11 implementations always results in regressions. And nobody notices them in the testing period. Unfortunately I can give you a long list of regression we had in Plasma 5.10 only on X11. This makes me being extremely conservative when it comes to changing the X11 code base. The number of developers fully understanding X11 is really low in our community and most of them are working on Wayland now. Nobody is going to detect if we introduce a regression in kglobalaccel prior to the release. And due to the nature of the frameworks distros will ship them and then we as the Plasma team have yet another shit storm because we have a broken global shortcuts handling. So yes I'm extremely careful when it comes to touching this code base. INLINE COMMENTS > dfaure wrote in kglobalaccel_x11.cpp:278-287 > I did not change one inch of that logic, I just moved it to > KKeyServer::xcbKeyPressEventToQt. I'm sorry I didn't notice. I first noticed this review and it has no dependency set. It looked like the code got dropped. REPOSITORY R268 KGlobalAccel REVISION DETAIL https://phabricator.kde.org/D6234 To: dfaure, graesslin Cc: #frameworks
D6234: KGlobalAccel: port to KKeyServer's new method symXModXToKeyQt, to fix numpad keys
dfaure added a comment. I'm completely agree about being careful, as long as it's not the point of not fixing bugs :-) If you can think of more shortcuts that I should check, I'm happy to add them to the unittest and test them with kglobalaccel. REPOSITORY R268 KGlobalAccel REVISION DETAIL https://phabricator.kde.org/D6234 To: dfaure, graesslin Cc: #frameworks
D6234: KGlobalAccel: port to KKeyServer's new method symXModXToKeyQt, to fix numpad keys
dfaure added a comment. Ping? it seems like you are OK with this after all? REPOSITORY R268 KGlobalAccel REVISION DETAIL https://phabricator.kde.org/D6234 To: dfaure, graesslin Cc: #frameworks
D6234: KGlobalAccel: port to KKeyServer's new method symXModXToKeyQt, to fix numpad keys
This revision was automatically updated to reflect the committed changes. Closed by commit R268:2c20ddff034e: KGlobalAccel: port to KKeyServer's new method symXModXToKeyQt, to fix numpad… (authored by dfaure). REPOSITORY R268 KGlobalAccel CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6234?vs=15488&id=18191 REVISION DETAIL https://phabricator.kde.org/D6234 AFFECTED FILES src/runtime/plugins/xcb/kglobalaccel_x11.cpp To: dfaure, graesslin Cc: #frameworks
