Fixed #5205: Ctrl+Alt+2 doesn't send ^[^@ by lhecker · Pull Request #5272 · microsoft/terminal

Leonard Hecker (lhecker) · 2020-04-07T20:15:56Z

Summary of the Pull Request

Fixes #5205, by replacing another use of MapVirtualKeyW with ToUnicodeEx.
The latter just seems to be much more consistent at translating key combinations in general.
In this particular case though it fixes the issue, because there's no differentiation in MapVirtualKeyW for whether it failed to return a character ('\0') or succeeded in turning ^@ into '\0'.
ToUnicodeEx on the other hand returns the success state separately from the translated character.

PR Checklist

Closes Ctrl+Alt+Shift+2 doesn't send NUL in VT input mode #5205
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Requires documentation to be updated
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: Ctrl+Alt+Shift+2 doesn't send NUL in VT input mode #5205

Detailed Description of the Pull Request / Additional comments

This PR changes the behavior of the Ctrl+Alt+Key handling slightly:
⚠️ ToUnicodeEx returns unshifted characters. ⚠️
For instance Ctrl+Alt+a is now turned into ^[^a. Due to how ASCII works this is essentially the same though because 'A' & 0b11111 and 'a' & 0b11111 are the same.

Validation Steps Performed

Run showkey -a
Ensured Ctrl+Alt+Space as well as Ctrl+Alt+Shift+2 are turned into ^[^@
Ensured other, random Ctrl+Alt+Key combination behave identical to the current master

Leonard Hecker (lhecker) · 2020-04-07T20:20:25Z

BTW I couldn't run tests locally, because I'm faced with the following error:

".\terminal\OpenConsole.sln" (Clean;Build target) (1) ->
".\terminal\src\cascadia\TerminalAzBridge\TerminalAzBridge.vcxproj" (Clean;Build target) (50) ->
  .\terminal\packages\Microsoft.Windows.CppWinRT.2.0.200316.3\build\native\Microsoft.Windows.CppWinRT.targets(164,13):
error MSB4006: There is a circular dependency in the target dependency graph involving target "ComputeGetResolvedWinMD".
[.\terminal\src\cascadia\TerminalAzBridge\TerminalAzBridge.vcxproj]

Mike Griese (zadjii-msft) · 2020-04-08T13:56:37Z

I'm not sure what exactly is causing that error you're seeing, but there are definitely some unit test breaks 😆 Sometimes just cleaning out the project it complains about will fix it.

From CI:

Summary of Non-passing Tests:
    Microsoft::Console::VirtualTerminal::InputTest::TerminalInputModifierKeyTests#metadataSet4 [Failed]
    Microsoft::Console::VirtualTerminal::InputTest::TerminalInputModifierKeyTests#metadataSet5 [Failed]
    Microsoft::Console::VirtualTerminal::InputTest::TerminalInputModifierKeyTests#metadataSet6 [Failed]
    Microsoft::Console::VirtualTerminal::InputTest::TerminalInputModifierKeyTests#metadataSet8 [Failed]
    Microsoft::Console::VirtualTerminal::InputTest::TerminalInputModifierKeyTests#metadataSet10 [Failed]
    Microsoft::Console::VirtualTerminal::InputTest::DifferentModifiersTest [Failed]
    Microsoft::Console::VirtualTerminal::InputEngineTest::RoundTripTest [Skipped]

Summary: Total=2136, Passed=2129, Failed=6, Blocked=0, Not Run=0, Skipped=1

TBH I rarely build the entire solution anymore, usually I just build only the projects I'm interested in by right clicking on them in VS:

your mileage may vary, but that's what works for me. You might be able to at least get the VirtualTerminal::InputTests building that way.

Mike Griese (zadjii-msft)

I'm pretty much okay with this, but let's see those test pass :)

Mike Griese (zadjii-msft) · 2020-04-08T14:01:30Z

+WORD TerminalInput::_ScanCodeFromVirtualKey(const WORD vkey) noexcept
+{
+    return LOWORD(MapVirtualKeyW(vkey, MAPVK_VK_TO_VSC));
+}


Part of me thinks we should move these helpers to some shared space, since this isn't the first time this has been written, but that's probably a better follow-up codehealth task.

Leonard Hecker (lhecker) · 2020-04-08T19:26:01Z

+                if (ch == 0)
+                {
+                    // There are a few vkeys that MapVirtualKeyW() above turns into 0,
+                    // while TerminalInput is still capable of processing them.
+                    // The important ones are tested in DifferentModifiersTest/TerminalInputNullKeyTests.
+                    continue;
+                }


Mike Griese (@zadjii-msft) In order for the tests to work again I disabled tests for certain Ctrl+Alt combinations as you can see here.
We could make the tests work if we pull in the ToUnicodeEx() logic into this file as well. If we do that though we should probably really create a helper file that contains this function separately. 😄

On the other hand I personally kinda doubt that this if condition makes this unit test any worse, because to fix this unit test we have to basically add a identical copy of the TerminalInput::_CharacterFromAltCtrlKeyEvent method.
And IMHO that way we'd just test for the current state of affairs of which OS methods we use instead for the actual results that we'd like to see for each character, you know? 🤔

Yea this is fine with me

Leonard Hecker (lhecker) · 2020-04-08T20:24:00Z

Mike Griese (@zadjii-msft) Now the build bot is facing the same issue I had earlier:

packages\Microsoft.Windows.CppWinRT.2.0.200316.3\build\native\Microsoft.Windows.CppWinRT.targets(164,13): Error MSB4006: There is a circular dependency in the target dependency graph involving target "ComputeGetResolvedWinMD".

What do? 😱

Dustin L. Howett (MSFT) (DHowett-MSFT) · 2020-04-09T00:47:33Z

maybe re-queueing it will help. maybe there was a VS update. :/

Dustin L. Howett (MSFT) (DHowett-MSFT) · 2020-04-09T00:47:37Z

/azp run

azure-pipelines · 2020-04-09T00:47:47Z

Azure Pipelines successfully started running 1 pipeline(s).

Mike Griese (zadjii-msft)

Thanks for fixing this too!

Mike Griese (zadjii-msft) · 2020-04-10T20:15:45Z

+    if (keyEvent.IsModifierPressed() && _searchWithModifier(keyEvent, senderFunc))
+    {
+        return true;
+    }


Yea moving this seems fine to me, it's not like the code that was before it was having any effect on this block, so it can life here just fine.

Mike Griese (zadjii-msft) · 2020-04-10T20:25:06Z

+                if (ch == 0)
+                {
+                    // There are a few vkeys that MapVirtualKeyW() above turns into 0,
+                    // while TerminalInput is still capable of processing them.
+                    // The important ones are tested in DifferentModifiersTest/TerminalInputNullKeyTests.
+                    continue;
+                }


Yea this is fine with me

Leonard Hecker (lhecker) · 2020-04-10T21:02:52Z

Mike Griese (@zadjii-msft) I believe I made a mistake in this PR. I tested it again just now and Ctrl+Alt+@ doesn't work as intended anymore. I'm not sure when this started to be broken...

Also there's the fact that ToUnicodeEx returns unshifted, lowercase characters by default, unlike the current code. I'm a bit concerned that this might break some Alt+Ctrl combinations.

Leonard Hecker (lhecker) · 2020-04-10T22:28:44Z

Mike Griese (@zadjii-msft) I guess I can't figure out anymore how I originally fixed the issue. 😐
Due to that I'll now add a special handling for ^@ using the old and proven test for vkey == VkKeyScanW(0).

One quick question regarding this PR though: Should I revert my addition of ToUnicodeEx?
It's benefit is that it can properly handle shift keys. For instance this PR can differentiate between:

Ctrl+Alt+. which yields ^[^. and
Ctrl+Alt+Shift+. which yields ^[^:

The current solution with MapVirtualKeyW only supports the unshifted character ..
Another benefit is that after merging this PR we can remove all of this special handling altogether.
Since ToUnicodeEx supports Shift we can now use this to easily differentiate between Ctrl+Alt+/ and Ctrl+Alt+? as well. All of the VkKeyScanW based code can thus be removed.

But the downside to the ToUnicodeEx based solution is that it changes the current behavior slightly:
When you now press Ctrl+Alt+A it'll technically generate ^[^a (i.e. lowercase "a").
Due to the way Ctrl masking works I believe that all currently supported Ctrl+Alt combinations should produce the same output as they did before though.

What are your thoughts about this?
I'd personally go ahead with ToUnicodeEx because it supports the Shift key and it allows us to clean up _searchWithModifier later on. But since I'm not familiar with MS internals, I cannot properly estimate what the risk factor for getting something wrong in this component and changing existing behavior is.

Leonard Hecker (lhecker) · 2020-05-30T00:06:06Z

I kinda forgot about this PR. 😅 I‘ll try to tackle this again soon.
Due to the changes in master this will require a slightly different approach.

github-actions · 2021-02-07T15:06:13Z

Misspellings found, please review:

belowm

To accept these changes, run the following commands from this repository on this branch

pushd $(git rev-parse --show-toplevel)
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"aspnet boostorg dahall fde fea fmtlib isocpp mintty NVDA pinam QOL unte vcrt what3words xamarin "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/5f8e3d16764a523f3b36f75858bbee977a0cf5d7.txt";
use File::Path qw(make_path);
make_path ".github/actions/spelling/expect";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"belowm VCRT Xamarin "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
popd

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

... misspelled, then please correct them instead of using the command.
... names, please add them to .github/actions/spelling/dictionary/names.txt.
... APIs, you can add them to a file in .github/actions/spelling/dictionary/.
... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage and it relates to a binary-ish string, please add a file path to the .github/actions/spelling/excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

github-actions · 2021-02-07T15:10:11Z

Misspellings found, please review:

belowm

To accept these changes, run the following commands from this repository on this branch

pushd $(git rev-parse --show-toplevel)
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"aspnet boostorg dahall fde fea fmtlib isocpp mintty NVDA pinam QOL unte vcrt what3words xamarin "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/5f8e3d16764a523f3b36f75858bbee977a0cf5d7.txt";
use File::Path qw(make_path);
make_path ".github/actions/spelling/expect";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"belowm VCRT Xamarin "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
popd

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

... misspelled, then please correct them instead of using the command.
... names, please add them to .github/actions/spelling/dictionary/names.txt.
... APIs, you can add them to a file in .github/actions/spelling/dictionary/.
... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage and it relates to a binary-ish string, please add a file path to the .github/actions/spelling/excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

Mike Griese (zadjii-msft)

why did we never merge this before...?

Deleted user (ghost) · 2021-02-08T15:33:23Z

Hello Mike Griese (@zadjii-msft)!

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.

Deleted user (ghost) · 2021-03-01T18:02:57Z

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

Handy links:

Mike Griese (zadjii-msft) added Area-Input Related to input processing (key presses, mouse, etc.) Area-VT Virtual Terminal sequence support labels Apr 8, 2020

Mike Griese (zadjii-msft) requested changes Apr 8, 2020

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Apr 8, 2020

Leonard Hecker (lhecker) commented Apr 8, 2020

View reviewed changes

Mike Griese (zadjii-msft) approved these changes Apr 10, 2020

View reviewed changes

Mike Griese (zadjii-msft) added the Needs-Second It's a PR that needs another sign-off label May 29, 2020

ghost requested review from Carlos Zamora (carlos-zamora), Leon Liang (leonMSFT) and Michael Niksa (miniksa) May 29, 2020 20:26

Carlos Zamora (carlos-zamora) approved these changes May 29, 2020

View reviewed changes

microsoft-github-updates Bot changed the base branch from master to main October 22, 2020 00:27

ghost requested review from Dustin L. Howett (DHowett) and PankajBhojwani October 22, 2020 00:28

ghost added Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty labels Oct 22, 2020

Fixed #5205: Ctrl+Alt+2 doesn't send ^[^@

c4ba2db

Leonard Hecker (lhecker) changed the title ~~Fixed #5205: Ctrl+Alt+Shift+2 doesn't send NUL in VT input mode~~ Fixed #5205: Ctrl+Alt+2 doesn't send ^[^@ Feb 7, 2021

Mike Griese (zadjii-msft) approved these changes Feb 8, 2021

View reviewed changes

Mike Griese (zadjii-msft) added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 8, 2021

ghost merged commit b009d06 into microsoft:main Feb 8, 2021

Leonard Hecker (lhecker) deleted the fix-5205-null-inputs branch February 8, 2021 15:37

ghost mentioned this pull request Mar 1, 2021

Ctrl+Alt+Shift+2 doesn't send NUL in VT input mode #5205

Closed

oback mentioned this pull request Apr 21, 2021

Ctrl + Alt + number keys not working in 1.7.1033.0 with Danish keyboard #9909

Closed

This pull request was closed.

Conversation

Leonard Hecker (lhecker) commented Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

Leonard Hecker (lhecker) commented Apr 7, 2020

Uh oh!

Mike Griese (zadjii-msft) commented Apr 8, 2020

Uh oh!

Mike Griese (zadjii-msft) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Mike Griese (zadjii-msft) Apr 8, 2020

Choose a reason for hiding this comment

Uh oh!

Leonard Hecker (lhecker) Apr 8, 2020

Choose a reason for hiding this comment

Uh oh!

Mike Griese (zadjii-msft) Apr 10, 2020

Choose a reason for hiding this comment

Uh oh!

Leonard Hecker (lhecker) commented Apr 8, 2020

Uh oh!

Dustin L. Howett (MSFT) (DHowett-MSFT) commented Apr 9, 2020

Uh oh!

Dustin L. Howett (MSFT) (DHowett-MSFT) commented Apr 9, 2020

Uh oh!

azure-pipelines Bot commented Apr 9, 2020

Uh oh!

Mike Griese (zadjii-msft) left a comment

Choose a reason for hiding this comment

Uh oh!

Mike Griese (zadjii-msft) Apr 10, 2020

Choose a reason for hiding this comment

Uh oh!

Mike Griese (zadjii-msft) Apr 10, 2020

Choose a reason for hiding this comment

Uh oh!

Leonard Hecker (lhecker) commented Apr 10, 2020

Uh oh!

Leonard Hecker (lhecker) commented Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Leonard Hecker (lhecker) commented May 30, 2020

Uh oh!

github-actions Bot commented Feb 7, 2021

Misspellings found, please review:

Uh oh!

github-actions Bot commented Feb 7, 2021

Misspellings found, please review:

Uh oh!

Mike Griese (zadjii-msft) left a comment

Choose a reason for hiding this comment

Uh oh!

Deleted user (ghost) commented Feb 8, 2021

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.

Uh oh!

Deleted user (ghost) commented Mar 1, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Leonard Hecker (lhecker) commented Apr 7, 2020 •

edited

Loading

Leonard Hecker (lhecker) commented Apr 10, 2020 •

edited

Loading

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.