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

Encode the style URL in iOS #2965

Merged
merged 2 commits into from
Oct 30, 2024
Merged

Encode the style URL in iOS #2965

merged 2 commits into from
Oct 30, 2024

Conversation

qqz003
Copy link
Contributor

@qqz003 qqz003 commented Oct 25, 2024

For iOS versions under 17.0, [NSURL URLWithString:] cannot create an NSURL object with the url string that containing any characters that need to be encoded. Consequently, it cannot load the custom style correctly if the style file contains such a URL.

@louwers
Copy link
Collaborator

louwers commented Oct 25, 2024

Please add a unit test to http_file_source.test.cpp.

Copy link

github-actions bot commented Oct 25, 2024

Bloaty Results (iOS) 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.0%    +392  [ = ]       0    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-2965-compared-to-main.txt

@qqz003
Copy link
Contributor Author

qqz003 commented Oct 28, 2024

Please add a unit test to http_file_source.test.cpp.

@louwers
The method resourceURL is not defined in http_file_source.hpp; it's only used for Apple platforms. So, maybe it's better to add the test to the class MLNResourceTests? However, I didn't find how to run the Darwin tests. They don't appear in the test navigator of Xcode, and I didn't find the related workflow (maybe included in some other workflow?).

@louwers louwers added the iOS label Oct 28, 2024
@louwers
Copy link
Collaborator

louwers commented Oct 28, 2024

@qqz003 Yes that sounds good.

The ios_test test target should include this test.

image

@qqz003
Copy link
Contributor Author

qqz003 commented Oct 30, 2024

@louwers Looks that I don't have the write access to merge the PR, could you please help to merge it? Thank you!

@louwers louwers merged commit d140d5e into maplibre:main Oct 30, 2024
28 checks passed
@louwers
Copy link
Collaborator

louwers commented Oct 30, 2024

@qqz003 Thanks for the reminder!

louwers added a commit to louwers/maplibre-native that referenced this pull request Nov 16, 2024
@louwers
Copy link
Collaborator

louwers commented Nov 17, 2024

@qqz003 @random3940 I had to revert this change due to regressions.

Happy to work with you to address this in another way...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants