-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Console beep: perform the sys-call directly #104966
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
Conversation
…cted is false (it's set by Console.SetOut)
…erRedirected is false (it's set by Console.SetOut)" This reverts commit be211cb.
Tagging subscribers to this area: @dotnet/area-system-console |
if (!Console.IsOutputRedirected) | ||
{ | ||
Console.Out.Write(BellCharacter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a test we can add that would have failed with the old impl here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I currently don't see a way to properly test it. We just call a sys-call when the output is redirected or perform a direct write when it's not, so I don't see a way to capture this information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can validate that the beep character is not being written to the TextWriter set with SetOut?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
{ | ||
Console.Error.Write(BellCharacter); | ||
return; | ||
ReadOnlySpan<byte> bell = "\u0007"u8; // Windows doesn't use terminfo, so the codepoint is hardcoded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be 2-byte char when Console.OutputEncoding.CodePage == UnicodeCodePage
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question! I've run following code:
using System.Text;
const int UnicodeCodePage = 1200;
foreach (Encoding encoding in new Encoding[] { Encoding.UTF8, Encoding.Unicode })
{
Console.OutputEncoding = encoding;
Console.WriteLine(Console.OutputEncoding);
Console.Beep();
Console.WriteLine(Console.OutputEncoding.CodePage == UnicodeCodePage);
Thread.Sleep(TimeSpan.FromSeconds(1.0));
}
with corerun.exe and it works as expected, so I guess it does not need 2-byte char for Unicode. But my testing was limited to Win 11.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you repro produce two jingles or just one? It only produces one jingle for me (from the UTF8 encoding iteration).
Try the following simpler repro - it does not produce any sound for me with this change:
using System.Text;
Console.OutputEncoding = Encoding.Unicode;
Console.Beep();
Thread.Sleep(10000);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you repro produce two jingles or just one?
Two (that is why I've added the sleep to the repro to have a pause between them).
Try the following simpler repro
It works on my machine. Does using a 2-byte buffer solves it for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkotas please excuse me, I was testing wrong implementation, as running the following command does not replace System.Console.dll
that is next to corerun.exe
:
.\dotnet.cmd build .\src\libraries\System.Console\tests\System.Console.Tests.csproj -c Release /t:Test
But running following does:
.\dotnet.cmd build .\src\libraries\System.Console\System.Console.sln -c Release
I was able to repro and will send a fix very soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix: #105094
Console.Error.Write(BellCharacter); | ||
return; | ||
ReadOnlySpan<byte> bell = "\u0007"u8; // Windows doesn't use terminfo, so the codepoint is hardcoded. | ||
int errorCode = WindowsConsoleStream.WriteFileNative(OutputHandle, bell, useFileAPIs: Console.OutputEncoding.CodePage != UnicodeCodePage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every time OutputHandle
is called, there would be a PInvoke to GetStdHandle
. Should the value be cahced?
And I believe the bell char will be printed when the program is called thru Program > somefile.txt
because the >
syntax will alter the result of GetStdHandle
. (some quick search)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I believe the bell char will be printed when the program is called thru Program > somefile.txt because the > syntax will alter the result of GetStdHandle.
Piping like that will cause Console.IsOutputRedirected to be true.
Every time OutputHandle is called, there would be a PInvoke to GetStdHandle. Should the value be cahced?
We've opted not to cache it in the past so that it continues to respect any calls made to SetStdHandle. Caching it would be another breaking change.
The goal of this PR is to address the feedback from #102685
cc @karakasa