Add support for functional key codes from kitty keyboard protocol

[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.

[TimonPost]

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.

[TimonPost]

One issue can be resolved quite easily. Just add some entries to KeyModifiers and make sure it is correctly filled.

[pianohacker]

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 kitty +kitten show_key -m kitty, a demo for the protocol). Here, I press Right Alt, then Left Alt, then release Right Alt, then Left Alt:

RIGHT_ALT PRESS 
CSI 57449 u

Alt+LEFT_ALT PRESS 
CSI 57443 ; 3 u

Alt+RIGHT_ALT RELEASE 
CSI 57449 ; 3 : 3 u

Alt+LEFT_ALT RELEASE 
CSI 57443 ; 3 : 3 u

The Alt modifier is only reported for the second Alt press, where I'm already holding down another Alt key.

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 xev). Which would you prefer an easier-to-understand API or one that matches APIs?

As far as the Keypad keys go, I agree. The duplication is confusing. When I was working on the extra modifiers PR, I had a thought about a fourth field for KeyEvent: extra, state, or something similar. The kitty keyboard protocol reports the of Caps Lock and Num Lock as additional modifiers, but I didn't want to add those to the modifiers field, as it seems like it would lead to confusion. ("My events that used to match don't match unless I mask out the state of Num Lock?") Maybe this extra field could have a flag like KeyEventState::KEYPAD?

[TimonPost]

But can you maybe do something like this:

let modifiers: KeyModifiers;

match modifier_key_code {
   LeftAlt | RightAlt => modifiers.set(Alt),
   LeftShift | RightShift =>  modifiers.set(Shift),
   LeftMeta | RightMeta => modifiers.set(Meta)
}

Or is that what you try to argue isnt possible?

[TimonPost]

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.

[TimonPost]

Which would you prefer an easier-to-understand API or one that matches APIs

I prefer consistency and less duplication. I want the user to be able to match on the modifiers of key_event and be able to tell if they pressed shift, alt, meta, ctrl. You can still keey the 'Left/Right' pairs in ModifierKeyCode. I think we might even try to support that on windows if possible.

[pianohacker]

I just pushed a couple of commits.

• 763ef18 makes the discussed change, including the modifier mask in the key press event.
• a9d8b33 makes a change to keypad key presses. They're now mapped to normal keycodes. There's a few things to note here:
  • Decimal is mapped to ., and Separator is mapped to ,. This is a locale-specific thing.
  • The Begin key might be a historical artifact... but it's the one thing on the keypad that doesn't have a normal equivalent.
  • There's a new state field, which currently only indicates whether the key press was on the keypad. I think it could also hold Num Lock and Caps Lock.

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.

[TimonPost]

Just a few comments but is good to go.

[TimonPost]

Thanks a lot @pianohacker !