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

Use http datasource for local urls #101

Conversation

janwiebe-jump
Copy link
Contributor

@janwiebe-jump janwiebe-jump commented Nov 16, 2023

Currently all local urls are retrieved by the DefaultDataSourceFactory. However, in my case I want to use localhost urls that return a redirect (302) to the actual mp3.
Therefore, since they are http/https urls, they should be handled by DefaultHttpDataSource so the redirect is followed.
This PR fixes the check isLocalUri to match local file uris only.

I have tested this with React Native Track Player.

@janwiebe-jump
Copy link
Contributor Author

@dcvz Would it be possible to take a look at this PR? Thanks a lot!

Copy link
Contributor

@dcvz dcvz left a comment

Choose a reason for hiding this comment

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

Looks good! Left one small comment -- overall I think there should be no big impact in using DefaultHttpDataSource for these, so let's go ahead

@dcvz
Copy link
Contributor

dcvz commented Mar 5, 2024

@janwiebe-jump I'm ready to merge this but the sample is failing to build. Looks like there's a syntax error in Utils.kt:

e: /Users/admin/actions-runner/_work/KotlinAudio/KotlinAudio/kotlin-audio/src/main/java/com/doublesymmetry/kotlinaudio/utils/Utils.kt: (15, 254): Unexpected tokens (use ';' to separate expressions on the same line)

@janwiebe-jump
Copy link
Contributor Author

@dcvz Sorry for that typo. Pushed the fix.

@dcvz dcvz merged commit b09b5c0 into doublesymmetry:main Mar 5, 2024
1 of 3 checks passed
@dcvz
Copy link
Contributor

dcvz commented Mar 5, 2024

Merged! Thanks for the contribution @janwiebe-jump

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