Extract glfw key event data to a struct

[bew]

Initial work for #1980 (comment)

Notes:

• I changed the struct KeyEvent used by IBUS handling to use embed a GLFWkeyevent struct, to avoid duplication, but I'm not sure it's a good idea after all, we can discuss this.
• the words scancode and keycode seems to be used interchangeably, I'd suggest to choose only one and stick with it. I think that scancode is more platform-agnostic. There is also device_code which could be used in the GLFWkeyevent struct, while we keep scancode in the plateform-specific KB handling code..
  What do you think?
• ⚠️ I did not test IBUS handling (not sure how..)

Is there a reason why there are almost no newlines nor comments in functions? It feels hard to read the code sometime.. (and splitting them in multiple functions might not always help)

[bew]

@Luflosi I'll need help for macos if you have time
Actually no, it was simple to fix the compilation, but I can't test if it still works 😬

[Luflosi]

You managed to make it work for macOS yourself, GJ 👍.

[bew]

Thanks @kovidgoyal, that was fast! Do you have any comments on the questions I raised in the PR description?

[kovidgoyal]

• I changed the struct KeyEvent used by IBUS handling to use embed a GLFWkeyevent struct, to avoid duplication, but I'm not sure it's a good idea after all, we can discuss this.

It's fine by me, I made the code more robust, to handle null text and ensure it is zero terminated.

• the words scancode and keycode seems to be used interchangeably, I'd suggest to choose only one and stick with it. I think that scancode is more platform-agnostic. There is also device_code which could be used in the GLFWkeyevent struct, while we keep scancode in the plateform-specific KB handling code..

I would actually prefer native_keycode but it's not that big of a deal.

What do you think?

• warning I did not test IBUS handling (not sure how..)

You can just start the ibus daemon if it is not already running and turn on some keyboard layout such as the one for abbreviations or unicode input or similar. It's been a while since I wrote that code so dont remember the details.

Is there a reason why there are almost no newlines nor comments in functions? It feels hard to read the code sometime.. (and splitting them in multiple functions might not always help)

newlines are matter of taste, I happen to find the number of newlines well suited to the way I read code. As for comments, I prefer to make code as clear as possible. I only add comments when something is counter-intuitive or complex or could benefit from a high level description. If something is unclear, feel free to ask and add a comment.

[bew]

Actually, this line looks wrong: Because this is not a scancode but the native keysym instead... when it should be xkb_keycode instead of xkb_sym here (for coherence with the name of the receiving field).

This also means that on_key_event then kitty's keymap handling used the value of the native keysym all along.. this doesn't sound right.. does it?

Do you actually want the native keycode (scancode) or the native keysym in kitty's keyboard handling?
I would say the native keycode would make more sense, but to avoid breaking user configs, we must (?) stick with keysym?

[bew]

Also note that keeping the keysym value as the "scancode" in GLFWkeyevent for linux would be inconsistent with macos, which seems to correctly put the native keycode in event struct.. (by doing [event keyCode], but which code is it? @Luflosi ?)

Now sure what to do... break user's linux configs? (might be ok, since this feature is probably not that used.. ?)

[bew]

We could also have a "migration" path (over N releases), where we add the keycode (good) and the keysym (bad) to the key event struct.

Then if a kitty shortcut like ctrl+XX matches the keysym, display a sort of toast message (in addition to doing the action) telling the user to run Kitty from a terminal, and print logs telling him to update the mapping with ctrl+YY instead (using the true keycode).

WDYT?

[Luflosi]

but which code is it

What do you mean exactly?

[bew]

Reading more, it looks like having the keyevent's "scancode" be the keysym value on linux seems to be the expected behavior (I was wrong...):

kitty/glfw/x11_window.c

Lines 2594 to 2597 in 4ec1a8d

	int _glfwPlatformGetKeyScancode(int key)
	{
	return glfw_xkb_sym_for_key(key);
	}

So maybe I can just change the keyevent field to native_keycode like you suggested, and document somewhere that on linux the native_keycode is not the xkb_keycode but the xkb_keysym..

[bew]

@Luflosi forget what I said, I think I got confused by native_keycode, because X11 has something called a keycode and a keysym, and it's the keysym value that is to be assigned to native_keycode.......

I'll make a PR to change the field name to native_keycode, and change vars named scancode to keycode where appropriate

[kovidgoyal]

No, we want scancode to be the XKB keysym value. Actual scancodes are
kernel/driver/hardware dependent numbers that are useless for anything
portable. Currently, if you wish to create shortcuts for keys that dont
exist in glfw you can do so using XKB symbolic names on linux and actual
scancode numbers on macOS. This functionality needs to remain, as I
doubt we will ever end up adding all possible keys to glfw.

On macOS they might make sense since the hardware is Apple
made as well, but not on Linux. You can, if you wish add the scancode as
an extra field to the struct.

So maybe to avoid confusion name it native_key instead of
native_keycode, that way there is no chance of mistaking it for an X11
keycode or a kernel scancode or whatever.