Check distance remaining before running OffRouteDetector logic#977
Conversation
danesfeder
force-pushed
the
dan-arrival-offroute
branch
from
a00edd4 to
b96d2fe
Compare
Before:
After:
|
Guardiola31337
left a comment
Guardiola31337
left a comment
There was a problem hiding this comment.
Added a couple nit picks - overall this looks great.
|
|
||
| Point currentPoint = Point.fromLngLat(location.getLongitude(), location.getLatitude()); | ||
|
|
||
| // Get distance from the current step to future point |
There was a problem hiding this comment.
What about extracting blocks of code into well-named (based on the comments) private methods? We should keep public methods as clean as possible so they read like pseudocode (i.e. no conditionals, fors, etc.).
| @Override | ||
| public boolean isUserOffRoute(Location location, RouteProgress routeProgress, MapboxNavigationOptions options) { | ||
|
|
||
| if (checkDistanceRemaining(routeProgress)) { |
There was a problem hiding this comment.
We should reflect this new check in the Javadoc ☝️
There was a problem hiding this comment.
It'd be great if we add a test in OffRouteDetectorTest around this new check.
There was a problem hiding this comment.
Both of these have been addressed 👍
danesfeder
force-pushed
the
dan-arrival-offroute
branch
2 times, most recently
from
5e94ec0 to
86258bd
Compare
Guardiola31337
left a comment
Guardiola31337
left a comment
There was a problem hiding this comment.
A couple of minor details not blocking the PR.
| } | ||
|
|
||
| @Test | ||
| public void isUserOffRoute_assertTrueWhenRouteDistanceRemainingIsZero() { |
|
|
||
| // If not offRoute at this point, do not continue with remaining logic | ||
| if (!isOffRoute) { | ||
| // Even though the current point is not considered off-route, check to see if the user is |
There was a problem hiding this comment.
Could we remove somehow these comments from here and include them into the Javadoc ☝️?
| return isMovingAwayFromManeuver(location, routeProgress, distancesAwayFromManeuver, currentPoint); | ||
| } | ||
|
|
||
| // If the user is considered off-route at this point, but they are close to the upcoming step, |
danesfeder
force-pushed
the
dan-arrival-offroute
branch
from
86258bd to
d933e0e
Compare
|
@Guardiola31337 I removed the comments, each of the methods already had Javadoc, so the comments you pointed out were redundant 👍 |
3 participants
After we arrive at the final waypoint along the route, we remove the off route listener, so off route events are no longer given to the user. Internally the core SDK was still running off route logic and would reset ever few seconds with
OffRouteDetector#isValidOffRoute.This PR adds a check to see if the route distance remaining is
0. If it is we returntruefor every off route check. This way, the snap logic returns the raw userLocation, which makes sense because we are no longer following a route.