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 geocoding API for setting multiple languages #446

Open
cammace opened this issue Apr 17, 2017 · 9 comments · Fixed by #512
Open

Add geocoding API for setting multiple languages #446

cammace opened this issue Apr 17, 2017 · 9 comments · Fixed by #512
Assignees
Labels
Milestone

Comments

@cammace
Copy link

cammace commented Apr 17, 2017

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

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

cc: @zugaldia @1ec5

@cbadhrinath
Copy link

@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
Copy link
Member

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
Copy link
Contributor

osana commented Nov 13, 2018

@zugaldia I will check this out

@osana
Copy link
Contributor

osana commented Nov 20, 2018

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
Copy link

cbadhrinath commented Nov 20, 2018

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

@zugaldia zugaldia reopened this Nov 20, 2018
@tobrun tobrun assigned osana and unassigned cammace Nov 26, 2018
@osana osana removed this from the v2.2.0 milestone Apr 13, 2019
@osana
Copy link
Contributor

osana commented Apr 14, 2019

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)

  • language is not specified
    https://api.mapbox.com/geocoding/v5/mapbox.places/-77.0366,38.8971.json?access_token=pk...
{
"id": "place.7673410831246050",
"wikidata": "Q61",
"text": "Washington"
}
  • one language is specified (for example: language=ru)
    https://api.mapbox.com/geocoding/v5/mapbox.places/-77.0366,38.8971.json?access_token=pk... language=ru
        {
          "id": "place.7673410831246050",
          "wikidata": "Q61",
          "text_ru": "Вашингтон",
          "language_ru": "ru",
          "text": "Вашингтон",
          "language": "ru"
        }

  • mulitlanguage request (for example: language=en_us,ru,es)
    https://api.mapbox.com/geocoding/v5/mapbox.places/-77.0366,38.8971.json?access_token=pk...&language=en_us,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:

  • remove AutoValue dependency for this module (done as one PR)
  • add special parsing for language dependent fields which should not be too hard once we have our own GsonTypeAdapters for both CarmenFeature and CarmenContext. (another PR)

cc @tobrun @zugaldia

@zugaldia
Copy link
Member

@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
Copy link
Contributor

osana commented Apr 15, 2019

@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.

@langsmith langsmith modified the milestones: v4.9.0, 4.10.0 Sep 25, 2019
@stale stale bot added the Archived Issue archived due to lack of activity label Mar 23, 2020
@stale
Copy link

stale bot commented Mar 23, 2020

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

@stale stale bot closed this as completed Mar 23, 2020
@langsmith langsmith reopened this Mar 23, 2020
@stale stale bot removed the Archived Issue archived due to lack of activity label Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants