Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable progressive keyboard enhancement protocol #4939

Merged
merged 3 commits into from Feb 28, 2023
Merged

Enable progressive keyboard enhancement protocol #4939

merged 3 commits into from Feb 28, 2023

Conversation

the-mikedavis
Copy link
Member

@the-mikedavis the-mikedavis commented Nov 29, 2022 •
edited

See the Kitty documentation on this: https://sw.kovidgoyal.net/kitty/keyboard-protocol/

Key events given by the terminal are limited by default: the terminal confuses keycodes like C-i and tab and can't tell the difference between backspace and S-backspace or ret and S-ret. We can detect support for an enhanced keyboard protocol and enable that protocol to be able to disambiguate these keys on terminals which support the protocol.

Closes #3725
Closes #3210
Closes #1085
Closes #5765

@the-mikedavis the-mikedavis added C-enhancement Category: Improvements A-helix-term Area: Helix term improvements upstream S-waiting-on-pr Status: This is waiting on another PR to be merged first labels Nov 29, 2022
@the-mikedavis the-mikedavis marked this pull request as draft November 29, 2022 22:39
@lesleyrs
Copy link
Contributor

lesleyrs commented Dec 1, 2022

Oh oops I just made #4961 that also adds C-i.

Even without progressive keyboard enhancement it's needed for it to work on Windows and it doesn't seem to affect Linux. Can you merge mine first please, as it closes some issues?

@the-mikedavis the-mikedavis mentioned this pull request Dec 12, 2022
Closed
This was referenced Jan 6, 2023
Closed
Closed
@the-mikedavis
Copy link
Member Author

The upstream crossterm PR was merged so we just wait on a release now. (Hopefully the next one will also include the PR for #5468.)

There's some odd behavior with the key events you get and the shift modifier so there may need to be more changes to the default keymap: the-mikedavis@4be72f7

@the-mikedavis the-mikedavis linked an issue Jan 11, 2023 that may be closed by this pull request
Key mapping for "Alt+[" not working #5447
Closed
@kirawi
Copy link
Member

kirawi commented Jan 19, 2023

Closes #1368

@the-mikedavis the-mikedavis linked an issue Jan 19, 2023 that may be closed by this pull request
keybindings with esc do not work (for me) #1368
Closed
@yyogo
Copy link
Contributor

yyogo commented Jan 29, 2023

Hey, crossterm just released 0.26, see #5468

@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed upstream S-waiting-on-pr Status: This is waiting on another PR to be merged first labels Jan 29, 2023
@the-mikedavis the-mikedavis mentioned this pull request Jan 30, 2023
Open
@the-mikedavis
Copy link
Member Author

I tested out 0.26 and there have been some new changes that caused a regression with the function that checks for the keyboard enhancement protocol. I'll post a PR to fix that so this will need to wait on the next crossterm release

@the-mikedavis the-mikedavis added upstream S-waiting-on-pr Status: This is waiting on another PR to be merged first and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 30, 2023
@pascalkuthe pascalkuthe mentioned this pull request Feb 1, 2023
Closed
@archseer
Copy link
Member

archseer commented Feb 7, 2023

Since there's often significant time between releases of crossterm, I wonder if we could fork it to the helix-editor org and use that for the time being -- seems a shame to hold this feature back because of a one line fix. We could ask for a patch release (0.26.1) but I don't want to pester the maintainer too much.

@the-mikedavis
Copy link
Member Author

Yeah that seems like a nice compromise to me 👍

We could just use this Cargo.toml patch that uses crossterm from the master branch but if we forked we would also publish to crates?

@archseer
Copy link
Member

archseer commented Feb 8, 2023

I think that's enough, even if we fork I'd list it as a git dependency. It's a workaround specific to our use case and I'd avoid someone else depending on it by accident

@the-mikedavis
Copy link
Member Author

the-mikedavis commented Feb 8, 2023 •
edited

This is now waiting on another PR to crossterm now that adds support for the "report alternate keys" part of the Kitty Keyboard Protocol: https://sw.kovidgoyal.net/kitty/keyboard-protocol/#report-alternate-keys.

Some keys arrive weirdly when the protocol is enabled: A-( arrives as A-S-9, A-) as A-S-0 and A-_ as A-S-minus. Kakoune gets around this by enabling the "report alternate keys" part of the spec. Here it pushes 5 for keyboard enhancement flags which is equivalent to

KeyboardEnhancementFlags::DISAMBIGUATE_ESCAPE_CODES |
    KeyboardEnhancementFlags::REPORT_ALTERNATE_KEYS

in crossterm. Then here it overwrites the unshifted key with the shifted key.

(Update: that was merged! 🎉)

@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed upstream S-waiting-on-pr Status: This is waiting on another PR to be merged first labels Feb 11, 2023
@the-mikedavis the-mikedavis marked this pull request as ready for review February 11, 2023 16:29
pascalkuthe
pascalkuthe reviewed Feb 11, 2023
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
helix-term/src/keymap/default.rs
@@ -363,7 +363,7 @@ pub fn default() -> HashMap<Mode, Keymap> {
"A-d" | "A-del" => delete_word_forward,
"C-u" => kill_to_line_start,
"C-k" => kill_to_line_end,
"C-h" | "backspace" => delete_char_backward,
"C-h" | "backspace" | "S-backspace" => delete_char_backward,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are there other keybindings that might have worked without this feature but don't work anymore now?
If that is the case should we offer a config option to dsiable this feature? Also I think this technically qualifies as a breaking change because people could have custom keybdings for backspace (or other ambiguous key-codes) like this which would stop working. I added the appropriate label so we don't forget about that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ambiguous keycodes like using C-i for tab or using C-backspace for C-h would change but I think those would be pretty rare. I'm not sure how I feel about a config option for this: if the protocol is enabled then you get more predictable and correct key events - you would only disable it if you wanted to use ambiguous keycodes which I don't think has any advantage

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds reasonable 👍 I think we should keep the breaking change label tough so this PR is a bit easier to find for people who do experience regressions.

@pascalkuthe pascalkuthe added the R-breaking-change This PR is a breaking change for some behavior label Feb 11, 2023
@pascalkuthe
Copy link
Member

I think this closes #5765 btw

@David-Else David-Else mentioned this pull request Feb 19, 2023
Merged
the-mikedavis and others added 3 commits February 26, 2023 13:31
@the-mikedavis
Update crossterm to 0.26.1
f1abf41 
Crossterm 0.26.x includes a breaking change for the command to set the
cursor shape. This commit includes a change which uses the new type.
@the-mikedavis
Enable the enhanced keyboard protocol if supported
07e5120
@lesleyrs @the-mikedavis
Add shift-backspace keybind alias for backspace (#4937)
1b50b60 
When the Kitty Keyboard Protocol is enabled, S-backspace is
distinguished from backspace with no modifiers. This is awkward when
typing because it's very easy to accidentally hold shift and press
backspace temporarily when typing capital letters.

Kakoune (which is also a Kitty Keyboard Protocol application) treats
S-backspace as backspace too:
https://github.com/mawww/kakoune/blob/3150e9b3cd8e61d9bc68245d67822614d4376cf4/src/input_handler.cc#L1275
@kirawi kirawi mentioned this pull request Feb 28, 2023
Closed
@archseer archseer merged commit 8dab8a0 into master Feb 28, 2023
@archseer archseer deleted the enable-keycode-disambiguation branch February 28, 2023 03:35
This was referenced Feb 28, 2023
Open
Closed
Closed
Closed
@the-mikedavis the-mikedavis mentioned this pull request Apr 2, 2023
Closed
@musjj musjj mentioned this pull request Dec 23, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pascalkuthe pascalkuthe pascalkuthe left review comments

Assignees
No one assigned
Labels
A-helix-term Area: Helix term improvements C-enhancement Category: Improvements R-breaking-change This PR is a breaking change for some behavior S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Milestone
No milestone
6 participants
@the-mikedavis @lesleyrs @kirawi @yyogo @archseer @pascalkuthe