From be211cb64533c8e9e2bc04e4595bb7be9bdc29ef Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 16 Jul 2024 15:20:12 +0200 Subject: [PATCH 1/5] first attempt: use Console.Out, but only when s_isOutTextWriterRedirected is false (it's set by Console.SetOut) --- .../System.Console/src/System/Console.cs | 2 +- .../src/System/ConsolePal.Android.cs | 2 +- .../src/System/ConsolePal.Unix.cs | 2 +- .../src/System/ConsolePal.Wasi.cs | 2 +- .../src/System/ConsolePal.WebAssembly.cs | 2 +- .../src/System/ConsolePal.Windows.cs | 20 ++++--------------- .../src/System/ConsolePal.iOS.cs | 2 +- 7 files changed, 10 insertions(+), 22 deletions(-) diff --git a/src/libraries/System.Console/src/System/Console.cs b/src/libraries/System.Console/src/System/Console.cs index d2db8f831d448b..a58e2e9aa3a92c 100644 --- a/src/libraries/System.Console/src/System/Console.cs +++ b/src/libraries/System.Console/src/System/Console.cs @@ -538,7 +538,7 @@ public static string Title [UnsupportedOSPlatform("tvos")] public static void Beep() { - ConsolePal.Beep(); + ConsolePal.Beep(s_isOutTextWriterRedirected); } [SupportedOSPlatform("windows")] diff --git a/src/libraries/System.Console/src/System/ConsolePal.Android.cs b/src/libraries/System.Console/src/System/ConsolePal.Android.cs index 2be7b49d595e9d..e3d6bca60030f1 100644 --- a/src/libraries/System.Console/src/System/ConsolePal.Android.cs +++ b/src/libraries/System.Console/src/System/ConsolePal.Android.cs @@ -94,7 +94,7 @@ public static string Title set => throw new PlatformNotSupportedException(); } - public static void Beep() => throw new PlatformNotSupportedException(); + public static void Beep(bool _) => throw new PlatformNotSupportedException(); public static void Beep(int frequency, int duration) => throw new PlatformNotSupportedException(); diff --git a/src/libraries/System.Console/src/System/ConsolePal.Unix.cs b/src/libraries/System.Console/src/System/ConsolePal.Unix.cs index 77a679150b297a..999ca99175460c 100644 --- a/src/libraries/System.Console/src/System/ConsolePal.Unix.cs +++ b/src/libraries/System.Console/src/System/ConsolePal.Unix.cs @@ -205,7 +205,7 @@ public static string Title } } - public static void Beep() + public static void Beep(bool _) { if (!Console.IsOutputRedirected) { diff --git a/src/libraries/System.Console/src/System/ConsolePal.Wasi.cs b/src/libraries/System.Console/src/System/ConsolePal.Wasi.cs index 4022de1a439fe6..dbd64fc9696fe0 100644 --- a/src/libraries/System.Console/src/System/ConsolePal.Wasi.cs +++ b/src/libraries/System.Console/src/System/ConsolePal.Wasi.cs @@ -96,7 +96,7 @@ public static string Title set => throw new PlatformNotSupportedException(); } - public static void Beep() => throw new PlatformNotSupportedException(); + public static void Beep(bool _) => throw new PlatformNotSupportedException(); public static void Clear() => throw new PlatformNotSupportedException(); public static void SetCursorPosition(int left, int top) => throw new PlatformNotSupportedException(); diff --git a/src/libraries/System.Console/src/System/ConsolePal.WebAssembly.cs b/src/libraries/System.Console/src/System/ConsolePal.WebAssembly.cs index e02991d6baf31c..6d7d7edce27be3 100644 --- a/src/libraries/System.Console/src/System/ConsolePal.WebAssembly.cs +++ b/src/libraries/System.Console/src/System/ConsolePal.WebAssembly.cs @@ -155,7 +155,7 @@ public static string Title set => throw new PlatformNotSupportedException(); } - public static void Beep() => throw new PlatformNotSupportedException(); + public static void Beep(bool _) => throw new PlatformNotSupportedException(); public static void Beep(int frequency, int duration) => throw new PlatformNotSupportedException(); diff --git a/src/libraries/System.Console/src/System/ConsolePal.Windows.cs b/src/libraries/System.Console/src/System/ConsolePal.Windows.cs index 2edf2da4cebb65..66eefc7cdd8b0e 100644 --- a/src/libraries/System.Console/src/System/ConsolePal.Windows.cs +++ b/src/libraries/System.Console/src/System/ConsolePal.Windows.cs @@ -675,30 +675,18 @@ public static unsafe string Title } } - public static void Beep() + public static void Beep(bool isOutTextWriterRedirected) { const char BellCharacter = '\u0007'; // Windows doesn't use terminfo, so the codepoint is hardcoded. - if (!Console.IsOutputRedirected) + if (!Console.IsOutputRedirected && !isOutTextWriterRedirected) { Console.Out.Write(BellCharacter); - return; } - - if (!Console.IsErrorRedirected) + else { - Console.Error.Write(BellCharacter); - return; + Interop.Kernel32.Beep(frequency: 800, duration: 200); } - - BeepFallback(); - } - - private static void BeepFallback() - { - const int BeepFrequencyInHz = 800; - const int BeepDurationInMs = 200; - Interop.Kernel32.Beep(BeepFrequencyInHz, BeepDurationInMs); } public static void Beep(int frequency, int duration) diff --git a/src/libraries/System.Console/src/System/ConsolePal.iOS.cs b/src/libraries/System.Console/src/System/ConsolePal.iOS.cs index 5360134bdc108d..bfc7d81c5afbec 100644 --- a/src/libraries/System.Console/src/System/ConsolePal.iOS.cs +++ b/src/libraries/System.Console/src/System/ConsolePal.iOS.cs @@ -99,7 +99,7 @@ public static string Title set => throw new PlatformNotSupportedException(); } - public static void Beep() => throw new PlatformNotSupportedException(); + public static void Beep(bool _) => throw new PlatformNotSupportedException(); public static void Beep(int frequency, int duration) => throw new PlatformNotSupportedException(); From e836ab1d825cfe7d51d74364f89919963f75ea92 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 16 Jul 2024 15:20:31 +0200 Subject: [PATCH 2/5] Revert "first attempt: use Console.Out, but only when s_isOutTextWriterRedirected is false (it's set by Console.SetOut)" This reverts commit be211cb64533c8e9e2bc04e4595bb7be9bdc29ef. --- .../System.Console/src/System/Console.cs | 2 +- .../src/System/ConsolePal.Android.cs | 2 +- .../src/System/ConsolePal.Unix.cs | 2 +- .../src/System/ConsolePal.Wasi.cs | 2 +- .../src/System/ConsolePal.WebAssembly.cs | 2 +- .../src/System/ConsolePal.Windows.cs | 20 +++++++++++++++---- .../src/System/ConsolePal.iOS.cs | 2 +- 7 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/libraries/System.Console/src/System/Console.cs b/src/libraries/System.Console/src/System/Console.cs index a58e2e9aa3a92c..d2db8f831d448b 100644 --- a/src/libraries/System.Console/src/System/Console.cs +++ b/src/libraries/System.Console/src/System/Console.cs @@ -538,7 +538,7 @@ public static string Title [UnsupportedOSPlatform("tvos")] public static void Beep() { - ConsolePal.Beep(s_isOutTextWriterRedirected); + ConsolePal.Beep(); } [SupportedOSPlatform("windows")] diff --git a/src/libraries/System.Console/src/System/ConsolePal.Android.cs b/src/libraries/System.Console/src/System/ConsolePal.Android.cs index e3d6bca60030f1..2be7b49d595e9d 100644 --- a/src/libraries/System.Console/src/System/ConsolePal.Android.cs +++ b/src/libraries/System.Console/src/System/ConsolePal.Android.cs @@ -94,7 +94,7 @@ public static string Title set => throw new PlatformNotSupportedException(); } - public static void Beep(bool _) => throw new PlatformNotSupportedException(); + public static void Beep() => throw new PlatformNotSupportedException(); public static void Beep(int frequency, int duration) => throw new PlatformNotSupportedException(); diff --git a/src/libraries/System.Console/src/System/ConsolePal.Unix.cs b/src/libraries/System.Console/src/System/ConsolePal.Unix.cs index 999ca99175460c..77a679150b297a 100644 --- a/src/libraries/System.Console/src/System/ConsolePal.Unix.cs +++ b/src/libraries/System.Console/src/System/ConsolePal.Unix.cs @@ -205,7 +205,7 @@ public static string Title } } - public static void Beep(bool _) + public static void Beep() { if (!Console.IsOutputRedirected) { diff --git a/src/libraries/System.Console/src/System/ConsolePal.Wasi.cs b/src/libraries/System.Console/src/System/ConsolePal.Wasi.cs index dbd64fc9696fe0..4022de1a439fe6 100644 --- a/src/libraries/System.Console/src/System/ConsolePal.Wasi.cs +++ b/src/libraries/System.Console/src/System/ConsolePal.Wasi.cs @@ -96,7 +96,7 @@ public static string Title set => throw new PlatformNotSupportedException(); } - public static void Beep(bool _) => throw new PlatformNotSupportedException(); + public static void Beep() => throw new PlatformNotSupportedException(); public static void Clear() => throw new PlatformNotSupportedException(); public static void SetCursorPosition(int left, int top) => throw new PlatformNotSupportedException(); diff --git a/src/libraries/System.Console/src/System/ConsolePal.WebAssembly.cs b/src/libraries/System.Console/src/System/ConsolePal.WebAssembly.cs index 6d7d7edce27be3..e02991d6baf31c 100644 --- a/src/libraries/System.Console/src/System/ConsolePal.WebAssembly.cs +++ b/src/libraries/System.Console/src/System/ConsolePal.WebAssembly.cs @@ -155,7 +155,7 @@ public static string Title set => throw new PlatformNotSupportedException(); } - public static void Beep(bool _) => throw new PlatformNotSupportedException(); + public static void Beep() => throw new PlatformNotSupportedException(); public static void Beep(int frequency, int duration) => throw new PlatformNotSupportedException(); diff --git a/src/libraries/System.Console/src/System/ConsolePal.Windows.cs b/src/libraries/System.Console/src/System/ConsolePal.Windows.cs index 66eefc7cdd8b0e..2edf2da4cebb65 100644 --- a/src/libraries/System.Console/src/System/ConsolePal.Windows.cs +++ b/src/libraries/System.Console/src/System/ConsolePal.Windows.cs @@ -675,18 +675,30 @@ public static unsafe string Title } } - public static void Beep(bool isOutTextWriterRedirected) + public static void Beep() { const char BellCharacter = '\u0007'; // Windows doesn't use terminfo, so the codepoint is hardcoded. - if (!Console.IsOutputRedirected && !isOutTextWriterRedirected) + if (!Console.IsOutputRedirected) { Console.Out.Write(BellCharacter); + return; } - else + + if (!Console.IsErrorRedirected) { - Interop.Kernel32.Beep(frequency: 800, duration: 200); + Console.Error.Write(BellCharacter); + return; } + + BeepFallback(); + } + + private static void BeepFallback() + { + const int BeepFrequencyInHz = 800; + const int BeepDurationInMs = 200; + Interop.Kernel32.Beep(BeepFrequencyInHz, BeepDurationInMs); } public static void Beep(int frequency, int duration) diff --git a/src/libraries/System.Console/src/System/ConsolePal.iOS.cs b/src/libraries/System.Console/src/System/ConsolePal.iOS.cs index bfc7d81c5afbec..5360134bdc108d 100644 --- a/src/libraries/System.Console/src/System/ConsolePal.iOS.cs +++ b/src/libraries/System.Console/src/System/ConsolePal.iOS.cs @@ -99,7 +99,7 @@ public static string Title set => throw new PlatformNotSupportedException(); } - public static void Beep(bool _) => throw new PlatformNotSupportedException(); + public static void Beep() => throw new PlatformNotSupportedException(); public static void Beep(int frequency, int duration) => throw new PlatformNotSupportedException(); From 1f2ec14c1e6ea1aadfc6e28b8ce9132e2eff339c Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 16 Jul 2024 15:41:00 +0200 Subject: [PATCH 3/5] 2nd approach: perform sys-call directly, don't use Console.Out --- .../src/System/ConsolePal.Windows.cs | 35 +++++++------------ 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/src/libraries/System.Console/src/System/ConsolePal.Windows.cs b/src/libraries/System.Console/src/System/ConsolePal.Windows.cs index 2edf2da4cebb65..946cf894d3c110 100644 --- a/src/libraries/System.Console/src/System/ConsolePal.Windows.cs +++ b/src/libraries/System.Console/src/System/ConsolePal.Windows.cs @@ -32,23 +32,25 @@ private static bool IsWindows7() return version.Major == 6 && version.Minor == 1; } + private static bool UseFileAPIs(bool isRedirected) => isRedirected || Console.InputEncoding.CodePage != UnicodeCodePage; + public static Stream OpenStandardInput() => GetStandardFile( Interop.Kernel32.HandleTypes.STD_INPUT_HANDLE, FileAccess.Read, - useFileAPIs: Console.InputEncoding.CodePage != UnicodeCodePage || Console.IsInputRedirected); + useFileAPIs: UseFileAPIs(Console.IsInputRedirected)); public static Stream OpenStandardOutput() => GetStandardFile( Interop.Kernel32.HandleTypes.STD_OUTPUT_HANDLE, FileAccess.Write, - useFileAPIs: Console.OutputEncoding.CodePage != UnicodeCodePage || Console.IsOutputRedirected); + useFileAPIs: UseFileAPIs(Console.IsOutputRedirected)); public static Stream OpenStandardError() => GetStandardFile( Interop.Kernel32.HandleTypes.STD_ERROR_HANDLE, FileAccess.Write, - useFileAPIs: Console.OutputEncoding.CodePage != UnicodeCodePage || Console.IsErrorRedirected); + useFileAPIs: UseFileAPIs(Console.IsErrorRedirected)); private static IntPtr InputHandle => Interop.Kernel32.GetStdHandle(Interop.Kernel32.HandleTypes.STD_INPUT_HANDLE); @@ -677,28 +679,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); - return; - } - - if (!Console.IsErrorRedirected) - { - Console.Error.Write(BellCharacter); - return; + ReadOnlySpan bell = "\u0007"u8; // Windows doesn't use terminfo, so the codepoint is hardcoded. + int errorCode = WindowsConsoleStream.WriteFileNative(OutputHandle, bell, UseFileAPIs(isRedirected: false)); + if (Interop.Errors.ERROR_SUCCESS == errorCode) + { + 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) @@ -1226,7 +1217,7 @@ private static unsafe int ReadFileNative(IntPtr hFile, Span buffer, bool i return errorCode; } - private static unsafe int WriteFileNative(IntPtr hFile, ReadOnlySpan bytes, bool useFileAPIs) + internal static unsafe int WriteFileNative(IntPtr hFile, ReadOnlySpan bytes, bool useFileAPIs) { if (bytes.IsEmpty) return Interop.Errors.ERROR_SUCCESS; From 42faac9b24623d9069bc4e154a61c12766d45158 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 16 Jul 2024 15:51:36 +0200 Subject: [PATCH 4/5] address code review feedback --- .../System.Console/src/System/ConsolePal.Windows.cs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Console/src/System/ConsolePal.Windows.cs b/src/libraries/System.Console/src/System/ConsolePal.Windows.cs index 946cf894d3c110..1a45c6ab338a84 100644 --- a/src/libraries/System.Console/src/System/ConsolePal.Windows.cs +++ b/src/libraries/System.Console/src/System/ConsolePal.Windows.cs @@ -32,25 +32,23 @@ private static bool IsWindows7() return version.Major == 6 && version.Minor == 1; } - private static bool UseFileAPIs(bool isRedirected) => isRedirected || Console.InputEncoding.CodePage != UnicodeCodePage; - public static Stream OpenStandardInput() => GetStandardFile( Interop.Kernel32.HandleTypes.STD_INPUT_HANDLE, FileAccess.Read, - useFileAPIs: UseFileAPIs(Console.IsInputRedirected)); + useFileAPIs: Console.InputEncoding.CodePage != UnicodeCodePage || Console.IsInputRedirected); public static Stream OpenStandardOutput() => GetStandardFile( Interop.Kernel32.HandleTypes.STD_OUTPUT_HANDLE, FileAccess.Write, - useFileAPIs: UseFileAPIs(Console.IsOutputRedirected)); + useFileAPIs: Console.OutputEncoding.CodePage != UnicodeCodePage || Console.IsOutputRedirected); public static Stream OpenStandardError() => GetStandardFile( Interop.Kernel32.HandleTypes.STD_ERROR_HANDLE, FileAccess.Write, - useFileAPIs: UseFileAPIs(Console.IsErrorRedirected)); + useFileAPIs: Console.OutputEncoding.CodePage != UnicodeCodePage || Console.IsErrorRedirected); private static IntPtr InputHandle => Interop.Kernel32.GetStdHandle(Interop.Kernel32.HandleTypes.STD_INPUT_HANDLE); @@ -682,7 +680,7 @@ public static void Beep() if (!Console.IsOutputRedirected) { ReadOnlySpan bell = "\u0007"u8; // Windows doesn't use terminfo, so the codepoint is hardcoded. - int errorCode = WindowsConsoleStream.WriteFileNative(OutputHandle, bell, UseFileAPIs(isRedirected: false)); + int errorCode = WindowsConsoleStream.WriteFileNative(OutputHandle, bell, useFileAPIs: Console.OutputEncoding.CodePage != UnicodeCodePage); if (Interop.Errors.ERROR_SUCCESS == errorCode) { return; From 4c64ab5c8e8105e36f3a256fd2ed90c8e6faf506 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 16 Jul 2024 15:57:19 +0200 Subject: [PATCH 5/5] Feedback on code review, you must address. --- src/libraries/System.Console/src/System/ConsolePal.Windows.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Console/src/System/ConsolePal.Windows.cs b/src/libraries/System.Console/src/System/ConsolePal.Windows.cs index 1a45c6ab338a84..49f2d42b97fe89 100644 --- a/src/libraries/System.Console/src/System/ConsolePal.Windows.cs +++ b/src/libraries/System.Console/src/System/ConsolePal.Windows.cs @@ -681,7 +681,7 @@ public static void Beep() { ReadOnlySpan bell = "\u0007"u8; // Windows doesn't use terminfo, so the codepoint is hardcoded. int errorCode = WindowsConsoleStream.WriteFileNative(OutputHandle, bell, useFileAPIs: Console.OutputEncoding.CodePage != UnicodeCodePage); - if (Interop.Errors.ERROR_SUCCESS == errorCode) + if (errorCode == Interop.Errors.ERROR_SUCCESS) { return; }