Enable changing the bell sound by zadjii-msft · Pull Request #11511 · microsoft/terminal

zadjii-msft · 2021-10-14T20:20:19Z

Summary of the Pull Request

Adds a per-profile setting for setting the audio sound for the bell. The setting is bellSound, it accepts a path. We'll use the file at that path as the sound for the bell. If it doesn't exist, then oh well, so sound for you.

It'll also secretly accept an array of paths. If you provide an array, it will pick one at random.

PR Checklist

Closes Specify an audio file for the audible bell #8366
I work here
Tests - lol this is the hackathon, I'm just messing around
Requires documentation to be updated

Validation Steps Performed

I'm not suggesting that anyone go to this post and download a zip full of honk.mp3s. I'm definitely not suggesting you add it to your settings like

"bellSound": [
    "C:\\Users\\migrie\\Downloads\\memes\\honks\\Honk1.mp3",
    "C:\\Users\\migrie\\Downloads\\memes\\honks\\Honk2.mp3",
    "C:\\Users\\migrie\\Downloads\\memes\\honks\\Honk3.mp3",
    "C:\\Users\\migrie\\Downloads\\memes\\honks\\Honk4.mp3",
    "C:\\Users\\migrie\\Downloads\\memes\\honks\\Honk-muffled1.mp3",
    "C:\\Users\\migrie\\Downloads\\memes\\honks\\Honk-muffled2.mp3",
    "C:\\Users\\migrie\\Downloads\\memes\\honks\\Honk-muffled3.mp3"
]

No, don't do that.

honk-001.mp4

It surprisingly works elevated
We should probably accept env vars in these paths
We may only want one MediaPlayer per terminal, rather than one per pane
We may want to validate the paths, and discard ones that don't exist.
- alternatively, meh

DHowett · 2021-10-14T22:57:23Z

~~HEAK YEAH IT HONMK~~ safely ignore me

src/cascadia/TerminalApp/Pane.cpp

src/cascadia/TerminalApp/Pane.h

src/cascadia/TerminalSettingsModel/JsonUtils.h

KalleOlaviNiemitalo · 2021-10-14T23:24:54Z

src/cascadia/TerminalApp/Pane.cpp

+                    winrt::Windows::Foundation::Uri uri{ sounds.GetAt(rand() % sounds.Size()) };
+                    auto source{ winrt::Windows::Media::Core::MediaSource::CreateFromUri(uri) };
+                    auto item{ winrt::Windows::Media::Playback::MediaPlaybackItem(source) };
+                    p.Source(item);
+                    p.Play();


What happens if the URI points to a video file or stream?

omfg it's great.

Say you had a file that was no.mp4. If you set the path to "bellSound": "%~%\\Downloads\\memes\\no.mp4", then it'll just play the entirety of the audio of the mp4 file. It's great. If you emit another bell in the middle, then it'll just start it over.

There's a lot of possibilities here.

src/cascadia/TerminalApp/Pane.cpp

Somehow, this also causes an exception on teardown north of `Windows.Media.MediaControl.dll!AudioStateMonitorImpl::remove_SoundLevelChanged()`, which is in OS code somewhere. Not sure there's anything we can actually do about this. There are piles of bugs on that function in the OS repo. I think we're going to have to just end up avoiding this entirely.

This reverts commit fb1ed58.

carlos-zamora · 2022-01-05T15:40:07Z

src/cascadia/TerminalSettingsModel/Profile.cpp

    profile->_Hidden = _Hidden;
    profile->_TabColor = _TabColor;
    profile->_Padding = _Padding;
+


this random newline seems unnecessary but meh haha

that actually seems to be the case with most of this file.

carlos-zamora · 2022-01-05T15:47:38Z

src/cascadia/TerminalApp/Pane.cpp

+            auto source{ winrt::Windows::Media::Core::MediaSource::CreateFromUri(uri) };
+            auto item{ winrt::Windows::Media::Playback::MediaPlaybackItem(source) };


Suggested change

auto source{ winrt::Windows::Media::Core::MediaSource::CreateFromUri(uri) };

auto item{ winrt::Windows::Media::Playback::MediaPlaybackItem(source) };

const auto source{ winrt::Windows::Media::Core::MediaSource::CreateFromUri(uri) };

const auto item{ winrt::Windows::Media::Playback::MediaPlaybackItem(source) };

nit

src/cascadia/TerminalApp/Pane.cpp

carlos-zamora · 2022-01-05T16:00:52Z

Mind filing a follow-up issue for an update to the SUI?

Co-authored-by: Carlos Zamora <[email protected]>

ghost · 2022-01-06T17:11:49Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

zadjii-msft · 2022-01-27T22:48:28Z

src/cascadia/TerminalSettingsModel/JsonUtils.h

+                    val.push_back(trait.FromJson(element));
+                }
+            }
+            else


note to self: this is the place I should just check for null and skip

Sorry for combining two fixes in one PR. I can separate if need be. * [x] Closes #12276: - `"bellSound": null` didn't work. This one was easier, and is atomically in bcc2ca0. Basically, we would deserialize that as an array with a single empty string in it, which we'd try to then play. I think it's more idomatic to have that deserialized as an empty array, which correctly falls back to playing the default sound. * [x] Closes #12258: - This one is the majority of the rest of the PR. If you leave the MediaPlayer open, then the media keys will _affect the Terminal_. More importantly, once the bell sounds, they'd replay the bell, which is insane. So the fix is to re-create the media player when we need it. We do this per-pane for simpler lifetime tracking. I'm not worried about the overhead of creating a mediaplayer here, since we're already throttling bells. * Originally added in #11511 * [x] Tested manually - Use [`no.mp4`](https://www.youtube.com/watch?v=x2w9TyCv2gk) for this since that's like, 17s long - Checked that closing panes / the terminal while a bell was playing didn't crash - Playing a bunch of bells at once works - closing a pane stops the bell it's playing - once the bell stops, the media keys went back to working for Spotify * [x] I work here

ghost · 2022-02-03T18:27:39Z

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

Handy links:

dark-swordsman · 2022-07-04T08:52:43Z

Can documentation be added to elaborate on where to put this into the JSON file? Or even better, can it be a UI item where you can just select the file you want to use?

Edit: For anyone else that comes across this, it's specifically profiles.list[].bellSound. It will warn you that it is not allowed, but it is allowed.

zadjii-msft · 2022-07-05T14:35:23Z

Add bellSound to the Settings UI #12103 tracks adding this setting to the UI
WIP for adding BellSound to schema #13035 was supposed to add it to the json schema, but actually didn't. It only added the type definition, but didn't actually add it to the profile. Whoops. It should still work until that's fixed, VsCode might just complain about it.

As noted in #11511 (comment). Missed in #13035

As noted in #11511 (comment). Missed in #13035 (cherry picked from commit 86dfefa) Service-Card-Id: 83891742 Service-Version: 1.14

As noted in #11511 (comment). Missed in #13035 (cherry picked from commit 86dfefa) Service-Card-Id: 83891743 Service-Version: 1.15

zadjii-msft added 4 commits October 14, 2021 12:37

it honks

be11388

many honks

8fbd32d

oh yea, it was that easy

4c4fde9

allow a single item as a list with length 1

cc9487e

ghost added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Oct 14, 2021

zadjii-msft changed the title ~~it honks~~ Enable changing the bell sound Oct 14, 2021

DHowett reviewed Oct 14, 2021

View reviewed changes

src/cascadia/TerminalApp/Pane.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/Pane.h Outdated Show resolved Hide resolved

src/cascadia/TerminalSettingsModel/JsonUtils.h Show resolved Hide resolved

KalleOlaviNiemitalo reviewed Oct 14, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

zadjii-msft self-assigned this Nov 1, 2021

zadjii-msft added 3 commits November 1, 2021 10:40

Merge remote-tracking branch 'origin/main' into dev/migrie/fhl/honk

0bb42ef

it's a singleton now

2c895b2

allow env vars in paths too

9f4ed0e

DHowett reviewed Nov 1, 2021

View reviewed changes

src/cascadia/TerminalApp/Pane.cpp Outdated Show resolved Hide resolved

thanks for nothing VS

684956d

DHowett reviewed Nov 3, 2021

View reviewed changes

src/cascadia/TerminalApp/Pane.cpp Show resolved Hide resolved

zadjii-msft added 4 commits November 16, 2021 09:18

Merge remote-tracking branch 'origin/main' into dev/migrie/fhl/honk

2eda14c

Revert "Experiment with removing the static MediaPlayer"

0234404

This reverts commit fb1ed58.

it's BODGY time

9393753

zadjii-msft assigned DHowett and unassigned zadjii-msft Nov 16, 2021

This comment has been minimized.

Sign in to view

zadjii-msft added 2 commits January 4, 2022 10:13

Merge remote-tracking branch 'origin/main' into dev/migrie/fhl/honk

28bb76e

spel

148f382

zadjii-msft requested a review from DHowett January 4, 2022 16:13

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Jan 5, 2022

ghost requested review from PankajBhojwani, carlos-zamora, leonMSFT and lhecker January 5, 2022 12:07

carlos-zamora approved these changes Jan 5, 2022

View reviewed changes

Apply suggestions from code review

9f9cd41

Co-authored-by: Carlos Zamora <[email protected]>

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 6, 2022

zadjii-msft mentioned this pull request Jan 6, 2022

Add bellSound to the Settings UI #12103

Open

ghost merged commit 9d7966a into main Jan 6, 2022

ghost deleted the dev/migrie/fhl/honk branch January 6, 2022 17:41

This was referenced Jan 24, 2022

A way to configure the bell sound #12235

Closed

Pressing Media keys in terminal after changing bell sound replays the bell #12258

Closed

zadjii-msft commented Jan 27, 2022

View reviewed changes

ghost removed the Needs-Second It's a PR that needs another sign-off label Jan 27, 2022

zadjii-msft mentioned this pull request Jan 28, 2022

Two belling fixes #12281

Merged

4 tasks

ghost mentioned this pull request Feb 3, 2022

Specify an audio file for the audible bell #8366

Closed

This was referenced Feb 16, 2022

bellSound missing from JSON schema #12504

Closed

Add bellsound profile setting MicrosoftDocs/terminal#508

Merged

zadjii-msft mentioned this pull request Jul 5, 2022

Fix the bellsound schema, for real #13433

Merged

DHowett pushed a commit that referenced this pull request Jul 5, 2022

Fix the bellsound schema, for real (#13433)

86dfefa

As noted in #11511 (comment). Missed in #13035

DHowett pushed a commit that referenced this pull request Jul 5, 2022

Fix the bellsound schema, for real (#13433)

e5239f8

As noted in #11511 (comment). Missed in #13035 (cherry picked from commit 86dfefa) Service-Card-Id: 83891742 Service-Version: 1.14

DHowett pushed a commit that referenced this pull request Jul 5, 2022

Fix the bellsound schema, for real (#13433)

0080b81

As noted in #11511 (comment). Missed in #13035 (cherry picked from commit 86dfefa) Service-Card-Id: 83891743 Service-Version: 1.15

j4james mentioned this pull request May 18, 2024

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

Closed

This pull request was closed.

		auto source{ winrt::Windows::Media::Core::MediaSource::CreateFromUri(uri) };
		auto item{ winrt::Windows::Media::Playback::MediaPlaybackItem(source) };

Conversation

zadjii-msft commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

PR Checklist

Validation Steps Performed

Uh oh!

DHowett commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KalleOlaviNiemitalo Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

This comment has been minimized.

carlos-zamora Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

carlos-zamora Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

carlos-zamora Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carlos-zamora commented Jan 5, 2022

Uh oh!

ghost commented Jan 6, 2022

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

zadjii-msft Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

ghost commented Feb 3, 2022

Uh oh!

dark-swordsman commented Jul 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zadjii-msft commented Jul 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

zadjii-msft commented Oct 14, 2021 •

edited

Loading

DHowett commented Oct 14, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

dark-swordsman commented Jul 4, 2022 •

edited

Loading