MapboxTelemetry - Optional Callback #38

electrostat · 2018-01-03T21:52:21Z

Make callback optional when creating MapboxTelemetry, so developer does not need to subscribe to the TelemetryClient Callback.

Guardiola31337

Forgetting below comments, what about adding a setter instead of constructor overloading? 👀 https://github.com/mapbox/mapbox-java/blob/mas-2.x.x/mapbox/libandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

If the listener should be optional it makes total sense to have a setter, it'd translate to a more flexible API which would mean that MapboxTelemetry is 💪

Guardiola31337 · 2018-01-08T10:54:41Z

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

-  private final Callback httpCallback;
-  private final SchedulerFlusher schedulerFlusher;
+  private Callback httpCallback;
+  private SchedulerFlusher schedulerFlusher;


Why does the final modifier need to be removed?

Guardiola31337 · 2018-01-08T10:56:00Z

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

@@ -55,12 +55,17 @@
  private PermissionCheckRunnable permissionCheckRunnable = null;

  public MapboxTelemetry(Context context, String accessToken, String userAgent, Callback httpCallback) {
+    this.httpCallback = httpCallback;
+
+    new MapboxTelemetry(context, accessToken, userAgent);


Why not calling the overloading constructor (this)?

Guardiola31337 · 2018-01-09T11:48:47Z

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

    initializeQueue();
-    AlarmReceiver alarmReceiver = obtainAlarmReceiver(httpCallback);
+


NIT empty line

Guardiola31337 · 2018-01-09T11:50:36Z

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

    }
  }

  private boolean startTelemetry() {
    if (!isTelemetryEnabled) {
      isTelemetryEnabled = true;
      optIn();
+


NIT empty line

Guardiola31337 · 2018-01-09T12:44:49Z

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

@@ -71,7 +76,10 @@ private void sendBatch(List<Event> batch, Callback callback) {
      .build();

    OkHttpClient client = setting.getClient();
-    client.newCall(request).enqueue(callback);
+
+    if (callback != null) {


That would mean that if a user doesn't set a callback (MapboxTelemetry#setHttpCallback) or if a user sets a callback to null, the request won't be executed, which is not valid. Ultimately, registering to a listener should be optional and should not affect the overall behavior ensuring that everything is still working without any action from the client/user.

This could be achieved if MapboxTelemetry implements Callback and assigning httpCallback to this, avoiding to change any other classes other than MapboxTelemetry.

It'd be great though to add a custom callback which would give us more flexibility on what actions to notify (i.e. to notify actions beyond onResponse and onFailure), more control over what and when actions need to be notified and it'd open also the possibility of having multiple listeners that users/clients could add/remove as needed.
👀 https://github.com/mapbox/mapbox-java/blob/mas-2.x.x/mapbox/libandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/TelemetryListener.java
https://github.com/mapbox/mapbox-java/blob/mas-2.x.x/mapbox/libandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java#L656
https://github.com/mapbox/mapbox-java/blob/mas-2.x.x/mapbox/libandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java#L724-L726
https://github.com/mapbox/mapbox-java/blob/mas-2.x.x/mapbox/libandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java#L700-L702
https://github.com/mapbox/mapbox-java/blob/mas-2.x.x/mapbox/libandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java#L78
https://github.com/mapbox/mapbox-java/blob/mas-2.x.x/mapbox/libandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java#L183-L191

Guardiola31337

This branch has conflicts that must be resolved

We'd need to rebase from master

👀 #38 (comment)
We should adapt/fix the overall implementation and tests based on ☝️

Guardiola31337 · 2018-01-15T12:06:29Z

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

@@ -53,37 +53,38 @@
  private boolean isOpted = false;
  private boolean serviceBound = false;
  private PermissionCheckRunnable permissionCheckRunnable = null;
+  protected CopyOnWriteArrayList<TelemetryListener> telemetryListeners = new CopyOnWriteArrayList<TelemetryListener>();


Why protected modifier? If it's only used within MapboxTelemetry it should be private.

Why initializing the array here instead of doing it "lazily" (when needed)?

Guardiola31337 · 2018-01-15T12:19:41Z

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

@@ -136,6 +137,16 @@ public void updateLocationPriority(@LocationEnginePriority.PowerMode int locatio
    }
  }

+  public void addTelemetryListener(TelemetryListener listener) {


Per consistency with removeTelemetryListener it'd be great to return a value (boolean) instead of nothing.

Guardiola31337 · 2018-01-15T12:21:12Z

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

@@ -136,6 +137,16 @@ public void updateLocationPriority(@LocationEnginePriority.PowerMode int locatio
    }
  }

+  public void addTelemetryListener(TelemetryListener listener) {
+    if (!this.telemetryListeners.contains(listener)) {


this is not adding any value here (not necessary).

If we'd use a Set (CopyOnWriteArraySet) instead of a List (CopyOnWriteArrayList) this check wouldn't be necessary (it's checked internally).

👀 Set<E> and List<E>

Guardiola31337 · 2018-01-15T12:25:27Z

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

@@ -136,6 +137,16 @@ public void updateLocationPriority(@LocationEnginePriority.PowerMode int locatio
    }
  }

+  public void addTelemetryListener(TelemetryListener listener) {
+    if (!this.telemetryListeners.contains(listener)) {
+      this.telemetryListeners.add(listener);


this is not adding any value here (not necessary).

Guardiola31337 · 2018-01-15T12:25:33Z

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

+  }
+
+  public boolean removeTelemetryListener(TelemetryListener listener) {
+    return this.telemetryListeners.remove(listener);


this is not adding any value here (not necessary).

Guardiola31337 · 2018-01-15T13:43:18Z

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

@@ -40,17 +44,17 @@ void updateUserAgent(String userAgent) {
    this.userAgent = userAgent;
  }

-  void sendEvents(List<Event> events, Callback callback) {
+  void sendEvents(List<Event> events, CopyOnWriteArrayList<TelemetryListener> telemetryListeners) {


This is not needed 👀 #38 (comment)

Guardiola31337 · 2018-01-15T13:43:27Z

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

    ArrayList<Event> batch = new ArrayList<>();
    batch.addAll(events);
-    sendBatch(batch, callback);
+    sendBatch(batch, telemetryListeners);


This is not needed 👀 #38 (comment)

Guardiola31337 · 2018-01-15T13:43:37Z

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

  }

  void updateDebugLoggingEnabled(boolean debugLoggingEnabled) {
    setting = setting.toBuilder().debugLoggingEnabled(debugLoggingEnabled).build();
  }

-  private void sendBatch(List<Event> batch, Callback callback) {
+  private void sendBatch(List<Event> batch, final CopyOnWriteArrayList<TelemetryListener> telemetryListeners) {


This is not needed 👀 #38 (comment)

Guardiola31337 · 2018-01-15T13:43:50Z

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

@@ -71,7 +75,22 @@ private void sendBatch(List<Event> batch, Callback callback) {
      .build();

    OkHttpClient client = setting.getClient();
-    client.newCall(request).enqueue(callback);
+
+    client.newCall(request).enqueue(new Callback() {


This is not needed 👀 #38 (comment)

Guardiola31337 · 2018-01-15T13:44:52Z

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

+
+public interface TelemetryListener {
+
+  void onHttpResponse(Response response) throws IOException;


👀 #38 (comment)

adapt the data that we're going to expose (not return directly what we receive from Callback 👀 https://github.com/mapbox/mapbox-java/blob/mas-2.x.x/mapbox/libandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/TelemetryListener.java#L21)

Guardiola31337 · 2018-01-15T13:53:16Z

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


 import static com.mapbox.services.android.telemetry.EventReceiver.EVENT_RECEIVER_INTENT;

-public class MapboxTelemetry implements FullQueueCallback, EventCallback {
+public class MapboxTelemetry implements FullQueueCallback, EventCallback, TelemetryListener {


This should be tackled differently 👀 #38 (comment)

Guardiola31337 · 2018-01-15T13:53:35Z

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

  }

  // For testing only
  MapboxTelemetry(Context context, String accessToken, String userAgent, EventsQueue queue,
-                  TelemetryClient telemetryClient, Callback httpCallback, SchedulerFlusher schedulerFlusher,
-                  Clock clock, LocalBroadcastManager localBroadcastManager) {
+                  TelemetryClient telemetryClient, TelemetryListener telemetryListener,


This should be tackled differently 👀 #38 (comment)

Guardiola31337 · 2018-01-15T13:56:28Z

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

    if (checkRequiredParameters(accessToken, userAgent)) {
-      telemetryClient.sendEvents(events, httpCallback);
+      telemetryClient.sendEvents(events, telemetryListeners);


This should be tackled differently 👀 #38 (comment)

Guardiola31337

Update from master and 🚀

Great job @electrostat

- add separate initializer for mapboxtelemetry - can now create MapboxTelemetry without a callback - Create custom TelemetryListener

electrostat added the bug label Jan 3, 2018

electrostat self-assigned this Jan 3, 2018

electrostat requested a review from Guardiola31337 January 3, 2018 21:52

electrostat added the ready for review label Jan 4, 2018

Guardiola31337 requested changes Jan 8, 2018

View reviewed changes

electrostat requested a review from Guardiola31337 January 8, 2018 19:44

Guardiola31337 requested changes Jan 9, 2018

View reviewed changes

electrostat requested a review from Guardiola31337 January 9, 2018 23:22

Guardiola31337 requested changes Jan 15, 2018

View reviewed changes

electrostat force-pushed the aa-optional-callback-param branch from 473f0b8 to 5c3a8de Compare January 17, 2018 21:26

electrostat requested a review from Guardiola31337 January 18, 2018 15:51

Guardiola31337 force-pushed the aa-optional-callback-param branch from 5afeb36 to 6b39504 Compare January 18, 2018 22:39

Guardiola31337 approved these changes Jan 18, 2018

View reviewed changes

Guardiola31337 force-pushed the aa-optional-callback-param branch 4 times, most recently from 42c9910 to 3fd9eb9 Compare January 18, 2018 23:59

Create Optional Callback

2fbaf81

- add separate initializer for mapboxtelemetry - can now create MapboxTelemetry without a callback - Create custom TelemetryListener

Guardiola31337 force-pushed the aa-optional-callback-param branch from 3fd9eb9 to 2fbaf81 Compare January 19, 2018 00:04

Guardiola31337 merged commit 81d8403 into master Jan 19, 2018

Guardiola31337 deleted the aa-optional-callback-param branch January 19, 2018 00:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MapboxTelemetry - Optional Callback #38

MapboxTelemetry - Optional Callback #38

electrostat commented Jan 3, 2018

Guardiola31337 left a comment

Guardiola31337 Jan 8, 2018

Guardiola31337 Jan 8, 2018

Guardiola31337 Jan 9, 2018

Guardiola31337 Jan 9, 2018

Guardiola31337 Jan 9, 2018

Guardiola31337 left a comment •

edited

Loading

Guardiola31337 Jan 15, 2018

Guardiola31337 Jan 15, 2018

Guardiola31337 Jan 15, 2018

Guardiola31337 Jan 15, 2018

Guardiola31337 Jan 15, 2018

Guardiola31337 Jan 15, 2018

Guardiola31337 Jan 15, 2018

Guardiola31337 Jan 15, 2018 •

edited

Loading

Guardiola31337 Jan 15, 2018 •

edited

Loading

Guardiola31337 Jan 15, 2018 •

edited

Loading

Guardiola31337 Jan 15, 2018 •

edited

Loading

Guardiola31337 Jan 15, 2018 •

edited

Loading

Guardiola31337 Jan 15, 2018

Guardiola31337 Jan 15, 2018

Guardiola31337 Jan 15, 2018

Guardiola31337 left a comment

		initializeQueue();
		AlarmReceiver alarmReceiver = obtainAlarmReceiver(httpCallback);


		public interface TelemetryListener {

		void onHttpResponse(Response response) throws IOException;

MapboxTelemetry - Optional Callback #38

MapboxTelemetry - Optional Callback #38

Conversation

electrostat commented Jan 3, 2018

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 • edited Loading

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 Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

Guardiola31337 Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

Guardiola31337 Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

Guardiola31337 Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

Guardiola31337 Jan 15, 2018 • edited Loading

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

Guardiola31337 left a comment •

edited

Loading

Guardiola31337 Jan 15, 2018 •

edited

Loading

Guardiola31337 Jan 15, 2018 •

edited

Loading

Guardiola31337 Jan 15, 2018 •

edited

Loading

Guardiola31337 Jan 15, 2018 •

edited

Loading

Guardiola31337 Jan 15, 2018 •

edited

Loading