Abstract the location engine from a specific provider #269

zugaldia · 2017-01-11T21:49:04Z

mention-bot · 2017-01-11T21:49:05Z

@zugaldia, thanks for your PR! By analyzing the history of the files in this pull request, we identified @cammace to be a potential reviewer.

zugaldia · 2017-01-12T16:46:42Z

This PR is ready is for a first round of feedback.

The main goal of this PR is to create a new abstract LocationEngine that can be implemented by different location providers (open source or proprietary), and decouple it from other components so that it can be used for telemetry, user tracking (map), navigation, or mocking use-cases.

In particular, this PR includes:

A new PermissionsManager and PermissionsListener that replaces the old PermissionsUtils to be more flexible checking for any kind of permission, not just location, at runtime.
A new set of LocationEngine and LocationEngineListener interfaces that abstract a high level API for a location provider engine. Priority is set via a generic LocationEnginePriority that encapsulates common priorities in Android.
Sample implementations for Lost (LostLocationEngine), Google Play Services (GoogleLocationEngine) and of a Mock provider (MockLocationEngine) using a Handler to simulate GPS updates.
A new LocationEngineActivity in the test app that shows how to switch between providers. The test app has been updated to the latest Lost version and it now includes a Google Play Services dependency.
GeocodingServiceActivity and GeocodingWidgetActivity are updated to use the latest Lost version too.
Some tail work from Remove deprecated v4 directions #260 that was left due to a failed rebase that was breaking the app build.

Still WIP, required next actions:

Review of the current approach by @mapbox/android
Fix Javadoc

Related mapbox/mapbox-gl-native#4331.

ghost · 2017-01-12T17:18:49Z

...etry/src/main/java/com/mapbox/services/android/telemetry/permissions/PermissionsManager.java

+
+  private final int REQUEST_PERMISSIONS_CODE = 0;
+
+  private Activity activity;


use weak references just in case?

would it be possible to remove activity completely? it's used for 2 items: permissions granted checks and requesting permissions. Imo I think callers of those method won't mind passing an reference to the Activity.

cammace

All looks good so far.

cammace · 2017-01-12T17:33:44Z

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

+
+/**
+ * Created by antonio on 1/11/17.
+ */


cough cough

sorry, sorry, fixed.

cammace · 2017-01-12T17:35:05Z

...try/src/main/java/com/mapbox/services/android/telemetry/permissions/PermissionsListener.java

+
+/**
+ * Created by antonio on 1/11/17.
+ */


tobrun

Looks great, small nits added

tobrun · 2017-01-12T18:16:55Z

mapbox/app/src/main/java/com/mapbox/services/android/testapp/location/LostLocationEngine.java

+      .build();
+  }
+
+  public static LocationEngine getLocationEngine(Context context) {


synchronised keyword for thread safety to avoid creating 2 instances?

good call, done.

tobrun · 2017-01-12T18:17:22Z

mapbox/app/src/main/java/com/mapbox/services/android/testapp/location/GoogleLocationEngine.java

+      .build();
+  }
+
+  public static LocationEngine getLocationEngine(Context context) {


synchronised keyword for thread safety to avoid creating 2 instances?

good call, done.

tobrun · 2017-01-12T18:18:34Z

...try/src/main/java/com/mapbox/services/android/telemetry/permissions/PermissionsListener.java

+
+public interface PermissionsListener {
+
+  void onExplanationNeeded(List<String> permissionsToExplain);


what does this method do?

Let the developer know that they need to provide an explanation to the user as indicated in the docs. Basically, it covers the ActivityCompat.shouldShowRequestPermissionRationale() == true flow.

tobrun · 2017-01-12T18:18:55Z

...etry/src/main/java/com/mapbox/services/android/telemetry/permissions/PermissionsManager.java

+/**
+ * Helps request permissions at runtime
+ */
+


tobrun · 2017-01-12T18:23:00Z

...etry/src/main/java/com/mapbox/services/android/telemetry/permissions/PermissionsManager.java

+
+  private final int REQUEST_PERMISSIONS_CODE = 0;
+
+  private Activity activity;


would it be possible to remove activity completely? it's used for 2 items: permissions granted checks and requesting permissions. Imo I think callers of those method won't mind passing an reference to the Activity.

zugaldia · 2017-01-12T19:35:22Z

@cammace All comments addressed, care to 👍 / 👎 ?

zugaldia added 3 commits January 11, 2017 16:44

remove v4 activity, again

bab483b

replace PermissionsUtils with a more core PermissionsManager

2862efe

implement changes in testapp

61baddb

zugaldia added the ⚠️ DO NOT MERGE label Jan 11, 2017

zugaldia added this to the v2.0.0 milestone Jan 11, 2017

zugaldia self-assigned this Jan 11, 2017

zugaldia added 5 commits January 12, 2017 11:24

update to latest lost version

5719f86

abstract location engine that any provider can implement

f4b53c2

sample location engines implementations

0a42d23

update permissions logic

7c194bf

sample location engine activity

cfe40d3

ghost reviewed Jan 12, 2017

View reviewed changes

cammace reviewed Jan 12, 2017

View reviewed changes

tobrun reviewed Jan 12, 2017

View reviewed changes

zugaldia added 6 commits January 12, 2017 13:50

let's not keep references to any activity or context

4fbe277

typo

c94f63a

only one is necessary to have location updates

882066a

add synchronized keyworkd

37fdc75

fix javadoc

42ec10a

small javadoc fix

77eedf9

zugaldia added 2 commits January 12, 2017 14:38

minor rename

910e28a

update CHANGELOG.md

1993b46

cammace approved these changes Jan 12, 2017

View reviewed changes

zugaldia merged commit 89d0ca4 into master Jan 12, 2017

zugaldia deleted the location-engine branch January 12, 2017 20:37

zugaldia removed the ⚠️ DO NOT MERGE label Jan 13, 2017

zugaldia changed the title ~~[WIP] Abstract the location engine from a specific provider~~ Abstract the location engine from a specific provider Jan 13, 2017

This was referenced Jan 13, 2017

Add Android-only LocationEngine implementation #274

Closed

Add LocationSource mapbox/mapbox-gl-native#4331

Closed

zugaldia mentioned this pull request Jan 27, 2017

[WIP] Extract telemetry into MAS mapbox/mapbox-gl-native#7892

Merged

2 tasks

zugaldia mentioned this pull request Feb 10, 2017

Release MAS v2.0.0-beta.1 #322

Closed

9 tasks

zugaldia mentioned this pull request Feb 22, 2017

Release MAS v2.0.0-beta.2 #349

Closed

9 tasks

zugaldia mentioned this pull request Mar 9, 2017

Release MAS v2.0.0-beta.3 #384

Closed

9 tasks

zugaldia mentioned this pull request Mar 17, 2017

Release MAS v2.0.0 #394

Closed

9 tasks

zugaldia mentioned this pull request Jun 30, 2017

Make location provider library dependency optional mapbox/mapbox-gl-native#9403

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abstract the location engine from a specific provider #269

Abstract the location engine from a specific provider #269

zugaldia commented Jan 11, 2017

mention-bot commented Jan 11, 2017

zugaldia commented Jan 12, 2017 •

edited

Loading

ghost Jan 12, 2017

tobrun Jan 12, 2017

cammace left a comment

cammace Jan 12, 2017

zugaldia Jan 12, 2017

cammace Jan 12, 2017

zugaldia Jan 12, 2017

tobrun left a comment

tobrun Jan 12, 2017

zugaldia Jan 12, 2017

tobrun Jan 12, 2017

zugaldia Jan 12, 2017

tobrun Jan 12, 2017

zugaldia Jan 12, 2017

tobrun Jan 12, 2017

zugaldia Jan 12, 2017

tobrun Jan 12, 2017

zugaldia commented Jan 12, 2017


		private final int REQUEST_PERMISSIONS_CODE = 0;

		private Activity activity;


		public interface PermissionsListener {

		void onExplanationNeeded(List<String> permissionsToExplain);

Abstract the location engine from a specific provider #269

Abstract the location engine from a specific provider #269

Conversation

zugaldia commented Jan 11, 2017

mention-bot commented Jan 11, 2017

zugaldia commented Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cammace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobrun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zugaldia commented Jan 12, 2017

zugaldia commented Jan 12, 2017 •

edited

Loading