-
-
Notifications
You must be signed in to change notification settings - Fork 53
Implement support for Kitty Keyboard Protocol #4769
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
base: master
Are you sure you want to change the base?
Conversation
|
Hi @egmontkob and @krobelus, any chance you could leave a review? Thank you! |
|
Supersedes #4768. |
|
My question would be: do we really need the |
|
As I've already noted in #4663 (and it hasn't changed since), I'm absolutely not familiar with Kitty's keyboard protocol, so I'm sorry but I can't help here. |
The hope dies last - I think of you when I think of terminals :) Sorry for the noise. |
|
@horkykuba could you please fix the formatting ( |
Done. |
|
@horkykuba I've fixed everything for the CI to pass. Could you please add some tests for https://github.com/MidnightCommander/mc/blob/master/tests/lib/terminal.c |
Thanks, it was already past Midnight when I was making the last edits :) I've also modified the
Ok, but it will take me a while. Should I implement console input by mocking it using |
5c71db3 to
eda9b61
Compare
ossilator
left a comment
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.
i did mostly only a formal review, without checking the actual protocol impl. before i do the latter, please add a comment with a high-level overview of what that code is supposed to do (basically a tldr version of the protocol spec, with a link to the actual spec), and short comments that outline the particular cases.
i'd prefer it if you clean up the series (that is, squash most commits to achieve atomicity), though zyv might disagree ...
I have no objections. I have added my commits on top just to show what I did to fix formatting, linters, and CI. Probably it would be still good to address your comments in a separate commit though, just to make it easier to evaluate that they are indeed addressed before we move further. |
Good question! I think this function is a scary, hairy, hardly testable monster. However, making it even more complicated is not helpful. Perhaps you could extract the parsing code into smaller functions that work on buffers. Then these functions could be easily tested, and you wouldn't need to mock things like There is no hurry. I would really like to have something like this included, but please understand that I also don't want to introduce a lot of potential breakage, which I can't deal with. And the worst part is that I don't have time. So even if it takes you a long time to come up with a tested and reviewable solution, it's absolutely worth it, because then it can be included. |
@ossilator It simply waits if a sequence is found in the
If it matches, it retrieves the modifier bits, checks for non-alphabet keys using a predefined table, and then corrects shifted keys as described in previous comment. Next, it checks if it is a non-Basic Latin Unicode character. If so, it converts it to UTF-8 and inserts the remaining bytes into the pending queue. Finally, it masks the resulting value with Throughout the process, it carefully handles possible following sequences, leaving them in the pending queue. Examples:
|
This is actually a very nice explanation! I'd love to see it as a comment in the code. |
ossilator
left a comment
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.
so now i made some effort to actually understand what this code does.
i didn't check the wider context this is embedded into.
but with these disclaimers in place, i'd say it looks reasonable.
mc-worker
left a comment
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.
I think, changes after changes is not a good practice. Since this PR is not merged, all these fixes should be moved to commits where appropriate changed were introduced.
b9dbba9 to
54cde8a
Compare
|
Squashed commits and done changes suggested by @aborodin |
ossilator
left a comment
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 nitpicks.
|
Made changes suggested by @ossilator and removed key sequence definitions duplicate to kitty in misc/mc.lib |
| { KEY_M_SHIFT | KEY_LEFT, ESC_STR "[1;2D", MCKEY_NOACTION }, | ||
|
|
||
| // more xterm keys with modifiers | ||
| { KEY_M_CTRL | KEY_PPAGE, ESC_STR "[5;5~", MCKEY_NOACTION }, |
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.
Posting some comments -- feels too early to do a full review (data structures might still change),
and I'm not sure how much time I can spend on this.
It might help to move the code-movement changes (that don't change any behavior) into a precursor commit,
to make it easier to see which of the sequences like [5;5~ have changed.
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.
This can't be done because all removed hard-coded xterm entries are handled by kitty core parser.
lib/tty/key.c
Outdated
| } | ||
| else | ||
| { | ||
| c = csi.params[0][0]; |
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.
this code is unnecesarily opaque,
can we assign things like csi.params[0][0] to variables using the names from the protocol?
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.
Done
| { | ||
| c = csi.params[0][0]; | ||
| if ((csi.params[0][2] || (c >= '0' && c <= '9'))) | ||
| modifiers &= ~KEY_M_SHIFT; |
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.
so we don't consume the shift modifier if it's an ASCII digit,
presumably to make bindings for shift-1 work?
But that suggests that things are probably broken for other keys... not sure.
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.
The current mc legacy protocol works with the actual final keys (which differ across many keyboard layouts), not with KEY_M_SHIFT + a number. So this should be fine. Yes, this applies to keyboard layouts where numbers are produced using Shift.
|
|
||
| // Mask the resulting value with 0x1F if the CTRL modifier is set | ||
| if ((modifiers & KEY_M_CTRL) && (c == ' ' || (c >= 0x40 && c <= 0x7e))) | ||
| c = XCTRL (c); |
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.
so we need to fall back to the legacy encoding so the existing (legacy) input queue works?
That seems bad; maybe it's fine as a temporary stepping stone.
It seems that right now, Russian ctrl-ж is treated like ж. I guess if it's not a regression that's fine.
When I type ж followed by ctrl-ж in a terminal that does not support the kitty protocol, I get.
$ showkey -a
ж 208 0320 0xd0
182 0266 0xb6
219 0333 0xdb
150 0226 0x96
so it seems that the ctrl modifier affects every single byte?
Presumably the results were bogus then.
If we want to do this properly, we should define a struct key_t and pass that to the input queue,
instead of the extra back-and-forth UTF-8 encoding.
I'm not sure whether this is necessary to avoid breakage,
but it's the right direction (if we move to a library that implements keyboard input, we'd want something like that anyway).
In any case feels like there's at least a regression with non-English layouts (as mentioned before).
For example if I type ctrl-o on a Russian keyboard, the key code looks like
$ printf '\e[>5u'; timeout 3 showkey -a; printf '\e[<u'
Press any keys - Ctrl-D will terminate this program
^[[1097::111;5u 27 0033 0x1b
91 0133 0x5b
49 0061 0x31
48 0060 0x30
57 0071 0x39
55 0067 0x37
58 0072 0x3a
58 0072 0x3a
49 0061 0x31
49 0061 0x31
49 0061 0x31
59 0073 0x3b
53 0065 0x35
117 0165 0x75
$ fish_key_reader -cV
# decoded from: \e\[1097::111\;5u
bind ctrl-щ 'do something'
bind ctrl-o 'do something' # physical key
to fix this we need to handle the base layout key,
and it probably implies that keyboard matching becomes
more complicated than simple equality.
(unless we don't allow binding "ctrl-щ" for now)
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.
I've tried ctrl-ж in several terminals (PuTTY, WezTerm, xterm, mlterm), and it produces either a plain ж or nothing, but never the bytes your terminal outputs (which is this character: ۖ). Which terminal did you test it in?
It's true that in my kitty implementation ctrl-щ results to щ, but most terminals (in non-kitty mode) don't distinguish between щ and ctrl-щ anyway, so it doesn't seem to be a regression. I added support for translating to the base layout key if a Ctrl or Alt + Unicode combination is received. However, doing this 100% correctly would require switching from "final" key definitions to "raw", physical key definitions, which is not possible in any non-kitty (or non-modifyOtherKeys etc.) mode.
Regarding fallback to the legacy encoding: I agree that the best approach is to maintain key information as purely as possible. But the current encoding is heavily spread across the source code, so I think it's better to handle this in separate steps - first merge kitty support, then beta test it, and finally upgrade mc's key protocol throughout the codebase.
| */ | ||
|
|
||
| if (pending_keys[0] == ESC_CHAR && pending_keys[1] == ESC_CHAR) | ||
| pending_keys++; |
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.
completely ignoring escape doesn't seem correct.
Legacy escape is ambiguous (which is solved by the kitty keyboard protocol),
it could either mean "alt modifier" or a literal escape key.
We interpret it as alt below (c = ALT (*pending_keys++);),
but that logic is broken by this line if the user presses "alt-escape" on a terminal that doesn't implement the kitty protocol such as tmux.
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.
In this phase, we're only discarding unknown escape sequences (unless it's kitty escape sequence). Even the master branch can't process escape + escape sequence generated by alt + function key. The case of alt + escape key is handled by the second hardcoded sequence in mc_default_keys[].
|
|
||
| c = seq_append[-1]; | ||
|
|
||
| // Get all parameter bytes and intermediate bytes before the final byte |
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.
IIRC SS3 (\eO…), does not have parameter bytes like CSI does,
so unless my memory is wrong, it seems wrong to skip through intermediate bytes here.
I think the SS3 commands relevant to us have only one character after the "O".
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.
This is workaround documented in the comment above:
Now check for all CSI and SS3 sequences, as these two are the de-facto standard for sending key sequences in most terminals. The only well-known deviations are (…) - xterm with modify*Keys:0 (the old behavior, which sends parametrized SS3 sequences), and many terminals that inherited this ill behavior, as SS3 can't have any parameters. As a workaround, we treat SS3 like CSI.
Even latest xterm sends parametrized SS3 in its default configuration (for example numpad *, +, - keys).
Ignore lib/stdckdint.h, which is generated by autoconf, *.log, which are generated by make check, and *.orig, which are generated by git mergetool. Signed-off-by: Jakub Horký <jakub.git@horky.net>
Add standard interfaces for outputting escape sequences and for applying parameters to parametrized terminfo capabilities. Support is added for both ncurses and S-Lang, but S-Lang supports a maximum of two parameters for any parametrized capability. Signed-off-by: Jakub Horký <jakub.git@horky.net>
The old functionality that "eats" all characters following an unknown escape sequence is removed, as it causes severe side effects (such as not responding to any keys following an unknown sequence for some time). This is superseded by parsing CSI and SS3 sequences, which are the de facto standard for sending any function keys. Resolves: MidnightCommander#3136 Signed-off-by: Jakub Horký <jakub.git@horky.net>
Signed-off-by: Jakub Horký <jakub.git@horky.net>
Make functions tty_lowlevel_getch() and getch_with_timeout() mockable by defining them as weak. Fake key input can be passed by mock_input(). Signed-off-by: Jakub Horký <jakub.git@horky.net>
The protocol is described here: https://sw.kovidgoyal.net/kitty/keyboard-protocol/ Check for kitty sequence when no entry is found in the keys tree, before the sequence is discarded. Resolves: MidnightCommander#4762 Signed-off-by: Jakub Horký <jakub.git@horky.net>
|
Made changes suggested by @krobelus and added some precursor commits. |
Just a heads up so that you don't get annoyed that you don't get any further feedback - I will be away for several weeks and won't be able to look into it before I leave :( |
You are lucky if you don't have to work on your vacation :) Happy holidays! |
| doc/devel/ | ||
| doc/html/ | ||
| .deps | ||
| lib/stdckdint.h |
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.
pedantically, all these entries are lacking the / prefix to anchor them at the root. but fixing that is for another commit.
|
|
||
| va_start (args, str); | ||
| p1 = va_arg (args, int); | ||
| p2 = va_arg (args, int); |
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.
pulling a fixed number of arguments in a formally varargs function sounds like a recipe for disaster.
a cleaner solution would be overloaded macros: https://stackoverflow.com/a/11763277/3685191
|
|
||
| /* --------------------------------------------------------------------------------------------- */ | ||
|
|
||
| int |
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.
in master, there is now an MC_TESTABLE macro, which should avoid the churn of making the function formally public.
| By default, Midnight Commander sends an escape sequence that enables the Kitty | ||
| Keyboard Protocol, which allows precise keyboard handling (including key | ||
| combinations such as C\-Enter or C\-S\-Enter) when connected from terminals that | ||
| support this protocol. If set to false, Midnight Commander will not attempt to |
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.
this probably should point out that it's expected to be harmless to attempt it on lame terminals, which makes this option mostly unnecessary.
Kitty Keyboard Protocol support
Proposed changes
I've added support for Kitty Keyboard Protocol.
I've done some basic testing it WezTerm and kitty terminals, and it should work.
The protocol is described here:
https://sw.kovidgoyal.net/kitty/keyboard-protocol/
Kitty sequence is checked when no entry is found in the keys tree, before the sequence is discarded.
Also replace paranoia code with parsing code. The old functionality that "eats" all characters following an unknown escape sequence is removed, as it causes severe side effects (such as not responding to any keys following an unknown sequence for some time). This is superseded by parsing CSI and SS3 sequences, which are the de facto standard for sending any function keys.
In the separate commit, functions tty_putp() and tty_tiparm() are added. This commit serves as a common base for some another PRs which I'm working on.
Resolves: #4762
Resolves: #3136