fix: Correct SUPER modifier key handling in kitty protocol

[gabyx]

What does this PR do:
Fix a bug in handling modifiers correctly for the kitty protocol.

Fixes: #4436, Comment here (maybe #4589)

How was it done?:
There is a problem in encode_kitty function which I tried to debug and hopefully fixed it.
I guess the boolean use_legacy which determines to encode legacy text keys is wrong.

• In the original any modifiers as SUPER (in nvim which sets KittyKeyboardFlags to 1)
  went through this legacy if which is wrong.

• Add a test case to check everythin works.

@wez : As the whole protocol handling is quite difficult and I could not match quite all switched to the protocol.
@happenslol: Thanks for the Nix flake to make debugging on NixOS work.

[wez]

Thanks for this! There's some more stuff to consider than is just in this PR.

Something that is absent is logic to populate these new key flags.
The only platform that supports those is unix. IIRC, this is the function that is responsible:

wezterm/window/src/os/x11/keyboard.rs

Line 383 in fde9267

pub fn get_key_modifiers(&self) -> Modifiers {

[wez]

Defining META as its own thing needs some care and attention throughout the wezterm crates as well as the termwiz crate, as we've always treated it as an alias of ALT, which you can see happening on line 515 above.

This particular line in this PR has no effect because it is evaluated after that line.

Breaking it out into its own thing needs to consider:

• What happens for non-kitty encodings when this modifier is used? Is it simply ignored?
• How does it get set? This PR doesn't seem to add any OS-specific logic to map the incoming key press from the windowing layer to actually set either the META or HYPER flags
• What happens to folks that have defined key assignments using META as the modifier? Do those effectively stop working when they upgrade? Is that ok? (probably?)

[gabyx]

@wez : Thanks for the reply, I am not yet done fixing my tests, did not yet adapt the asserts (will do!).

• Thanks for the hint to populate the flags. So that means Meta and Hyper will not work on MacOS?.
• Is it possible to leave the modifiers in this PR and come up with another one to gracefullt add these under Unix at least?

I am still not quite sure, if the kitty thing is all correct, but at least it seams so for the legacy_key part.
I truely appreciate what you did here, did you look at the kitty's source code for all that encode_kitty stuff?
Its such a convoluted complicated spec haha.

[wez]

Only X11 (and Wayland) have META and HYPER modifiers as a concept. Neither Windows nor macOS have equivalents.

I think it is reasonable to define these new modifier flags. I don't think we need to go to extreme lengths to handle META/HYPER elsewhere or for backwards compatibility, but we do need to consider how to handle them for the non-kitty keyboard scenario, which may just be "pretend they don't exist". Will need to compare and contrast behavior with other unix terminal emulators when using those keys.

I did not read the kitty source for this; only the spec docs, which I agree are not especially clear in a couple of places.

[gabyx]

@wez: Ok, thanks for the information about Meta and Hyper. This is very useful. So to not clutter up this PR as a bugfix. lets go with the pragmatic approach and first merge this fix. Like it is presented now.
Is that good enough?

• So basically this PR fixes SUPER modifier key handling in kitty protocol.

Other notes for later:

• Kitty looks at the following keys keys from xkb. So it uses the real keys not the xkb modifiers which are are only. So I suspect wezterm needs to detect that differently than using mod_is_active ?

  Note: Hyper and Meta -> "the USB standard only supports 4 modifier keys and that doesn’t include Hyper." 1. This means an USB keyboard will not be able to send such a Key. However on *nix -> with xkb, you can (if you manage to pull it off, map any keycode to any Modifier 1-5 (mod1 - mod5). And wezterm at the moment registers:

  *Real Xkb

  • SHIFT <- xkb::MOD_NAME_SHIFT [implemented]
  • CTRL <- xkb::MOD_NAME_CTRL [implemented]
  • ALT <- xkb::MOD_NAME_ALT := Mod1 [implemented] (so what ever is Mod1 maps to ALT in wezterm)
  • SUPER <- xkb::MOD_NAME_LOGO := Mod4 [implemented] (so what ever is Mod4 maps to SUPER in wezterm)

  I suggest that we introduce MOD1-MOD5 as new wezterm modifiers (only handles really on *nix with xkb)

  • Mod1 <- xkb:MOD_NAME_MOD1 := Mod1 [not implemented]
  • ...
  • Mod5 <- xkb::MOD_NAME_MOD2 := Mod5

  (See the issue)

[wez]

I think this PR is mergeable as-is.

Re: other modifiers and the future, are you proposing the addition of MOD1..MOD5 to the Modifiers type in wezterm-input-types? I think that those could be added (even though eg: ALT is already implicitly handled under a different name) in a similar way to the LEFT_XXX and RIGHT_XXX flags; those are populated if the system can make the distinction, but are otherwise non-essential and can then be inspected by eg: the kitty protocol encoding logic.

A potential wrinkle with that is: the flags type is 16 bits wide and there aren't enough available bits to represent 5 additional modifiers, so perhaps we should just add the mods that are not currently represented. I'd like to avoid making Modifiers 32 bits wide if we can avoid it, but if it doesn't change the size of aggregate types (like the size of the entries in various key tables, where the cardinality * the size of the struct represents an increase in RAM usage) then there's no real argument against it.

[gabyx]

@wez: Jeah it might be the only sane way to add Mod1-Mod5 to the table an check against where the other xkb types are handled. This addition will not hurt wezterm. But we still dont know which keys Mod1-Mod5 will be what “thing” for the kitty protocol. (to what Mod? did Hyper map?)

I think, Kitty is doing this by looking at the code: its checking the virtual modifier table with x11 calls and xkb and this is somewhat involved and as far as I could grasp was that this will correlate Super_L and other modifiers like Hyper_L/R to the 8 different modifiers xkb knows (Shift, Ctrl, Lock, Mod1-5) (xmodmap -pm). When we know this table (kitty does that at startup)
we know for lets say Hyper_L maps to Mod3 (the user chose so) , if Mod3 was pressed, then we can encode a “Hyper” modifier in the kitty protocol.

So basically function:

wezterm/window/src/os/x11/keyboard.rs

Line 383 in fde9267

pub fn get_key_modifiers(&self) -> Modifiers {

is somewhat only partially correct, it checks for Mod4 and relating this to Super. But the user might have changed this, it maybe has chosen that a certain key activates Super_L an mapped this to Mod3.

I hope my explanations were correct and understandable. Here is a statement from the xkb maintainers: xkbcommon/libxkbcommon#232 (comment)

• I think without implementing the exact same code as in kitty for X11(a solutiuon over virtual modifier table) and Wayland (workaround with a loop over the keys
  wezterm preassumes some standard mapping from Alt, Shift, Ctrl, Super to Mod1-5.

Currently : I am trying to test to implement the wayland C code and fiddeling a bit within the function : keyboard.rs:update_keymap (see : https://github.com/gabyx/wezterm/tree/feature/identify-modifiers-wayland)

@wez: You can merge this PR when ever you are ready. I will not add more stuff.

[wez]

Thanks!