Add location accuracy and bearing to geofence event data #8
Conversation
| @@ -138,13 +138,19 @@ - (BOOL)_isRegionMonitoringPossible:(CLAuthorizationStatus)status { | |||
| ); | |||
| } | |||
|
|
|||
| - (void)_sendRegionChangeEventWithIdentifier:(NSString *)identifier didEnter:(BOOL)didEnter didExit:(BOOL)didExit { | |||
| - (void)_sendRegionChangeEventWithIdentifier:(NSString *)identifier didEnter:(BOOL)didEnter didExit:(BOOL)didExit locationManager:(CLLocationManager *)locationManager { | |||
There was a problem hiding this comment.
locationManager is available as local property. Why do you want to pass it along?
There was a problem hiding this comment.
You're right - I'm an iOS noob and wasn't sure if it's safe to use the local prop. I just propagated it from the caller method, where it's parameter too.
| NSDictionary *body = @{ | ||
| @"region": @{ | ||
| @"identifier": identifier, | ||
| }, | ||
| @"didEnter": @(didEnter), | ||
| @"didExit": @(didExit), | ||
| @"accuracy": @(locationManager.location != nil ? locationManager.location.horizontalAccuracy : 99999), | ||
| @"bearing": @(locationManager.location != nil ? locationManager.location.course : 0), | ||
| @"location": @{ |
There was a problem hiding this comment.
If the location is nil, what will the location be in JavaScript-land? I think it should be null instead of { latitude: 0, longitude: 0 }. Do you agree?
| NSDictionary *body = @{ | ||
| @"region": @{ | ||
| @"identifier": identifier, | ||
| }, | ||
| @"didEnter": @(didEnter), | ||
| @"didExit": @(didExit), | ||
| @"accuracy": @(locationManager.location != nil ? locationManager.location.horizontalAccuracy : 99999), | ||
| @"bearing": @(locationManager.location != nil ? locationManager.location.course : 0), |
There was a problem hiding this comment.
Same as the comment about the location, what if location is nil? Course 0 means 0 degrees which is north. Seems to be invalid if we pass 0 if the location is nil.
Perhaps we can put the location properties, like accuracy and course or bearing, in the location property.
There was a problem hiding this comment.
null-able location property sounds good - we just need to make the format same for both platforms.
There was a problem hiding this comment.
Yea, I would prefer the iOS naming, but I guess you applied the Android naming. I think most important is that we keep the names the same on both platforms, like you mentioned.
I do wonder about horizontalAccuracy vs accuracy. On iOS there is also verticalAccuracy, so naming it accuracy may not be accurate (pun intended). Are there any equivalents in Android, or is there just one accuracy?
There was a problem hiding this comment.
You can find the iOS docs of the location object at https://developer.apple.com/documentation/corelocation/cllocation.
|
@martijndeh should I fix the issues or will you do it? |
|
If you have time please do. 👍 |
|
How is it going? |
|
@martijndeh sorry - didn't have time to clean this up yet, sorry ;( |
No description provided.