Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@moko256
Copy link
Contributor

@moko256 moko256 commented Nov 18, 2021

This PR adds SystemSound.play support.

On win32, it calls MessageBeep.

On winuwp, it is not implemented because Windows App Cert Kit warns the usage of MessageBeep and winrt seems not to have its equivalent API.
I tried ElementSoundPlayer, but it seems to work on xbox and can be called only on xaml's thread.

This PR will fix flutter/flutter#62143.

No changes in flutter/tests.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@flutter-dashboard
Copy link

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you for your contribution!

LGTM stamp from a Japanese personal seal

@dkwingsmt
Copy link
Contributor

dkwingsmt commented Apr 7, 2022

The general code is fine. But I have questions about the design, because it is only able to play MB_OK, while Win32 supports 10 different sounds.

I would like to have a parameter of an enum to support all of them, but then comes the portability problem for other platforms.

Shall we name it "Win32Beep.play" for now, and support a parameter that is the literal value of the beep kind?

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

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

LGTM

@cbracken
Copy link
Member

cbracken commented Apr 7, 2022

@dkwingsmt I was wondering the same thing. We do already use the SystemSound.alert value on other platforms (e.g. Android). This PR is consistent with how we model channel values in other parts of the embedders, so I think that sort of refactor is probably beyond the scope of this change.

I do think it's worth some thought how we might do this -- especially as I just spent a couple days on testing for similar code with regards to semantics to resolve flutter/flutter#101217.

I think we can investigate that separately from this change though.

@cbracken cbracken added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 7, 2022
@dkwingsmt
Copy link
Contributor

Yeah I didn't realize it's implementing a feature that already exists in the Framework.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

affects: desktop cla: yes platform-windows waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Attempting to dismiss a non-dismissable modal dialog on desktop should play system alert sound

5 participants