serenity
serenity copied to clipboard
SerenityOS
LibGUI: Handle application keyboard shortcuts in keydown event handlers
Moving keyboard shortcuts to be handled by keydown handlers allows widgets to prevent the shortcut behavior from activating.
After some fixes to make tools properly determine whether a KeyEvent is accepted or ignored, this allows editing a layer name in PixelPaint without changing tools, where previously pressing 'n' for example would cause the Pen Tool to be selected.
Note that I haven't gone through and tested this change with every single application, some widgets may not properly be ignoring keydown events where they should be. If such a widget is focused, this will result in all shortcuts for an application being eaten and not activated. To fix this, we need to enforce that all keydown event handlers properly set Event.is_accepted() to false (it defaults to true) in cases where the event should continue bubbling.
Testing this in TextEditor and HackStudio, things seemed to work as I would expect, but there may be some widgets that I missed that may be improperly eating events.
This should probably also be expanded to mouse shortcuts, but to keep the scope of this PR simple, I left that code as-is.
Fixed some indentation that VSCode left in :( hopefully it passes linting now.
Surely the LibGUI change will affect other applications? I doubt PixelPaint is the only one affected by the reversal of the order that actions and keydown events are processed?
It shouldn't affect cases where the keydown event is allowed to bubble correctly. I think it's likely that some widgets will not be properly calling event.ignore() when they don't act on an event because it hasn't been necessary for the events to bubble in most cases. If the goal is to have everything working, I can go through and test all the applications, but I don't know if I can guarantee I'll catch every widget since I don't know them all that well.
I can go through and test all the applications, but I don't know if I can guarantee I'll catch every widget since I don't know them all that well.
If it turns out that a ton of widgets need changed, might ping Andreas to take a look before you go editing 100 files.
I did some testing and ended up fixing some issues with my implementation of this system in Window.cpp and Widget.cpp. In the process, I noticed some applications that weren't calling event.ignore() correctly and fixed those, the fixes for those are quite simple and allow shortcuts to work properly in those applications.
Code looks fine to me, but I tried this out and the first thing I noticed is that the keyboard shortcuts in Terminal don't work. :^(
Code looks fine to me, but I tried this out and the first thing I noticed is that the keyboard shortcuts in Terminal don't work. :^(
I've added a fix for this. Unfortunately, it's not exactly an ideal fix since it hardcodes that Ctrl+Shift combos are ignored by the terminal widget. Any suggestions on a better solution for this are welcome, but for now there is a FIXME added to make sure people are aware of this in future.
One idea I had was that key combos shouldn't be passed to the terminal when there's a selection, which I've noted in the FIXME, but since that only covers the case of the copy action, I didn't integrate that yet. We need a general solution for Ctrl+Shift+K clear terminal output shortcut to also work.
Another alternative is just to say that all shortcuts should take precedence over any terminal input, but I'm not sure what kind of implications that would have since I don't know much about escape sequences can be used in the terminal.
Another alternative is just to say that all shortcuts should take precedence over any terminal input, but I'm not sure what kind of implications that would have since I don't know much about escape sequences can be used in the terminal.
Updated to this behavior instead, since this is probably the better solution (at least for now) since it should match the previous behavior.