-
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
Make location provider optional #502
Conversation
edf4d81
to
01bd7c1
Compare
@Guardiola31337 So excited to see this happening. |
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.
Looking great so far, just one remark on LocationSourceProvider
private void initLocationSourceDictionary(Context context) { | ||
locationSourceDictionary = new HashMap<>(3); | ||
locationSourceDictionary.put("Google Play Services", GoogleLocationEngine.getLocationEngine(context)); | ||
locationSourceDictionary.put("Lost", LostLocationEngine.getLocationEngine(context)); |
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.
above doesn't seem to validate if it's in the classpath or not
|
bf5a464
to
7340234
Compare
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?
We should revisit the provider strategy. I run some tests and the problem was that when you set the priority to
Not necessary anymore.
|
23b50d9 adds/fixes new APIs to get
LocationEngineProvider locationEngineProvider = new LocationEngineProvider(context);
locationEngine = locationEngineProvider.obtainBestLocationEngineAvailable();
LocationEngineProvider locationEngineProvider = new LocationEngineProvider(this);
locationEngine = locationEngineProvider.obtainLocationEngineBy(LocationEngine.Type.GOOGLE_PLAY_SERVICES);
if (locationEngine == null) {
locationEngine = locationEngineProvider.obtainBestLocationEngineAvailable();
} Or directly requesting 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.
Apart from ☝️ this PR is ready for review. |
678a54a
to
23b50d9
Compare
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 comments, none of them blocking the PR. Can't wait to start using the new APIs.
mapbox/app/build.gradle
Outdated
@@ -59,6 +59,11 @@ dependencies { | |||
// Google Play Services | |||
compile rootProject.ext.dep.gmsLocation | |||
|
|||
// LOST | |||
compile(rootProject.ext.dep.lost) { | |||
exclude group: 'com.google.guava' |
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.
If we upgrade the dependency to the latest LOST that Guava exclusion is no longer necessary.
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.
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.
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.
@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.
*/ | ||
private synchronized void buildAndroidLocationEngine() { |
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 is synchronized
no longer needed?
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.
@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.
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.
@Guardiola31337 For thread safety, did your tests include that scenario (i.e. multi-threading)?
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.
@Guardiola31337 just seeing this is an Activity
, I though this was part of the library, thanks for pointing that out. Nevermind.
|
||
// LOST | ||
provided(rootProject.ext.dep.lost) { | ||
exclude group: 'com.google.guava' |
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.
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.
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.
@@ -90,7 +90,7 @@ | |||
<item>Mock</item> | |||
<item>Android</item> | |||
<item>Lost</item> | |||
<item>Google Play Services</item> | |||
<item>Google_Play_Services</item> |
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 this necessary to match the new Type
enum? If so, 👍 .
d3584da
to
2ec7c2d
Compare
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.
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?
|
||
import java.lang.ref.WeakReference; | ||
|
||
/** | ||
* Sample LocationEngine using Google Play Services | ||
*/ | ||
public class GoogleLocationEngine extends LocationEngine implements | ||
class GoogleLocationEngine extends LocationEngine implements |
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.
This still needs to be public inorder not to break the API
@@ -31,7 +28,7 @@ | |||
private WeakReference<Context> context; | |||
private GoogleApiClient googleApiClient; | |||
|
|||
public GoogleLocationEngine(Context context) { | |||
private GoogleLocationEngine(Context context) { |
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.
needs to remain public to not break API
@@ -17,20 +15,25 @@ | |||
/** | |||
* Sample LocationEngine using the Open Source Lost library | |||
*/ | |||
public class LostLocationEngine extends LocationEngine implements LocationListener { | |||
class LostLocationEngine extends LocationEngine implements |
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.
needs to remain public to not break API
|
||
private static LocationEngine instance; | ||
|
||
private WeakReference<Context> context; | ||
private LostApiClient lostApiClient; | ||
|
||
public LostLocationEngine(Context context) { | ||
private LostLocationEngine(Context context) { |
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.
needs to remain public to not break API
…nd address PR comments
…that CI passes (chicken-egg problem)
9694aad
to
ab8f6c6
Compare
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) |
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.
LocationEngine
implementations for Android core, LOST, and Google Play Services backendsbuild.gradle
as optional dependenciesAm I missing something? @mapbox/android
👀 @zugaldia @tobrun for review