fix: send correct codes with del and esc #5025
Conversation
With kitty keyboard protocol enabled, both DEL and ESC key sends legacy keys - DEL sends ^H and ESC sends ^[. To be in line with kitty keyboard protocol, they should send "CSI 3 ~" and "CSI 27 u" respectively.
|
Running a local compile using this right now and would love to see it merged. |
| @@ -1648,8 +1648,9 @@ impl KeyEvent { | |||
| { | |||
| // Check for simple text generating keys | |||
| match &self.key { | |||
| // Don't send legacy codes with DEL and ESC | |||
| Char('\x1b') | Char('\x7f') => {} | |||
| Char('\x08') => return '\x7f'.to_string(), | |||
There was a problem hiding this comment.
What was the original reason for swapping DEL and BS (backspace)?
There was a problem hiding this comment.
That's the nature of legacy terminal protocol. See the Xterm control sequences, how it sends with ^H? ^H is DEL.
Just one of those things, like how \n needs to be translated by the terminal driver into \r\n so that the teletype in question will both return the carriage and advance the printer roll.
There was a problem hiding this comment.
Such arcane things might be good to be linked/documented in code... IMO =)
|
This is a small change, and it fixes an aggravating bug which interferes with, for example, neovim, which expects that a terminal which reports kitty protocol will not send The diff has one insertion and one delete, and comes with tests. Please merge this PR. Thank you. |
|
I'm affected by this on the daily, I can work around it but it is very annoying :( The PR is small and has tests, maybe @wez can have a look at it? |
|
I would also like to understand what has held up this PR. Has it just fallen through the cracks? It has been open for 234 days, with six pages of merged PRs since then. Is there some concern about the correctness of the change? If it's that, you can check this chart in the spec and see that this PR contains the correct sequences for DEL and ESC. Is it just because the Kitty protocol implementation has been buggy for so long that updating it to be correct will break user configurations? I've certainly configured a number of things to work around the bug, but I'd be very happy to revert those changes. I always feel guilty putting pressure on open source maintainers, and I don't do it often. This PR is tiny, and really just needs @wez or someone else with the commit bit to press merge. I deeply appreciate the effort it takes to write an entire terminal and give it away for free, and I also think your users would very much appreciate a merge on this PR. It's already received approval (in July) so it's just pressing the button, right? |
I'm not sure how much my approval means. I'm not an official contributor. |
It's not just a matter of clicking the merge button and hoping for the best. It has to actually be read, reviewed, the spec needs to re-read, reviewed, and the impact of this change needs to be understood on all systems and combinations before it can be decided that it is actually good to merge. This is the role of the maintainer: someone has to actually consider these things and the impact to all of the users without blindingly shipping a breaking change. As much as I want to trust in the OP, I don't know them, and I've been burned by other seemingly simple PRs causing regressions that take more energy to run down and figure out. Why have other PRs been merged ahead of this one? It takes time to get into the zone and page back in the various specifications, so I tend to opportunistically triage and review/merge where my bandwidth allows. I haven't been in the mode for keyboard encoding in a while. All of this takes time and energy. That energy is diminished when folks make comments like the three above this and I have to take the time out of what I was doing to come here to make you stop pinging me. Despite your comment about how you don't like putting pressure on maintainers, what you are doing here by demanding an explanation and at-mentioning me is exactly that, and it is very entitled behavior. Please stop it. It will not make me review this any faster, in fact I generally will avoid looking at the associated issue or PR specifically, and wezterm in general, for a while so that this behavior is not being enabled. But also because: it is draining to put up with this sort of vampirism. I've spent a couple of decades giving and giving to opensource but there are limits. |
|
Hey Wez, sorry for the ping since I started it. There was no ill intention
behind it, I didn't know it would be such a problem.
Take care of yourself, wezterm is awesome and you've done something
amazing, thank you.
…On Wed, Oct 9, 2024, 03:57 Wez Furlong ***@***.***> wrote:
I would also like to understand what has held up this PR. Has it just
fallen through the cracks?
It has been open for 234 days, with six pages of merged PRs since then. Is
there some concern about the correctness of the change?
If it's that, you can check this chart in the spec
<https://sw.kovidgoyal.net/kitty/keyboard-protocol/#functional-key-definitions>
and see that this PR contains the correct sequences for DEL and ESC.
Is it just because the Kitty protocol implementation has been buggy for so
long that updating it to be correct will break user configurations? I've
certainly configured a number of things to work around the bug, but I'd be
very happy to revert those changes.
I always feel guilty putting pressure on open source maintainers, and I
don't do it often. This PR is tiny, and really just needs @wez
<https://github.com/wez> or someone else with the commit bit to press
merge. I deeply appreciate the effort it takes to write an entire terminal
and give it away for free, and I also think your users would very much
appreciate a merge on this PR.
It's already received approval (in July) so it's just pressing the button,
right?
It's not just a matter of clicking the merge button and hoping for the
best. It has to actually be read, reviewed, the spec needs to re-read,
reviewed, and the impact of this change needs to be understood on all
systems and combinations before it can be decided that it is actually good
to merge. This is the role of the maintainer: someone has to actually
consider these things and the impact to all of the users without blindingly
shipping a breaking change. As much as I want to trust in the OP, I don't
know them, and I've been burned by other seemingly simple PRs causing
regressions that take more energy to run down and figure out.
Why have other PRs been merged ahead of this one? It takes time to get
into the zone and page back in the various specifications, so I tend to
opportunistically triage and review/merge where my bandwidth allows. I
haven't been in the mode for keyboard encoding in a while.
All of this takes time and energy. That energy is diminished when folks
make comments like the three above this and I have to take the time out of
what I was doing to come here to make you stop pinging me. Despite your
comment about how you don't like putting pressure on maintainers, what you
are doing here by demanding an explanation and at-mentioning me is exactly
that, and it is very entitled behavior.
Please stop it.
It will not make me review this any faster, in fact I generally will avoid
looking at the associated issue or PR specifically, and wezterm in general,
for a while so that this behavior is not being enabled. But also because:
it is draining to put up with this sort of vampirism. I've spent a couple
of decades giving and giving to opensource but there are limits.
—
Reply to this email directly, view it on GitHub
<#5025 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFATL2EATLWKUKSMB73H7DZ2SESDAVCNFSM6AAAAABDNJBL4CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBRGEYTQOBSHE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
workaround while this PRs isn't merged: Add this keybinding {
key = "Delete",
action = wezterm.action_callback(function(win, pane)
local proc = pane:get_foreground_process_name()
if proc ~= "nvim" then
win:perform_action(
act.SendKey({
key = "Delete"
}),
pane
)
return
end
local key_code = utf8.char(0x1b) .. "[3;1~"
win:perform_action(act.SendString(key_code), pane)
end),
}where |
|
@wez, I don't think it's in any way appropriate to respond to
By accusing me of vampirism. I thought perhaps that pointing out that @tmccombs had already approved the changes might help move things forward. The standard you're setting with this behavior makes it impossible for anyone to advocate for a specific PR getting merged. Read what I wrote. Try to rephrase it into anything else which would be politer and more respectful of your time and effort, while still indicating a preference that this PR be looked at. If that's how you feel about your users, you can take down the website encouraging people to use your terminal for what is, for many of us, their profession. Step away from it for awhile. Get some help here if you can't respond to a request for triage without lashing out. Really do whatever you'd like, I'm no longer using WezTerm, a decision which predated my seeing you lose your cool on the issue board (again), but that does make me more confident in that decision. No one is entitled to your hard work and effort, but everyone is entitled to be treated with respect. I treated you with respect. You did not reciprocate. |
|
@mnemnion I stand by what I said. Perhaps you can't see how what you wrote netted out negative, in which case I encourage you to seek some coaching on your communication. You could have omitted 90% of what you wrote to indicate that you're interested in seeing this merged and it would have been received much better. That's all the time I want to spend on you; I wish you well. |
|
I created a simple script to make it easier to debug what codes are sent by the kitty keyboard protocol: https://gist.github.com/tmccombs/d5b0cc52508769c6b30ebe240008bbff Here is the comparison of the results I get for certain keys, including del and esc, that differ beteween wezterm 20240203-110809-5046fc22 and kitty 0.37.0:
I haven't tested this branch with the script yet. I'll try and get to that within the next few days. Footnotes |
|
I think that probably |
| @@ -1648,8 +1648,9 @@ impl KeyEvent { | |||
| { | |||
| // Check for simple text generating keys | |||
| match &self.key { | |||
| // Don't send legacy codes with DEL and ESC | |||
| Char('\x1b') | Char('\x7f') => {} | |||
There was a problem hiding this comment.
| Char('\x1b') | Char('\x7f') => {} | |
| // Esc should only be passed through as-is if disambiguate_escape_codes is false | |
| Char('\x1b') if flags.contains(KittyKeyboardFlags::DISAMBIGUATE_ESCAPE_CODES) => {} | |
| // In kitty mode, DEL is always sent as the special escape \e[3~ | |
| Char('\x7f') => () |
Although... it seems like currently having flags set to 0 might behave basically the same as setting it to 1. I'm not sure if that is intentional.
With kitty keyboard protocol enabled, both DEL and ESC key sends legacy keys - DEL sends
^Hand ESC sends^[. To be in line with kitty keyboard protocol, they should sendCSI 3 ~andCSI 27 urespectively based on the https://sw.kovidgoyal.net/kitty/keyboard-protocol/#functional-key-definitions.Fixes #4785