Implement `Console.Beep` via bell character instead of Windows API #102685

karakasa · 2024-05-25T10:27:49Z

Fix #102211

The behavioral breaking change would require a later document change.

implement Console.Beep via BEL char, rather than the original pinvoke.

stdout/err when redirected

dotnet-policy-service · 2024-05-25T14:25:11Z

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

karakasa · 2024-05-25T15:26:41Z

@jkotas Could you kindly send the DL for me?

jkotas · 2024-05-25T15:37:00Z

@jkotas Could you kindly send the DL for me?

The breaking change notification should be done by @dotnet/area-system-console owners once the PR is merged.

karakasa · 2024-05-25T15:46:07Z

The breaking change notification should be done by @dotnet/area-system-console owners once the PR is merged.

Ah, sorry. Per the bot message I thought this needs to be done by me.

adamsitnik

@karakasa thank you for providing high quality contribution (I really liked the tests)!

adamsitnik · 2024-07-15T11:18:29Z

jkotas added the breaking-change label on May 25

@jkotas it's more like a bug fix to me. Do you really believe we should provide breaking change docs for this PR?

stephentoub · 2024-07-15T12:37:10Z

src/libraries/System.Console/src/System/ConsolePal.Windows.cs

+
+            if (!Console.IsOutputRedirected)
+            {
+                Console.Out.Write(BellCharacter);


Won't this break beeps if someone has used Console.SetOut to hook console writing?

Console.SetOut will make Console.IsOutputRedirected return true.

Console.SetOut will make Console.IsOutputRedirected return true.

That is not the case.

using System.Text; var ms = new MemoryStream(); var orig = Console.Out; Console.SetOut(new StreamWriter(ms) { AutoFlush = true }); Console.WriteLine(Console.IsOutputRedirected); Console.SetOut(orig); Console.WriteLine(Encoding.UTF8.GetString(ms.ToArray()));

That prints false.

Yeah, I just tried this:

Console.SetOut(new StringWriter()); Console.Beep();

It used to beep. With this PR, it no longer does.

@adamsitnik

jkotas · 2024-07-15T12:47:41Z

Do you really believe we should provide breaking change docs for this PR?

Console.Beep is going to produce whatever jingle is configured for the bell character in your Windows or terminal settings. It is going to be very different sound from the Beep that it produces today. I think it qualifies as a breaking change.

Run the following to see the difference:

Console.Beep();

Thread.Sleep(1000);

const char BellCharacter = '\u0007';
Console.Out.Write(BellCharacter);

Also, the docs need to be updated. https://learn.microsoft.com/en-us/dotnet/api/system.console.beep says that "the beep plays at a frequency of 800 hertz for a duration of 200 milliseconds." and "Beep wraps a call to the Windows Beep function" that is not going to be the case after this change.

jkotas · 2024-07-15T13:06:25Z

src/libraries/System.Console/src/System/ConsolePal.Windows.cs

+                return;
+            }
+
+            if (!Console.IsErrorRedirected)


The Unix version of this function does not write to stderr. Should we try to be consistent between Windows and Unix - produce the bell character for !Console.IsOutputRedirected only?

stephentoub · 2024-07-16T12:25:19Z

src/libraries/System.Console/src/System/ConsolePal.Windows.cs

+
+            if (!Console.IsOutputRedirected)
+            {
+                Console.Out.Write(BellCharacter);


If we really want to do what this PR is doing, we shouldn't be going to Console.Out. We should instead do what the unix implementation does, which is going directly to the stdout handle / file descriptor, such that it won't target Console.Out and any custom writer it may have been redirected to, e.g.

runtime/src/libraries/System.Console/src/System/ConsolePal.Unix.cs

Line 1121 in 25a5085

=> WriteTerminalAnsiString(value, Interop.Sys.FileDescriptors.STDOUT_FILENO, mayChangeCursorPosition: false);

In the meantime, though, I suggest this PR be reverted.

In the meantime, though, I suggest this PR be reverted.

I am going to give it a try and if I can not implement it in 1h I am going to revert the PR. Does that sound OK @stephentoub ?

Yup, thanks.

That said, I'm still not sure about the premise of this change. Isn't it strange that Beep with and without parameters are now fundamentally different? This goes to Jan's points about this being a breaking change.

That said, I'm still not sure about the premise of this change.

#102211 (comment)

That said, I'm still not sure about the premise of this change.

#102211 (comment)

I know what's written in the issue. What I meant was I'm not sure it's a desirable change, given the other Beep overload and back compat. It is a breaking change to be documented, at a minimum.

I've sent #104966

karakasa added 2 commits May 25, 2024 16:44

fix dotnet#102211

53d94f4

implement Console.Beep via BEL char, rather than the original pinvoke.

ensure the new Beep doesn't write to

f1ca00d

stdout/err when redirected

ghost added the area-System.Console label May 25, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 25, 2024

karakasa marked this pull request as ready for review May 25, 2024 12:12

build-analysis bot mentioned this pull request May 25, 2024

System.IO.Net5Compat.Tests and System.IO.Tests suddenly exiting with error 137 #100558

Closed

jkotas added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label May 25, 2024

dotnet-policy-service bot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label May 25, 2024

karakasa mentioned this pull request May 25, 2024

[Breaking change]: Implement System.Console.Beep() via bell character instead of Windows API dotnet/docs#41078

Closed

3 tasks

stephentoub requested a review from adamsitnik July 9, 2024 15:41

adamsitnik approved these changes Jul 15, 2024

View reviewed changes

adamsitnik merged commit 06c8a3c into dotnet:main Jul 15, 2024

adamsitnik added this to the 9.0.0 milestone Jul 15, 2024

stephentoub reviewed Jul 15, 2024

View reviewed changes

jkotas reviewed Jul 15, 2024

View reviewed changes

stephentoub reviewed Jul 16, 2024

View reviewed changes

adamsitnik mentioned this pull request Jul 16, 2024

Console beep: perform the sys-call directly #104966

Merged

github-actions bot locked and limited conversation to collaborators Aug 16, 2024

Implement Console.Beep via bell character instead of Windows API #102685

Implement Console.Beep via bell character instead of Windows API #102685

Uh oh!

Conversation

karakasa commented May 25, 2024

Uh oh!

dotnet-policy-service bot commented May 25, 2024

Uh oh!

karakasa commented May 25, 2024

Uh oh!

jkotas commented May 25, 2024

Uh oh!

karakasa commented May 25, 2024

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

adamsitnik commented Jul 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Implement `Console.Beep` via bell character instead of Windows API #102685

Implement `Console.Beep` via bell character instead of Windows API #102685

stephentoub Jul 16, 2024 •

edited

Loading

jkotas commented Jul 15, 2024 •

edited

Loading

stephentoub Jul 16, 2024 •

edited

Loading