Make location provider optional by Guardiola31337 · Pull Request #502 · mapbox/mapbox-java

Guardiola31337 · 2017-07-12T17:07:14Z

Per mapbox-gl-native/#9403 we're removing location dependencies on the map SDK. They'll be pulled in from here (MAS), so this PR addresses that.

Bundle default LocationEngine implementations for Android core, LOST, and Google Play Services backends
Add LOST and Google Play Services to build.gradle as optional dependencies
At runtime, detect whether LOST or Google Play Services are available
Let the developer set a priority preference for location provider backend (i.e. if more than one provider is available, be able to set one as preferred)

Am I missing something? @mapbox/android

👀 @zugaldia @tobrun for review

zugaldia · 2017-07-12T19:51:49Z

@Guardiola31337 So excited to see this happening.

tobrun

Looking great so far, just one remark on LocationSourceProvider

tobrun · 2017-07-13T13:06:51Z

...try/src/main/java/com/mapbox/services/android/telemetry/location/LocationSourceProvider.java

+  private void initLocationSourceDictionary(Context context) {
+    locationSourceDictionary = new HashMap<>(3);
+    locationSourceDictionary.put("Google Play Services", GoogleLocationEngine.getLocationEngine(context));
+    locationSourceDictionary.put("Lost", LostLocationEngine.getLocationEngine(context));


above doesn't seem to validate if it's in the classpath or not

Guardiola31337 · 2017-07-18T19:05:53Z

GoogleLocationEngine is not working properly (not receiving location changes)
Add LocationEngineChainSupplier remaining tests
Add LocationEngineProvider tests

Guardiola31337 · 2017-09-26T21:20:03Z

Let the developer set a priority preference for location provider backend (i.e. if more than one provider is available, be able to set one as preferred)

Usage:

LocationEngineProvider locationEngineProvider = new LocationEngineProvider(this);
locationEngine = locationEngineProvider.obtainAvailableLocationEngines()
  .get(LocationEngineProvider.GOOGLE_PLAY_SERVICES);
if (locationEngine == null) {
  locationEngine = locationEngineProvider.obtainAvailableLocationEngines()
    .get(LocationEngineProvider.ANDROID);
}

What do you think @zugaldia?

GoogleLocationEngine is not working properly (not receiving location changes)

We should revisit the provider strategy. I run some tests and the problem was that when you set the priority to HIGH_ACCURACY we're using the GPS_PROVIDER and this provider takes some time to start receiving location updates (indoors takes forever). I tried to change the priority to LOW_POWER or BALANCED_POWER_ACCURACY and it worked. Maybe we could add timeout timers so if we don't get any updates on a specific provider and timer elapses use a different provider. Another option to explore is using Criteria as previously suggested.

Add LocationEngineChainSupplier remaining tests

Not necessary anymore. LocationEngineChainSupplier was removed.

Add LocationEngineProvider tests

LocationEngine classes are full of static methods and singletons which make adding tests difficult. I guess this could wait until we rewrite those classes.

Guardiola31337 · 2017-09-27T09:00:49Z

23b50d9 adds/fixes new APIs to get LocationEngines through LocationEngineProvider:

obtainBestLocationEngineAvailable() which returns the best LocationEngine available:
- Return: GoogleLocationEngine, LostLocationEngine or AndroidLocationEngine
- Criteria: GOOGLE_PLAY_SERVICES > LOST > ANDROID
- @NonNull ~> If GoogleLocationEngine and LostLocationEngine aren't available AndroidLocationEngine is always returned
- Usage:

LocationEngineProvider locationEngineProvider = new LocationEngineProvider(context);
locationEngine = locationEngineProvider.obtainBestLocationEngineAvailable();

obtainLocationEngineBy(LocationEngine.Type type) which returns the LocationEngine by type or null if not available:
- Options: LocationEngine.Type types (GOOGLE_PLAY_SERVICES, LOST, ANDROID, MOCK)
- @Nullable ~> If the location engine you're asking for is not available a null is returned
- Usage:

LocationEngineProvider locationEngineProvider = new LocationEngineProvider(this);
locationEngine = locationEngineProvider.obtainLocationEngineBy(LocationEngine.Type.GOOGLE_PLAY_SERVICES);
if (locationEngine == null) {
  locationEngine = locationEngineProvider.obtainBestLocationEngineAvailable();
}

Or directly requesting ANDROID location engine which won't be null (always available):

LocationEngineProvider locationEngineProvider = new LocationEngineProvider(this);
locationEngine = locationEngineProvider.obtainLocationEngineBy(LocationEngine.Type.GOOGLE_PLAY_SERVICES);
if (locationEngine == null) {
  locationEngine = locationEngineProvider.obtainLocationEngineBy(LocationEngine.Type.ANDROID);
}

That ☝️ allows you to:

LocationEngineProvider locationEngineProvider = new LocationEngineProvider(this);
locationEngine = locationEngineProvider.obtainLocationEngineBy(LocationEngine.Type.ANDROID);

Could be useful/interesting in some use cases.

Add javadoc to public methods and classes (documentation)

Apart from ☝️ this PR is ready for review.

👀 @zugaldia @tobrun

zugaldia

A few comments, none of them blocking the PR. Can't wait to start using the new APIs.

zugaldia · 2017-10-02T14:12:28Z

mapbox/app/build.gradle


+    // LOST
+    compile(rootProject.ext.dep.lost) {
+        exclude group: 'com.google.guava'


If we upgrade the dependency to the latest LOST that Guava exclusion is no longer necessary.

I tested the sample app with lost 3.0.3 (atm latest version) and I found that it wasn't working properly so I'd rather keep lost 1.1.1 until lostzen/lost#241 gets fixed.

@Guardiola31337 I'd actually argue the opposite. With this PR the developer now has the freedom to switch location providers if they don't meet their needs. I think that all that lostzen/lost#241 shows is that LOST fails under certain scenarios. I'd still upgrade to the latest version of LOST and leave it up to the developer decide if that scenario is a blocker for them.

zugaldia · 2017-10-02T14:13:12Z

...pp/src/main/java/com/mapbox/services/android/testapp/geocoding/GeocodingServiceActivity.java

-   * Builds Android location engine
+   * Builds location engine
   */
-  private synchronized void buildAndroidLocationEngine() {


Why is synchronized no longer needed?

@zugaldia
Don't like to answering with a question but I'm pretty sure you have more context:
Why was synchronized in the first place? Why getLocationEngine() from AndroidLocationEngine, LostLocationEngine and GoogleLocationEngine need to be synchronized?

BTW, I've tested it and I didn't find any issues so apparently is working as expected.

@Guardiola31337 For thread safety, did your tests include that scenario (i.e. multi-threading)?

@Guardiola31337 just seeing this is an Activity, I though this was part of the library, thanks for pointing that out. Nevermind.

zugaldia · 2017-10-02T14:18:54Z

mapbox/libandroid-telemetry/build.gradle

+
+    // LOST
+    provided(rootProject.ext.dep.lost) {
+        exclude group: 'com.google.guava'


Same here: let's update to the latest version. If the latest version is an issue to the developer, now they have a mechanism to switch to a different provider.

👀 #502 (comment)

zugaldia · 2017-10-02T14:20:47Z

mapbox/app/src/main/res/values/strings.xml

        <item>Android</item>
        <item>Lost</item>
-        <item>Google Play Services</item>
+        <item>Google_Play_Services</item>


Was this necessary to match the new Type enum? If so, 👍 .

zugaldia · 2017-10-02T14:24:41Z

@tobrun @cammace It'd be great if you could give this PR a pass before merging to make sure there're no concerns on the Maps/Navigation side.

Guardiola31337 · 2017-10-04T20:14:04Z

2f856d6 and 2ec7c2d addressed your comments @zugaldia

Ready for a second round of 👀

cammace

Looks like this won't affect Navigation much, we do fall back on the LOST location engine currently in case a user doesn't pass in a location engine. This change I believe will require us to bring the LOST dependency directly into the nav sdk?

cammace · 2017-10-05T17:16:28Z

...metry/src/main/java/com/mapbox/services/android/telemetry/location/GoogleLocationEngine.java

 * Sample LocationEngine using Google Play Services
 */
-public class GoogleLocationEngine extends LocationEngine implements
+class GoogleLocationEngine extends LocationEngine implements


This still needs to be public inorder not to break the API

cammace · 2017-10-05T17:17:23Z

...metry/src/main/java/com/mapbox/services/android/telemetry/location/GoogleLocationEngine.java

  private GoogleApiClient googleApiClient;

-  public GoogleLocationEngine(Context context) {
+  private GoogleLocationEngine(Context context) {


needs to remain public to not break API

cammace · 2017-10-05T17:18:13Z

...lemetry/src/main/java/com/mapbox/services/android/telemetry/location/LostLocationEngine.java

 * Sample LocationEngine using the Open Source Lost library
 */
-public class LostLocationEngine extends LocationEngine implements LocationListener {
+class LostLocationEngine extends LocationEngine implements


needs to remain public to not break API

cammace · 2017-10-05T17:18:20Z

...lemetry/src/main/java/com/mapbox/services/android/telemetry/location/LostLocationEngine.java

  private LostApiClient lostApiClient;

-  public LostLocationEngine(Context context) {
+  private LostLocationEngine(Context context) {


needs to remain public to not break API

…lable

…ary anymore

…nd address PR comments

…that CI passes (chicken-egg problem)

… in some cases

Guardiola31337 · 2017-10-05T17:41:13Z

@cammace

Looks like this won't affect Navigation much, we do fall back on the LOST location engine currently in case a user doesn't pass in a location engine. This change I believe will require us to bring the LOST dependency directly into the nav sdk?

Lost dependency is not needed anymore we could use:

LocationEngineProvider locationEngineProvider = new LocationEngineProvider(context);
locationEngine = locationEngineProvider.obtainBestLocationEngineAvailable();

which will return the best location engine available.

Further information on how to use the new APIs 👉 #502 (comment)

Guardiola31337 added libandroid-telemetry ⚠️ DO NOT MERGE labels Jul 12, 2017

Guardiola31337 requested review from tobrun and zugaldia July 12, 2017 17:07

Guardiola31337 force-pushed the pg-location-optional branch from edf4d81 to 01bd7c1 Compare July 12, 2017 17:09

Guardiola31337 mentioned this pull request Jul 12, 2017

[android] Make location provider optional mapbox/mapbox-gl-native#9488

Merged

tobrun requested changes Jul 13, 2017

View reviewed changes

Guardiola31337 force-pushed the pg-location-optional branch from bf5a464 to 7340234 Compare September 26, 2017 20:43

Guardiola31337 changed the title ~~[WIP] Make location provider optional~~ Make location provider optional Sep 27, 2017

Guardiola31337 added Ready For Review and removed ⚠️ DO NOT MERGE labels Sep 27, 2017

Guardiola31337 force-pushed the pg-location-optional branch 2 times, most recently from 678a54a to 23b50d9 Compare September 29, 2017 10:44

zugaldia approved these changes Oct 2, 2017

View reviewed changes

zugaldia requested a review from cammace October 2, 2017 14:23

Guardiola31337 force-pushed the pg-location-optional branch 5 times, most recently from d3584da to 2ec7c2d Compare October 4, 2017 19:51

cammace approved these changes Oct 4, 2017

View reviewed changes

tobrun approved these changes Oct 5, 2017

View reviewed changes

cammace suggested changes Oct 5, 2017

View reviewed changes

Guardiola31337 added 9 commits October 5, 2017 19:28

[WIP] make location provider optional

1c91c8a

[WIP] fix location engine provider providing only those that are avai…

da1717e

…lable

fix location engine provider public api and remove classes not necess…

909ad1e

…ary anymore

fix location engine provider public APIs

9972cd4

bump dependencies (build tools, support libraries, mapbox and lost) a…

e456599

…nd address PR comments

ignore location engine coming from mapbox-android-sdk momentarily so …

0b731fc

…that CI passes (chicken-egg problem)

fix naming

fceceeb

fix some breaking API changes

0d84f5a

fix obtain best location engine not returning default location engine…

ab8f6c6

… in some cases

Guardiola31337 force-pushed the pg-location-optional branch from 9694aad to ab8f6c6 Compare October 5, 2017 17:28

cammace approved these changes Oct 5, 2017

View reviewed changes

Guardiola31337 merged commit 2137263 into master Oct 5, 2017

Guardiola31337 deleted the pg-location-optional branch October 5, 2017 19:30

cammace mentioned this pull request Oct 6, 2017

Release Mapbox-Java 2.2.6 #594

Closed

14 tasks

Guardiola31337 restored the pg-location-optional branch October 9, 2017 14:33

Guardiola31337 deleted the pg-location-optional branch October 9, 2017 14:35

Guardiola31337 mentioned this pull request Nov 17, 2017

[Android] crash with beta.5 - LostLocationEngine mapbox/mapbox-gl-native#10484

Closed

Guardiola31337 mentioned this pull request Dec 12, 2017

Link of class LocationSource failed mapbox/mapbox-gl-native#10660

Closed

Guardiola31337 mentioned this pull request Aug 6, 2018

GPS Not Enabling + Not finding Location mapbox/mapbox-navigation-android#1121

Closed

Conversation

Guardiola31337 commented Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zugaldia commented Jul 12, 2017

Uh oh!

tobrun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 commented Jul 18, 2017

Uh oh!

Guardiola31337 commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Guardiola31337 commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zugaldia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zugaldia commented Oct 2, 2017

Uh oh!

Guardiola31337 commented Oct 4, 2017

Uh oh!

cammace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Guardiola31337 commented Jul 12, 2017 •

edited

Loading

Guardiola31337 commented Sep 26, 2017 •

edited

Loading

Guardiola31337 commented Sep 27, 2017 •

edited

Loading

Guardiola31337 Oct 3, 2017 •

edited

Loading

Guardiola31337 commented Oct 5, 2017 •

edited

Loading