-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
handle Unicode encoding in Console.Beep #105094
Conversation
Tagging subscribers to this area: @dotnet/area-system-console |
bool isUnicode = Console.OutputEncoding.CodePage == UnicodeCodePage; | ||
|
||
// Windows doesn't use terminfo, so the codepoint is hardcoded. | ||
ReadOnlySpan<byte> bell = isUnicode ? stackalloc byte[2] { 7, 0 } : stackalloc byte[1] { 7 }; |
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 assume we don't need to worry about endianness here?
@@ -679,8 +679,13 @@ public static void Beep() | |||
{ | |||
if (!Console.IsOutputRedirected) | |||
{ | |||
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); | |||
bool isUnicode = 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.
Are there other encodings that are not UnicodeCodePage
and that may end up the bell characters into more than one byte? Would it make more sense to just encode the bell character using the OutputEncoding
instead of trying to guess what the encoding may do 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.
You are right. Moreover, this method should never be on a hot path so we don't really gain anything by hardcoding the encoded bytes. I've changed the implementation, PTAL
int errorCode = WindowsConsoleStream.WriteFileNative(OutputHandle, bell, useFileAPIs: Console.OutputEncoding.CodePage != UnicodeCodePage); | ||
if (errorCode == Interop.Errors.ERROR_SUCCESS) | ||
Span<byte> bell = stackalloc byte[10]; | ||
if (Console.OutputEncoding.TryGetBytes("\u0007", bell, out int bytesWritten)) |
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.
OutputEncoding
can be a custom encoding provided by the user. Nothing guarantees that it is going to encode the bell character into less than 10 bytes. For example, the encoding can be used as a hook to introduce some ridiculous escape sequences.
I would keep this simple a just let the encoding to allocate the array.
…gh to encode a single character for custom encoding
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.
If we think the allocation here for Beep matter, then this looks fine. Otherwise, you can simplify it down to just always using GetBytes and call it a day.
https://github.com/dotnet/runtime/pull/104966/files#r1679690576