Wrapping Telemetry Longitude Data #4477
Conversation
…p() in JNI's nativeLatLngForPixel()
| import java.util.Objects; | ||
|
|
||
| import dalvik.annotation.TestTargetClass; |
There was a problem hiding this comment.
The addition of this import seems unnecessary.
…Javadoc more clear. Making tests consistent.
|
Just made the changes recommended by @zugaldia. When CI approves I'll merge this into |
|
CI is happy! Time to merge! |
|
Rebased and merged! |
| @@ -551,7 +553,16 @@ protected Void doInBackground(Void... voids) { | |||
| jsonObject.putOpt(MapboxEvent.ATTRIBUTE_SOURCE, evt.get(MapboxEvent.ATTRIBUTE_SOURCE)); | |||
| jsonObject.putOpt(MapboxEvent.ATTRIBUTE_SESSION_ID, evt.get(MapboxEvent.ATTRIBUTE_SESSION_ID)); | |||
| jsonObject.putOpt(MapboxEvent.KEY_LATITUDE, evt.get(MapboxEvent.KEY_LATITUDE)); | |||
| jsonObject.putOpt(MapboxEvent.KEY_LONGITUDE, evt.get(MapboxEvent.KEY_LONGITUDE)); | |||
There was a problem hiding this comment.
This should be wrapped using LatLng.wrap here rather than duplicating the conditionals for wrapping.
There was a problem hiding this comment.
Normally I'd agree that conditional wrapping should be avoided, however in this case it's needed. Reason being is that Latitude and Longitude data can also come from other events besides location. Specifically gesture events like map.click and map.dragend. This is handled automatically via json.putOpt() for Latitude because it's value (if one exists) can always be used as is.
There was a problem hiding this comment.
@bleege My point is that we should reuse LatLng.wrap, which already has all the correct logic, whenever we add coordinates to the event queue. There's no need to duplicate the code.
There was a problem hiding this comment.
@jfirebaugh Ah... my mistake. I saw the comment attached to Line 554 and thought you were referring to the jsonObject.putOpt() for Longitude and didn't follow that you were referring to wrap tests itself. I follow your thinking now about creating a new LatLng with a constant Latitude and the specific Longitude to be wrapped.
|
Did you audit the other places |
|
The primary goal was to minimally impact the code base this late in the release process, so this was deliberately scoped to the Telemetry issue from #4444. We can and should do a further investigation for the next release. |
Closes #4475