[Geolocation] Expose option for distance filtering on location updates.

[christopherdro]

My original implementation involved creating a RCT_ENUM_CONVERTER with CLLocationAccuracy on iOS and a Hashmap on Android that would convert string values to doubles for distance filtering.

I got this to work just fine but realized that I made things more complicated than they needed to be and simplified everything by just having the option be a decimal value (in meters) that works both for iOS and Android.

The only thing i'm not sure about is if we can set arbitrary values for CLLocationManager's distance filter.
@nicklockwood Any idea?

[facebook-github-bot]

By analyzing the blame information on this pull request, we identified @mkonicek, @nicklockwood and @sahrens to be potential reviewers.

[lelandrichardson]

@christopherdro my understanding is that the distance filter is separate from the idea of "accuracy", but is instead related to minimum distance for a "change" in location to have occurred. It seems that in this case, using an arbitrary double value should be just fine.

[christopherdro]

@lelandrichardson That's correct the idea of accuracy and distance are different.

Here is what docs say about distance filter.

The minimum distance (measured in meters) a device must move horizontally before an update event is generated.

I figured it would be just fine but with iOS you just never know.

[lelandrichardson]

It seems to me like using a double should work fine 👍

[rt2zz]

+1 confirmed working

[rt2zz]

Upon further testing, I believe there is an edge case where on Android 0 is treated as no distance filter but on iOS actually does filter results (perhaps 0 -> infinity)

To make this more intuitive we could set 0 to kCLDistanceFilterNone

[nicklockwood]

@rt2xx that makes sense to me.

[facebook-github-bot]

@christopherdro updated the pull request.

[christopherdro]

@rt2zz Nice catch!

Entering 0 as an option would still default RCT_DEFAULT_LOCATION_ACCURACY since the conditional would still evaluate it as false.

This should now be working properly with the updated nested ternary operator.

[facebook-github-bot]

@christopherdro updated the pull request.

[christopherdro]

Possible solution for #1240

[nicklockwood]

@facebook-github-bot shipit

[facebook-github-bot]

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1701598750054106/int_phab to review.

[jrichardlai]

@christopherdro should we move _locationManager.distanceFilter = _observerOptions.distanceFilter outside of the if (!_locationManager) so this could be changed when stoping and watching the location again? Also it seems that beginLocationUpdatesWithDesiredAccuracy is called from getCurrentPosition and it is not initialized from beginLocationUpdatesWithDesiredAccuracy would this be an issue if the value is nil or just be a noop?

[christopherdro]

I think this change was made by @nicklockwood after he accepted my PR.

I'm not quite sure I understand the issue with beginLocationUpdatesWIthDesiredAccuracy. Could you possible give an example of what the issue is?

[jrichardlai]

I just tested calling getCurrentPosition from the start and it seems that it does not crash because structs have a default value:

I think using RCT_DEFAULT_LOCATION_ACCURACY or kCLDistanceFilterNone should be used here.

[jrichardlai]

Nevermind there is a fix 49b2805
Update: actually this was for desiredAccuracy

[janaka120]

is distanceFilter working properly now.

[rt2zz]

yes