Skip to content

Enable progressive keyboard enhancement protocol - #4939

Merged
archseer merged 3 commits into
masterfrom
enable-keycode-disambiguation
Feb 28, 2023
Merged

Enable progressive keyboard enhancement protocol#4939
archseer merged 3 commits into
masterfrom
enable-keycode-disambiguation

Conversation

@the-mikedavis

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

Copy link
Copy Markdown
Member

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

lesleyrs commented Dec 1, 2022

Copy link
Copy Markdown
Contributor

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
the-mikedavis force-pushed the enable-keycode-disambiguation branch from 4a4b1de to b6b1bea Compare January 11, 2023 02:49
@the-mikedavis

the-mikedavis commented Jan 11, 2023

Copy link
Copy Markdown
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

kirawi commented Jan 19, 2023

Copy link
Copy Markdown
Member

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

yyogo commented Jan 29, 2023

Copy link
Copy Markdown
Contributor

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

the-mikedavis commented Jan 30, 2023

Copy link
Copy Markdown
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

archseer commented Feb 7, 2023

Copy link
Copy Markdown
Member

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

the-mikedavis commented Feb 7, 2023

Copy link
Copy Markdown
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?

@the-mikedavis
the-mikedavis force-pushed the enable-keycode-disambiguation branch from b6b1bea to 88d1efa Compare February 7, 2023 16:06
@archseer

archseer commented Feb 8, 2023

Copy link
Copy Markdown
Member

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

the-mikedavis commented Feb 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 force-pushed the enable-keycode-disambiguation branch from 88d1efa to 44de88a Compare February 8, 2023 23:01
@the-mikedavis
the-mikedavis force-pushed the enable-keycode-disambiguation branch from 44de88a to b77fb5e Compare February 11, 2023 16:28
@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
Comment thread Cargo.toml Outdated
Comment thread helix-term/src/keymap/default.rs
"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,

@pascalkuthe pascalkuthe Feb 11, 2023 •
edited
Loading

Copy link
Copy Markdown
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

@the-mikedavis the-mikedavis Feb 11, 2023

Copy link
Copy Markdown
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

@pascalkuthe pascalkuthe Feb 11, 2023

Copy link
Copy Markdown
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

pascalkuthe commented Feb 11, 2023

Copy link
Copy Markdown
Member

I think this closes #5765 btw

@David-Else David-Else mentioned this pull request Feb 19, 2023
Merged
@the-mikedavis
the-mikedavis force-pushed the enable-keycode-disambiguation branch from b77fb5e to dba95f5 Compare February 26, 2023 19:29
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
@the-mikedavis
the-mikedavis force-pushed the enable-keycode-disambiguation branch from dba95f5 to 1b50b60 Compare February 26, 2023 19:31
@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
Closed
Closed
Closed
Closed
@the-mikedavis the-mikedavis mentioned this pull request Apr 2, 2023
Closed
@hovsater hovsater mentioned this pull request Aug 7, 2023
Closed
@musjj musjj mentioned this pull request Dec 23, 2023
Open
@ds1sqe ds1sqe mentioned this pull request Jan 5, 2026
Closed
@Delapouite Delapouite mentioned this pull request Apr 30, 2026
Closed
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