Fix connection state (#130)

* Update connectState before invoking connectionCallbacks

* Rm unnecessary semicolon

* Extract all connection related logic into testable connection manager class

* Add tests coverage for remaining FusedLocationProviderApiImpl methods

* Test coverage for FusedLocationServiceConnectionManager

* Update state in connection manager before invoking callbacks

* Remove unnecessary parameter from disconnect method

lost/src/main/java/com/mapzen/android/lost/api/LocationServices.java

@@ -2,6 +2,7 @@
 
 import com.mapzen.android.lost.internal.DwellServiceIntentFactory;
 import com.mapzen.android.lost.internal.FusedLocationProviderApiImpl;
+import com.mapzen.android.lost.internal.FusedLocationServiceConnectionManager;
 import com.mapzen.android.lost.internal.GeofencingApiImpl;
 import com.mapzen.android.lost.internal.GeofencingServiceIntentFactory;
 import com.mapzen.android.lost.internal.PendingIntentIdGenerator;
@@ -16,7 +17,7 @@ public class LocationServices {
    * Entry point for APIs concerning location updates.
    */
   public static final FusedLocationProviderApi FusedLocationApi =
-      new FusedLocationProviderApiImpl();
+      new FusedLocationProviderApiImpl(new FusedLocationServiceConnectionManager());
 
   /**
    * Entry point for APIs concerning geofences.

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

@@ -1,15 +1,5 @@
 package com.mapzen.android.lost.internal;
 
-import android.app.PendingIntent;
-import android.content.ComponentName;
-import android.content.Context;
-import android.content.Intent;
-import android.content.ServiceConnection;
-import android.location.Location;
-import android.os.IBinder;
-import android.os.Looper;
-import android.util.Log;
-
 import com.mapzen.android.lost.api.FusedLocationProviderApi;
 import com.mapzen.android.lost.api.LocationAvailability;
 import com.mapzen.android.lost.api.LocationCallback;
@@ -19,8 +9,16 @@
 import com.mapzen.android.lost.api.PendingResult;
 import com.mapzen.android.lost.api.Status;
 
+import android.app.PendingIntent;
+import android.content.ComponentName;
+import android.content.Context;
+import android.content.Intent;
+import android.content.ServiceConnection;
+import android.location.Location;
+import android.os.IBinder;
+import android.os.Looper;
+
 import java.io.File;
-import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
 
@@ -30,95 +28,74 @@
 public class FusedLocationProviderApiImpl
     implements FusedLocationProviderApi {
 
-  private static final String TAG = FusedLocationProviderApiImpl.class.getSimpleName();
-
   private Context context;
   private FusedLocationProviderService service;
-  private enum ConnectState { IDLE, CONNECTING, CONNECTED };
-  private ConnectState connectState;
+  private FusedLocationServiceConnectionManager serviceConnectionManager;
 
+  private FusedLocationServiceConnectionManager.EventCallbacks eventCallbacks =
+      new FusedLocationServiceConnectionManager.EventCallbacks() {
+    @Override public void onConnect(Context context) {
+      FusedLocationProviderApiImpl.this.context = context;
 
-  private final ServiceConnection serviceConnection = new ServiceConnection() {
-    @Override public void onServiceConnected(ComponentName name, IBinder binder) {
+      Intent intent = new Intent(context, FusedLocationProviderService.class);
+      context.startService(intent);
+
+      intent = new Intent(context, FusedLocationProviderService.class);
+      context.bindService(intent, serviceConnection, Context.BIND_AUTO_CREATE);
+    }
+
+    @Override public void onServiceConnected(IBinder binder) {
       FusedLocationProviderService.FusedLocationProviderBinder fusedBinder =
           (FusedLocationProviderService.FusedLocationProviderBinder) binder;
-      if (connectState != ConnectState.IDLE) {
-        if (fusedBinder != null) {
-          service = fusedBinder.getService();
-        }
-
-        if (!connectionCallbacks.isEmpty()) {
-          for (LostApiClient.ConnectionCallbacks callbacks : connectionCallbacks) {
-            callbacks.onConnected();
-          }
-        }
-        connectState = ConnectState.CONNECTED;
+      if (fusedBinder != null) {
+        service = fusedBinder.getService();
       }
-      Log.d(TAG, "[onServiceConnected]");
     }
 
-    @Override public void onServiceDisconnected(ComponentName name) {
-      if (connectState != ConnectState.IDLE) {
-        if (!connectionCallbacks.isEmpty()) {
-          for (LostApiClient.ConnectionCallbacks callbacks : connectionCallbacks) {
-            callbacks.onConnectionSuspended();
-          }
-        }
-        connectState = ConnectState.IDLE;
+    @Override public void onDisconnect(LostApiClient client, boolean disconnectService) {
+      if (disconnectService) {
+        service.disconnect(client);
       }
-      Log.d(TAG, "[onServiceDisconnected]");
+      context.unbindService(serviceConnection);
+      Intent intent = new Intent(context, FusedLocationProviderService.class);
+      context.stopService(intent);
+      service = null;
     }
   };
 
-  Set<LostApiClient.ConnectionCallbacks> connectionCallbacks;
+  private final ServiceConnection serviceConnection = new ServiceConnection() {
+    @Override public void onServiceConnected(ComponentName name, IBinder binder) {
+      serviceConnectionManager.onServiceConnected(binder);
+    }
 
+    @Override public void onServiceDisconnected(ComponentName name) {
+      serviceConnectionManager.onServiceDisconnected();
+    }
+  };
 
-  public boolean isConnecting() {
-    return connectState == ConnectState.CONNECTING;
+  public FusedLocationProviderApiImpl(FusedLocationServiceConnectionManager connectionManager) {
+    serviceConnectionManager = connectionManager;
+    serviceConnectionManager.setEventCallbacks(eventCallbacks);
   }
 
-  public FusedLocationProviderApiImpl() {
-    connectionCallbacks = new HashSet<>();
-    connectState = ConnectState.IDLE;
+  public boolean isConnecting() {
+    return serviceConnectionManager.isConnecting();
   }
 
-  public void connect(Context context, LostApiClient.ConnectionCallbacks callbacks) {
-    if (connectState == ConnectState.IDLE) {
-      this.context = context;
-      connectState = ConnectState.CONNECTING;
-
-      Intent intent = new Intent(context, FusedLocationProviderService.class);
-      context.startService(intent);
-
-      intent = new Intent(context, FusedLocationProviderService.class);
-      context.bindService(intent, serviceConnection, Context.BIND_AUTO_CREATE);
-    }
-    if (callbacks != null) {
-      connectionCallbacks.add(callbacks);
-    }
+  public void addConnectionCallbacks(LostApiClient.ConnectionCallbacks callbacks) {
+    serviceConnectionManager.addCallbacks(callbacks);
   }
 
-  public void disconnect() {
-    disconnect(null, true);
+  public void connect(Context context, LostApiClient.ConnectionCallbacks callbacks) {
+    serviceConnectionManager.connect(context, callbacks);
   }
 
-  public void disconnect(LostApiClient client, boolean stopService) {
-    if (connectState != ConnectState.IDLE) {
-      if (connectState == ConnectState.CONNECTED) {
-        service.disconnect(client);
-      }
-      connectState = ConnectState.IDLE;
-      if (stopService) {
-        context.unbindService(serviceConnection);
-        Intent intent = new Intent(context, FusedLocationProviderService.class);
-        context.stopService(intent);
-        service = null;
-      }
-    }
+  public void disconnect(LostApiClient client) {
+    serviceConnectionManager.disconnect(client);
   }
 
   public boolean isConnected() {
-    return connectState == ConnectState.CONNECTED;
+    return serviceConnectionManager.isConnected();
   }
 
   @Override public Location getLastLocation(LostApiClient client) {

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

@@ -0,0 +1,95 @@
+package com.mapzen.android.lost.internal;
+
+import com.mapzen.android.lost.api.LostApiClient;
+import com.mapzen.android.lost.api.LostApiClient.ConnectionCallbacks;
+
+import android.content.Context;
+import android.os.IBinder;
+
+import java.util.HashSet;
+import java.util.Set;
+
+public class FusedLocationServiceConnectionManager {
+
+  public interface EventCallbacks {
+    void onConnect(Context context);
+    void onServiceConnected(IBinder binder);
+    void onDisconnect(LostApiClient client, boolean disconnectService);
+  }
+
+  private enum ConnectState { IDLE, CONNECTING, CONNECTED }
+
+  private EventCallbacks eventCallbacks;
+  private ConnectState connectState;
+  Set<ConnectionCallbacks> connectionCallbacks;
+
+  public FusedLocationServiceConnectionManager() {
+    connectionCallbacks = new HashSet<>();
+    connectState = ConnectState.IDLE;
+  }
+
+  public void setEventCallbacks(EventCallbacks callbacks) {
+    eventCallbacks = callbacks;
+  }
+
+  public void addCallbacks(ConnectionCallbacks callbacks) {
+    if (callbacks != null) {
+      connectionCallbacks.add(callbacks);
+    }
+  }
+
+  public boolean isConnected() {
+    return connectState == ConnectState.CONNECTED;
+  }
+
+  public boolean isConnecting() {
+    return connectState == ConnectState.CONNECTING;
+  }
+
+  public void connect(Context context, ConnectionCallbacks callbacks) {
+    if (connectState == ConnectState.IDLE) {
+      connectState = ConnectState.CONNECTING;
+
+      if (eventCallbacks != null) {
+        eventCallbacks.onConnect(context);
+      }
+    }
+    addCallbacks(callbacks);
+  }
+
+  public void disconnect(LostApiClient client) {
+    if (connectState != ConnectState.IDLE) {
+      boolean disconnectService = (connectState == ConnectState.CONNECTED);
+      connectState = ConnectState.IDLE;
+      if (eventCallbacks != null) {
+        eventCallbacks.onDisconnect(client, disconnectService);
+      }
+    }
+  }
+
+  public void onServiceConnected(IBinder binder) {
+    if (connectState != ConnectState.IDLE) {
+      connectState = ConnectState.CONNECTED;
+      if (eventCallbacks != null) {
+        eventCallbacks.onServiceConnected(binder);
+      }
+
+      if (!connectionCallbacks.isEmpty()) {
+        for (LostApiClient.ConnectionCallbacks callbacks : connectionCallbacks) {
+          callbacks.onConnected();
+        }
+      }
+    }
+  }
+
+  public void onServiceDisconnected() {
+    if (connectState != ConnectState.IDLE) {
+      connectState = ConnectState.IDLE;
+      if (!connectionCallbacks.isEmpty()) {
+        for (LostApiClient.ConnectionCallbacks callbacks : connectionCallbacks) {
+          callbacks.onConnectionSuspended();
+        }
+      }
+    }
+  }
+}

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

@@ -34,7 +34,7 @@ public LostApiClientImpl(Context context, ConnectionCallbacks callbacks) {
       }
     } else if (fusedApi.isConnecting()) {
       if (connectionCallbacks != null) {
-        fusedApi.connectionCallbacks.add(connectionCallbacks);
+        fusedApi.addConnectionCallbacks(connectionCallbacks);
       }
     } else {
       fusedApi.connect(context, connectionCallbacks);
@@ -50,7 +50,7 @@ public LostApiClientImpl(Context context, ConnectionCallbacks callbacks) {
 
     getSettingsApiImpl().disconnect();
     getGeofencingImpl().disconnect();
-    getFusedLocationProviderApiImpl().disconnect();
+    getFusedLocationProviderApiImpl().disconnect(null);
   }
 
   @Override public boolean isConnected() {

lost/src/test/java/com/mapzen/android/lost/internal/FusedLocationProviderApiImplTest.java

@@ -14,13 +14,16 @@
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.Mockito;
 import org.robolectric.RobolectricGradleTestRunner;
 import org.robolectric.annotation.Config;
 
 import java.io.File;
 
 import static org.fest.assertions.api.Assertions.assertThat;
+import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 import static org.robolectric.RuntimeEnvironment.application;
@@ -35,14 +38,20 @@ public class FusedLocationProviderApiImplTest {
   private LostApiClient secondClient;
   private FusedLocationProviderApiImpl api;
   private FusedLocationProviderService service;
+  private FusedLocationServiceConnectionManager connectionManager;
 
   @Before public void setUp() throws Exception {
     mockService();
     client = new LostApiClient.Builder(mock(Context.class)).build();
 
     // do not call connect on this!
     secondClient = new LostApiClient.Builder(mock(Context.class)).build();
-    api = new FusedLocationProviderApiImpl();
+    connectionManager = spy(new FusedLocationServiceConnectionManager());
+    Mockito.doCallRealMethod().when(connectionManager).setEventCallbacks(any(
+        FusedLocationServiceConnectionManager.EventCallbacks.class));
+    Mockito.doCallRealMethod().when(connectionManager).connect(any(Context.class), any(
+        LostApiClient.ConnectionCallbacks.class));
+    api = new FusedLocationProviderApiImpl(connectionManager);
     api.connect(application, null);
     service = api.getService();
   }
@@ -61,6 +70,40 @@ private void mockService() {
     assertThat(!secondClient.isConnected());
   }
 
+  @Test public void shouldSetEventCallbacks() {
+    verify(connectionManager).setEventCallbacks(any(
+        FusedLocationServiceConnectionManager.EventCallbacks.class));
+  }
+
+  @Test public void isConnecting_shouldCallConnectionManager() {
+    api.isConnecting();
+    verify(connectionManager).isConnecting();
+  }
+
+  @Test public void addConnectionCallbacks_shouldCallConnectionManager() {
+    TestConnectionCallbacks callbacks = new TestConnectionCallbacks();
+    api.addConnectionCallbacks(callbacks);
+    verify(connectionManager).addCallbacks(callbacks);
+  }
+
+  @Test public void connect_shouldCallConnectionManager() {
+    Context context = mock(Context.class);
+    TestConnectionCallbacks callbacks = new TestConnectionCallbacks();
+    api.connect(context, callbacks);
+    verify(connectionManager).connect(context, callbacks);
+  }
+
+  @Test public void disconnect_shouldCallConnectionManager() {
+    LostApiClient client = mock(LostApiClient.class);
+    api.disconnect(client);
+    verify(connectionManager).disconnect(client);
+  }
+
+  @Test public void isConnected_shouldCallConnectionManager() {
+    api.isConnected();
+    verify(connectionManager).isConnected();
+  }
+
   @Test public void getLastLocation_shouldCallService() {
     api.getLastLocation(client);
     verify(service).getLastLocation(client);
@@ -138,5 +181,4 @@ public void requestLocationUpdates_shouldThrowException() {
     api.getLocationListeners();
     verify(service).getLocationListeners();
   }
-
 }