-
Notifications
You must be signed in to change notification settings - Fork 117
Adding Vietnamese and Italian support to Localization Plugin #1098
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
Conversation
1ec5
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, I sort of jumped the gun on mapbox/mapbox-gl-native-ios#173: Mapbox Streets v8.3 hasn’t been released yet, even though the documentation has been public for a few weeks. I merged mapbox/mapbox-gl-native-ios#173 anyways, because the iOS/macOS map SDK already implements a fallback to the native name by coalescing name_* properties with name: mapbox/mapbox-gl-native#12387. When v8.3 comes out, the SDK will automatically show Vietnamese instead of falling back to the native name. Does this plugin have a similar fallback strategy?
plugin-localization/src/main/java/com/mapbox/mapboxsdk/plugins/localization/MapLocale.java
Outdated
Show resolved
Hide resolved
| */ | ||
| static final LatLngBounds VIETNAM_BBOX = new LatLngBounds.Builder() | ||
| .include(new LatLng(8.383333, 102.216667)) | ||
| .include(new LatLng(23.666667, 109.466667)).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only part of the bounding box for Vietnam in OpenStreetMap, which includes some disputed territories.
If I understand correctly, these bounding boxes are used to center the map over the country most closely associated with a particular language by default. Even though the documented use case for these bounding boxes would be quite benign, getCountryBounds() is a public method that an application could use in other ways. The precision of these bounding boxes could raise questions about the inconsistent handling of disputed territories, of which there are now at least two examples.
Going forward, it may be prudent to replace these bounding boxes with zoom levels and simple point features roughly centered over each country’s mainland, or at least to align them to the bounding boxes returned by the Mapbox Geocoding API or the geometries in Mapbox Boundaries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point @1ec5, but in my opinion, this shouldn't be a huge issue since we're providing rough bounding boxes, not precise polygons. Additionally, using a point and a zoom level is pixel density-dependent and wouldn't be easy to hardcode as a constant.
How about we add the "Approximate" keyword to the Javadoc to not set the wrong expectations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d agree except that the Vietnam bounding box is about half as wide as it would be if it included the disputed South China Sea Islands, while the China boundary extends well to the south to include them. The comments currently cite OpenStreetMap, but the bounding boxes don’t match OpenStreetMap either.
If there’s a technical constraint preventing us from using the same bounding boxes as the Mapbox Geocoding API or Mapbox Boundaries, then I think the comments should warn developers to convert the bounding boxes to points before use and shouldn’t mention OSM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback, you two. I'll touch up this pr once 8.3 is actually out.
3564230 to
dfea693
Compare
dfea693 to
4ade496
Compare
4ade496 to
b19b527
Compare
LukasPaczos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM code-wise
|
Yea, meant to give an update on this pr now that you're back from your recent PTO @LukasPaczos .
I did @1ec5's suggestions of removing all OSM mentions and changing the javadocs to say that the bounding box areas are merely approximate. Regarding @1ec5's suggestions about boundaries and geocoding:
|

Resolves #1097 by adding Vietnamese language support to the Localization Plugin.
cc @1ec5