| graesslin |
This required adding a new method symXModXToKeyQt since
symXToKeyQt (without modifier as input) has no way to find out
whether to use XK_KP_1 or XK_1. For this reason symXToKeyQt is now
deprecated.
Includes a unittest for keyQtToSymX, keyQtToModX and symXModXToKeyQt.
CCBUG: 183458
After porting kglobalaccel to KKeyServer::xcbKeyPressEventToQt,
the following global shortcuts were successfully tested:
Ctrl+1, Ctrl+Num+1, Ctrl+Num+/, Ctrl+F1, Ctrl+& (implicit shift)
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
| src/platforms/xcb/kkeyserver.cpp | ||
|---|---|---|
| 160–181 | This looks very unrelated to the described change. Maybe an own commit? | |
| 774 | xcb_is_keypad_key is not part of any xcb component KWindowSystem looks for. | |
| 792 | why are you calling a deprecated method from a new method? | |
| src/platforms/xcb/kkeyserver_x11.h | ||
| 151–161 | if it's getting deprecated it must be wrapped in ifdef, shouldn't it? | |
| 160 | for new code I would find it better to use the proper types of either uint32_t or quint32. | |
| src/platforms/xcb/kkeyserver.cpp | ||
|---|---|---|
| 160–181 | Well those are the XK_KP_* codes, i.e. Num Keypad keys, so it's related. But yeah, it would probably work without this change, it just seems best to update the full list from Qt. Do you insist on a separate commit? | |
| 774 | Oh. OK, then I'll go back to >= and <=, it's just as simple anyway. | |
| 792 | it was simpler, but ok, I'll refactor ;) | |
| src/platforms/xcb/kkeyserver_x11.h | ||
| 151–161 | now that it's not called by the new method, it's possible indeed ;) | |
| 160 | Not very symmetrical with keyQtToSymX, but whatever makes you happy. | |
| src/platforms/xcb/kkeyserver.cpp | ||
|---|---|---|
| 976–980 | I would say yes, because e.g. Ctrl+& (on qwerty) is going to include Shift no matter which application is calling this method, and it should be removed (just like Qt does when recording "Ctrl+&" rather than "Ctrl+Shift+&" or "Ctrl+Shift+7". But anyway in practice the only other caller of this method is kwin, and it only cares for [Ctrl|Alt|None]+[Left|Right|Up|Down|Space|Return|Escape], if I read the code correctly. See https://lxr.kde.org/ident?_i=xcbKeyPressEventToQt&_remember=1 So, while technically this code could stay in kglobalaccel, it still seems more correct to me to have it here in the generic "xcb key event to keyQt conversion" code (and this avoids calling xcb_key_symbols_alloc again in kglobalaccel). | |
| src/platforms/xcb/kkeyserver_x11.h | ||
| 161 | strange, I see this: typedef struct xcb_key_press_event_t { uint16_t state; /**< */ [...] and grep modifiers /usr/include/xcb/xproto.h| grep uint shows consistent use of uint16_t. xkb_mod_mask_t seems to come from xcbcommon which is unrelated to the APIs used here, right? | |
Sorry for having missed your pings. I have something like 1000 unread mails in the frameworks folder :-(
Your change caused a regression in KWin and now a unit test is failing. Please see: https://build.kde.org/user/graesslin/my-views/view/mgraesslin%20maintained/job/Plasma%20kwin%20kf5-qt5%20SUSEQt5.9/134/testReport/junit/(root)/TestSuite/kwin_testWindowSelection/
I'm trying to run that unittest, but it aborts without Xwayland, and even after upgrading to OpenSuSE Leap 42.3, I still don't find a package containing Xwayland. It seems to be part of recent Xorg?
I have xorg-x11-server-7.6_1.18.3-22.1.x86_64
@dfaure according to our Docker image the SUSE package you need is xorg-x11-server-wayland
To me the question is rather: can we ensure that other users of KKeyServer didn't get broken by the change. What KWin did could be considered a bug or an application misuse. KWin used the Qt key and compared it directly to Qt::Key_Enter. That one doesn't work anymore as it's no longer Qt::Key_Enter, but KeyEnter | NumpadModifier. So any usage of KKeyServer which just didn't care about whether it's numpad or not will be broken if it did a direct key comparison. Or if they had a special numpad handling it will be broken. Looking at lxr I fear that also kscreenlocker might be broken now.
That's the problem we have with changing this legacy code as I mentioned in the other bug report - even if it's a bug fix or a sensible extension like in this case. The code around just breaks as it expects a special behavior and we are not able to find all the cases without it breaking just everywhere around us. I hope you understand better why I was so pessimistic about the change in the other review. It's just my experience that touching this code breaks stuff.
Thanks for the investigation. Actually the deprecated method wasn't supposed to change behaviour. This was actually a consequence of the rework of the patch, which initially didn't have this problem :-)
Proper fix is up for review: https://phabricator.kde.org/D7296
Sorry, to be a bit late, but after updating to kwindowsystem 5.38, global shortcuts "meta+shift+<any_arrow_key>" and "alt+shift+<any_arrow_key>" stopped working on my system (Arch Linux). They can be configured no-problem (they are recognized and saved in configuration), but they do nothing afterwards. I'm writing it here because I've tracked the problem down to this commit. Is it reproducible on your side?
Confirmed. I just today experienced the "Rectangular screenshot" keyboard shortcut (Meta+Shift+PrintScreen) having stopped working on two machines.
This seems to be a big problem. Kglobalaccell got more new bugs reported this week, than normally in a year.
When I press Meta+Shift+PrintScreen previously I got QKeySequence(Meta+Shift+Print) which is correct but now I get QKeySequence(Meta+Shift+SysReq) which fails to map (on German layout SysReq is the key and Shift+SysReq results in PrintScreen)
Thanks for the reports. These shortcuts get converted correctly (https://commits.kde.org/kwindowsystem/af6d5ab12b2d99b39a73ed8df903368470ded55b), so the problem must be in the shift handling in xcbKeyPressEventToQt().
Alt+Shift+right bug fixed in https://phabricator.kde.org/D7829
Meta+Shift+Print is another issue, I get
KGlobalAccelImpl::x11KeyPress: keyQt= "1300000a" "Meta+Shift+SysReq"
Qt (at shortcut recording time) sees this as Meta+Shift+Print while KWindowSystem (xcbKeyPressEventToQt) sees it as Meta+Shift+SysReq.
Qt does xkb_state_key_get_one_sym( state=51 , code=6b ) = ff61 (XK_Print)
xcbKeyPressEventToQt does xcb_key_press_lookup_keysym e->state=51 keyModX= 41 keySym0= ff61 (XK_Print) keySym1= ff15 (XK_Sys_Req)
(all numbers are hex)
(51 or 41 is the same, that's just removing Numlock)
I'm at a loss at this point though. Should xcbKeyPressEventToQt rather use xkb_state_key_get_one_sym to be closer to the code in Qt? Or what am I missing?
Looking at xmodmap -pke output I would have expected that any use of Shift+Print leads to SysReq, but even xev disagrees. Are those tools outdated nowadays?
Short google gave me: https://lists.freedesktop.org/archives/xcb/2010-November/006572.html - please have a look at the reply, it might be also our issue here.