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

Add custom url for map component #3160

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

petersmythe
Copy link

@petersmythe petersmythe commented Apr 2, 2024

Dear MIT App Inventor community

Firstly, thanks for an awesome project. My kids love it!

I was checking out the Map component, and I see that it makes use of the osmdroid library which in turn connects to 1 of 3 tile sources: OpenStreetMap and United States Geological Survey. I observe the OpenStreetMap terms and conditions for using their tile servers:

https://operations.osmfoundation.org/policies/tiles/#:~:text=As%20a%20result%2C%20we%20require,free%20for%20everyone%20to%20use.%20Our%20tile%20servers%20are%20not.

As a result, we require that users of the tiles abide by this tile usage policy.

OpenStreetMap data is free for everyone to use. Our tile servers are not.

With this in mind, I propose a 4th MapType (in addition to Roads, Aerial and Terrain), being Custom, which allows a diligent/heavy/commercial user to set a custom URL using the standard TMS format e.g. https://tile.openstreetmap.org/{z}/{x}/{y}.png. The default map (Roads using OpenStreetMap) will still work for the majority of App Inventor users just exploring the functionality.

A bonus side effect of this fix is that users can now connect to any other online basemaps that they find interesting, and possibly the Map component will be further enhanced in the future to include overlay layers too.

I ask that you please review the changes that I have made, noting that I still need to write the accompanying documentation etc. All the ant tests pass. I do not have a local copy of the buildserver running.

One possible bug that I discovered is https://community.appinventor.mit.edu/t/literal-left-curly-bracket-cannot-exist-in-simpleproperty-description/112129 which states that a literal { cannot exist in the description of a property, e.g. https://tile.openstreetmap.org/{z}/{x}/{y}.png. I have had to include an ugly workaround with square brackets, but I would like to know if there is a better solution.
EDIT: resolved with this hint

Another problem that I am facing is in the Designer GUI (ODE), under Appearance > MapType, the dropdown box still only shows the original 3 types (Roads, Aerial, Terrain) even though the code works perfectly and the Blocks GUI shows all 4 types correctly. I am sure an experienced developer will be able to point out the solution to this novice very easily.
EDIT: resolved with this hint

image
image

Kindly guide me through the remainder of your contribution process, please. I understand that there is a new release coming up in a week's time - it is possible to incorporate it in there?

@AppInventorWorkerBee
Copy link
Collaborator

Can one of the admins verify this patch?

@petersmythe
Copy link
Author

@ewpatton, could you please assign for review?

@petersmythe
Copy link
Author

petersmythe commented Apr 2, 2024

Also TODO:

  • validate the CustomURL input
  • update documentation with decent examples
  • what about iOS equivalent code?

Copy link
Member

@ewpatton ewpatton left a comment

Choose a reason for hiding this comment

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

This is a pretty good first attempt. I've left a few comments. We will also need to add project upgrade paths in YoungAndroidFormUpgrader.java and versioning.js to handle the bump in the Map component's version number. Both of these are no-ops since you're just adding a property.

@petersmythe
Copy link
Author

This is a pretty good first attempt. I've left a few comments. We will also need to add project upgrade paths in YoungAndroidFormUpgrader.java and versioning.js to handle the bump in the Map component's version number. Both of these are no-ops since you're just adding a property.

Done

@petersmythe
Copy link
Author

I read somewhere in the documentation about removing the label pull request: needs reply to review and adding pull request: needs review again, but as a new contributor, I do not have the rights to do so.

@petersmythe
Copy link
Author

Ready for review @ewpatton

@petersmythe
Copy link
Author

I am not going to be able to add the equivalent code for iOS - will that be OK, @ewpatton ?

Copy link
Author

@petersmythe petersmythe left a comment

Choose a reason for hiding this comment

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

Ready

@petersmythe
Copy link
Author

Is there anything more that I can do for you to accept this PR @ewpatton ?

@ewpatton
Copy link
Member

ewpatton commented May 9, 2024

I am not going to be able to add the equivalent code for iOS - will that be OK, @ewpatton ?

That's fine. I will work on implementing the iOS version.

Copy link
Member

@ewpatton ewpatton left a comment

Choose a reason for hiding this comment

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

A few more minor things otherwise LGTM.

@petersmythe
Copy link
Author

Ready for review again.

@petersmythe
Copy link
Author

petersmythe commented May 26, 2024

I am not going to be able to add the equivalent code for iOS - will that be OK, @ewpatton ?

That's fine. I will work on implementing the iOS version.

I am trying to set up a macOS development environment myself, but running into this problem: https://community.appinventor.mit.edu/t/help-required-building-ios-on-macos-vm/118307 @ewpatton

EDIT: RESOLVED

Copy link
Author

@petersmythe petersmythe Jun 8, 2024

Choose a reason for hiding this comment

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

  • TODO: Needs iOS port

Copy link
Author

Choose a reason for hiding this comment

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

@petersmythe
Copy link
Author

petersmythe commented Jun 9, 2024

  • TODO: I recently discovered that MockMap also needs to be worked on.

@petersmythe
Copy link
Author

Question for @ewpatton before I commit the iOS port, I would like to test it, is that possible?

https://community.appinventor.mit.edu/t/build-ios-ad-hoc-ipa/119540

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

Successfully merging this pull request may close these issues.

3 participants