From f9d8b9ccbb857ccf9d3ea884a9898751ca93dd06 Mon Sep 17 00:00:00 2001 From: Chuck Greb Date: Sat, 8 Apr 2017 00:05:09 +0200 Subject: [PATCH] Simplify service shutdown to prevent crash on screen rotation (#180) --- ...eLocationListenerSingleClientActivity.java | 4 +- .../android/lost/internal/ClientManager.java | 1 - .../FusedLocationProviderService.java | 5 --- .../FusedLocationProviderServiceImpl.java | 5 --- .../lost/internal/LostClientManager.java | 45 ++++++++++--------- .../FusedLocationProviderServiceImplTest.java | 23 ---------- .../lost/internal/LostClientManagerTest.java | 23 +--------- 7 files changed, 27 insertions(+), 79 deletions(-) diff --git a/lost-sample/src/main/java/com/example/lost/MultipleLocationListenerSingleClientActivity.java b/lost-sample/src/main/java/com/example/lost/MultipleLocationListenerSingleClientActivity.java index c668582..7939c5a 100644 --- a/lost-sample/src/main/java/com/example/lost/MultipleLocationListenerSingleClientActivity.java +++ b/lost-sample/src/main/java/com/example/lost/MultipleLocationListenerSingleClientActivity.java @@ -101,7 +101,7 @@ private void initLocationTracking() { return; } - long interval = 3 * 60 * 1000; // 3 minutes + long interval = 30 * 1000; // 30 seconds LocationRequest request = LocationRequest.create() .setPriority(LocationRequest.PRIORITY_HIGH_ACCURACY) .setFastestInterval(interval) @@ -109,7 +109,7 @@ private void initLocationTracking() { LocationServices.FusedLocationApi.requestLocationUpdates(lostApiClient, request, listener); - interval = 30 * 1000; // 30 seconds + interval = 15 * 1000; // 15 seconds request = LocationRequest.create() .setPriority(LocationRequest.PRIORITY_HIGH_ACCURACY) .setFastestInterval(interval) diff --git a/lost/src/main/java/com/mapzen/android/lost/internal/ClientManager.java b/lost/src/main/java/com/mapzen/android/lost/internal/ClientManager.java index fc664b8..e440187 100644 --- a/lost/src/main/java/com/mapzen/android/lost/internal/ClientManager.java +++ b/lost/src/main/java/com/mapzen/android/lost/internal/ClientManager.java @@ -43,7 +43,6 @@ ReportedChanges sendPendingIntent(Context context, Location location, void reportProviderDisabled(String provider); void notifyLocationAvailability(final LocationAvailability availability); boolean hasNoListeners(); - void shutdown(); Map> getLocationListeners(); Map> getPendingIntents(); Map> getLocationCallbacks(); diff --git a/lost/src/main/java/com/mapzen/android/lost/internal/FusedLocationProviderService.java b/lost/src/main/java/com/mapzen/android/lost/internal/FusedLocationProviderService.java index b8926b2..8c6ede6 100644 --- a/lost/src/main/java/com/mapzen/android/lost/internal/FusedLocationProviderService.java +++ b/lost/src/main/java/com/mapzen/android/lost/internal/FusedLocationProviderService.java @@ -50,11 +50,6 @@ public FusedLocationProviderService getService() { serviceImpl = new FusedLocationProviderServiceImpl(this, LostClientManager.shared()); } - @Override public void onDestroy() { - super.onDestroy(); - serviceImpl.shutdown(); - } - public Location getLastLocation(LostApiClient client) { return serviceImpl.getLastLocation(client); } diff --git a/lost/src/main/java/com/mapzen/android/lost/internal/FusedLocationProviderServiceImpl.java b/lost/src/main/java/com/mapzen/android/lost/internal/FusedLocationProviderServiceImpl.java index 2e11943..2619d4a 100644 --- a/lost/src/main/java/com/mapzen/android/lost/internal/FusedLocationProviderServiceImpl.java +++ b/lost/src/main/java/com/mapzen/android/lost/internal/FusedLocationProviderServiceImpl.java @@ -40,11 +40,6 @@ public FusedLocationProviderServiceImpl(Context context, ClientManager manager) locationEngine = new FusionEngine(context, this); } - public void shutdown() { - locationEngine.setRequest(null); - clientManager.shutdown(); - } - public Location getLastLocation(LostApiClient client) { return locationEngine.getLastLocation(); } diff --git a/lost/src/main/java/com/mapzen/android/lost/internal/LostClientManager.java b/lost/src/main/java/com/mapzen/android/lost/internal/LostClientManager.java index af4a728..860d39c 100644 --- a/lost/src/main/java/com/mapzen/android/lost/internal/LostClientManager.java +++ b/lost/src/main/java/com/mapzen/android/lost/internal/LostClientManager.java @@ -13,6 +13,7 @@ import android.location.Location; import android.os.Handler; import android.os.Looper; +import android.support.annotation.VisibleForTesting; import android.util.Log; import java.util.HashMap; @@ -55,37 +56,37 @@ public static LostClientManager shared() { return instance; } - public void addClient(LostApiClient client) { + @Override public void addClient(LostApiClient client) { clients.put(client, new LostClientWrapper(client)); } - public void removeClient(LostApiClient client) { + @Override public void removeClient(LostApiClient client) { clients.remove(client); } - public boolean containsClient(LostApiClient client) { + @Override public boolean containsClient(LostApiClient client) { return clients.containsKey(client); } - public int numberOfClients() { + @Override public int numberOfClients() { return clients.size(); } - public void addListener(LostApiClient client, LocationRequest request, + @Override public void addListener(LostApiClient client, LocationRequest request, LocationListener listener) { throwIfClientNotAdded(client); clients.get(client).locationListeners().add(listener); listenerToLocationRequests.put(listener, request); } - public void addPendingIntent(LostApiClient client, LocationRequest request, + @Override public void addPendingIntent(LostApiClient client, LocationRequest request, PendingIntent callbackIntent) { throwIfClientNotAdded(client); clients.get(client).pendingIntents().add(callbackIntent); intentToLocationRequests.put(callbackIntent, request); } - public void addLocationCallback(LostApiClient client, LocationRequest request, + @Override public void addLocationCallback(LostApiClient client, LocationRequest request, LocationCallback callback, Looper looper) { throwIfClientNotAdded(client); clients.get(client).locationCallbacks().add(callback); @@ -99,7 +100,7 @@ private void throwIfClientNotAdded(LostApiClient client) { } } - public boolean removeListener(LostApiClient client, LocationListener listener) { + @Override public boolean removeListener(LostApiClient client, LocationListener listener) { final Set listeners = clients.get(client).locationListeners(); boolean removed = false; @@ -112,7 +113,7 @@ public boolean removeListener(LostApiClient client, LocationListener listener) { return removed; } - public boolean removePendingIntent(LostApiClient client, PendingIntent callbackIntent) { + @Override public boolean removePendingIntent(LostApiClient client, PendingIntent callbackIntent) { final Set pendingIntents = clients.get(client).pendingIntents(); boolean removed = false; @@ -125,7 +126,7 @@ public boolean removePendingIntent(LostApiClient client, PendingIntent callbackI return removed; } - public boolean removeLocationCallback(LostApiClient client, LocationCallback callback) { + @Override public boolean removeLocationCallback(LostApiClient client, LocationCallback callback) { final Set callbacks = clients.get(client).locationCallbacks(); boolean removed = false; @@ -148,7 +149,7 @@ public boolean removeLocationCallback(LostApiClient client, LocationCallback cal * @param location * @return */ - public ReportedChanges reportLocationChanged(final Location location) { + @Override public ReportedChanges reportLocationChanged(final Location location) { return iterateAndNotify(location, getLocationListeners(), listenerToLocationRequests, new Notifier() { @Override public void notify(LostApiClient client, LocationListener listener) { @@ -166,7 +167,7 @@ public ReportedChanges reportLocationChanged(final Location location) { * @param location * @return */ - public ReportedChanges sendPendingIntent(final Context context, + @Override public ReportedChanges sendPendingIntent(final Context context, final Location location, final LocationAvailability availability, final LocationResult result) { return iterateAndNotify(location, @@ -177,7 +178,7 @@ public ReportedChanges sendPendingIntent(final Context context, }); } - public ReportedChanges reportLocationResult(Location location, + @Override public ReportedChanges reportLocationResult(Location location, final LocationResult result) { return iterateAndNotify(location, getLocationCallbacks(), callbackToLocationRequests, new Notifier() { @@ -187,11 +188,11 @@ public ReportedChanges reportLocationResult(Location location, }); } - public void updateReportedValues(ReportedChanges changes) { + @Override public void updateReportedValues(ReportedChanges changes) { reportedChanges.putAll(changes); } - public void reportProviderEnabled(String provider) { + @Override public void reportProviderEnabled(String provider) { for (LostClientWrapper wrapper : clients.values()) { for (LocationListener listener : wrapper.locationListeners()) { listener.onProviderEnabled(provider); @@ -199,7 +200,7 @@ public void reportProviderEnabled(String provider) { } } - public void reportProviderDisabled(String provider) { + @Override public void reportProviderDisabled(String provider) { for (LostClientWrapper wrapper : clients.values()) { for (LocationListener listener : wrapper.locationListeners()) { listener.onProviderDisabled(provider); @@ -207,7 +208,7 @@ public void reportProviderDisabled(String provider) { } } - public void notifyLocationAvailability(final LocationAvailability availability) { + @Override public void notifyLocationAvailability(final LocationAvailability availability) { for (LostClientWrapper wrapper : clients.values()) { for (LocationCallback callback : wrapper.locationCallbacks()) { notifyAvailability(wrapper.client(), callback, availability); @@ -215,7 +216,7 @@ public void notifyLocationAvailability(final LocationAvailability availability) } } - public boolean hasNoListeners() { + @Override public boolean hasNoListeners() { for (LostClientWrapper wrapper : clients.values()) { if (!wrapper.locationListeners().isEmpty() || !wrapper.pendingIntents().isEmpty() @@ -227,11 +228,11 @@ public boolean hasNoListeners() { return true; } - public void shutdown() { + @VisibleForTesting void clearClients() { clients.clear(); } - public Map> getLocationListeners() { + @Override public Map> getLocationListeners() { final Map> clientToListeners = new HashMap<>(); for (LostApiClient client : clients.keySet()) { clientToListeners.put(client, clients.get(client).locationListeners()); @@ -240,7 +241,7 @@ public Map> getLocationListeners() { return clientToListeners; } - public Map> getPendingIntents() { + @Override public Map> getPendingIntents() { final Map> clientToPendingIntents = new HashMap<>(); for (LostApiClient client : clients.keySet()) { clientToPendingIntents.put(client, clients.get(client).pendingIntents()); @@ -249,7 +250,7 @@ public Map> getPendingIntents() { return clientToPendingIntents; } - public Map> getLocationCallbacks() { + @Override public Map> getLocationCallbacks() { final Map> clientToLocationCallbacks = new HashMap<>(); for (LostApiClient client : clients.keySet()) { clientToLocationCallbacks.put(client, clients.get(client).locationCallbacks()); diff --git a/lost/src/test/java/com/mapzen/android/lost/internal/FusedLocationProviderServiceImplTest.java b/lost/src/test/java/com/mapzen/android/lost/internal/FusedLocationProviderServiceImplTest.java index 59df3e5..db204ea 100644 --- a/lost/src/test/java/com/mapzen/android/lost/internal/FusedLocationProviderServiceImplTest.java +++ b/lost/src/test/java/com/mapzen/android/lost/internal/FusedLocationProviderServiceImplTest.java @@ -80,7 +80,6 @@ public class FusedLocationProviderServiceImplTest extends BaseRobolectricTest { @After public void tearDown() { client.disconnect(); otherClient.disconnect(); - clientManager.shutdown(); } private void mockService() { @@ -649,28 +648,6 @@ private File getTestGpxTrace() throws IOException { assertThat(shadowLocationManager.getRequestLocationUpdateListeners()).isEmpty(); } - @Test public void shutdown_shouldUnregisterLocationUpdateListeners() throws Exception { - api.requestLocationUpdates(client, LocationRequest.create(), - new TestLocationListener()); - - api.shutdown(); - assertThat(shadowLocationManager.getRequestLocationUpdateListeners()).isEmpty(); - } - - @Test public void shutdown_shouldClearListeners() { - api.requestLocationUpdates(client, LocationRequest.create(), - new TestLocationListener()); - api.shutdown(); - assertThat(api.getLocationListeners()).isEmpty(); - } - - @Test public void shutdown_shouldClearPendingIntents() { - api.requestLocationUpdates(client, LocationRequest.create(), - mock(PendingIntent.class)); - api.shutdown(); - assertThat(api.getPendingIntents()).isEmpty(); - } - @Test public void requestLocationUpdates_shouldModifyOnlyClientListeners() { client.connect(); api.requestLocationUpdates(client, LocationRequest.create(), diff --git a/lost/src/test/java/com/mapzen/android/lost/internal/LostClientManagerTest.java b/lost/src/test/java/com/mapzen/android/lost/internal/LostClientManagerTest.java index a0e9a4c..26bb9f9 100644 --- a/lost/src/test/java/com/mapzen/android/lost/internal/LostClientManagerTest.java +++ b/lost/src/test/java/com/mapzen/android/lost/internal/LostClientManagerTest.java @@ -31,12 +31,12 @@ @Config(constants = BuildConfig.class, sdk = 21, manifest = Config.NONE) public class LostClientManagerTest extends BaseRobolectricTest { - ClientManager manager = LostClientManager.shared(); + LostClientManager manager = LostClientManager.shared(); Context context = mock(Context.class); LostApiClient client = new LostApiClient.Builder(context).build(); @After public void tearDown() { - manager.shutdown(); + manager.clearClients(); } @Test public void shouldHaveZeroClientCount() { @@ -258,23 +258,4 @@ public void addLocationCallback_shouldThrowExceptionIfClientWasNotAdded() throws manager.removeClient(client); assertThat(manager.getLocationCallbacks().get(client)).isNull(); } - - @Test public void shutdown_shouldClearAllMaps() { - manager.addClient(client); - LocationRequest request = LocationRequest.create(); - TestLocationListener listener = new TestLocationListener(); - manager.addListener(client, request, listener); - - PendingIntent pendingIntent = mock(PendingIntent.class); - manager.addPendingIntent(client, request, pendingIntent); - - TestLocationCallback callback = new TestLocationCallback(); - Looper looper = mock(Looper.class); - manager.addLocationCallback(client, request, callback, looper); - - manager.shutdown(); - assertThat(manager.getLocationListeners()).isEmpty(); - assertThat(manager.getPendingIntents()).isEmpty(); - assertThat(manager.getLocationCallbacks()).isEmpty(); - } }