Include Profile.BellSound as a media resource #19289

DHowett · 2025-08-27T15:26:47Z

I legitimately cannot figure out how I forgot this. Bell should support all the same validation as other media resources! Technically this means you can set bellSound to desktopWallpaper, but... we'll pretend that makes sense.

I reworked the viewmodel to be a little more sensible. It no longer requires somebody else to check that its files exist. The settings UI now also displays File not found in the preview for the bell if it is a single file which failed validation!

I legitimately cannot figure out how I forgot this. Bell should support all the same validation as other media resources! Technically this means you can set `bellSound` to `desktopWallpaper`, but... we'll pretend that makes sense.

DHowett · 2025-08-27T15:27:27Z

src/cascadia/TerminalApp/TerminalPaneContent.cpp

                    if (sounds && sounds.Size() > 0)
                    {
-                        winrt::hstring soundPath{ wil::ExpandEnvironmentStringsW<std::wstring>(sounds.GetAt(rand() % sounds.Size()).c_str()) };
+                        winrt::hstring soundPath{ sounds.GetAt(rand() % sounds.Size()).Resolved() };


If any resource failed validation, we'll just call playBellSound(L""). This fails like any other invalid file failed in the past.

My original plan was to have the resolver remove entries that were illegal. however, that would also remove them from the user settings file. Instead, I fell back to the existing behavior.

DHowett · 2025-08-27T15:27:49Z

src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp

-                    newSounds.push_back(sound);
-                }
+                newSounds.resize(inheritedSounds.Size()); // fill with null
+                inheritedSounds.GetMany(0, newSounds);


drive by optimization

DHowett · 2025-08-27T15:28:29Z

src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp

        {
-            get_self<BellSoundViewModel>(sound)->FileExists(false);
+            // If the resource was resolved to something other than its path, show the path!
+            _ShowDirectory = true;


@carlos-zamora since you wrote bell sounds, you might care about this. it means that if you use a relative path to your bell sound file, the settings UI will always display the folder it resolved to

Nice! I'm totally ok with that. Means that we're being very clear about what's being loaded.

DHowett · 2025-08-27T15:28:58Z

src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp

-            }
-        }
-
-        co_await winrt::resume_foreground(_dispatcher);


we don't need this any more because the settings model did it and populated them all with .Ok()

DHowett · 2025-08-27T15:29:24Z

src/cascadia/TerminalSettingsEditor/ProfileViewModel.idl

    runtimeclass BellSoundViewModel : Windows.UI.Xaml.Data.INotifyPropertyChanged
    {
-        String Path;
+        String Path { get; };


never written; removed setter

DHowett · 2025-08-27T15:29:32Z

src/cascadia/TerminalSettingsModel/Profile.cpp

-        }
+        std::vector<IMediaResource> sounds;
+        sounds.resize(_BellSound->Size()); // fill with null
+        _BellSound->GetMany(0, sounds);


another drive by optimization

src/cascadia/TerminalSettingsModel/Profile.cpp

carlos-zamora

Mainly blocking over having the preview display the full path vs the file name.

Re:desktopWallpaper is an accepted value
Tested it locally;. User can't set it via the SUI since they have to use a filepicker so that's good. User can go into their settings.json and set desktopWallpaper there. We don't get a warning, whereas if they put foo they would.
It's not ideal, but at the same time, if the user is setting bellSound to desktopWallpaper, they kinda did it to themselves.
If anything, I guess this is like a P3 backlogged bug imo.

carlos-zamora · 2025-08-27T17:48:55Z

src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp

-            std::filesystem::path filePath{ std::wstring_view{ currentSound.GetAt(0) } };
-            return hstring{ filePath.filename().wstring() };
+            return currentSound.GetAt(0).Resolved();


Used to just show the filename. Now it shows the full path. Can we get this to just show the filename again? The full path is a lot of extra text that isn't really meaningful to the user here.

Something like this from DisplayPath() above

auto resolvedPath{ currentSound.GetAt(0).Resolved() }; const std::filesystem::path filePath{ std::wstring_view{ resolvedPath } }; return hstring{ filePath.filename().wstring() };

Good catch, my miss. Thanks.

carlos-zamora · 2025-08-27T17:51:40Z

src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp

        {
-            get_self<BellSoundViewModel>(sound)->FileExists(false);
+            // If the resource was resolved to something other than its path, show the path!
+            _ShowDirectory = true;


Nice! I'm totally ok with that. Means that we're being very clear about what's being loaded.

…if not found!

lhecker · 2025-08-27T23:17:06Z

src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp

+                newSounds.resize(inheritedSounds.Size()); // fill with null
+                inheritedSounds.GetMany(0, newSounds);


This could use wil::to_vector instead. We use it in 3 places already (and hopefully more soon).

lhecker · 2025-08-27T23:17:58Z

src/cascadia/TerminalSettingsModel/Profile.cpp

+        std::vector<IMediaResource> sounds;
+        sounds.resize(_BellSound->Size()); // fill with null
+        _BellSound->GetMany(0, sounds);


I legitimately cannot figure out how I forgot this. Bell should support all the same validation as other media resources! Technically this means you can set `bellSound` to `desktopWallpaper`, but... we'll pretend that makes sense. I reworked the viewmodel to be a little more sensible. It no longer requires somebody else to check that its files exist. The settings UI now also displays `File not found` in the _preview_ for the bell if it is a single file which failed validation! (cherry picked from commit 4272151) Service-Card-Id: PVTI_lADOAF3p4s4BBcTlzgeF9qU Service-Version: 1.24

Include Profile.BellSound as a media resource

2635a8b

I legitimately cannot figure out how I forgot this. Bell should support all the same validation as other media resources! Technically this means you can set `bellSound` to `desktopWallpaper`, but... we'll pretend that makes sense.

DHowett commented Aug 27, 2025

View reviewed changes

src/cascadia/TerminalSettingsModel/Profile.cpp Show resolved Hide resolved

DHowett added this to 1.24 Servicing Pipeline Aug 27, 2025

github-project-automation bot moved this to To Cherry Pick in 1.24 Servicing Pipeline Aug 27, 2025

DHowett requested review from carlos-zamora and lhecker August 27, 2025 16:22

carlos-zamora requested changes Aug 27, 2025

View reviewed changes

microsoft-github-policy-service bot added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Aug 27, 2025

DHowett added 2 commits August 27, 2025 13:26

Bell preview: use the filename, but also display a not found warning …

1d87487

…if not found!

Use the viewmodel we so carefully prepared...

aaece11

carlos-zamora approved these changes Aug 27, 2025

View reviewed changes

lhecker approved these changes Aug 27, 2025

View reviewed changes

to_vector

e379259

DHowett enabled auto-merge (squash) August 27, 2025 23:56

carlos-zamora approved these changes Aug 27, 2025

View reviewed changes

DHowett merged commit 4272151 into main Aug 28, 2025
17 of 19 checks passed

DHowett deleted the dev/duhowett/bell-is-media-too-dummy branch August 28, 2025 00:05

DHowett moved this from To Cherry Pick to Cherry Picked in 1.24 Servicing Pipeline Sep 5, 2025

		newSounds.resize(inheritedSounds.Size()); // fill with null
		inheritedSounds.GetMany(0, newSounds);

Include Profile.BellSound as a media resource #19289

Include Profile.BellSound as a media resource #19289

Uh oh!

Conversation

DHowett commented Aug 27, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carlos-zamora left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DHowett commented Aug 27, 2025 •

edited

Loading