-
Notifications
You must be signed in to change notification settings - Fork 120
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
Several geocoding fixes and additions #512
Conversation
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.
A few questions, otherwise we're very close.
@@ -43,6 +45,7 @@ protected void onCreate(Bundle savedInstanceState) { | |||
autocomplete.setAccessToken(Utils.getMapboxAccessToken(this)); | |||
autocomplete.setType(GeocodingCriteria.TYPE_POI); | |||
autocomplete.setLimit(10); | |||
autocomplete.setLanguages(Locale.JAPAN.toString()); |
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.
@cammace did you intend to leave Japan's locale here?
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.
removed.
super(); | ||
builder = new MapboxGeocoding.Builder(); | ||
builder = new MapboxGeocoding.Builder<>(); |
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.
Why was this change necessary? It looks like a change on our standard builder approach.
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.
Was receiving a lint warning unchecked assignment
so I added the <>
which seemed to make it happy. Can switch back if there's an issue?
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.
Oh that's alright, I just wanted to make sure new MapboxGeocoding.Builder();
still was an option.
private String matchingText; | ||
@SerializedName("matching_place_name") | ||
private String matchingPlaceName; | ||
private String language; |
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.
Maybe I missed them but I can't seem to find tests for these new fields.
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.
👍 The methods are now being tested.
@@ -25,7 +24,7 @@ | |||
* @see <a href="https://www.mapbox.com/android-sdk/examples/geocoding/">Mapbox example</a> | |||
* @since 1.0.0 | |||
*/ | |||
public class GeocoderAutoCompleteView extends AutoCompleteTextView { | |||
public class GeocoderAutoCompleteView extends android.support.v7.widget.AppCompatAutoCompleteTextView { |
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.
Huh, good find.
Ready for another review @zugaldia |
This PR:
GeocoderAdapter.java
to non-public since user has no reason for usingTYPE_DISTRICT
andTYPE_LOCALITY
result filters addedplace_type
support in Carman Featurematching_text
support in Carman Featurematching_place_name
support in Carman Featurelanguage
support in Carman FeatureI haven't added support for
text_{language}
,place_name_{language}
and language_{language} since the field name depends on different languages. These are rarely used in the geocoder however so it might be better just to ticket out if it is too much work implementing before 2.2.0 release.