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

Fix #8458: Handle all Ctrl-key combinations #8870

Merged
1 commit merged into from
Feb 2, 2021
Merged

Fix #8458: Handle all Ctrl-key combinations #8870

1 commit merged into from
Feb 2, 2021

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Jan 24, 2021

Pressing Ctrl+\ produces ^\ using the US keyboard layout, thanks to Ctrl-key mappings inside the keyboard layout, whereas some layouts, like the UK extended layout, don't contain those. This causes the character value to be zero and previously caused no VT sequence to be generated under these situations. This PR employs MapVirtualKeyW to infer the missing characters.
As a side effect this PR effectively causes all major keys on the keyboard to produce Ctrl+combinations now.

PR Checklist

Validation Steps Performed

Compared all major keys in combination with Ctrl with the app store version of Terminal using showkey in WSL. All keys that previously worked still appear to continue to work.

@ghost ghost 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.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Jan 24, 2021
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.

God why is input such a disaster 🙃

Thanks for looking into this and fixing it

@lhecker
Copy link
Member Author

lhecker commented Jan 25, 2021

It might take me until next weekend to fix the failing unit tests by the way.

@lhecker
Copy link
Member Author

lhecker commented Jan 30, 2021

Turns out there was a reason why _translateDefaultMapping happens at the very end of that method. Unit tests catched it and I added an appropriate comment.

Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

Wonderful. Thanks.

@miniksa miniksa added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 2, 2021
@ghost
Copy link

ghost commented Feb 2, 2021

Hello @miniksa!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 3b9e612 into microsoft:main Feb 2, 2021
@ghost
Copy link

ghost commented Mar 1, 2021

🎉Windows Terminal Preview v1.7.572.0 has been released which incorporates this pull request.:tada:

Handy links:

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. Priority-2 A description (P2) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sequence CTRL+\ is not detected in some keyboard layouts
3 participants