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.

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

Add support for the BEL control in Windows Terminal #7679

Merged
merged 4 commits into from
Oct 1, 2020

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Sep 19, 2020

Summary of the Pull Request

This PR makes the Windows Terminal play an audible sound when the BEL control character is output.

References

PR Checklist

  • Closes Terminal doesn’t support audible bell (BEL, 0x7) #4046
  • CLA signed.
  • Tests added/passed
  • Documentation updated.
  • Schema updated.
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan.

Detailed Description of the Pull Request / Additional comments

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 Steps Performed

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.

@ghost ghost 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. labels Sep 19, 2020
@skyline75489
Copy link
Collaborator

Somewhat related to MSFT:14062483.

@DHowett
Copy link
Member

DHowett commented Sep 24, 2020

Ah. ‘483 is a “complaint” that we changed the bell from a “ding” to the STOP sound in Windows 8(?) or so. I should cut that deliverable: we aren’t going to change it in conhost.

@j4james
Copy link
Collaborator Author

j4james commented Sep 24, 2020

I'm not sure what's up with the CI bot. It appears to be failing in a feature test on this line:

VERIFY_WIN32_BOOL_SUCCEEDED(GetConsoleScreenBufferInfoEx(consoleOutputHandle, &sbiex));

I can't imagine that has anything to do with the changes in this PR.

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔔

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(leaving a block to formulate my thoughts)

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 29, 2020
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know what, I think I've come up with what I think Dustin's block is over - we probably want the bell to be played by the TermControl layer, not the Terminal layer. That way each control implementation can control how the sound is played. Sure, right now the TermControl and the WPF Control will probably want to do the same thing, but someone could theoretically imagine another control that wanted to be able to customize this further.

Piping this up one more level shouldn't be too hard...

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 29, 2020
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, yea this is better. Should we go ahead and implement this for the WPF control at the same time as well? (I won't block over that, we can always file a follow-up for that as well)

@j4james
Copy link
Collaborator Author

j4james commented Sep 29, 2020

I'm a bit out of my depth here. Am I right in thinking I actually need to turn this into an event handler of some sort? So TermControl would then just fire the event, and something like TerminalPage would have registered a handler that does the actual sound playing? Then the WPF control could assumedly hook into the event in a similar way.

I've going to do a bit more digging and see how some of the other event handling code works, but I just want to make sure this is what you actually had in mind. Of if you think it's better for me to leave this for someone that knows what they're doing and I can do that to.

@zadjii-msft
Copy link
Member

@j4james oh no, I don't think it needs to be that complicated. The WPFControl lives at the same layer as the TermControl (it's just embedded in VS instead of in the Terminal app). So in the same way that TermControl registers the function pointer for the callback with Terminal, so too would the WPF Control

@DHowett
Copy link
Member

DHowett commented Sep 29, 2020

Mike anticipated my block perfectly! I think that we should pipe this up out of TermControl as an event that TerminalPage/TerminalApp can receive. It's annoying plumbing work, but it'll enable us to light up features like "tab highlight on BEL" (or "put 🔔 icon in tab on BEL") and flip the configuration for whether bells sound to being TerminalApp-only instead of having to push it down into TerminalControl via ITerminalSettings.

You can mimic the TabColorChanged or Initialized events -- they use TYPED_EVENT, the former with IInspectable arguments which pretty much indicates "these aren't used ignore them" -- and slap them down in the .idl and .h

TYPED_EVENT(X) emits three members: an event subscriber (token X(lambda)), an event unsubscriber (void X(token)) and a functor that dispatches to all listeners, _XHandler.

@j4james
Copy link
Collaborator Author

j4james commented Sep 29, 2020

OK I've added the event handling now, and hooked up the TerminalPage class to that event to play the sound. I'm still no closer to making this work in the WPF control though. The only way I can see the cs code talking to TerminalCore was via native methods exported from HwndTerminal, which seems way more complicated than I expected. If you don't mind, maybe the WPF integration is best left for a follow-up PR, because I'm not going to be able to do much more on this for a day or two.

@DHowett
Copy link
Member

DHowett commented Sep 29, 2020

Thanks for doing the event work for Cascadia. We can definitely hold WPF back for another change. It usually gets that treatment anyway 😄

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent as always. Thanks!

@DHowett DHowett merged commit 09cc5f4 into microsoft:master Oct 1, 2020
@jarlva
Copy link

jarlva commented Oct 10, 2020

Is there a fix for the silent bell?
The bell is still silent in ubuntu 20 ( printf "\a" ) and windows cmd (echo ^G).

Terminal version 1.3.2651.0
Windows 2004
WSL2 Linux 4.19.128.

@skyline75489
Copy link
Collaborator

@jarlva I think you can expect this to be fixed in Windows Terminal 1.5, which will be released to public near the end of October.

@j4james j4james deleted the feature-terminal-bel branch October 15, 2020 20:52
DHowett pushed a commit that referenced this pull request Oct 15, 2020
Adds a new setting, `bellStyle`, to be able to disable the audible bell
added in #7679. Currently, this setting accepts two values:
* `audible`: play a noise on a bell
* `none`: Don't play a noise.

In the future, we can add a `"bellStyle": "visible"` for flashing the
Terminal instead of making a noise on bell.

## Validation Steps Performed
Pressing <kbd>Ctrl+G</kbd> in cmd, and hitting enter is an easy way of
triggering a bell. I set the setting to `none`, and presto, the bell
stopped.

Closes #2360
@ghost
Copy link

ghost commented Nov 11, 2020

🎉Windows Terminal Preview v1.5.3142.0 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terminal doesn’t support audible bell (BEL, 0x7)
6 participants