Connection callbacks not stored if fused location service is already connected

[ecgreb]

Description

While debugging https://github.com/mapzen/lost/issues/162 I found that we are not properly handling ConnectionCallbacks when the fused location service is already connected.

When a new LostApiClient is connected we check if the service is already running. If it is, then the ConnectionCallbacks.onConnected() is invoked but the callbacks are not stored in the FusedLocationServiceConnectionManager.

LostApiClientImpl.java

  @Override public void connect() {
    ...
    FusedLocationProviderApiImpl fusedApi = getFusedLocationProviderApiImpl();
    if (fusedApi.isConnected()) {
      if (connectionCallbacks != null) {
        connectionCallbacks.onConnected();
      }
    } else if (fusedApi.isConnecting()) {
      if (connectionCallbacks != null) {
        fusedApi.addConnectionCallbacks(connectionCallbacks);
      }
    } else {
      fusedApi.connect(context, connectionCallbacks);
    }
  }

This is potentially problematic if the service is suspended by the system and FusedLocationServiceManager.onServiceDisconnected() fires these clients will never be notified via ConnectionCallbacks.onConnectionSuspended(), nor will ConnectionCallbacks.onConnected() be invoked when the service is reconnected.

From the docs for the ServiceConnection interface:

Called when a connection to the Service has been lost. This typically happens when the process hosting the service has crashed or been killed. This does not remove the ServiceConnection itself -- this binding to the service will remain active, and you will receive a call to onServiceConnected when the Service is next running.

Steps to Reproduce

1. Connect one instance of LostApiClient.
2. In the ConnectionCallbacks instance for the first client create and connect a new LostApiClient instance with a new instance of ConnectionCallbacks.
3. Using the debugger you can see that FusedLocationServiceConnectionManager.connectionCallbacks only contains the original set of ConnectionCallbacks as the second set has not been added.

Solution

fusedApi.addConnectionCallbacks(connectionCallbacks) should be called for all connection callbacks even if the FusedLocationApi is already connected.

Lost & Android Version

Lost 2.2.0-rc1