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

remove buggy url encoding #830

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KnowYourLines
Copy link

Fixes #500

@Reda1337
Copy link

How much will it take to merge this fix? it worked for me locally but i would love if it's available in the package.

@RomualdPercereau
Copy link
Collaborator

Could you please give some example of context working after and before? Is it still working on with filename with spaces?

@RomualdPercereau
Copy link
Collaborator

This look like to be the opposite of this fix 3fe5480

@KnowYourLines @Reda1337

@KnowYourLines
Copy link
Author

KnowYourLines commented Dec 6, 2023

This look like to be the opposite of this fix 3fe5480

@KnowYourLines @Reda1337

Yes that fix causes this problem. #628 (comment) from that PR points this out.

Frankly, I don't believe URI encoding should be done by this library to begin with as there are very likely more effective libraries that accomplish URI encoding and would allow encoded URIs to be passed to this library.

It would make more sense for the users of this library to be responsible for passing an appropriate URI to this library rather than have default URI encoding in this library that enables some inputs to work but prevents others from working.

The current URI encoding in this library seems like a premature optimization since it arguably causes as many if not more more problems than it solves.

@RomualdPercereau

@OlivierCo
Copy link

Can we merge it?

@OlivierCo
Copy link

@RomualdPercereau I had to revert the change made in this 3fe5480. and now it works perfectly fine. So Could we merge please

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.

Some url sound not playing in IOS
4 participants