Getting location updates using FusedLocationProvider

[Dvik]

Problem description

For getting the location, FusedLocationProvider API would be a better idea, given that Google recommends using it.
FusedLocationProvider API uses all available location data to give as accurate of a location as possible, as quickly as possible, with as little power consumption as possible. It has a different API, though one that is similar in some respects to the LocationManager API. It was released as a part of Play Services some 4 years ago.

I propose that we move to this. The transition won't be very steep.

[lognaturel]

@Dvik I'm not seeing a definitive answer to this but it seems this change could be done while still keeping Play Services 10.0.1, is that right? As discussed in #478, we do want to delay that update.

[Dvik]

Yeah, this change can be done without updating play services.

[lognaturel]

Ok, how about we start making incremental progress on #507 for the next release (~2 weeks) and then start the API change at the beginning of the next release cycle? That will give more time to identify any subtle behavior changes.

As you refactor, one thing you may want to think about is providing an abstraction for all API calls as described by Martin Fowler here.

[nap2000]

This sounds very great, one of the features I'd like to see is the location being continuously captured while the FormEntryActivity is open. The rationale would be:

• Much faster capture of an accurate gps reading
• Locations are available for the timer log to record coordinates in the background for each question

The key concern is presumably battery life. The timer log will probably only be enabled for a small percentage of forms, although that may change if it proves useful. Also for a form that has no location widgets and is not recording a time log then having a location capture running while data is being collected would seem to be wasteful. However maybe the FusedLocationProvider API is sufficiently efficient that this is not an issue.

[nribeka]

I can have a look at this one issue and I think there's a callback that's getting called when the location service detect location changes.

[lognaturel]

@nribeka It'd be great to make progress on the location aspect of the audit logging as @nap2000 was describing. I know you're out for about the next month, would you mind sharing what you have so far and transitioning it to someone else? Thanks!

[lognaturel]

If someone would like to look into this one, @nribeka's start can be found here!

[MukundAnanthu]

@lognaturel Looking into this issue. Would be interested in taking it up!

[lognaturel]

Very interesting article on the Android Developers Blog today - https://android-developers.googleblog.com/2017/06/reduce-friction-with-new-location-apis.html

[heyjamesknight]

Hey everyone! I'm a new contributor working with the Nafundi folks—migrating the FusedLocationProvider is the first improvement we've decided to tackle.

While looking at the existing Location code, I came up with six classes (all Activities) that will need to be updated from LocationManager to the Fused Location Provider. To accomplish this, we'll abstract the new code away into what we’ll call the “LocationClient”. This new class will keep the details of retrieving and tracking location updates out of the Activities and make it easier for us to update or migrate them again in the future.

With the LocationClient in place, we’ll then move to migrating the Activities to use our new class instead of the system’s LocationManager. As a side benefit, this should simplify the Activities quite a bit.

While we’re doing this we’ll also correct a number of minor issues we came across (mostly repeated/unnecessary statements, a couple of formatting errors, some inefficient string concatenation, etc.).

Although testing the LocationClient itself is probably not worth it, we should consider adding some instrumentation tests to the Activities before performing the refactor/abstraction work so we can make sure they retain their intended behaviour—especially considering I’m not as personally familiar with them. At the very least I would recommend testing the Activities that return values via Intents.

Here’s a quick recap of the specific changes we intend to make:

LocationClient

• Update LocationManager code to FusedLocationProviderApi and abstract into new LocationClient class.
• Remove existing “provider” code (not needed with Fused).
• Ensure first returned location is not cached (may not be an issue with Fused).

GeoShapeGoogleMapActivity, GeoShapeOsmMapActivity, GeoTraceGoogleMapActivity, GeoTraceOsmMapActivity

• Refactor LocationManager code to LocationClient.
• Fix minor errors (unnecessary lines, repeated statements, etc.)

GeoPointMapActivity, GeoPointOsmMapActivity

• Move anti-cache measures into LocationClient.
• Same as above.

Instrumentation Tests

• Mock out our new LocationClient for testing purposes.
• Add Instrumentation Tests where critical and feasible.

If anyone has any questions or suggestions just let me know.

[srsudar]

This sounds like a great plan. I love getting code out of Activitys. The hardest part sounds to me like it will be getting instrumentation tests for the current location users. A quick google search didn't turn up anything obvious for Espresso and GPS testing. Hopefully that means it will be easy and I'm overthinking it.

Depending on what the LocationClient ends up looking like, I might still like to see tests. But maybe I'm the only one.

[lognaturel]

Something just occurred to me. The geopoint widget without map is the most widely used of these location-based widgets. It currently does not require having Google Play Services installed and it would be nice to maintain that since we know that some users on older devices do not have Play Services installed. @jknightco I see that you did not explicitly mention any projected changes to GeoPointActivity. Ideally it could use the new LocationClient when available but fall back to LocationManager if needed.

[heyjamesknight]

@lognaturel ah not sure how GeoPointActivity got left out, it should definitely be included in the list.

What do you think about hiding the LocationManager/Play Services decision inside of of LocationClient? LC can check if Play Services are available and start up the FusedLocationProvider if they are, or LocationManager if they're not.

[heyjamesknight]

@lognaturel draft of the dual LocationClients here: https://github.com/PembrokeStudio/collect/tree/issue-508-locationupdate/collect_app/src/main/java/org/odk/collect/android/location

LocationClient is the shared interface, AndroidLocationClient uses LocationManager, and GoogleLocationClient uses Play Services. LocationClients provides a static method for retrieving the correct implementation.

None of these have been tested yet but they shouldn't need significant changes from how they are now. I believe the provided methods should answer all of our existing use cases but I'll have to double check.

Let me know what you think!

[lognaturel]

I only took a quick look but it is looking fantastic! The fallback behavior seems great and in fact is relevant for the OpenStreetMap versions of the map-based widgets as well as for the map-less geopoint so it's really good to have in there (the Google ones need Play Services for the map).

[heyjamesknight]

Awesome, thanks Hélène. First thing next week I'll add tests and then move on to modifying the Activities.

…

On Fri, Jul 21, 2017 at 10:58 PM Hélène Martin ***@***.***> wrote: I only took a quick look but it is looking fantastic! The fallback behavior seems great and in fact is relevant for the OpenStreetMap versions of the map-based widgets as well as for the map-less geopoint so it's really good to have in there (the Google ones need Play Services for the map). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#508 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACWrAWWuCfz8WWx6n90nk4np6kEcWoj3ks5sQRDjgaJpZM4MTL4h> .

[lognaturel]

Closed in #1336