Decouple OnMyLocationChangeListener from UserLocationView

[tobrun]

In MapView (after going through MapboxMap)

 void setOnMyLocationChangeListener(@Nullable MapboxMap.OnMyLocationChangeListener listener){
    mUserLocationView.setOnMyLocationChangeListener(listener);
}

We have a method to add OnLocationChangeListener callback. This currently uses UserLocationView. We used to provide all the these locational changes through UserLocationView but with the introduction of LocationServices this is no longer needed.

[tobrun]

I'm now also noticing we have 2 Location calbacks:

    @Override
    public void onLocationChanged(Location location) {

    }

    @Override
    public void onMyLocationChange(@Nullable Location location) {

    }

[bleege]

Just seeing this now as part of #5282 and wondering if we really want to set MapboxMap.setOnMyLocationChangeListener() as deprecated. Yes, LocationServices is what powers the User Location data but this seems like a much easier / less verbose API for people to use than having developers have to register as a com.mapbox.mapboxsdk.location.LocationListener. The level of abstraction may also be helpful in future proofing the API going forward too. For example, to implement the common "zoom to user location when found" use case:

// Current Setup
// Can be done inside `MapView.getMapAsync()`
mapboxMap.setOnMyLocationChangeListener(new MapboxMap.OnMyLocationChangeListener() {
    @Override
    public void onMyLocationChange(@Nullable Location location) {
        if (location != null) {
            mapboxMap.setCameraPosition(new CameraPosition.Builder()
                                 .target(new LatLng(location))
                                 .zoom(16)
                                 .bearing(0)
                                 .tilt(0)
                                 .build());
            mapboxMap.setOnMyLocationChangeListener(null);
        }
     }
});

// New Setup
final Context context = this;  // Needs to be done outside of `MapView.getMapAsync()`
...
LocationServices.getLocationServices(context).addLocationListener(new LocationListener() {
    @Override
    public void onLocationChanged(Location location) {
        mapboxMap.setCameraPosition(new CameraPosition.Builder()
                  .target(new LatLng(location))
                  .zoom(16)
                  .bearing(0)
                  .tilt(0)
                  .build());
        LocationServices.getLocationServices(context).removeLocationListener(this);
     }
});

[tobrun]

This issue is not so much about having a method to register for location updates, it's about having 2 different interfaces that do the exactly the same thing. That said I'm not keen on reintroducing this mainly because of separation of concerns. Making this deprecated now allows us to remove location from Mapview/MapboxMap. At some point this could be migrated into a separate library or made a part of MAS. FWIW Google is doing the same where they are pointing people to use google play services instead.

I'm not sure what you mean by:

final Context context = this;  // Needs to be done outside of `MapView.getMapAsync()`

You can reference the Activity from an inner class by:

LocationServices.getLocationServices(MainActivity.this).removeLocationListener(this);

[bleege]

To me this is about having a simple and consistent API for developers using the MyLocation functionality in the SDK. Everything related to MyLocation is at the MapboxMap level so it seems odd to single one part out and force developers to have to find some non immediately obvious way to make use of it for their apps, which is especially confusing for people new to Mapbox Android SDK. In other words, if MapboxMap.setOnMyLocationChangeListener() is deprecated and ultimately removed then doesn't logic follow that the following should also be deprecated and removed (they're currently not):

MapboxMap.OnMyBearingTrackingModeChangeListener()
MapboxMap.OnMyLocationTrackingModeChangeListener()
MapboxMap.getMyLocation()
MapboxMap.setMyLocationEnabled()
MapboxMap.isMyLocationEnabled()

I agree that it's generally not good to repeat code (aka the DRY principle), but in this case MapboxMap.setOnMyLocationChangeListener() is providing value by being a consistent API with the rest of the MyLocation functionality and is not repeating functionality as it uses the same LocationServices under the hood.

[zugaldia]

I think this should eventually be tackled as part of #4331, as supported by #4908 (comment), which will require a semver change.

Also, I'd like to explore the idea of having MAS or the SDK to provide a default implementation based on LOST even if we move to decouple the location provider.

@tobrun @bleege I suggest we remove this ticket from the android-v4.1.0 milestone and revisit it at a later release.

[tobrun]

@zugaldia revisiting later would result in reverted the commit. The deprecation is not found in previous release and agreed that this is part of #4331. I will revert the changes and PR it.

[tobrun]

Deprecation has been reverted in #5387 and merged into release-android-v4.1.0.

[zugaldia]

@tobrun 👍 thanks.

[bleege]

Cool. Thanks for tackling this @tobrun @zugaldia. 👍

[bleege]

While working on an internal app this morning I noticed that the @Deprecated tag itself wasn't removed as part of the fix (2f8fd61#commitcomment-18044582) . I'm going to re-open this ticket and remove it so that everything ships in 4.1.0 as planned.

[bleege]

Removed the remaining @Deprecated annotation and merged into the release-android-v4.1.0 branch. I'm going to fire off a new SNAPSHOT build now to make this available sooner.