Fix KeyAvailable and ReadKey() in ConsolePal.Windows. #104984

mawosoft · 2024-07-16T17:24:32Z

Contributes to #102425.

dotnet-policy-service · 2024-07-16T17:24:58Z

Tagging subscribers to this area: @dotnet/area-system-console
See info in area-owners.md if you want to be subscribed.

mawosoft · 2024-07-17T14:16:12Z

cc @adamsitnik
May warrant a backport?

adamsitnik

@mawosoft big thanks for your contribution! Could you please add the missing comments?

In the meantime, I am going to buy a new battery for my numpad keyboard so I can test all scenarios manually.

Thank you!

src/libraries/System.Console/src/System/ConsolePal.Windows.cs

[skip ci]

adamsitnik

@mawosoft big thanks for addressing my feedback! I bought the battery for my numeric keyboard and was able to test the fix.

I've used following app for testing:

Console.OutputEncoding = System.Text.Encoding.UTF8;
Console.WriteLine($"Output encoding is: {Console.OutputEncoding}");

bool callKeyAvailableFirst = args.Length > 0;
if (callKeyAvailableFirst) Console.WriteLine("The app will call Console.KeyAvailable first.");

ConsoleKeyInfo info = default;
do
{
    if (callKeyAvailableFirst)
    {
        while (!Console.KeyAvailable) ;
    }

    info = Console.ReadKey(intercept: true);
    Console.WriteLine($"Got: key: {info.Key}, mod: {info.Modifiers}, char: {info.KeyChar}");
} while (info.Key != ConsoleKey.Escape);

I've built your fork and was running the app using corerun:

git clone https://github.com/mawosoft/runtime.git --branch fix-consolepal-windows fix-consolepal-windows
# skipped for brevity
D:\projects\forks\fix-consolepal-windows\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\corerun.exe .\bin\Release\net8.0\KeyTests.dll keyAvailable

Bug 1: Inconsistent logic between KeyAvailable and ReadKey
This affects input via an Alt plus numpad number sequence
Unfortunately, the opposite happens, because ReadKey() silently eats these keys in anticipation of a subsequent synthesized

I've tried the following combination: "Alt+NumPad1" and the outcome is the same (nothing gets printed). @mawosoft am I missing something?

However, on most keyboards, those keys exist twice - once on the numpad, and once on the arrow pad and control pad.

Do you mean the Home, PageUp and others?

I've tried the following combination: "Alt+PageUp" (not form the numpad) and with your fix the output gets printed, without it, it does not 👍

mawosoft · 2024-07-19T14:55:27Z

Bug 1: Inconsistent logic between KeyAvailable and ReadKey
This affects input via an Alt plus numpad number sequence
Unfortunately, the opposite happens, because ReadKey() silently eats these keys in anticipation of a subsequent synthesized

I've tried the following combination: "Alt+NumPad1" and the outcome is the same (nothing gets printed). @mawosoft am I missing something?

With the new version and for Alt down, Num1 down, Num1 up, Alt up, you should get: key: 18, mod: None, char: ☺, (which is codepoint 0x263a) after releasing the Alt key. Should also be independent of the NumLock state.

I didn't test it with this build, but I'm using almost the same code in a proprietary app, and there both automated and manual tests show the expected result.

However, on most keyboards, those keys exist twice - once on the numpad, and once on the arrow pad and control pad.

Do you mean the Home, PageUp and others?

Yep. I've listed them in the comments now: Insert, PageUp/Down, Home/End, arrow keys.

mawosoft · 2024-07-20T11:39:03Z

@adamsitnik Afterthought: You probably know this already, but just in case...

If you run this in some kind of terminal app, e.g. the terminal inside Visual Studio, you may get different results depending on how the terminal app translates those keys.

adamsitnik · 2024-07-22T09:20:10Z

If you run this in some kind of terminal a

I am using Windows Terminal on Windows 11 x64

With the new version and for Alt down, Num1 down, Num1 up, Alt up, you should get: key: 18, mod: None, char: ☺, (which is codepoint 0x263a) after releasing the Alt key. Should also be independent of the NumLock state.

I've tried that with no luck:

Press Alt
Press Num1
Release Num1
Release Alt

I didn't test it with this build,

Is there any chance you could try it locally? Your PR is overall an improvement, but it would be great co clarify that before merging.

Here you can find what is required to build dotnet/runtime: https://github.com/dotnet/runtime/blob/main/docs/workflow/requirements/windows-requirements.md

git clone https://github.com/mawosoft/runtime.git --branch fix-consolepal-windows fix-consolepal-windows
cd fix-consolepal-windows
.\build.cmd -c Release -subset clr+libs+libs.tests

This should produce corerun.exe which just runs the code from the fist argument that you provide (a path to a .dll with test app).

.\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\corerun.exe $pathToTestApp.dll

Corerun is going to use the local build of dotnet/runtime.

If you apply some changes to System.Console, you need to run following command to get the files updated:

.\dotnet.cmd build .\src\libraries\System.Console\System.Console.sln -c Release

there both automated and manual tests show the expected result.

May I ask how have you implemented automated tests for it? Have you just mocked the input for the IsReadKeyEvent method?

mawosoft · 2024-07-22T10:29:59Z

If you run this in some kind of terminal a

I am using Windows Terminal on Windows 11 x64

Can you run this in a regular ConHost? I should note that I did all testing on Win10 x64. Not sure if Win11 could throw a wrench into things.

Anyway, with Windows Terminal or any other you basically get kbd input --> translate to virtual terminal sequence -> send VT sequence -> read and interpret VT sequence -> synthesize kbd events, which may not match the original input at all.

Is there any chance you could try it locally? Your PR is overall an improvement, but it would be great co clarify that before merging.

Not before Wednesday. I guess we have to postpone it then, as you suggested initially.

May I ask how have you implemented automated tests for it? Have you just mocked the input for the IsReadKeyEvent method?

No, I used the native SendInput (user32) to run through most possible key combos in Conhost. It's a custom impl, not AppDriver or something. It's a bit tricky to sync properly, and while automated, I only ran them locally, not in a CI environment.

adamsitnik

Can you run this in a regular ConHost?

I did. It's the same. FWIW the following condition is true and false is being returned, but there is no follow up event that this comment mentions:

runtime/src/libraries/System.Console/src/System/ConsolePal.Windows.cs

Lines 237 to 243 in 05240f1

    
           // Possible Alt+NumPad unicode key sequence which surfaces by a subsequent 
        
           // Alt keyup event with uChar (see above). 
        
           ConsoleKey key = (ConsoleKey)keyCode; 
        
           if (key is >= ConsoleKey.NumPad0 and <= ConsoleKey.NumPad9) 
        
           { 
        
               // Alt+Numpad keys (as received if NumLock is on). 
        
               return false;

@mawosoft I've spent more time testing your code. It solves one of the two described problems and does not seem to introduce any new ones, so I believe it's worth merging now. If you find some free time and a way to fix the other issue, please send a follow up PR.

adamsitnik · 2024-07-22T12:19:53Z

src/libraries/System.Console/src/System/ConsolePal.Windows.cs

+                        // Possible Alt+NumPad unicode key sequence which surfaces by a subsequent
+                        // Alt keyup event with uChar (see above).
+                        ConsoleKey key = (ConsoleKey)keyCode;
+                        if (key is >= ConsoleKey.NumPad0 and <= ConsoleKey.NumPad9)
+                        {
+                            // Alt+Numpad keys (as received if NumLock is on).
+                            return false;
+                        }
+
+                        // If Numlock is off, the physical Numpad keys are received as navigation or
+                        // function keys. The EnhancedKey flag tells us whether these virtual keys
+                        // really originate from the numpad, or from the arrow pad / control pad.
+                        if ((keyState & ControlKeyState.EnhancedKey) == 0)
+                        {
+                            // If the EnhancedKey flag is not set, the following virtual keys originate
+                            // from the numpad.
+                            if (key is ConsoleKey.Clear or ConsoleKey.Insert)
+                            {
+                                // Skip Clear and Insert (usually mapped to Numpad 5 and 0).
+                                return false;
+                            }
+
+                            if (key is >= ConsoleKey.PageUp and <= ConsoleKey.DownArrow)
+                            {
+                                // Skip PageUp/Down, End/Home, and arrow keys.
+                                return false;
+                            }
+                        }


nit: the indentation is one level too deep. It's not worth starting the CI for this change, this can be fixed in a follow-up PR.

Suggested change

// Possible Alt+NumPad unicode key sequence which surfaces by a subsequent

// Alt keyup event with uChar (see above).

ConsoleKey key = (ConsoleKey)keyCode;

if (key is >= ConsoleKey.NumPad0 and <= ConsoleKey.NumPad9)

{

// Alt+Numpad keys (as received if NumLock is on).

return false;

}

// If Numlock is off, the physical Numpad keys are received as navigation or

// function keys. The EnhancedKey flag tells us whether these virtual keys

// really originate from the numpad, or from the arrow pad / control pad.

if ((keyState & ControlKeyState.EnhancedKey) == 0)

{

// If the EnhancedKey flag is not set, the following virtual keys originate

// from the numpad.

if (key is ConsoleKey.Clear or ConsoleKey.Insert)

{

// Skip Clear and Insert (usually mapped to Numpad 5 and 0).

return false;

}

if (key is >= ConsoleKey.PageUp and <= ConsoleKey.DownArrow)

{

// Skip PageUp/Down, End/Home, and arrow keys.

return false;

}

}

// Possible Alt+NumPad unicode key sequence which surfaces by a subsequent

// Alt keyup event with uChar (see above).

ConsoleKey key = (ConsoleKey)keyCode;

if (key is >= ConsoleKey.NumPad0 and <= ConsoleKey.NumPad9)

{

// Alt+Numpad keys (as received if NumLock is on).

return false;

}

// If Numlock is off, the physical Numpad keys are received as navigation or

// function keys. The EnhancedKey flag tells us whether these virtual keys

// really originate from the numpad, or from the arrow pad / control pad.

if ((keyState & ControlKeyState.EnhancedKey) == 0)

{

// If the EnhancedKey flag is not set, the following virtual keys originate

// from the numpad.

if (key is ConsoleKey.Clear or ConsoleKey.Insert)

{

// Skip Clear and Insert (usually mapped to Numpad 5 and 0).

return false;

}

if (key is >= ConsoleKey.PageUp and <= ConsoleKey.DownArrow)

{

// Skip PageUp/Down, End/Home, and arrow keys.

return false;

}

}

mawosoft · 2024-07-22T16:01:02Z

@adamsitnik Thanks for the tests.

If the follow-up event (the Alt keyup with the unicode char) doesn't appear, it indicates something driver-related. Is your numeric keyboard by any chance just a separate number block used in addition to the default keyboard? Do the Alt Numpad keys produce 'special' chars in other apps, e.g. in a plain old cmd.exe?

An alternative would be to use the On-Screen Keyboard where you can enable the numpad section regardless of your physical keyboard layout.

adamsitnik · 2024-07-23T07:26:53Z

Is your numeric keyboard by any chance just a separate number block used in addition to the default keyboard

It is. To be exact it's Microsoft Sculpt Ergonomic Desktop

Do the Alt Numpad keys produce 'special' chars in other apps, e.g. in a plain old cmd.exe?

They don't.

An alternative would be to use the On-Screen Keyboard where you can enable the numpad section regardless of your physical keyboard layout.

I've tried that and the effect was the same. (to be exact I've enabled NumLock in the options, then started the app, pressed the right alt and then selected 1 from NumPad).

mawosoft · 2024-07-24T15:27:18Z

@adamsitnik Well, its definitely some problem with your system/keyboard.

I managed to run a test via CI that simulates pressing Alt+Numpad1:

Workflow yml
Workflow result
Program.cs

mawosoft added 2 commits May 19, 2024 22:35

Fix KeyAvailable and ReadKey() in ConsolePal.Windows.

93ee8d5

Merge remote-tracking branch 'origin/main' into fix-consolepal-windows

f41c487

dotnet-issue-labeler bot added the area-System.Console label Jul 16, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 16, 2024

Fix typo.

136c756

This was referenced Jul 16, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

The job running on agent NetCore-Public ran longer than the maximum time #104044

Closed

adamsitnik reviewed Jul 18, 2024

View reviewed changes

src/libraries/System.Console/src/System/ConsolePal.Windows.cs Outdated Show resolved Hide resolved

src/libraries/System.Console/src/System/ConsolePal.Windows.cs Show resolved Hide resolved

mawosoft added 2 commits July 18, 2024 16:06

Merge remote-tracking branch 'origin/main' into fix-consolepal-windows

ede58ba

[skip ci]

Apply suggestions from code review.

05240f1

build-analysis bot mentioned this pull request Jul 18, 2024

'System.Net.NameResolution.Functional.Tests' Failure #105092

Closed

adamsitnik reviewed Jul 19, 2024

View reviewed changes

adamsitnik approved these changes Jul 22, 2024

View reviewed changes

adamsitnik added this to the 9.0.0 milestone Jul 22, 2024

adamsitnik merged commit 38c120d into dotnet:main Jul 22, 2024
81 of 84 checks passed

adamsitnik mentioned this pull request Jul 25, 2024

Bugs in Console members KeyAvailable and ReadKey() on Windows platform #102425

Closed

github-actions bot locked and limited conversation to collaborators Aug 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix KeyAvailable and ReadKey() in ConsolePal.Windows. #104984

Fix KeyAvailable and ReadKey() in ConsolePal.Windows. #104984

mawosoft commented Jul 16, 2024 •

edited by adamsitnik

Loading

dotnet-policy-service bot commented Jul 16, 2024

mawosoft commented Jul 17, 2024

adamsitnik left a comment

adamsitnik left a comment

mawosoft commented Jul 19, 2024

mawosoft commented Jul 20, 2024

adamsitnik commented Jul 22, 2024

mawosoft commented Jul 22, 2024 •

edited

Loading

adamsitnik left a comment •

edited

Loading

adamsitnik Jul 22, 2024

mawosoft commented Jul 22, 2024

adamsitnik commented Jul 23, 2024

mawosoft commented Jul 24, 2024

	// Possible Alt+NumPad unicode key sequence which surfaces by a subsequent
	// Alt keyup event with uChar (see above).
	ConsoleKey key = (ConsoleKey)keyCode;
	if (key is >= ConsoleKey.NumPad0 and <= ConsoleKey.NumPad9)
	{
	// Alt+Numpad keys (as received if NumLock is on).
	return false;

Fix KeyAvailable and ReadKey() in ConsolePal.Windows. #104984

Fix KeyAvailable and ReadKey() in ConsolePal.Windows. #104984

Conversation

mawosoft commented Jul 16, 2024 • edited by adamsitnik Loading

dotnet-policy-service bot commented Jul 16, 2024

mawosoft commented Jul 17, 2024

adamsitnik left a comment

Choose a reason for hiding this comment

adamsitnik left a comment

Choose a reason for hiding this comment

mawosoft commented Jul 19, 2024

mawosoft commented Jul 20, 2024

adamsitnik commented Jul 22, 2024

mawosoft commented Jul 22, 2024 • edited Loading

adamsitnik left a comment • edited Loading

Choose a reason for hiding this comment

adamsitnik Jul 22, 2024

Choose a reason for hiding this comment

mawosoft commented Jul 22, 2024

adamsitnik commented Jul 23, 2024

mawosoft commented Jul 24, 2024

mawosoft commented Jul 16, 2024 •

edited by adamsitnik

Loading

mawosoft commented Jul 22, 2024 •

edited

Loading

adamsitnik left a comment •

edited

Loading