Now with unittest for the xcbKeyPressEventToQt function.
Details
Details
Adding a global shortcut with Alt+Shift+Right works now.
Previously it would decode it at "Alt+" at keypress time.
Meta+Shift+Print works again as well, and the unittest tests more cases.
Diff Detail
Diff Detail
- Repository
- R278 KWindowSystem
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
How do we know that this doesn't break other code? This code is for example used in KWin's Alt+Tab handling which is it's own fair beast. Any changes here might break the code as it might be bug-to-bug compatible.
We had the changes one month in master and nobody noticed the severe brokeness, how are we going to ensure that further patches to this extremely fragile code will not introduce further regressions?
Comment Actions
My main concern remains: how do we know that it doesn't introduce further regressions? Did you test that Alt+tab and alt+shift+tab still works? Also when changing keyboard layout?
For this frameworks release I think it's too late: the regression potential is just too high.
Comment Actions
I've tested meta+shift+arrow, alt+shift+arrow, meta+shift+PrtSc and they are working for me, thanks! So far I didn't notice any regressions. I'll keep this version around and will report problem if I see one.
Unfortunately I'm unable to properly review the code itself since I have no knowledge in this area.
Comment Actions
Thanks for the test -- I hope you remembered to revert locally the commit 68e35f234c in KGlobalAccel so that this code is called again.
Comment Actions
I tested that Alt+Tab / Alt+Shift+Tab still works, and put that into the unittest. Anything else that worries you?
We had the changes one month in master and nobody noticed the severe brokeness, how are we going to ensure that further patches to this extremely fragile code will not introduce further regressions?
By unittesting the main key handling method, not just the additional conversion methods, and by committing the changes at the very beginning of a release cycle to maximize testing.
With the additional testing I am more confident now about this patch.
I think it should go in - but indeed certainly not for 5.39, rather just after 5.39.
The right solution when code is fragile and we fear changing it, is to make it less fragile, by removing duplication (done), adding unittests (done) and doing more user-testing (to be done again...).
Leaving such code untouched because we fear touching it, is not the right solution.
Comment Actions
I hope you remembered to revert locally the commit.
Of course I didn't, sorry. But now I did. I reverted the KGlobalAccel commit, tested that previous bugs reappeared, then applied this patch and tested again - bugs are gone.
So now it is properly tested on my side.
Comment Actions
@graesslin If this isn't currently merge-worthy, is there anything we can do to make it so?
Comment Actions
I'm still afraid that this will cause regressions again when introduced. There is just too much weird stuff in keyboard handling. I would prefer if we do such improvements only on Wayland and don't touch the X code any more. But I also don't want to block. Commit on your own risk, if something breaks on X,. I don't care.