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 recommended OSM tile URL #5815

Merged
merged 1 commit into from
Nov 17, 2023
Merged

Use recommended OSM tile URL #5815

merged 1 commit into from
Nov 17, 2023

Conversation

lognaturel
Copy link
Member

@lognaturel lognaturel commented Oct 31, 2023

Supports HTTP/2 and HTTP/3 so aliases are no longer required to support connection concurrency

Why is this the best possible solution? Were any other approaches considered?

This is what openstreetmap/operations#737 recommends. I verified I could launch a map with OSM tiles.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Worst case is that OSM tiles stop working.

Do we need any specific form for testing your changes? If so, please attach one.

No. Can use the filled form map.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

  • added or modified tests for any new or changed behavior
  • run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
  • added a comment above any new strings describing it for translators
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

Supports HTTP/2 and HTTP/3 so aliases are no longer required to support connection concurrency
@lognaturel
Copy link
Member Author

lognaturel commented Oct 31, 2023

🤔 But I imagine osmdroid doesn't take advantage of HTTP/2 features when downloading tiles? So maybe this will slow down tile loading? Even if that is the case, I don't think it would be significant.

@seadowg seadowg marked this pull request as draft November 6, 2023 09:53
@seadowg
Copy link
Member

seadowg commented Nov 6, 2023

Marking this as a draft as some of the checklist isn't done.

@lognaturel
Copy link
Member Author

lognaturel commented Nov 16, 2023

I had to think about implications a little more. I do think it's possible this will make downloads a bit slower but I doubt it would be meaningful if that's the case. osmdroid's examples have been updated similarly so I think it's the right move: https://github.com/osmdroid/osmdroid/blob/master/osmdroid-android/src/main/java/org/osmdroid/tileprovider/tilesource/TileSourceFactory.java#L100

I don't believe there are any meaningful tests to write for this.

@lognaturel lognaturel marked this pull request as ready for review November 16, 2023 00:33
@seadowg seadowg merged commit 21e150b into getodk:master Nov 17, 2023
6 checks passed
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.

None yet

2 participants