Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[android] Add a way to use a custom location source #8710

Merged
merged 2 commits into from
Apr 20, 2017

Conversation

Guardiola31337
Copy link
Contributor

Fixes #8668

👀 @tobrun

@Guardiola31337 Guardiola31337 added Android Mapbox Maps SDK for Android feature Google Maps parity For feature parity with the Google Maps SDK for Android or iOS localization Human language support and internationalization labels Apr 11, 2017
Copy link
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

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

Looking great, could we add a test around this? maybe adding a mock example in the test app (could be integrated in an existing activity)? Changelog entry would be appreciated

* location source.
*/
@UiThread
public void setLocationSource(@Nullable LocationEngine locationSource) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's call this setLocationEngine.

@@ -362,6 +365,11 @@ void setMyLocationEnabled(boolean locationEnabled) {
myLocationView.setEnabled(locationEnabled);
}

void setLocationSource(LocationEngine locationSource) {
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, let's call it setLocationEngine().

@@ -58,6 +58,7 @@

private LatLng latLng;
private Location location;
private LocationEngine locationSource;
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather call it locationEngine even if LocationSource is our default implementation.

@@ -561,22 +563,29 @@ public void setContentPadding(int[] padding) {
contentPaddingY = (padding[1] - padding[3]) / 2;
}

public void setLocationSource(LocationEngine locationSource) {
Copy link
Member

Choose a reason for hiding this comment

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

Same: setLocationEngine().

private static class GpsLocationListener implements LocationEngineListener {

private WeakReference<MyLocationView> userLocationView;
private WeakReference<LocationEngine> locationSource;
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, let's call the variable locationEngine.

@Override
public void onClick(View view) {
if (mapboxMap != null) {
mapboxMap.setLocationSource(locationServices);
Copy link
Member

@zugaldia zugaldia Apr 18, 2017

Choose a reason for hiding this comment

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

@Guardiola31337 Do we need to set a location source every time the FAB is clicked? Wouldn't we need to set it just once if it hasn't been set before? Also, I don't see a getLocationSource() method to check that, or alternatively a isCustomLocationSourceSet().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zugaldia you're right, setting it once would be enough.
I don't see the necessity of having getLocationSource or isCustomLocationSourceSet methods, because if it isn't set we use LocationSource (default). What would be the use case?

Copy link
Member

Choose a reason for hiding this comment

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

What would be the use case?

@Guardiola31337 Check if it's been set or not. For example, this activity could've been written like:

if (mapboxMap.getLocationSource() == null) {
  mapboxMap.setLocationSource(locationServices);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zugaldia there is always a location source. It can't be null. So it doesn't make sense.

Copy link
Member

Choose a reason for hiding this comment

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

@Guardiola31337 Alternatively:

if (mapboxMap.getLocationSource() instanceof MyCustomLocationSource) {
  // Safe cast
}

@Guardiola31337 Guardiola31337 merged commit c6be40b into master Apr 20, 2017
@Guardiola31337 Guardiola31337 deleted the pg-8668-location-source branch April 20, 2017 18:10
@tobrun tobrun mentioned this pull request May 2, 2017
12 tasks
@tobrun tobrun added this to the android-v5.1.0 milestone May 4, 2017
@Ehekatl
Copy link

Ehekatl commented May 15, 2017

I manage to use this to fix issue #8639 #8848
However after I change location source to my own location engine in background service, the map view lost the ability to update user location gradually and start jumping to the new position immediately on location change.
Is there any way to solve it?

@zugaldia
Copy link
Member

@Ehekatl thanks for the report. I'd like to explore the root cause for this misbehavior. Could you open a new ticket and, if possible, share the code for your custom location engine? For reference, we provide a sample implementation of a custom location engine on CustomLocationEngineActivity

@tobrun tobrun mentioned this pull request Jun 9, 2017
12 tasks
@tobrun tobrun mentioned this pull request Jun 21, 2017
11 tasks
@tobrun tobrun mentioned this pull request Jun 30, 2017
16 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android feature Google Maps parity For feature parity with the Google Maps SDK for Android or iOS localization Human language support and internationalization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants