Refactor ClientManager (#244)

* remove ReportedChanges

* add documentation

build.gradle

@@ -7,7 +7,7 @@ buildscript {
   }
 
   dependencies {
-    classpath 'com.android.tools.build:gradle:2.3.2'
+    classpath 'com.android.tools.build:gradle:2.3.3'
     classpath 'com.github.dcendents:android-maven-gradle-plugin:1.5'
   }
 }

lost-sample/build.gradle

@@ -4,7 +4,7 @@ buildscript {
   }
 
   dependencies {
-    classpath 'com.android.tools.build:gradle:2.3.2'
+    classpath 'com.android.tools.build:gradle:2.3.3'
   }
 }
 

lost/build.gradle

@@ -3,7 +3,7 @@ buildscript {
     jcenter()
   }
   dependencies {
-    classpath 'com.android.tools.build:gradle:2.3.2'
+    classpath 'com.android.tools.build:gradle:2.3.3'
     classpath 'com.github.dcendents:android-maven-gradle-plugin:1.3'
     classpath 'net.researchgate:gradle-release:2.4.0'
   }

lost/src/main/java/com/mapzen/android/lost/internal/AndroidHandlerFactory.java

@@ -0,0 +1,15 @@
+package com.mapzen.android.lost.internal;
+
+import android.os.Handler;
+import android.os.Looper;
+
+/**
+ * Concrete implementation of {@link HandlerFactory} used by {@link LostClientManager}.
+ */
+class AndroidHandlerFactory implements HandlerFactory {
+
+  @Override public void run(Looper looper, Runnable runnable) {
+    Handler handler = new Handler(looper);
+    handler.post(runnable);
+  }
+}

lost/src/main/java/com/mapzen/android/lost/internal/ClientManager.java

@@ -34,14 +34,12 @@ void addLocationCallback(LostApiClient client, LocationRequest request,
   boolean removeListener(LostApiClient client, LocationListener listener);
   boolean removePendingIntent(LostApiClient client, PendingIntent callbackIntent);
   boolean removeLocationCallback(LostApiClient client, LocationCallback callback);
-  ReportedChanges reportLocationChanged(Location location);
-  ReportedChanges sendPendingIntent(Context context, Location location,
+  void reportLocationChanged(Location location);
+  void sendPendingIntent(Context context, Location location,
       LocationAvailability availability, LocationResult result);
-  ReportedChanges reportLocationResult(Location location, final LocationResult result);
-  void updateReportedValues(ReportedChanges changes);
+  void reportLocationResult(Location location, final LocationResult result);
   void notifyLocationAvailability(final LocationAvailability availability);
   boolean hasNoListeners();
-  void shutdown();
   Map<LostApiClient, Set<LocationListener>> getLocationListeners();
   Map<LostApiClient, Set<PendingIntent>> getPendingIntents();
   Map<LostApiClient, Set<LocationCallback>> getLocationCallbacks();

lost/src/main/java/com/mapzen/android/lost/internal/Clock.java

@@ -1,5 +1,10 @@
 package com.mapzen.android.lost.internal;
 
+import android.location.Location;
+
 public interface Clock {
-  long getCurrentTimeInMillis();
+  long MS_TO_NS = 1000000;
+
+  long getSystemElapsedTimeInNanos();
+  long getElapsedTimeInNanos(Location location);
 }

lost/src/main/java/com/mapzen/android/lost/internal/FusedLocationProviderApiImpl.java

@@ -87,7 +87,6 @@ public void onLocationChanged(final Location location) throws RemoteException {
       context.unbindService(this);
       isBound = false;
     }
-    clientManager.shutdown();
     service = null;
   }
 

lost/src/main/java/com/mapzen/android/lost/internal/FusedLocationServiceCallbackManager.java

@@ -30,7 +30,7 @@ public class FusedLocationServiceCallbackManager {
   void onLocationChanged(Context context, Location location, ClientManager clientManager,
       IFusedLocationProviderService service) {
 
-    ReportedChanges changes = clientManager.reportLocationChanged(location);
+    clientManager.reportLocationChanged(location);
 
     LocationAvailability availability = null;
     try {
@@ -42,14 +42,9 @@ void onLocationChanged(Context context, Location location, ClientManager clientM
     ArrayList<Location> locations = new ArrayList<>();
     locations.add(location);
     final LocationResult result = LocationResult.create(locations);
-    ReportedChanges pendingIntentChanges = clientManager.sendPendingIntent(
-        context, location, availability, result);
+    clientManager.sendPendingIntent(context, location, availability, result);
 
-    ReportedChanges callbackChanges = clientManager.reportLocationResult(location, result);
-
-    changes.putAll(pendingIntentChanges);
-    changes.putAll(callbackChanges);
-    clientManager.updateReportedValues(changes);
+    clientManager.reportLocationResult(location, result);
   }
 
   /**

lost/src/main/java/com/mapzen/android/lost/internal/FusionEngine.java

@@ -40,7 +40,7 @@ public FusionEngine(Context context, Callback callback) {
 
   @Override public Location getLastLocation() {
     final List<String> providers = locationManager.getAllProviders();
-    final long minTime = clock.getCurrentTimeInMillis() - RECENT_UPDATE_THRESHOLD_IN_MILLIS;
+    final long minTime = clock.getSystemElapsedTimeInNanos() - RECENT_UPDATE_THRESHOLD_IN_NANOS;
 
     Location bestLocation = null;
     float bestAccuracy = Float.MAX_VALUE;
@@ -51,7 +51,7 @@ public FusionEngine(Context context, Callback callback) {
         final Location location = locationManager.getLastKnownLocation(provider);
         if (location != null) {
           final float accuracy = location.getAccuracy();
-          final long time = location.getTime();
+          final long time = clock.getElapsedTimeInNanos(location);
           if (time > minTime && accuracy < bestAccuracy) {
             bestLocation = location;
             bestAccuracy = accuracy;
@@ -224,8 +224,8 @@ public static boolean isBetterThan(Location locationA, Location locationB) {
       return true;
     }
 
-    if (SystemClock.getTimeInNanos(locationA)
-        > SystemClock.getTimeInNanos(locationB) + RECENT_UPDATE_THRESHOLD_IN_NANOS) {
+    if (clock.getElapsedTimeInNanos(locationA)
+        > clock.getElapsedTimeInNanos(locationB) + RECENT_UPDATE_THRESHOLD_IN_NANOS) {
       return true;
     }
 

lost/src/main/java/com/mapzen/android/lost/internal/HandlerFactory.java

@@ -0,0 +1,11 @@
+package com.mapzen.android.lost.internal;
+
+import android.os.Looper;
+
+/**
+ * Generic interface for {@link AndroidHandlerFactory} which can be seamlessly replaced for test
+ * class when running test suite.
+ */
+interface HandlerFactory {
+  void run(Looper looper, Runnable runnable);
+}

lost/src/main/java/com/mapzen/android/lost/internal/LocationRequestReport.java

@@ -0,0 +1,21 @@
+package com.mapzen.android.lost.internal;
+
+import com.mapzen.android.lost.api.LocationRequest;
+
+import android.location.Location;
+
+/**
+ * Wrapper object used to store info about when a {@link Location} was last reported for a given
+ * {@link LocationRequest}. Used by {@link LostClientManager} to determine if a new location should
+ * should be reported to a given client listener.
+ */
+class LocationRequestReport {
+
+  Location location;
+  final LocationRequest locationRequest;
+
+  LocationRequestReport(LocationRequest request) {
+    this.locationRequest = request;
+  }
+
+}

lost/src/main/java/com/mapzen/android/lost/internal/LostClientManager.java

@@ -21,6 +21,7 @@
 import java.util.Set;
 
 import static com.mapzen.android.lost.api.FusedLocationProviderApi.KEY_LOCATION_CHANGED;
+import static com.mapzen.android.lost.internal.Clock.MS_TO_NS;
 
 /**
  * Used by {@link LostApiClientImpl} to manage connected clients and by
@@ -31,27 +32,27 @@ public class LostClientManager implements ClientManager {
   private static final String TAG = ClientManager.class.getSimpleName();
 
   private static LostClientManager instance;
+  private final Clock clock;
+  private final HandlerFactory handlerFactory;
+
   private HashMap<LostApiClient, LostClientWrapper> clients;
+  private Map<LocationListener, LocationRequestReport> listenerToReports;
+  private Map<PendingIntent, LocationRequestReport> intentToReports;
+  private Map<LocationCallback, LocationRequestReport> callbackToReports;
 
-  private Map<LocationListener, LocationRequest> listenerToLocationRequests;
-  private Map<PendingIntent, LocationRequest> intentToLocationRequests;
-  private Map<LocationCallback, LocationRequest> callbackToLocationRequests;
-  private ReportedChanges reportedChanges;
+  LostClientManager(Clock clock, HandlerFactory handlerFactory) {
+    this.clock = clock;
+    this.handlerFactory = handlerFactory;
 
-  LostClientManager() {
     clients = new HashMap<>();
-
-    listenerToLocationRequests = new HashMap<>();
-    intentToLocationRequests = new HashMap<>();
-    callbackToLocationRequests = new HashMap<>();
-
-    reportedChanges = new ReportedChanges(new HashMap<LocationRequest, Long>(),
-        new HashMap<LocationRequest, Location>());
+    listenerToReports = new HashMap<>();
+    intentToReports = new HashMap<>();
+    callbackToReports = new HashMap<>();
   }
 
   public static LostClientManager shared() {
     if (instance == null) {
-      instance = new LostClientManager();
+      instance = new LostClientManager(new SystemClock(), new AndroidHandlerFactory());
     }
     return instance;
   }
@@ -76,22 +77,22 @@ public static LostClientManager shared() {
       LocationListener listener) {
     throwIfClientNotAdded(client);
     clients.get(client).locationListeners().add(listener);
-    listenerToLocationRequests.put(listener, request);
+    listenerToReports.put(listener, new LocationRequestReport(request));
   }
 
   @Override public void addPendingIntent(LostApiClient client, LocationRequest request,
       PendingIntent callbackIntent) {
     throwIfClientNotAdded(client);
     clients.get(client).pendingIntents().add(callbackIntent);
-    intentToLocationRequests.put(callbackIntent, request);
+    intentToReports.put(callbackIntent, new LocationRequestReport(request));
   }
 
   @Override public void addLocationCallback(LostApiClient client, LocationRequest request,
       LocationCallback callback, Looper looper) {
     throwIfClientNotAdded(client);
     clients.get(client).locationCallbacks().add(callback);
     clients.get(client).looperMap().put(callback, looper);
-    callbackToLocationRequests.put(callback, request);
+    callbackToReports.put(callback, new LocationRequestReport(request));
   }
 
   private void throwIfClientNotAdded(LostApiClient client) {
@@ -109,7 +110,7 @@ private void throwIfClientNotAdded(LostApiClient client) {
       removed = true;
     }
 
-    listenerToLocationRequests.remove(listener);
+    listenerToReports.remove(listener);
     return removed;
   }
 
@@ -122,7 +123,7 @@ private void throwIfClientNotAdded(LostApiClient client) {
       removed = true;
     }
 
-    intentToLocationRequests.remove(callbackIntent);
+    intentToReports.remove(callbackIntent);
     return removed;
   }
 
@@ -136,7 +137,7 @@ private void throwIfClientNotAdded(LostApiClient client) {
     }
 
     clients.get(client).looperMap().remove(callback);
-    callbackToLocationRequests.remove(callback);
+    callbackToReports.remove(callback);
     return removed;
   }
 
@@ -149,8 +150,8 @@ private void throwIfClientNotAdded(LostApiClient client) {
    * @param location
    * @return
    */
-  @Override public ReportedChanges reportLocationChanged(final Location location) {
-    return iterateAndNotify(location, getLocationListeners(), listenerToLocationRequests,
+  @Override public void reportLocationChanged(final Location location) {
+    iterateAndNotify(location, getLocationListeners(), listenerToReports,
         new Notifier<LocationListener>() {
           @Override public void notify(LostApiClient client, LocationListener listener) {
             listener.onLocationChanged(location);
@@ -167,31 +168,27 @@ private void throwIfClientNotAdded(LostApiClient client) {
    * @param location
    * @return
    */
-  @Override public ReportedChanges sendPendingIntent(final Context context,
+  @Override public void sendPendingIntent(final Context context,
       final Location location, final LocationAvailability availability,
       final LocationResult result) {
-    return iterateAndNotify(location,
-        getPendingIntents(), intentToLocationRequests, new Notifier<PendingIntent>() {
+    iterateAndNotify(location,
+        getPendingIntents(), intentToReports, new Notifier<PendingIntent>() {
           @Override public void notify(LostApiClient client, PendingIntent pendingIntent) {
             fireIntent(context, pendingIntent, location, availability, result);
           }
         });
   }
 
-  @Override public ReportedChanges reportLocationResult(Location location,
+  @Override public void reportLocationResult(Location location,
       final LocationResult result) {
-    return iterateAndNotify(location,
-        getLocationCallbacks(), callbackToLocationRequests, new Notifier<LocationCallback>() {
+    iterateAndNotify(location,
+        getLocationCallbacks(), callbackToReports, new Notifier<LocationCallback>() {
           @Override public void notify(LostApiClient client, LocationCallback callback) {
             notifyCallback(client, callback, result);
           }
         });
   }
 
-  @Override public void updateReportedValues(ReportedChanges changes) {
-    reportedChanges.putAll(changes);
-  }
-
   @Override public void notifyLocationAvailability(final LocationAvailability availability) {
     for (LostClientWrapper wrapper : clients.values()) {
       for (LocationCallback callback : wrapper.locationCallbacks()) {
@@ -212,10 +209,6 @@ private void throwIfClientNotAdded(LostApiClient client) {
     return true;
   }
 
-  @Override public void shutdown() {
-    reportedChanges.clearAll();
-  }
-
   @VisibleForTesting void clearClients() {
     clients.clear();
   }
@@ -261,8 +254,7 @@ private void fireIntent(Context context, PendingIntent intent, Location location
   private void notifyCallback(LostApiClient client, final LocationCallback callback,
       final LocationResult result) {
     final Looper looper = clients.get(client).looperMap().get(callback);
-    final Handler handler = new Handler(looper);
-    handler.post(new Runnable() {
+    handlerFactory.run(looper, new Runnable() {
       @Override public void run() {
         callback.onLocationResult(result);
       }
@@ -280,37 +272,34 @@ private void notifyAvailability(LostApiClient client, final LocationCallback cal
     });
   }
 
-  private <T> ReportedChanges iterateAndNotify(Location location,
-      Map<LostApiClient, Set<T>> clientToObj, Map<T, LocationRequest> objToLocationRequest,
+  private <T> void iterateAndNotify(Location location,
+      Map<LostApiClient, Set<T>> clientToObj, Map<T, LocationRequestReport> objToLocationRequest,
       Notifier notifier) {
-    Map<LocationRequest, Long> updatedRequestToReportedTime = new HashMap<>();
-    Map<LocationRequest, Location> updatedRequestToReportedLocation = new HashMap<>();
     for (LostApiClient client : clientToObj.keySet()) {
       if (clientToObj.get(client) != null) {
         for (final T obj : clientToObj.get(client)) {
-          LocationRequest request = objToLocationRequest.get(obj);
-          Long lastReportedTime = reportedChanges.timeChanges().get(request);
-          Location lastReportedLocation = reportedChanges.locationChanges().get(request);
-          if (lastReportedTime == null && lastReportedLocation == null) {
-            updatedRequestToReportedTime.put(request, location.getTime());
-            updatedRequestToReportedLocation.put(request, location);
+          LocationRequestReport report = objToLocationRequest.get(obj);
+          LocationRequest request = report.locationRequest;
+          Location lastReportedLocation = report.location;
+          if (lastReportedLocation == null) {
+            report.location = location;
             notifier.notify(client, obj);
           } else {
-            long intervalSinceLastReport = location.getTime() - lastReportedTime;
+            long lastReportedTime = clock.getElapsedTimeInNanos(lastReportedLocation);
+            long locationTime = clock.getElapsedTimeInNanos(location);
+            long intervalSinceLastReport = (locationTime - lastReportedTime) / MS_TO_NS;
             long fastestInterval = request.getFastestInterval();
             float smallestDisplacement = request.getSmallestDisplacement();
             float displacementSinceLastReport = location.distanceTo(lastReportedLocation);
             if (intervalSinceLastReport >= fastestInterval &&
                 displacementSinceLastReport >= smallestDisplacement) {
-              updatedRequestToReportedTime.put(request, location.getTime());
-              updatedRequestToReportedLocation.put(request, location);
+              report.location = location;
               notifier.notify(client, obj);
             }
           }
         }
       }
     }
-    return new ReportedChanges(updatedRequestToReportedTime, updatedRequestToReportedLocation);
   }
 
   interface Notifier<T> {