This commit requires KF 5.36.
BUG: 183458
Details
Details
the following global shortcuts were successfully tested:
Ctrl+1, Ctrl+Num+1, Ctrl+Num+/, Ctrl+F1, Ctrl+& (implicit shift)
Diff Detail
Diff Detail
- Repository
- R268 KGlobalAccel
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
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.
| src/runtime/plugins/xcb/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. | |
Comment Actions
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.
Comment Actions
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.
| src/runtime/plugins/xcb/kglobalaccel_x11.cpp | ||
|---|---|---|
| 278–287 | I did not change one inch of that logic, I just moved it to KKeyServer::xcbKeyPressEventToQt. | |
Comment Actions
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.
| src/runtime/plugins/xcb/kglobalaccel_x11.cpp | ||
|---|---|---|
| 278–287 | 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. | |
Comment Actions
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.