New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for functional key codes from kitty keyboard protocol #691
Add support for functional key codes from kitty keyboard protocol #691
Conversation
Like you said it is a bit odd now that KeyCode and Keypad contain similar key events and that modifiers now are duplicated in KeyCode::Modifiers and KeyModifiers struct. I would love to see if we can bring the two closer together. I prefer no duplication as it only confuses the user on where to find certain events. e.g. it might match on KeyCode::Up but rather KeyCode::Keypad(Up) is sent. Or both are sent which can also cause confusion. Similar to modifiers, if you get KeyCode::Modifier but the KeyEvent.modifiers don't contain this modifier it might be confusing as well. |
One issue can be resolved quite easily. Just add some entries to KeyModifiers and make sure it is correctly filled. |
The modifier mask being set for the modifier's keypress event is an interesting one. The precedent is to not set the modifier bits for the keypress, which I do think makes sense. Take the following example (from
The It is somewhere else to look for the info, and it is confusing. However, it does match the precedent of kitty (and X11, as reported by As far as the |
But can you maybe do something like this:
Or is that what you try to argue isnt possible? |
I agree those so called modifiers Caps Lock, Num Lock, Super, Hyper are to vaque should not be added to the key modifiers. I want to reserver that for SHIFT, CTRL, ALT, META keys. |
I prefer consistency and less duplication. I want the user to be able to match on the |
I just pushed a couple of commits.
Speaking of, Caps Lock feels worth adding in a followup PR. It would be nice for something reading a password, for instance, to be able to warn if Caps Lock is on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments but is good to go.
a9d8b33
to
a8ae84a
Compare
Thanks a lot @pianohacker ! |
Followup to #688 .
Open question: is the nested structure for the new keycodes the best way? I didn't want to inflate the
KeyCode
enum, but the new structure results in somewhat verbose names.