Skip to content
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

Jump to bottom

Terminal doesn’t support audible bell (BEL, 0x7) #4046

Closed
AnFunctionArray opened this issue Dec 23, 2019 · 7 comments · Fixed by #7679
Closed

Terminal doesn’t support audible bell (BEL, 0x7) #4046

AnFunctionArray opened this issue Dec 23, 2019 · 7 comments · Fixed by #7679
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v2.0

Comments

@AnFunctionArray
Copy link

Environment

Microsoft Windows [Version 10.0.19030.1]

Windows Terminal 0.7.3451.0

Steps to reproduce

echo Ctrl+G

Expected behavior

A beep

Actual behavior

Silent
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Dec 23, 2019
@AnFunctionArray AnFunctionArray changed the title Please support beeping Beep is not producing a sound Dec 23, 2019
@DHowett-MSFT DHowett-MSFT changed the title Beep is not producing a sound Terminal doesn’t support audible bell (BEL, 0x7) Dec 23, 2019
@DHowett-MSFT
Copy link
Contributor

I thought we had a duplicate for this, but it looks like we don’t. We had #2952, but that was tracking the pseudoconsole infrastructure for bell support.

@DHowett-MSFT DHowett-MSFT added Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Task It's a feature request, but it doesn't really need a major design. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Dec 23, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Dec 23, 2019
@DHowett-MSFT
Copy link
Contributor

This will land after #780.

@DHowett-MSFT DHowett-MSFT added this to the Terminal v1.0 milestone Dec 23, 2019
@ffes
Copy link

ffes commented Dec 23, 2019

Probably related to #2360

@j4james j4james mentioned this issue Jan 9, 2020
Merged
5 tasks
DHowett-MSFT pushed a commit that referenced this issue Jan 17, 2020
@j4james @DHowett
Dispatch more C0 control characters from the VT state machine (#4171)
0586955 
This commit moves the handling of the `BEL`, `BS`, `TAB`, and `CR`
controls characters into the state machine (when in VT mode), instead of
forwarding them on to the default string writer, which would otherwise
have to parse them out all over again.

This doesn't cover all the control characters, but `ESC`, `SUB`, and
`CAN` are already an integral part of the `StateMachine` itself; `NUL`
is filtered out by the `OutputStateMachineEngine`; and `LF`, `FF`, and
`VT`  are due to be implemented as part of PR #3271.

Once all of these controls are handled at the state machine level, we
can strip out all the VT-specific code from the `WriteCharsLegacy`
function, which should simplify it considerably. This would also let us
simplify the `Terminal::_WriteBuffer` implementation, and the planned
replacement stream writer for issue #780.

On the conhost side, the implementation is handled as follows:

* The `BS` control is dispatched to the existing `CursorBackward`
  method, with a distance of 1.
* The `TAB` control is dispatched to the existing `ForwardTab` method,
  with a tab count of 1.
* The `CR` control required a new dispatch method, but the
  implementation was a simple call to the new `_CursorMovePosition` method
  from PR #3628.
* The `BEL` control also required a new dispatch method, as well as an
  additional private API in the `ConGetSet` interface. But that's mostly
  boilerplate code - ultimately it just calls the `SendNotifyBeep` method.

On the Windows Terminal side, not all dispatch methods are implemented.

* There is an existing `CursorBackward` implementation, so `BS` works
  OK.
* There isn't a `ForwardTab` implementation, but `TAB` isn't currently
  required by the conpty protocol.
* I had to implement the `CarriageReturn` dispatch method, but that was
  a simple call to `Terminal::SetCursorPosition`.
* The `WarningBell` method I've left unimplemented, because that
  functionality wasn't previously supported anyway, and there's an
  existing issue for that (#4046).

## Validation Steps Performed

I've added a state machine test to confirm that the updated control
characters are now forwarded to the appropriate dispatch handlers. But
since the actual implementation is mostly relying on existing
functionality, I'm assuming that code is already adequately tested
elsewhere. That said, I have also run various manual tests of my own,
and confirmed that everything still worked as well as before.

References #3271
References #780
References #3628
References #4046
@cinnamon-msft cinnamon-msft added Help Wanted We encourage anyone to jump in on these. v1-Scrubbed labels Jan 23, 2020
This was referenced Jun 18, 2020
Closed
Closed
@KalleOlaviNiemitalo KalleOlaviNiemitalo mentioned this issue Aug 12, 2020
Closed
@zadjii-msft zadjii-msft added the good first issue This is a fix that might be easier for someone to do as a first contribution label Aug 18, 2020
@angelog0
Copy link

When is the BELL/Beep scheduled for? Initially it was planned for version 1.0.. or not?

@j4james j4james mentioned this issue Sep 19, 2020
Merged
6 tasks
@ghost ghost added the In-PR This issue has a related PR label Sep 19, 2020
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements and removed In-PR This issue has a related PR labels Oct 1, 2020
DHowett pushed a commit that referenced this issue Oct 1, 2020
@j4james
Add support for the BEL control in Windows Terminal (#7679)
09cc5f4 
This commit makes the Windows Terminal play an audible sound when the
`BEL` control character is output.

The `BEL` control was already being forwarded through conpty, so it was
just a matter of hooking up the `WarningBell` dispatch method to
actually play a sound. I've used the `PlaySound` API to output the sound
configured for the "Critical Stop" system event (aka _SystemHand_),
since that is the sound used in conhost.

## Validation

I've manually confirmed that the terminal produces the expected sound
when executing `echo ^G` in a cmd shell, or `printf "\a"` in a WSL bash
shell.

References:
* There is a separate issue (#1608) to deal with configuring the `BEL`
  to trigger visual forms of notification.
* There is also an issue (#2360) requesting an option to disable the
  `BEL`.

Closes #4046
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Oct 1, 2020
@ghost
Copy link

ghost commented Nov 11, 2020

🎉This issue was addressed in #7679, which has now been successfully released as Windows Terminal Preview v1.5.3142.0.:tada:

Handy links:

@zadjii-msft zadjii-msft mentioned this issue Sep 20, 2021
Closed
@sebimoe
Copy link

sebimoe commented May 18, 2024

I'm having issue described in #6568 in v1.19.11213.0

Is the bell supposed to be playing Critical Stop sound?

@j4james
Copy link
Collaborator

j4james commented May 18, 2024

@sebimoe You can configure the bell sound in the settings.json file. See PR #11511.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v2.0
Development

Successfully merging a pull request may close this issue.

Add support for the BEL control in Windows Terminal j4james/terminal
8 participants
@sebimoe @ffes @j4james @DHowett-MSFT @AnFunctionArray @zadjii-msft @angelog0 @cinnamon-msft