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

Added the ability to configure "SMTC integration" for Windows apps. #183

Merged
merged 1 commit into from
Jun 9, 2017

Conversation

ndbroadbent
Copy link
Contributor

@ndbroadbent ndbroadbent commented May 16, 2017

This change solves the following problem:

I was testing my game on Windows, and I pressed the "play" key on my keyboard, because I wanted to unpause a song on Spotify. Instead of unpausing Spotify, my game took over the controls and one of the sound effects randomly started playing.

This change allows me to disable the "SMTC integration" on Windows for all of my sound effects, so that my game stops pretending to be a music player.

More information at: https://docs.microsoft.com/en-us/uwp/api/Windows.Media.SystemMediaTransportControls

The way I've written this is not very pretty, but it's backwards compatible. But I can imagine a nicer way to deal with platform-specific options like this, so that it's easier to add more in the future. The JS constructor could also use some work - I find the arguments very confusing.

Also note that this can't be a global setting. It needs to be configurable for each individual sound. For example, you might have some sound effects that you don't want the user to control, but you also play some songs that they should be able to control.

sound.js Outdated
@@ -31,7 +39,7 @@ function Sound(filename, basePath, onError) {
this._pan = 0;
this._numberOfLoops = 0;
this._speed = 1;
RNSound.prepare(this._filename, this._key, (error, props) => {
RNSound.prepare(this._filename, this._key, enableSMTCIntegration, (error, props) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens on iOS and Android here? Will the RNSound.prepare method receive the callback as argument 4 rather than 3 as expected? I haven't tried it but this may cause problems - I suspect the signature of the iOS and Android prepare methods would need adjustment.

It may be better to pass options as a JS object here directly so we can add other options later in native code without needing to rework this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, can't believe I forgot to update iOS and Android!

But yes that's a great idea, I will replace this an options object so that it can be extended in the future. And will also update iOS and Android!

@ndbroadbent
Copy link
Contributor Author

@benvium I've updated the PR to pass the options object directly to each native module, and have fixed iOS and Android.

@benvium
Copy link
Collaborator

benvium commented Jun 9, 2017

Great - thanks.

@benvium benvium merged commit 03ef91c into zmxv:master Jun 9, 2017
aidan-doherty pushed a commit to aidan-doherty/react-native-sound that referenced this pull request Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants