Skip to content

fix: explicitly convert iOS volume to double #46

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

Conversation

chriszs
Copy link
Contributor

@chriszs chriszs commented Jun 8, 2024

What does this Pull Request do?

Fixes a suspected type conversion issue which made setVolume fail in some cases by explicitly converting volume to double from NSNumber (the type it's annotated in the React Native module) in Swift.

Why is this Pull Request needed?

To prevent setVolume from failing.

Are there any points in the code the reviewer needs to double check?

Someone who's more experienced with RN native development should double check me on this approach.

Are there any Pull Requests open in other repos which need to be merged with this?

No

Addresses Issue(s):

GitHub Issue

Fixes a conversion issue which made setVolume fail in some cases
Closes jwplayer#43
@chriszs chriszs requested a review from a team as a code owner June 8, 2024 09:31
@Jmilham21
Copy link
Collaborator

Makes sense. I suppose we should also update setSpeed to avoid using Double as well.

@AmitaiB @david-almaguer please confirm but LGTM

Copy link
Contributor

@david-almaguer david-almaguer left a comment

Choose a reason for hiding this comment

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

I'm fine using NSNumber instead of Double since this also matches the following method signature: RCT_EXTERN_METHOD(setVolume: (nonnull NSNumber *)reactTag :(nonnull NSNumber *)volume)

@chriszs
Copy link
Contributor Author

chriszs commented Jun 24, 2024

@david-almaguer I suspect that signature may be why the type conversion issue happened in the first place, though I don't know the root cause for sure. double -> number is also supported by React Native.

Copy link
Contributor

@AmitaiB AmitaiB left a comment

Choose a reason for hiding this comment

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

LGTM

@AmitaiB AmitaiB linked an issue Jun 28, 2024 that may be closed by this pull request
@Jmilham21 Jmilham21 merged commit 27e5245 into jwplayer:master Jul 1, 2024
@chriszs chriszs deleted the fix/gh-43-properly-convert-ios-volume-to-double branch July 1, 2024 15:39
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.

[BUG] setVolume method does not work on iOS
4 participants