Skip to content

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

Merged
merged 5 commits into from
Jul 17, 2024

Conversation

adamsitnik
Copy link
Member

The goal of this PR is to address the feedback from #102685

cc @karakasa

Copy link
Contributor

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

if (!Console.IsOutputRedirected)
{
Console.Out.Write(BellCharacter);
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member

@stephentoub stephentoub left a 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.
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

@jkotas jkotas Jul 17, 2024

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);

Copy link
Member Author

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?

Copy link
Member Author

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.

Copy link
Member Author

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);
Copy link
Contributor

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)

Copy link
Member

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.

@adamsitnik adamsitnik merged commit 8602bb3 into dotnet:main Jul 17, 2024
83 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants