Skip to content

Fix ToastContentBuilder audio ms-winsoundevent and ms-appx #3755

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

Merged
3 commits merged into from
Feb 16, 2021

Conversation

andrewleader
Copy link
Contributor

Fixes #3753

Add support for using ms-windsoundevent and ms-appx in the toast content audio builder (previously we were incorrectly throwing an exception when devs provided those). Also added unit tests to ensure this works correctly and ensure we correctly throw on https audio urls and other non-supported ones.

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

Throws when you try to use ms-winsoundevent: or ms-appx:, both of which are supported by Windows. Today it only supports absolute file paths (C:\audio.mp3), which are only supported by desktop apps.

What is the new behavior?

Doesn't throw for those.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

@ghost
Copy link

ghost commented Feb 12, 2021

Thanks andrewleader for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost added the bug 🐛 An unexpected issue that highlights incorrect behavior label Feb 12, 2021
Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Code looks good.

@EmilJunker did you want to try out this new PR package to see if it resolves your issue when it's done building? You can find instructions in our Wiki on using PR packages.

@ghost
Copy link

ghost commented Feb 16, 2021

Hello @michael-hawker!

Because this pull request has the auto merge :zap: 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.

@Kyaa-dost Kyaa-dost added this to the 7.0 milestone Feb 16, 2021
@Kyaa-dost
Copy link
Contributor

@EmilJunker Feel free to try it out as mentioned above. Building fine and running smoothly for me 🚀

@EmilJunker
Copy link

I just tested it on my code, it works now as it should 👍

@ghost ghost merged commit 4e54ec5 into master Feb 16, 2021
@ghost ghost deleted the aleader/toast-audio-fix branch February 16, 2021 21:34
@michael-hawker
Copy link
Member

🦙❤ Thanks @EmilJunker for trying this out and helping us validate the PR! 🎉🎉🎉

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merge ⚡ bug 🐛 An unexpected issue that highlights incorrect behavior notifications 🔔
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The ToastContentBuilder is unable to set the toast notification sound to a ms-winsoundevent sound
4 participants