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

Decouple MapView from LOST #2954

Closed
ljbade opened this issue Nov 6, 2015 · 10 comments
Closed

Decouple MapView from LOST #2954

ljbade opened this issue Nov 6, 2015 · 10 comments
Assignees
Labels
Android Mapbox Maps SDK for Android

Comments

@ljbade
Copy link
Contributor

ljbade commented Nov 6, 2015

Currently MapView takes care of setting up LOST to retrieve location updates and handles things such as pausing updates when the app gets backgrounded.

Google Maps takes a different approach where you implement a LocationSource interface to feed in location data to the my location layer. They provide an implementation for Google Plays navigation library, but you could just as easily plug in LOST.

I could not see a benefit to this approach before. In fact even Google Maps used to take care of location data for you, but they have deprecated that API.

The major reason I can see for needing to separate this is the Android M location permissions model. See #2100. Basically only the app has control and insight into whether it has been granted the needed permissions, or if they have been revoked while the app was in the background.

We blindly assume you can resume GPS after return from background which is fine for pre-Android M. We also save the GPS on/off state in MapView's onSaveInstanceState which results in a crash if the user revokes a previously given permission.

Instead the app needs to be in charge of saving GPS on/off state and pausing/resuming the location provider. To do this will require us to add a LocationSource interface.

/cc @bleege @tobrun @zugaldia

@ljbade ljbade added the Android Mapbox Maps SDK for Android label Nov 6, 2015
@ljbade ljbade added this to the android-v2.3.0 milestone Nov 6, 2015
@tobrun
Copy link
Member

tobrun commented Nov 6, 2015

where you implement a LocationSource interface to feed in location data to the my location layer.

This is great, even if this is not the main gist behind this issue, this is still a nice configuration to have.

@bleege bleege mentioned this issue Nov 12, 2015
3 tasks
@bleege bleege self-assigned this Nov 12, 2015
@bleege
Copy link
Contributor

bleege commented Nov 12, 2015

Instead the app needs to be in charge of saving GPS on/off state and pausing/resuming the location provider.

Agreed. This is also going to be needed for implementing Telemetry. While the "app" is going to be the ultimate arbiter of enabled / disabled I'm thinking it's going to be likely that we'll need to check status in the MapView's onPause() and onResume() lifecycle methods.

@ljbade
Copy link
Contributor Author

ljbade commented Nov 12, 2015

@bleege, yeah I think we can check for permission but I think it would be over stepping our role to "ask" for permission. So perhaps we check for permission in onResume, if we don't have it we log an error to logcat and turn of isMyLocationEnabled.

We should mention this in the JavaDocs for setMyLocationEnabled

@ljbade
Copy link
Contributor Author

ljbade commented Nov 12, 2015

Will help apps where the developer does not realise the implication of revoked permissions while an app is running. So we prevent a crash at least.

@tobrun
Copy link
Member

tobrun commented Nov 13, 2015

@bleege
Google has a how-to video on handling permissions when working with Google Play Services here

@tobrun
Copy link
Member

tobrun commented Dec 4, 2015

@bleege

One thing I have discussed with @ljbade was to decouple Location updates from the MapView by introducing a LocationService. The end user is responsible for adding the service to the AndroidManifest (as he is now already responsible for adding relevant locational permissions).

All the components (MapView, Metrics, etc.) who want to receive location updates can bind to this service. Also end users could separately from MapView register on location updates.

LMK what you think on this subject.

@bleege bleege modified the milestones: android-v2.3.0, android-v2.4.0 Dec 4, 2015
bleege added a commit that referenced this issue Dec 7, 2015
@bleege
Copy link
Contributor

bleege commented Dec 8, 2015

The end user is responsible for adding the service to the AndroidManifest (as he is now already responsible for adding relevant locational permissions).

The difference is that the permissions are Android wide and not specific to a 3rd Party library like Mapbox in this case. I'm not totally against this, but I'd prefer a route that didn't require developers to have to add configuration stuff to make Mapbox features work.

@bleege
Copy link
Contributor

bleege commented Dec 9, 2015

Rebased and Merged.

@bleege bleege closed this as completed Dec 9, 2015
@zugaldia
Copy link
Member

zugaldia commented Dec 9, 2015

@bleege great to see this landing. What would be the best place to get sample code of the new behavior?

@bleege
Copy link
Contributor

bleege commented Dec 9, 2015

@zugaldia For now the best spot is in UserLocationView in toggleGPS() and onLocationUpdated().

@bleege bleege mentioned this issue Dec 18, 2015
13 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
Projects
None yet
Development

No branches or pull requests

5 participants