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.

Sign up for GitHub

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
Merged

Console beep: perform the sys-call directly #104966

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
be211cb
first attempt: use Console.Out, but only when s_isOutTextWriterRedire…
adamsitnik Jul 16, 2024
e836ab1
Revert "first attempt: use Console.Out, but only when s_isOutTextWrit…
adamsitnik Jul 16, 2024
1f2ec14
2nd approach: perform sys-call directly, don't use Console.Out
adamsitnik Jul 16, 2024
42faac9
address code review feedback
adamsitnik Jul 16, 2024
4c64ab5
Feedback on code review, you must address.
adamsitnik Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 8 additions & 19 deletions src/libraries/System.Console/src/System/ConsolePal.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -677,28 +677,17 @@ public static unsafe string Title

public static void Beep()
{
const char BellCharacter = '\u0007'; // Windows doesn't use terminfo, so the codepoint is hardcoded.

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?

return;
}

if (!Console.IsErrorRedirected)
{
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 •
edited
Loading

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

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.

if (errorCode == Interop.Errors.ERROR_SUCCESS)
{
return;
}
}

BeepFallback();
}

private static void BeepFallback()
{
const int BeepFrequencyInHz = 800;
const int BeepDurationInMs = 200;
Interop.Kernel32.Beep(BeepFrequencyInHz, BeepDurationInMs);
Interop.Kernel32.Beep(frequency: 800, duration: 200);
}

public static void Beep(int frequency, int duration)
Expand Down Expand Up @@ -1226,7 +1215,7 @@ private static unsafe int ReadFileNative(IntPtr hFile, Span<byte> buffer, bool i
return errorCode;
}

private static unsafe int WriteFileNative(IntPtr hFile, ReadOnlySpan<byte> bytes, bool useFileAPIs)
internal static unsafe int WriteFileNative(IntPtr hFile, ReadOnlySpan<byte> bytes, bool useFileAPIs)
{
if (bytes.IsEmpty)
return Interop.Errors.ERROR_SUCCESS;
Expand Down