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.

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

Add Ctrl+Backspace support #3935

Merged
7 commits merged into from
Jan 13, 2020
Merged

Add Ctrl+Backspace support #3935

7 commits merged into from
Jan 13, 2020

Conversation

mkitzan
Copy link
Contributor

@mkitzan mkitzan commented Dec 12, 2019

Summary of the Pull Request

Changes the Ctrl+Backspace input sequence and how it is processed by InputStateMachineEngine. Now Ctrl+Backspace deletes a whole word at a time on CMD and PS.

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Changed the input sequence for Ctrl+Backspace to \x1b\x8 so the sequence would pass through _DoControlCharacter. Changed _DoControlCharacter to process \b in a way which forms the correct INPUT_RECORDs to delete whole words.

Validation Steps Performed

Ctrl+Backspace works 🎉

@mkitzan mkitzan marked this pull request as ready for review December 12, 2019 21:39
@mkitzan mkitzan changed the title Adds Ctrl+Backspace support Add Ctrl+Backspace support Dec 13, 2019
@zadjii-msft zadjii-msft added Area-Input Related to input processing (key presses, mouse, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. labels Dec 16, 2019
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Thanks for trying to put together a fix here. However, I don't think this works as the right fix. It unfortunately breaks Ctrl+Bksp in WSL in conhost, by causing conhost to instead emit the VT sequence for Ctrl+Alt+Bksp. Take a look at the following table, composed by running showkey -a in:

  • gnome-terminal
  • conhost alone, both with and without the PR applied
  • conpty by using vtpipeterm to use conhost as the "terminal", both without and with the PR applied
Key Chord gnome-terminal conhost (before PR) conhost (with fix) conpty (before PR) conpty (with fix)
Bksp DEL (0x7f) DEL (0x7f) DEL (0x7f) DEL (0x7f) DEL (0x7f)
Ctrl+Bksp ^H (0x08) ^H (0x08) ^[^H (0x1b08) ^H (0x08) ^[^H (0x1b08)
h h (0x68) h (0x68) h (0x68) h (0x68) h (0x68)
Ctrl+h ^H (0x08) ^H (0x08) ^H (0x08) ^H (0x08) ^[^H (0x1b08)
Del ^[[3~ ^[[3~ ^[[3~ ^[[3~ ^[[3~
Ctrl+Del ^[[3;5~ ^[[3;5~ ^[[3;5~ ^[[3;5~ ^[[3;5~
Alt+Bksp ^[DEL (0x1b7f) ^[DEL (0x1b7f) ^[DEL (0x1b7f)
Ctrl+Alt+Bksp ^[^H (0x1b7f) ^[^H (0x1b7f) ^[^H (0x1b7f)

Fundamentally, there's a problem where clients reading VT input can't differentiate between Ctrl+Bksp and Ctrl+h because they have the same encoding (^H), but that's not the problem here, so we won't try and fix it. Win32 client apps can differentiate, but not VT (read:WSL) ones.

I think the problem might be that conpty (InputStateMachineEngine) needs to pick one of those two as what it translates ^H to, and we might have picked wrong. Conpty is treating ^H as Ctrl+h, not Ctrl+Bksp. If we simply change InputStateMachineEngine to process a ^H into a Ctrl+Bksp rather than a Ctrl+h, does that fix the bug instead?

This might have some side effects where Win32 client apps running in a conpty session are unable to read a Ctrl+h keypress (and will treat it like Ctrl+Bksp), but I honestly think that's okay.

Below is some more raw output I gathered from conechokey while investigating.

Conhost only, Before PR, normal (not VT) input:

# bksp
Down: 1 Repeat: 1 KeyCode: 0x8 ScanCode: 0xe Char: \b (0x8) KeyState: 0x20
Down: 0 Repeat: 1 KeyCode: 0x8 ScanCode: 0xe Char: \b (0x8) KeyState: 0x20

# ctrl+bksp
Down: 1 Repeat: 1 KeyCode: 0x11 ScanCode: 0x1d Char: \0 (0x0) KeyState: 0x28
Down: 1 Repeat: 1 KeyCode: 0x8 ScanCode: 0xe Char: ⌂  (0x7f) KeyState: 0x28
Down: 0 Repeat: 1 KeyCode: 0x8 ScanCode: 0xe Char: ⌂  (0x7f) KeyState: 0x28
Down: 0 Repeat: 1 KeyCode: 0x11 ScanCode: 0x1d Char: \0 (0x0) KeyState: 0x20

# h
Down: 1 Repeat: 1 KeyCode: 0x48 ScanCode: 0x23 Char: h  (0x68) KeyState: 0x20
Down: 0 Repeat: 1 KeyCode: 0x48 ScanCode: 0x23 Char: h  (0x68) KeyState: 0x20

# ctrl+h
Down: 1 Repeat: 1 KeyCode: 0x11 ScanCode: 0x1d Char: \0 (0x0) KeyState: 0x28
Down: 1 Repeat: 1 KeyCode: 0x48 ScanCode: 0x23 Char: \b (0x8) KeyState: 0x28
Down: 0 Repeat: 1 KeyCode: 0x48 ScanCode: 0x23 Char: \b (0x8) KeyState: 0x28
Down: 0 Repeat: 1 KeyCode: 0x11 ScanCode: 0x1d Char: \0 (0x0) KeyState: 0x20

Conpty, Before PR, normal (not VT) input:

# bksp
Down: 1 Repeat: 1 KeyCode: 0x8 ScanCode: 0xe Char: \b (0x8) KeyState: 0x0
Down: 0 Repeat: 1 KeyCode: 0x8 ScanCode: 0xe Char: \b (0x8) KeyState: 0x0

# ctrl+bksp
Down: 1 Repeat: 1 KeyCode: 0x11 ScanCode: 0x1d Char: \0 (0x0) KeyState: 0x8
Down: 1 Repeat: 1 KeyCode: 0x48 ScanCode: 0x23 Char: \b (0x8) KeyState: 0x8
Down: 0 Repeat: 1 KeyCode: 0x48 ScanCode: 0x23 Char: \b (0x8) KeyState: 0x8
Down: 0 Repeat: 1 KeyCode: 0x11 ScanCode: 0x1d Char: \0 (0x0) KeyState: 0x0

# h
Down: 1 Repeat: 1 KeyCode: 0x48 ScanCode: 0x23 Char: h  (0x68) KeyState: 0x0
Down: 0 Repeat: 1 KeyCode: 0x48 ScanCode: 0x23 Char: h  (0x68) KeyState: 0x0

# ctrl+h
Down: 1 Repeat: 1 KeyCode: 0x11 ScanCode: 0x1d Char: \0 (0x0) KeyState: 0x8
Down: 1 Repeat: 1 KeyCode: 0x48 ScanCode: 0x23 Char: \b (0x8) KeyState: 0x8
Down: 0 Repeat: 1 KeyCode: 0x48 ScanCode: 0x23 Char: \b (0x8) KeyState: 0x8
Down: 0 Repeat: 1 KeyCode: 0x11 ScanCode: 0x1d Char: \0 (0x0) KeyState: 0x0

Conpty, After PR, normal (not VT) input:

Down: 1 Repeat: 1 KeyCode: 0x8 ScanCode: 0xe Char: \b (0x8) KeyState: 0x0
Down: 0 Repeat: 1 KeyCode: 0x8 ScanCode: 0xe Char: \b (0x8) KeyState: 0x0

Down: 1 Repeat: 1 KeyCode: 0x11 ScanCode: 0x1d Char: \0 (0x0) KeyState: 0x8
Down: 1 Repeat: 1 KeyCode: 0x8 ScanCode: 0xe Char: ⌂  (0x7f) KeyState: 0x8
Down: 0 Repeat: 1 KeyCode: 0x8 ScanCode: 0xe Char: ⌂  (0x7f) KeyState: 0x8
Down: 0 Repeat: 1 KeyCode: 0x11 ScanCode: 0x1d Char: \0 (0x0) KeyState: 0x0

Down: 1 Repeat: 1 KeyCode: 0x48 ScanCode: 0x23 Char: h  (0x68) KeyState: 0x0
Down: 0 Repeat: 1 KeyCode: 0x48 ScanCode: 0x23 Char: h  (0x68) KeyState: 0x0

Down: 1 Repeat: 1 KeyCode: 0x11 ScanCode: 0x1d Char: \0 (0x0) KeyState: 0x8
Down: 1 Repeat: 1 KeyCode: 0x8 ScanCode: 0xe Char: ⌂  (0x7f) KeyState: 0x8
Down: 0 Repeat: 1 KeyCode: 0x8 ScanCode: 0xe Char: ⌂  (0x7f) KeyState: 0x8
Down: 0 Repeat: 1 KeyCode: 0x11 ScanCode: 0x1d Char: \0 (0x0) KeyState: 0x0

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 17, 2019
@mkitzan
Copy link
Contributor Author

mkitzan commented Dec 17, 2019

Thanks for the review @zadjii-msft. That's a lot of good info. I'll work on making InputStateMachine process Ctrl+has Ctrl+Bksp.

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 17, 2019
@mkitzan
Copy link
Contributor Author

mkitzan commented Dec 17, 2019

With some new changes making InputStateMachine process ^H as Ctrl+Bksp, CMD and PS will delete whole words. In WSL, ^H does not follow the same path through InputStateMachine as in CMD or PS (apparently). Once I figure out how to send WSL the correct INPUT_RECORD, we should be in business.

@mkitzan
Copy link
Contributor Author

mkitzan commented Dec 17, 2019

@zadjii-msft WSL standalone doesn't delete whole words when Ctrl+Bksp is pressed, which explains why I was having my problems above. After testing the behavior of ^H with this new commit in showkey -a and conechokey, I believe this solves the problem without messing with the VT sequences like my last version. Thanks for pointing out those command line tools btw, very helpful.

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Yea this is much better. Thanks for looking into this!

@@ -352,6 +352,9 @@ void InputEngineTest::C0Test()
case L'\t': // Tab
writeCtrl = false;
break;
case L'\b': // backspace
wch = '\x7f';
expectedWch = '\x7f';
Copy link
Member

Choose a reason for hiding this comment

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

nit: this should probably have a break too, even though it's the last case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, the latest commit added the break statement

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Dec 18, 2019
@ghost ghost requested a review from leonMSFT January 9, 2020 04:42
@zadjii-msft
Copy link
Member

@msftbot merge this in 168 hours

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 9, 2020
@ghost
Copy link

ghost commented Jan 9, 2020

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Thu, 16 Jan 2020 13:29:30 GMT, which is in 7 days

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

This has been one of my biggest pain points. Thank you!

@zadjii-msft
Copy link
Member

@msftbot forget everything I just told you

@ghost
Copy link

ghost commented Jan 13, 2020

Hello @zadjii-msft!

Because you've told me to reset the custom auto-merge settings, I'll use the configured settings for this repository when I'm merging this pull request.

@ghost ghost merged commit 2b79bd0 into microsoft:master Jan 13, 2020
@mkitzan mkitzan deleted the Ctrl+Backspace branch January 14, 2020 00:16
@mkitzan
Copy link
Contributor Author

mkitzan commented Jan 14, 2020

Glad to have this one merged! Thanks

@BalintFarkas
Copy link

Thanks for making this @mkitzan , I came here looking for an issue and saw that there's an already merged fix, great!

DHowett-MSFT pushed a commit that referenced this pull request Jan 24, 2020
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
Changes the <kbd>Ctrl+Backspace</kbd> input sequence and how it is processed by `InputStateMachineEngine`. Now <kbd>Ctrl+Backspace</kbd> deletes a whole word at a time (tested on WSL, CMD, and PS).

<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? -->
## References

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #755
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed -> made minor edits to tests
* [ ] Requires documentation to be updated
* [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #755

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments
Changed the input sequence for <kbd>Ctrl+Backspace</kbd> to `\x1b\x8` so the sequence would pass through `_DoControlCharacter`. Changed `_DoControlCharacter` to process `\b` in a way which forms the correct `INPUT_RECORD`s to delete whole words.

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
<kbd>Ctrl+Backspace</kbd> works 🎉

(cherry picked from commit 2b79bd0)
@ghost
Copy link

ghost commented Jan 27, 2020

🎉Windows Terminal Preview v0.8.10261.0 has been released which incorporates this pull request.:tada:

Handy links:

@Pytal
Copy link

Pytal commented Jan 27, 2020

Is it possible to get this working with git bash?

@DHowett-MSFT
Copy link
Contributor

You’ll want to look into inputrc

@wmertens
Copy link

wmertens commented Apr 3, 2020

I'm on 0.10.781.0 but ctrl-backspace simply seems to send ctrl-h in WSL2 :(

@zadjii-msft
Copy link
Member

@wmertens You'll want to read #4397

@DHowett-MSFT
Copy link
Contributor

🎉 Once again, thanks for the contribution!

This pull request was included in a set of conhost changes that was just
released with Windows Insider Build 19603.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Input Related to input processing (key presses, mouse, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ctrl-backspace does not delete back to the previous wordbreak
7 participants