mapbox / mapbox-java

The Mapbox Java SDK – Java wrappers around Mapbox APIs and other location data
https://docs.mapbox.com/android/java/overview/
MIT License
417 stars 121 forks source link

Add geocoding API for setting multiple languages #446

Open cammace opened 7 years ago

cammace commented 7 years ago

Currently we only allow the setting of one Language for the geocoder. The Geocoding API will allow multiple Locales to be passed in. https://github.com/mapbox/MapboxGeocoder.swift/issues/104

Note this needs to be exposed in both MapboxGeocoder and Geocoding widget.

cc: @zugaldia @1ec5

cbadhrinath commented 5 years ago

@cammace Is this actually fixed now. I'm using the mapbox-sdk-services:4.1.0-SNAPSHOT for my Android project.

MapboxGeocoding.builder() .accessToken("") .query(Point.fromLngLat(-123.1207, 49.2827)) .languages(new Locale("ru"), new Locale("zh-CN")) .geocodingTypes(GeocodingCriteria.TYPE_PLACE) .mode(GeocodingCriteria.MODE_PLACES) .build(); Still returning only one language. Not returing all requested language. Could you please confirm this.

zugaldia commented 5 years ago

Thank you @cbadhrinath.

@osana could you look into this? I don't think we have a test covering this usecase (multiple languages in the request and multiple languages in the response).

osana commented 5 years ago

@zugaldia I will check this out

osana commented 5 years ago

Thank you for reporting this. I confirm that there is a problem in supporting https://api.mapbox.com/geocoding/v5/mapbox.places/-77.0366,38.8971.json?access_token=pk...&language=en_us,ru,es

We correctly do the request for multiple languages but the response object is not parsed correctly.

Here is one of the CarmenFeatures returned from the above request

{
"id": "region.3403",
"short_code": "US-DC",
"wikidata": "Q61",
"text_en_us": "District of Columbia",
"language_en_us": "en",
"text": "District of Columbia",
"language": "en",
"text_ru": "Вашингтон",
"language_ru": "ru",
"text_es": "Distrito de Columbia",
"language_es": "es"
},
cbadhrinath commented 5 years ago

@osana Thanks for confirming the issue. I guess this needs to be re-opened. I'm using the WebAPI for now.

osana commented 5 years ago

Here are a couple of examples of possible responses based on the language requests. It is easier to show the difference in case of CarmenContext (text). The same applies to CarmenFeature (text, placeNames)

        {
          "id": "place.7673410831246050",
          "wikidata": "Q61",
          "text_ru": "Вашингтон",
          "language_ru": "ru",
          "text": "Вашингтон",
          "language": "ru"
        }
{ "id": "place.7673410831246050",
"wikidata": "Q61",
"text_en_us": "Washington",
"language_en_us": "en",
"text": "Washington",
"language": "en",
"text_ru": "Вашингтон",
"language_ru": "ru",
"text_es": "Washington",
"language_es": "es"
} 

Geocoding service relies on AutoValue library to generate GsonTypeAdapters for its models. In the example above, fields that start with text are not known ahead of time and have to be treated specially. That means that CarmenContext needs to have a custom GsonTypeAdapter. The same applies to CarmenFeature as it contains text_* fields as well. In other words, 2 out of 3 geocoding model classes need a custom parser and cannot use the one generated by AutoValue.

I think we should:

cc @tobrun @zugaldia

zugaldia commented 5 years ago

@osana thank you for looking into this, next steps make sense to me with the exception that if we're gonna remove AutoValue as a dependency, shouldn't we remove it for all other services too for consistency?

/cc: @karenell the current API response is hard to parse on mobile due to its current structure. For future API versions, if possible, we'd love to be involved to work together in an approach that fits mobile better.

osana commented 5 years ago

@zugaldia At the moment geocoding service is part of services library. geocoding service is not released by itself at the moment. There will be very little size saving when AutoValue is removed from geocoding because as you pointed out the library will still stay in the jar (as it will be used by other modules). Still, there will be fewer classes as we will not need to generate wrappers for the model classes.

I think the proposed first step is just a way to break the task in two more manageable tasks. It can be done on the same PR as well.

It would be great to know the sense of urgency for this. Search is important.

stale[bot] commented 4 years ago

This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.