Skip to content
This repository has been archived by the owner on Apr 23, 2023. It is now read-only.

Update engine's location requests #215

Merged
merged 5 commits into from
Jun 16, 2017
Merged

Update engine's location requests #215

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6deadaa
Add equals & hashCode to LocationRequest
sarahsnow1 Jun 6, 2017
42b8e74
remove location request and update engine when location updates removed
sarahsnow1 Jun 6, 2017
d0d1712
Add sample for multiple clients w/diff request priorities
sarahsnow1 Jun 7, 2017
cf85c3f
newline
sarahsnow1 Jun 7, 2017
c10762d
batch request removal and improve request management
sarahsnow1 Jun 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lost/src/main/aidl/com/mapzen/android/lost/internal/IFusedLocationProviderService.aidl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ interface IFusedLocationProviderService {

void requestLocationUpdates(in LocationRequest request);

void removeLocationUpdates(in LocationRequest request);
void removeLocationUpdates(in List<LocationRequest> requests);

void setMockMode(boolean isMockMode);

Expand Down
7 changes: 7 additions & 0 deletions lost/src/main/java/com/mapzen/android/lost/api/LocationRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@ public static LocationRequest create() {
return new LocationRequest();
}

public LocationRequest(LocationRequest incoming) {
this.setInterval(incoming.getInterval());
this.setFastestInterval(incoming.getFastestInterval());
this.setSmallestDisplacement(incoming.getSmallestDisplacement());
this.setPriority(incoming.getPriority());
}

public long getInterval() {
return interval;
}
Expand Down
19 changes: 9 additions & 10 deletions lost/src/main/java/com/mapzen/android/lost/internal/FusedLocationProviderApiImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import android.os.Looper;
import android.os.RemoteException;

import java.util.List;
import java.util.Map;
import java.util.Set;

Expand Down Expand Up @@ -178,23 +179,21 @@ private void requestLocationUpdatesInternal(LocationRequest request) {
}
}

private void removeLocationUpdatesInternal(Set<LocationRequest> requests) {
private void removeLocationUpdatesInternal(List<LocationRequest> requests) {
if (requests == null) {
return;
}
for (LocationRequest request : requests) {
try {
service.removeLocationUpdates(request);
} catch (RemoteException e) {
throw new RuntimeException(e);
}
try {
service.removeLocationUpdates(requests);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great should really cut down on thrashing of the LocationManager listeners when multiple requests are removed!

} catch (RemoteException e) {
throw new RuntimeException(e);
}
}

@Override public PendingResult<Status> removeLocationUpdates(LostApiClient client,
LocationListener listener) {
throwIfNotConnected(client);
Set<LocationRequest> requests = requestManager.removeLocationUpdates(client, listener);
List<LocationRequest> requests = requestManager.removeLocationUpdates(client, listener);
removeLocationUpdatesInternal(requests);
boolean hasResult = LostClientManager.shared().removeListener(client, listener);
checkAllListenersPendingIntentsAndCallbacks();
Expand All @@ -204,7 +203,7 @@ private void removeLocationUpdatesInternal(Set<LocationRequest> requests) {
@Override public PendingResult<Status> removeLocationUpdates(LostApiClient client,
PendingIntent callbackIntent) {
throwIfNotConnected(client);
Set<LocationRequest> requests = requestManager.removeLocationUpdates(client, callbackIntent);
List<LocationRequest> requests = requestManager.removeLocationUpdates(client, callbackIntent);
removeLocationUpdatesInternal(requests);
boolean hasResult = LostClientManager.shared().removePendingIntent(client, callbackIntent);
checkAllListenersPendingIntentsAndCallbacks();
Expand All @@ -214,7 +213,7 @@ private void removeLocationUpdatesInternal(Set<LocationRequest> requests) {
@Override public PendingResult<Status> removeLocationUpdates(LostApiClient client,
LocationCallback callback) {
throwIfNotConnected(client);
Set<LocationRequest> requests = requestManager.removeLocationUpdates(client, callback);
List<LocationRequest> requests = requestManager.removeLocationUpdates(client, callback);
removeLocationUpdatesInternal(requests);
boolean hasResult = LostClientManager.shared().removeLocationCallback(client, callback);
checkAllListenersPendingIntentsAndCallbacks();
Expand Down
6 changes: 4 additions & 2 deletions lost/src/main/java/com/mapzen/android/lost/internal/FusedLocationProviderService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
import android.os.RemoteException;
import android.support.annotation.Nullable;

import java.util.List;

/**
* Service which runs the fused location provider in the background.
*/
Expand All @@ -36,9 +38,9 @@ public class FusedLocationProviderService extends Service {
delegate.requestLocationUpdates(request);
}

@Override public void removeLocationUpdates(LocationRequest request)
@Override public void removeLocationUpdates(List<LocationRequest> requests)
throws RemoteException {
delegate.removeLocationUpdates(request);
delegate.removeLocationUpdates(requests);
}

@Override public void setMockMode(boolean isMockMode) throws RemoteException {
Expand Down
5 changes: 3 additions & 2 deletions .../src/main/java/com/mapzen/android/lost/internal/FusedLocationProviderServiceDelegate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import android.support.annotation.RequiresPermission;

import java.io.File;
import java.util.List;

import static android.Manifest.permission.ACCESS_COARSE_LOCATION;
import static android.Manifest.permission.ACCESS_FINE_LOCATION;
Expand Down Expand Up @@ -46,8 +47,8 @@ public void requestLocationUpdates(LocationRequest request) {
locationEngine.addRequest(request);
}

public void removeLocationUpdates(LocationRequest request) {
locationEngine.removeRequest(request);
public void removeLocationUpdates(List<LocationRequest> requests) {
locationEngine.removeRequests(requests);
}

public void setMockMode(boolean isMockMode) {
Expand Down
6 changes: 4 additions & 2 deletions lost/src/main/java/com/mapzen/android/lost/internal/LocationEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
import android.os.Looper;
import android.support.annotation.RequiresPermission;

import java.util.List;

import static android.Manifest.permission.ACCESS_COARSE_LOCATION;
import static android.Manifest.permission.ACCESS_FINE_LOCATION;

Expand Down Expand Up @@ -51,9 +53,9 @@ public void addRequest(LocationRequest request) {
*
* @param request Valid location request to enable.
*/
public void removeRequest(LocationRequest request) {
public void removeRequests(List<LocationRequest> requests) {
if (request != null) {
this.request.removeRequest(request);
this.request.removeRequests(requests);
disable();
if (!this.request.getRequests().isEmpty()) {
enable();
Expand Down
4 changes: 2 additions & 2 deletions lost/src/main/java/com/mapzen/android/lost/internal/LocationRequestUnbundled.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ public void addRequest(LocationRequest request) {
requests.add(request);
}

public void removeRequest(LocationRequest request) {
requests.remove(request);
public void removeRequests(List<LocationRequest> requests) {
this.requests.removeAll(requests);
fastestInterval = calculateFastestInterval();
}

Expand Down
31 changes: 14 additions & 17 deletions lost/src/main/java/com/mapzen/android/lost/internal/LostRequestManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@

import android.app.PendingIntent;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;

/**
* Manages requests for {@link ClientCallbackWrapper}s so that the system can properly remove
Expand All @@ -19,7 +19,7 @@
public class LostRequestManager implements RequestManager {

private static LostRequestManager instance;
private Map<ClientCallbackWrapper, Set<LocationRequest>> clientCallbackToLocationRequests;
private Map<ClientCallbackWrapper, List<LocationRequest>> clientCallbackToLocationRequests;

LostRequestManager() {
clientCallbackToLocationRequests = new HashMap<>();
Expand Down Expand Up @@ -50,19 +50,19 @@ public static LostRequestManager shared() {
registerRequest(wrapper, request);
}

@Override public Set<LocationRequest> removeLocationUpdates(LostApiClient client,
@Override public List<LocationRequest> removeLocationUpdates(LostApiClient client,
LocationListener listener) {
ClientCallbackWrapper wrapper = getWrapper(client, listener);
return getRequestOnlyUsedBy(wrapper);
}

@Override public Set<LocationRequest> removeLocationUpdates(LostApiClient client,
@Override public List<LocationRequest> removeLocationUpdates(LostApiClient client,
PendingIntent callbackIntent) {
ClientCallbackWrapper wrapper = getWrapper(client, callbackIntent);
return getRequestOnlyUsedBy(wrapper);
}

@Override public Set<LocationRequest> removeLocationUpdates(LostApiClient client,
@Override public List<LocationRequest> removeLocationUpdates(LostApiClient client,
LocationCallback callback) {
ClientCallbackWrapper wrapper = getWrapper(client, callback);
return getRequestOnlyUsedBy(wrapper);
Expand All @@ -73,28 +73,25 @@ private <T> ClientCallbackWrapper getWrapper(LostApiClient client, T callback) {
}

private void registerRequest(ClientCallbackWrapper wrapper, LocationRequest request) {
Set<LocationRequest> requests = clientCallbackToLocationRequests.get(wrapper);
List<LocationRequest> requests = clientCallbackToLocationRequests.get(wrapper);
if (requests == null) {
requests = new HashSet<>();
requests = new ArrayList();
clientCallbackToLocationRequests.put(wrapper, requests);
}
requests.add(request);
LocationRequest r = new LocationRequest(request);
requests.add(r);
}

private Set<LocationRequest> getRequestOnlyUsedBy(ClientCallbackWrapper wrapper) {
Set<LocationRequest> requestsToRemove = clientCallbackToLocationRequests.get(wrapper);
private List<LocationRequest> getRequestOnlyUsedBy(ClientCallbackWrapper wrapper) {
List<LocationRequest> requestsToRemove = clientCallbackToLocationRequests.get(wrapper);
if (requestsToRemove == null) {
return requestsToRemove;
return null;
}
clientCallbackToLocationRequests.remove(wrapper);
for (ClientCallbackWrapper w : clientCallbackToLocationRequests.keySet()) {
Set<LocationRequest> requests = clientCallbackToLocationRequests.get(w);
requestsToRemove.removeAll(requests);
}
return requestsToRemove;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can't rule out the possibility of more than one client registering the same request but that seems kind of gross.

What if a LocationRequest generated a unique internal id for itself based on the time it was registered? Would that simplify the logic here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this would simplify the logic because I'd still have to iterate over the request to unique id map to ensure the request id was the one corresponding to the client wrapper I wanted to remove requests for. I would also have to add a step to update request ids for remaining client wrappers after this step. Am I missing something though? Is this how you imagine it be used?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly I was looking for a way to not have to search all other clients to see if another one is using any of the requestsToRemove before removing them but maybe a unique id is not the best way to achieve that.

What if we just made a copy of each incoming request so two clients could have equal requests without it being the same instance?

}

Map<ClientCallbackWrapper, Set<LocationRequest>> getClientCallbackMap() {
Map<ClientCallbackWrapper, List<LocationRequest>> getClientCallbackMap() {
return clientCallbackToLocationRequests;
}
}
8 changes: 4 additions & 4 deletions lost/src/main/java/com/mapzen/android/lost/internal/RequestManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import android.app.PendingIntent;

import java.util.Set;
import java.util.List;

/**
* Keeps track of which {@link LocationListener}s, {@link PendingIntent}s, and
Expand All @@ -24,9 +24,9 @@ void requestLocationUpdates(LostApiClient client, LocationRequest request,
void requestLocationUpdates(LostApiClient client, LocationRequest request,
PendingIntent callbackIntent);

Set<LocationRequest> removeLocationUpdates(LostApiClient client, LocationListener listener);
List<LocationRequest> removeLocationUpdates(LostApiClient client, LocationListener listener);

Set<LocationRequest> removeLocationUpdates(LostApiClient client, PendingIntent callbackIntent);
List<LocationRequest> removeLocationUpdates(LostApiClient client, PendingIntent callbackIntent);

Set<LocationRequest> removeLocationUpdates(LostApiClient client, LocationCallback callback);
List<LocationRequest> removeLocationUpdates(LostApiClient client, LocationCallback callback);
}
24 changes: 12 additions & 12 deletions lost/src/test/java/com/mapzen/android/lost/internal/FusedLocationProviderApiImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@
import android.os.Looper;
import android.support.annotation.NonNull;

import java.util.HashSet;
import java.util.Set;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.TimeUnit;

import static org.fest.assertions.api.Assertions.assertThat;
Expand Down Expand Up @@ -204,34 +204,34 @@ public void removeLocationUpdates_callback_shouldThrowIfNotConnected() throws Ex
@Test public void removeLocationUpdates_listener_shouldCallService() throws Exception {
LocationListener listener = new TestLocationListener();
LocationRequest request = LocationRequest.create();
Set<LocationRequest> requests = new HashSet<>();
List<LocationRequest> requests = new ArrayList<>();
requests.add(request);
when(requestManager.removeLocationUpdates(connectedClient, listener)).
thenReturn(requests);
api.removeLocationUpdates(connectedClient, listener);
verify(service).removeLocationUpdates(any(LocationRequest.class));
verify(service).removeLocationUpdates(any(List.class));
}

@Test public void removeLocationUpdates_pendingIntent_shouldCallService() throws Exception {
PendingIntent callbackIntent = mock(PendingIntent.class);
LocationRequest request = LocationRequest.create();
Set<LocationRequest> requests = new HashSet<>();
List<LocationRequest> requests = new ArrayList<>();
requests.add(request);
when(requestManager.removeLocationUpdates(connectedClient, callbackIntent)).
thenReturn(requests);
api.removeLocationUpdates(connectedClient, callbackIntent);
verify(service).removeLocationUpdates(request);
verify(service).removeLocationUpdates(requests);
}

@Test public void removeLocationUpdates_callback_shouldCallService() throws Exception {
TestLocationCallback callback = new TestLocationCallback();
LocationRequest request = LocationRequest.create();
Set<LocationRequest> requests = new HashSet<>();
List<LocationRequest> requests = new ArrayList<>();
requests.add(request);
when(requestManager.removeLocationUpdates(connectedClient, callback)).
thenReturn(requests);
api.removeLocationUpdates(connectedClient, callback);
verify(service).removeLocationUpdates(request);
verify(service).removeLocationUpdates(requests);
}

@Test(expected = IllegalStateException.class)
Expand Down Expand Up @@ -387,28 +387,28 @@ public void setMockTrace_shouldThrowIfNotConnected() throws Exception {
throws Exception {
TestLocationListener listener = new TestLocationListener();
LocationRequest request = LocationRequest.create();
Set<LocationRequest> requests = new HashSet<>();
List<LocationRequest> requests = new ArrayList<>();
requests.add(request);
when(requestManager.removeLocationUpdates(connectedClient, listener)).
thenReturn(requests);
api.requestLocationUpdates(connectedClient, request, listener);
api.removeLocationUpdates(connectedClient, listener);
verify(service).removeLocationUpdates(request);
verify(service).removeLocationUpdates(requests);
}

@Test public void removeLocationUpdates_shouldNotKillEngineIfListenerStillActive()
throws Exception {
TestLocationListener listener1 = new TestLocationListener();
TestLocationListener listener2 = new TestLocationListener();
LocationRequest request = LocationRequest.create();
Set<LocationRequest> requests = new HashSet<>();
List<LocationRequest> requests = new ArrayList<>();
requests.add(request);
when(requestManager.removeLocationUpdates(connectedClient, listener1)).
thenReturn(requests);
api.requestLocationUpdates(connectedClient, request, listener1);
api.requestLocationUpdates(connectedClient, LocationRequest.create(), listener2);
api.removeLocationUpdates(connectedClient, listener1);
verify(service).removeLocationUpdates(request);
verify(service).removeLocationUpdates(requests);
}

@Test public void removeLocationUpdates_listener_shouldCallRequestManager() {
Expand Down
14 changes: 11 additions & 3 deletions .../test/java/com/mapzen/android/lost/internal/FusedLocationProviderServiceDelegateTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;

import static android.content.Context.LOCATION_SERVICE;
import static android.location.LocationManager.GPS_PROVIDER;
Expand Down Expand Up @@ -135,8 +137,10 @@ public class FusedLocationProviderServiceDelegateTest extends BaseRobolectricTes

@Test public void removeLocationUpdates_shouldUnregisterAllListeners() throws Exception {
LocationRequest request = LocationRequest.create();
List requests = new ArrayList();
requests.add(request);
delegate.requestLocationUpdates(request);
delegate.removeLocationUpdates(request);
delegate.removeLocationUpdates(requests);
assertThat(shadowLocationManager.getRequestLocationUpdateListeners()).isEmpty();
}

Expand Down Expand Up @@ -265,8 +269,10 @@ private void initTestGpxTrace() throws IOException {
throws Exception {
LocationRequest locationRequest =
LocationRequest.create().setPriority(PRIORITY_BALANCED_POWER_ACCURACY);
List requests = new ArrayList();
requests.add(locationRequest);
delegate.requestLocationUpdates(locationRequest);
delegate.removeLocationUpdates(locationRequest);
delegate.removeLocationUpdates(requests);
assertThat(shadowLocationManager.getRequestLocationUpdateListeners()).isEmpty();
}

Expand Down Expand Up @@ -338,8 +344,10 @@ private void initTestGpxTrace() throws IOException {

@Test public void removeLocationUpdates_locationCallback_shouldUnregisterAllListeners() {
LocationRequest request = LocationRequest.create();
List requests = new ArrayList();
requests.add(request);
delegate.requestLocationUpdates(request);
delegate.removeLocationUpdates(request);
delegate.removeLocationUpdates(requests);
assertThat(shadowLocationManager.getRequestLocationUpdateListeners()).isEmpty();
}

Expand Down
4 changes: 3 additions & 1 deletion lost/src/test/java/com/mapzen/android/lost/internal/FusionEngineTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,10 @@ public void getLastLocation_shouldReturnNullIfNoLocationPermissionsGranted() thr

@Test public void addRequest_shouldDisableLocationUpdatesForNullRequest() throws Exception {
LocationRequest request = LocationRequest.create();
List requests = new ArrayList();
requests.add(request);
fusionEngine.addRequest(request);
fusionEngine.removeRequest(request);
fusionEngine.removeRequests(requests);
assertThat(shadowLocationManager.getRequestLocationUpdateListeners()).isEmpty();
}

Expand Down
Loading