Update engine's location requests #215
Conversation
|
Tested and resolves #213 🎉 |
| .setFastestInterval(0) | ||
| .setSmallestDisplacement(0) | ||
| .setInterval(1000); // 1 sec | ||
| LocationServices.FusedLocationApi.requestLocationUpdates(client, request, noPowerListener); |
There was a problem hiding this comment.
Needs a runtime permission check here. Currently crashes on fresh install.
There was a problem hiding this comment.
Nice catch, I'll create a new activity and migrate these examples in a future PR #217
| .setSmallestDisplacement(0) | ||
| .setInterval(1000); // 1 sec | ||
|
|
||
| LocationServices.FusedLocationApi.requestLocationUpdates(otherClient, request, |
| } | ||
| } | ||
|
|
||
| public void removeAllRequests() { |
There was a problem hiding this comment.
As a possible future enhancement, should this also be invoked when the last client is disconnected to avoid this issue of needing to explicitly remove all requests before shutting down a client?
There was a problem hiding this comment.
Good idea, we've run into some recent issues with this behavior. I updated this ticket to capture the work: #214
| Set<LocationRequest> requests = clientCallbackToLocationRequests.get(w); | ||
| requestsToRemove.removeAll(requests); | ||
| } | ||
| return requestsToRemove; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
| private Set<LocationRequest> getRequestOnlyUsedBy(ClientCallbackWrapper wrapper) { | ||
| Set<LocationRequest> requestsToRemove = clientCallbackToLocationRequests.get(wrapper); | ||
| if (requestsToRemove == null) { | ||
| return requestsToRemove; |
There was a problem hiding this comment.
This could be simplified as return null;.
| } | ||
| for (LocationRequest request : requests) { | ||
| try { | ||
| service.removeLocationUpdates(request); |
There was a problem hiding this comment.
Sending a separate request to the service for each request to be removed will cause a disable/enable cycle for each one. This has the side effect of sending out multiple location updates in rapid succession via FusionEngine#checkLastKnownAndNotify(provider).
I think active location listeners would be protected from this effect if they have a fastestInterval associated with their request but it might be worth considering a batch removal operation.
|
@sarahlensing anything that we could help with on our side to help getting this PR merged? |
| throw new RuntimeException(e); | ||
| } | ||
| try { | ||
| service.removeLocationUpdates(requests); |
There was a problem hiding this comment.
This is great should really cut down on thrashing of the LocationManager listeners when multiple requests are removed!
Update engine's location requests (lostzen#215)
Overview
Before this PR,
LocationRequests were not removed from the underlyingLocationEngine. This meant that the location requirements we requested from the system could sometimes be too aggressive. This PR adds behavior to remove allLocationRequests for a givenLostApiClient+LocationListener/PendingIntent/LocationCallbackpair which are not used by any other pairs.Proposed Changes
LostRequestManagerclass to manage whichLostApiClient+LocationListener/PendingIntent/LocationCallbackpairs have made whichLocationRequestsClientCallbackWrapperto allow for a multi-key hash inLostRequestManagerFusedLocationProviderApi#requestLocationUpdateshas been updated to registerClientCallbackWrapper/LocationRequestpairs with theLostRequestManagerFusedLocationProviderApi#removeLocationUpdateshas been updated to unregisterClientCallbackWrapper/LocationRequestpairs with theLostRequestManager, retrieve allLocationRequests used only by thatClientCallbackWrapper, and remove theseLocationRequests from theLocationEngineLocationEngine#setRequesthas been removed and split intoLocationEngine#addRequest,LocationEngine#removeRequest, andLocationEngine#removeAllRequestsLocationEngine#removeRequestdisables and re-enables the engine (if there are still remaining requests), we no longer need to check to see if the engine should be disabled inFusedLocationProviderApi#checkAllListenersPendingIntentsAndCallbacks(ticket to remove this code in a follow up PR)Followup work will add more tests
Closes #213