Conversation
| it.altitude.toFloat(), | ||
| System.currentTimeMillis() | ||
| ) | ||
| trueHeading = (magneticHeading!! + geoField.declination + 360) % 360 |
There was a problem hiding this comment.
Would feel "safer" with
| trueHeading = (magneticHeading!! + geoField.declination + 360) % 360 | |
| magneticHeading?.let { mh -> | |
| trueHeading = (mh + geoField.declination + 360) % 360 | |
| } |
| trySend(Result.success(locationResultList)) | ||
| } | ||
|
|
||
| sensorHandler.start() |
There was a problem hiding this comment.
What happens if this gets called twice (e.g. from two calls to watchLocation)
| } | ||
|
|
||
| awaitClose { | ||
| sensorHandler.stop() |
There was a problem hiding this comment.
Hmm how about if a user sets multiple watches?
Would think this call would make more sense inside clearWatch when watchLocationHandlers is empty. But I guess then there's the case of the app closing, we'll want to unregister the listener as well?
There was a problem hiding this comment.
There seems to be a method for heading @ FusedLocationProvider - https://developers.google.com/android/reference/com/google/android/gms/location/FusedOrientationProviderClient#requestOrientationUpdates(com.google.android.gms.location.DeviceOrientationRequest,%20java.util.concurrent.Executor,%20com.google.android.gms.location.DeviceOrientationListener)
This wouldn't work for the fallback, but just wondering if you evaluated this option (possibly using this handler for fallback without play services)
| it.toOSLocationResult( | ||
| magneticHeading = sensorHandler.magneticHeading, | ||
| trueHeading = sensorHandler.trueHeading, | ||
| headingAccuracy = sensorHandler.headingAccuracy | ||
| ) |
There was a problem hiding this comment.
I don't know how big of an issue this is, but it returns the same heading if there's multiple locations being sent at once, which for default requests to watchLocation shouldn't happen, but may happen depending on the value passed for interval, location results may be sent in batches.
Again, I don't know if this is that much of an issue because you're setting it to the latest location below, but you can probably get the headings for each location by doing some minor changes in IONGLOCSensorHandler
| /** | ||
| * Handler for device sensors to calculate heading. | ||
| */ | ||
| internal class IONGLOCSensorHandler(context: Context) : SensorEventListener { |
There was a problem hiding this comment.
Don't want be too bothersome, but wouldn't mind some tests for this 😅 - particularly the computing of heading from sensor data + location.
Maybe AI could help drafting up some tests quickly? If you agree ofc.
| altitude = this.altitude, | ||
| accuracy = this.accuracy, | ||
| altitudeAccuracy = if (IONGLOCBuildConfig.getAndroidSdkVersionCode() >= Build.VERSION_CODES.O) this.verticalAccuracyMeters else null, | ||
| heading = trueHeading ?: magneticHeading ?: course ?: -1f, |
There was a problem hiding this comment.
What does #Location#bearing return if hasBearing is false? Just thinking if the value before this PR where there's no bearing would be -1, or something else?
| } | ||
|
|
||
| override fun onAccuracyChanged(sensor: Sensor, accuracy: Int) { | ||
| if (sensor.type == Sensor.TYPE_MAGNETIC_FIELD) { |
There was a problem hiding this comment.
Wonder the reasoning behind this accuracy mapping? Also I see this same block twice in code, could be extracted to its own method?
| every { sensorHandler.magneticHeading } returns null | ||
| every { sensorHandler.trueHeading } returns null | ||
| every { sensorHandler.headingAccuracy } returns null |
There was a problem hiding this comment.
We could add an extra test where sensorHandler returns data, to make sure that it is returned by the Controller.
OS-pedrogustavobilro
changed the title
addWatch
2 participants
Description
This PR adds support for
heading,magneticHeading,trueHeading,headingAccuracy, andcourseContext
https://outsystemsrd.atlassian.net/browse/RMET-4897
Type of changes
Tests
Tested on Pixel 8 Pro with Capacitor example app, ODC sample app, and O11 sample app