fix: key bindings for urxvt, add home/end combinations #396
Conversation
|
cc @maaslalani |
|
Let's make sure we verify this one very carefully, including in both |
|
All right I think there are two different changes here. I will split the PR which will simplify. |
|
Ready for another look - I have split up the PR into bite-sized commits, so that they become easier to review individually. |
|
@knz Could you do me a favor and rebase this once more? |
|
Done. |
| @@ -382,10 +382,10 @@ var sequences = map[string]Key{ | |||
| "\x1b[2;3~": {Type: KeyInsert, Alt: true}, | |||
| "\x1b[3~": {Type: KeyDelete}, | |||
| "\x1b[3;3~": {Type: KeyDelete, Alt: true}, | |||
| "\x1b[1~": {Type: KeyHome}, | |||
There was a problem hiding this comment.
While you're right and most every terminal seems to send \x1b[H for the home key, I do find references to this sequence being used for home as well. We should probably keep it?
There was a problem hiding this comment.
Same question/remark for KeyEnd, too.
There was a problem hiding this comment.
Fwiw, I added this after testing against home and receiving that value (same case for end). As long as there’s no collision, I’d also err on keeping it.
There was a problem hiding this comment.
On which terminal was that?
There was a problem hiding this comment.
To be quite honest I don't remember anymore, though do I think it's worth investigating.
There was a problem hiding this comment.
I'll update this commit and keep the previous sequences for KeyHome and KeyEnd for now. If we can confirm they aren't used anywhere we can still remove them eventually.
There was a problem hiding this comment.
Ok, pushed my changes to your fork.
There was a problem hiding this comment.
Ok this works for me. thanks for the edit!
key.go
Outdated
|
|
||
| "\x1b[H": {Type: KeyHome}, // vt100 | ||
| "\x1b[1;3H": {Type: KeyHome, Alt: true}, // vt100 | ||
| "\x1b[F": {Type: KeyHome}, // vt100 |
There was a problem hiding this comment.
I think this should read KeyEnd. Fixing.
There was a problem hiding this comment.
Fixed and pushed, but noticed you corrected the mistake yourself in one of the later commits.
Here the list of keys did not change, I simply re-ordered the lines in the file.
|
@knz Help me understand why the last commit improves troubleshootability? I find the function keys a bit all over the place still. Edit: Just to clarify, I'm happy with the change, I'm just wondering if I'm missing something obvious. |
As I was cross-checking the codes for the function keys with the source code of all the terminal emulators listed in the termenv docs, I was doing them in order of function key (starting at F1, then F2, etc). It was super hard to do because they were not in function key order in key.go. That's why I rearranged them. But feel free to drop that latest commit if you believe it's not worth the hassle. |
knz
changed the title
|
Thank you @knz, great work 🙌 |
We started supporting insert in charmbracelet#418, but then accidentally removed it during a rebase in charmbracelet#396. Oops.
Fixes #310.
Fixes #403.
cc @muesli