Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
The road-snapped callback receives a full platform location on both sides — CLLocation on iOS, Location on Android — and forwards only the coordinate. Anything drawing its own location marker therefore has no direction to orient it by, and has to infer one from consecutive points. RoadSnappedLocationUpdatedEvent and its raw counterpart now carry the heading and speed the platform already reported. Both are nullable, because both platforms have a way of saying they do not know: iOS leaves CLLocation.course and .speed negative, Android leaves hasBearing() and hasSpeed() false. Those arrive as null rather than as zero, which a consumer would otherwise read as heading due north at a standstill.
be230fe to
6ff6ad7
Compare
jokerttu
left a comment
There was a problem hiding this comment.
The heading and speed mapping is the part that can go wrong, and it lives in the Swift and Kotlin files. The Dart test doesn't cover it. As if you delete the >= 0 check in Swift and the hasBearing() check in Kotlin, the test still passes, because it only sends values through the channel.
Could you move the mapping into a small helper on each side, and hen test the helper directly in ConvertTests.swift and ConvertTest.kt. See how convertsions from platform types to Dto has been done for other object as a ref.
Three cases each:
- -1 becomes null
- 0 stays 0.0
- on Android, hasBearing() == false becomes null
The 0 case is the important one. It is easy to assume 0 gets filtered out too, and a test makes it clear that it doesn't.
| // CLLocation reports an unavailable course or speed as a negative | ||
| // value. Passed on as-is a stationary device would read as heading | ||
| // due north at negative speed, so those arrive as nil instead. |
There was a problem hiding this comment.
| // CLLocation reports an unavailable course or speed as a negative | |
| // value. Passed on as-is a stationary device would read as heading | |
| // due north at negative speed, so those arrive as nil instead. | |
| // GMSRoadSnappedLocationProviderListener documents -1 for a course or | |
| // speed it cannot supply. Only a negative value is unavailable: a course | |
| // of 0 is due north and a speed of 0 is a standstill, both forwarded. |
| /// Direction of travel in degrees clockwise from true north, or null where | ||
| /// the platform reports none. | ||
| /// | ||
| /// A stationary device has no direction to report, and neither platform | ||
| /// invents one: iOS leaves `CLLocation.course` negative and Android leaves | ||
| /// `Location.hasBearing()` false. Both arrive here as null rather than as | ||
| /// zero, which would read as due north. | ||
| final double? heading; | ||
|
|
||
| /// Speed over ground in metres per second, or null where the platform | ||
| /// reports none. Negative iOS values and absent Android values are null. | ||
| final double? speed; |
There was a problem hiding this comment.
| /// Direction of travel in degrees clockwise from true north, or null where | |
| /// the platform reports none. | |
| /// | |
| /// A stationary device has no direction to report, and neither platform | |
| /// invents one: iOS leaves `CLLocation.course` negative and Android leaves | |
| /// `Location.hasBearing()` false. Both arrive here as null rather than as | |
| /// zero, which would read as due north. | |
| final double? heading; | |
| /// Speed over ground in metres per second, or null where the platform | |
| /// reports none. Negative iOS values and absent Android values are null. | |
| final double? speed; | |
| /// Direction of travel in degrees clockwise from true north, or null where | |
| /// the platform has no direction to report. | |
| /// | |
| /// Null means unavailable, not stationary. A device heading due north | |
| /// reports 0. | |
| final double? heading; | |
| /// Speed over ground in meters per second, never negative, or null where the | |
| /// platform has no speed to report. | |
| /// | |
| /// Null means unavailable, not stopped. A stationary device reports 0. | |
| final double? speed; |
| // A Location without a bearing or a speed still answers 0f for | ||
| // both. Passed on as-is, a stationary device would read as | ||
| // heading due north, so an absent value is sent as null. |
There was a problem hiding this comment.
| // A Location without a bearing or a speed still answers 0f for | |
| // both. Passed on as-is, a stationary device would read as | |
| // heading due north, so an absent value is sent as null. | |
| // A Location without a bearing or a speed still answers 0f for | |
| // both. Passed on as-is, a missing bearing would read as | |
| // heading due north, so an absent value is sent as null. |
| /// Direction of travel in degrees clockwise from true north, or null where | ||
| /// the platform reports none. See [RoadSnappedLocationUpdatedEvent.heading]. | ||
| final double? heading; | ||
|
|
||
| /// Speed over ground in metres per second, or null where the platform | ||
| /// reports none. See [RoadSnappedLocationUpdatedEvent.speed]. | ||
| final double? speed; |
There was a problem hiding this comment.
| /// Direction of travel in degrees clockwise from true north, or null where | |
| /// the platform reports none. See [RoadSnappedLocationUpdatedEvent.heading]. | |
| final double? heading; | |
| /// Speed over ground in metres per second, or null where the platform | |
| /// reports none. See [RoadSnappedLocationUpdatedEvent.speed]. | |
| final double? speed; | |
| /// Direction of travel in degrees clockwise from true north, or null where | |
| /// the platform has no direction to report. | |
| /// | |
| /// Null means unavailable, not stationary. A device heading due north | |
| /// reports 0. | |
| final double? heading; | |
| /// Speed over ground in meters per second, never negative, or null where the | |
| /// platform has no speed to report. | |
| /// | |
| /// Null means unavailable, not stopped. A stationary device reports 0. | |
| final double? speed; |
Road-snapped location events now carry the heading and speed the platform
location already reports.
Both native callbacks receive a full platform location —
CLLocationon iOS,Locationon Android — and forwarded only the coordinate. An app drawing itsown location marker on top of the navigation session therefore had no direction
to orient it by, and had to infer a bearing from consecutive points, which is
unreliable at low speed and visibly wrong when stationary.
RoadSnappedLocationUpdatedEventandRoadSnappedRawLocationUpdatedEventgainheadingandspeed.Both are nullable, because both platforms have a way of saying they do not know:
iOS leaves
CLLocation.courseand.speednegative, Android leaveshasBearing()andhasSpeed()false. Those map tonullrather than to0,which a consumer would otherwise read as heading due north at a standstill.
Fixes #760
On the generated files
Regenerating with the pinned
pigeon: 25.3.2reproduces the Dart and Swiftoutput exactly, but the checked-in
messages.g.ktdiffers by ~550 lines ofreflow from what
ktfmt --google-styleproduces here — the ktfmt bundled bycom.ncorti.ktfmt.gradle:0.21.0appears to be older than the CLI available tome. Verified by regenerating and formatting the unchanged input, which shows the
same ~550 lines. Rather than carry that into this PR,
messages.g.ktholds onlythe two hunks this change implies. It should regenerate cleanly in your CI.
Pre-launch Checklist