Check for Location Permission #12

electrostat · 2017-11-14T20:22:19Z

Before requesting location updates through the location engine, check to see if the application already has location permission. If not, prevent location updates from running and fall back on an exponential backoff system for checking on the status of the location permission. Once permission checks pass, allow location engine to operate normally.

Guardiola31337 · 2017-11-20T12:04:23Z

liblocation/src/main/java/com/mapbox/services/android/core/permissions/ExponentialBackoff.java

+   * </p>
+   */
+  public final long getElapsedTimeMillis() {
+    return (System.nanoTime() - startTimeNanos) / 1000000;


NIT Magic number

Guardiola31337 · 2017-11-20T12:04:29Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

@@ -184,7 +188,7 @@ private void sendEvents(List<Event> events, Callback httpCallback) {
  }

  private boolean startTelemetry() {
-    if (!isTelemetryEnabled) {
+    if (!isTelemetryEnabled && checkLocationPermission()) {


Adding the check here would mean that

mapbox-events-android/libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

Lines 190 to 192 in a2ec694

schedulerFlusher.register();

Clock clock = obtainClock();

schedulerFlusher.schedule(clock.giveMeTheElapsedRealtime());

won't be executed unless the user has the permissions granted and we don't want that.

The expected behavior is to have everything working except the location-related stuff.
We added the following // TODO comments to analyze if it would make sense to add the check within TelemetryService or not:

mapbox-events-android/libtelemetry/src/main/java/com/mapbox/services/android/telemetry/TelemetryService.java

Line 39 in a2ec694

// TODO Check for location permissions here

mapbox-events-android/libtelemetry/src/main/java/com/mapbox/services/android/telemetry/TelemetryService.java

Line 51 in a2ec694

// TODO If the location receiver was not created (permissions were not granted) do not unregister

The idea of not creating the service if the permissions are not granted is great but we should keep the rest of functionalities working.

Now it's a good moment to extract that block of code into a well-named method.

Guardiola31337

Why not exploring the idea commented in #12 (comment)?

The idea of not creating the service if the permissions are not granted is great but we should keep the rest of functionalities working.

It would be also more performant because we wouldn't create the service at all if the permissions are not granted (fewer resources, less memory, 💪 battery). What do you think?

In any case, we should take care of any potential leaks 👇

Guardiola31337 · 2017-11-21T12:30:13Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/TelemetryService.java

+  }
+
+  private void permissionBackoff(final Context context) {
+    final Handler handler = new Handler();


This code may cause a memory leak.
👀 How to Leak a Context: Handlers & Inner Classes

Guardiola31337 · 2017-12-01T12:18:26Z

liblocation/src/main/java/com/mapbox/services/android/core/permissions/ExponentialBackoff.java

+ * In this version, the interval creation is deterministic and has no concept of
+ * maxElapsedTimeMillis, it's up to the application to stop. It also adjusts its default values.
+ */
+public class ExponentialBackoff {


This class is only used internally, so no need to be public (we don't need to expose it publicly). The same thing happens with the properties and the methods included.

Guardiola31337 · 2017-12-01T12:31:45Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

      registerEventReceiver();
      isOpted = true;
+      if (checkLocationPermission()) {


Adding the check here would mean that

mapbox-events-android/libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

Lines 203 to 204 in 22f1465

registerEventReceiver();

isOpted = true;

will be executed and we don't want that.

No need to register the receiver in charge of dealing with the location probes if permissions are not granted. Besides that, isOpted should only be true if previous actions startLocation() and registerEventReceiver() were executed, meaning that location is opted. If not, we may have issues when optLocationOut().

Guardiola31337 · 2017-12-01T12:37:12Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

+    handler.postDelayed(permissionCheckRunnable, nextWaitTime);
+  }
+
+  private final class PermissionCheckRunnable implements Runnable {


Why is PermissionCheckRunnable a nested class of MapboxTelemetry? Is it necessary? Could we extract PermissionCheckRunnable class into a PermissionCheckRunnable.java file/class?

Guardiola31337 · 2017-12-01T12:40:10Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/TelemetryService.java

@@ -48,8 +47,10 @@ public int onStartCommand(Intent intent, int flags, int startId) {

  @Override
  public void onDestroy() {
-    // TODO If the location receiver was not created (permissions were not granted) do not unregister
-    unregisterLocationReceiver();
+    if (locationReceiver != null) {


I think this is not necessary because if we never create TelemetryService we can't get a null locationReceiver. This is because how we're dealing with the TelemetryService creation in MapboxTelemetry. Am I missing something?

Guardiola31337

Looking good so far.

Some minor details.
We should also include some unit tests around the new location permission check.

Guardiola31337 · 2017-12-05T09:46:15Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/ExponentialBackoff.java

+class ExponentialBackoff {
+
+  /** The default initial interval value in milliseconds (10 seconds). */
+  public static final int DEFAULT_INITIAL_INTERVAL_MILLIS = 10_000;


Access can be private

The IDE hints you about the visibility, take advantage of it.

The same thing happens with the properties and the methods 👇

Guardiola31337 · 2017-12-05T10:09:17Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

+
+  void locationAndReceiverSetup() {
+    registerEventReceiver();
+    isOpted = true;


#12 (comment)

Besides that, isOpted should only be true if previous actions startLocation() and registerEventReceiver() were executed, meaning that location is opted.

startLocation() and registerEventReceiver() might fail, order matters.

Guardiola31337 · 2017-12-05T10:33:52Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/PermissionCheckRunnable.java

+  @Override
+  public void run() {
+    if (PermissionsManager.areLocationPermissionsGranted(context)) {
+      mapboxTelemetry.locationAndReceiverSetup();


If isTelemetryEnabled or isOpted change their value between intervals will cause problems.
No need to create locationAndReceiverSetup method because we could use the public method optIn which will check those variables too.

Guardiola31337 · 2017-12-05T10:39:54Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

+  }
+
+  private void permissionBackoff() {
+    Handler handler = new Handler();


There's no need to create a handler and a counter here because we're already doing it in PermissionCheckRunnable (it may be interesting to pass them in through the constructor though).

Guardiola31337 · 2017-12-05T10:43:21Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

+
+    PermissionCheckRunnable permissionCheckRunnable = new PermissionCheckRunnable(context, this);
+
+    long nextWaitTime = counter.nextBackOffMillis();


No need to calculate nextWaitTime and calling handler.postDelayed(permissionCheckRunnable, nextWaitTime) because of ☝️
What about calling permissionCheckRunnable.run() instead?

Guardiola31337 · 2017-12-06T12:47:37Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/ExponentialBackoff.java

+   */
+  private long startTimeNanos;
+
+  public ExponentialBackoff() {


#12 (comment)

Access can be package-private

Guardiola31337 · 2017-12-06T12:47:44Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/ExponentialBackoff.java

+  }
+
+  /** Sets the interval back to the initial retry interval and restarts the timer. */
+  public final void reset() {


Access can be package-private

Guardiola31337 · 2017-12-06T12:48:12Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/ExponentialBackoff.java

+  /**
+   * Get the next back off interval
+   */
+  public long nextBackOffMillis() {


Access can be package-private

#12 (comment)
And the rest of the methods 👇

Guardiola31337 · 2017-12-06T13:08:13Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/PermissionCheckRunnable.java

+  @Override
+  public void run() {
+    if (PermissionsManager.areLocationPermissionsGranted(context)) {
+      mapboxTelemetry.optIn();


I guess this won't work properly because mapboxTelemetry.optIn() 👉 optLocationIn() 👉 checkLocationPermission() 👉 permissionBackoff() which will create a new instance of PermissionCheckRunnable so the intervals will be reset (loosing the exponential backoff behavior).
What about creating a lazy initialization method for PermissionCheckRunnable creation?

Guardiola31337 · 2017-12-06T13:13:23Z

libtelemetry/src/test/java/com/mapbox/services/android/telemetry/MapboxTelemetryTest.java

@@ -473,4 +473,37 @@ public void checksUnregisterReceiverWhenOptedOut() throws Exception {

    verify(mockedLocalBroadcastManager, times(1)).unregisterReceiver(eq(theEventReceiver));
  }
+
+  @Test
+  public void checksLocationPermissionSet() throws Exception {


This is the same exact test as the checksOptedIn one. We should remove it.

Guardiola31337 · 2017-12-06T13:18:49Z

libtelemetry/src/test/java/com/mapbox/services/android/telemetry/MapboxTelemetryTest.java

+  }
+
+  @Test
+  public void checksLocationPermissionNotSet() throws Exception {


Here we're not testing the location permissions.
It actually returns false because telemetry is not enabled and isTelemetryEnabled is false

mapbox-events-android/libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

Line 200 in 04732b8

if (isTelemetryEnabled && !isOpted && checkLocationPermission()) {

We should revisit this test.

Guardiola31337 · 2017-12-07T18:59:56Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/PermissionCheckRunnable.java

+import com.mapbox.services.android.core.permissions.PermissionsManager;
+
+class PermissionCheckRunnable implements Runnable {
+  private static PermissionCheckRunnable permissionCheckRunnable;


Private field 'permissionCheckRunnable' is never used

Guardiola31337 · 2017-12-11T13:05:58Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

+
+  private void permissionBackoff() {
+    PermissionCheckRunnable permissionCheckRunnable = obtainPermissionCheckRunnable();
+    permissionCheckRunnable.run();


I think that if we call permissionCheckRunnable.run() here the intervals will be reset (losing the exponential backoff behavior), not completely sure though we should test it.
Related 👉 https://github.com/mapbox/mapbox-events-android/pull/12/files#r155230708

Tested and works.

Guardiola31337

#12 (comment)

Tested and works.

Still wondering why it works as expected 🤔

Anyway, we should revisit this later because I was thinking about firing “permissionBackoff” https://github.com/mapbox/mapbox-events-android/pull/12/files#diff-d0541eaee9bf537cfb1016de23583b33R295 and then not worry about it anymore from MapboxTelemetry because PermissionCheckRunnable could run by its own managing the intervals internally through ExponentialBackoff and notify MapboxTelemetry when the permissions are granted and at that point stop/reset everything so we don’t leak anything and leaving MapboxTelemetry ready just in case the user remove the permissions and “permissionBackoff” would need to be fired again.
This is the scenario that we need to take into account, the happy path and also the unhappy path which in our case is what happens when the user remove the permissions? Does the whole machinery still work/behave as expected?

Apart from that, we should also test thoroughly that we’re not leaking any object.

…ore artifact out of libcore and liblocation modules

…tener public

* Add AudioType Support * NavigationUtils + Refactor

* add missing event field to nav events and gson builder with nav events serializers * add missing telemetry client events tests and expose missing navigation DTOs setters

- make priority editable

- adjust placement of @LocationEnginePriority.PowerMode annotations

- add in exponential backoff - permission check - integration into system

electrostat added enhancement in progress labels Nov 14, 2017

electrostat self-assigned this Nov 14, 2017

electrostat requested a review from Guardiola31337 November 15, 2017 19:46

electrostat added ready for review and removed in progress labels Nov 15, 2017

electrostat force-pushed the aa-permission-check branch from e634e6b to 092448c Compare November 16, 2017 20:13

Guardiola31337 requested changes Nov 20, 2017

View reviewed changes

electrostat requested a review from Guardiola31337 November 20, 2017 18:09

Guardiola31337 requested changes Nov 21, 2017

View reviewed changes

electrostat requested a review from Guardiola31337 November 27, 2017 22:31

Guardiola31337 requested changes Dec 1, 2017

View reviewed changes

electrostat requested a review from Guardiola31337 December 4, 2017 19:43

Guardiola31337 requested changes Dec 5, 2017

View reviewed changes

electrostat requested a review from Guardiola31337 December 5, 2017 19:35

Guardiola31337 requested changes Dec 6, 2017

View reviewed changes

electrostat requested a review from Guardiola31337 December 6, 2017 20:53

Guardiola31337 reviewed Dec 7, 2017

View reviewed changes

electrostat requested a review from Guardiola31337 December 7, 2017 22:34

Guardiola31337 reviewed Dec 11, 2017

View reviewed changes

electrostat requested a review from Guardiola31337 December 11, 2017 18:46

electrostat added the RELEASE BLOCKING label Dec 12, 2017

Guardiola31337 approved these changes Dec 13, 2017

View reviewed changes

Guardiola31337 and others added 7 commits December 13, 2017 13:51

port location code from MAS and implement a solution to generate libc…

d1e9e1c

…ore artifact out of libcore and liblocation modules

cleanup gradle dependencies

62a8c0f

make location engine priority constants class and location engine lis…

e542db3

…tener public

libtelemetry initial commit

a22be49

release android-core-0.1.0 (#9)

df5ba7c

release android-core-0.1.1 (#11)

d14d06d

add libtelemetry CI instrumentation tests support (#15)

418ab90

Guardiola31337 and others added 7 commits December 13, 2017 13:51

fix service not starting/stopping properly (#14)

5cbcf54

Add AudioType Support (#13)

5bca830

* Add AudioType Support * NavigationUtils + Refactor

Fix navigation events (#16)

1c26638

* add missing event field to nav events and gson builder with nav events serializers * add missing telemetry client events tests and expose missing navigation DTOs setters

Make Priority Editable (#18)

313c0e0

- make priority editable

Adjust PowerMode Annotations (#21)

a00843e

- adjust placement of @LocationEnginePriority.PowerMode annotations

update CI image (#23)

da584b3

Check Location Permission

79148a3

- add in exponential backoff - permission check - integration into system

electrostat force-pushed the aa-permission-check branch from a41bc97 to 79148a3 Compare December 13, 2017 18:57

Merge branch 'master' into aa-permission-check

667615a

electrostat merged commit ab6e7ca into master Dec 13, 2017

electrostat deleted the aa-permission-check branch December 13, 2017 20:07

Guardiola31337 mentioned this pull request Dec 15, 2017

General clean up #28

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for Location Permission #12

Check for Location Permission #12

electrostat commented Nov 14, 2017

Guardiola31337 Nov 20, 2017

Guardiola31337 Nov 20, 2017

Guardiola31337 left a comment

Guardiola31337 Nov 21, 2017

Guardiola31337 Dec 1, 2017

Guardiola31337 Dec 1, 2017

Guardiola31337 Dec 1, 2017

Guardiola31337 Dec 1, 2017

Guardiola31337 left a comment

Guardiola31337 Dec 5, 2017

Guardiola31337 Dec 5, 2017

Guardiola31337 Dec 5, 2017

Guardiola31337 Dec 5, 2017

Guardiola31337 Dec 5, 2017

Guardiola31337 Dec 5, 2017

Guardiola31337 Dec 6, 2017

Guardiola31337 Dec 6, 2017

Guardiola31337 Dec 6, 2017

Guardiola31337 Dec 6, 2017

Guardiola31337 Dec 6, 2017

Guardiola31337 Dec 6, 2017

Guardiola31337 Dec 6, 2017

Guardiola31337 Dec 7, 2017

Guardiola31337 Dec 11, 2017

electrostat Dec 11, 2017

Guardiola31337 left a comment

	schedulerFlusher.register();
	Clock clock = obtainClock();
	schedulerFlusher.schedule(clock.giveMeTheElapsedRealtime());


		PermissionCheckRunnable permissionCheckRunnable = new PermissionCheckRunnable(context, this);

		long nextWaitTime = counter.nextBackOffMillis();

Check for Location Permission #12

Check for Location Permission #12

Conversation

electrostat commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Guardiola31337 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

Guardiola31337 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

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

Guardiola31337 left a comment

Choose a reason for hiding this comment