adopt core sdk ArrivalObserver change to better support waypoint case #2928
Conversation
JunDai
added
✓ ready for review
backwards incompatible
JunDai
force-pushed
the
jd-quick-fix
branch
2 times, most recently
from
0426a2a to
5254312
Compare
| // Not needed in this example | ||
| return true | ||
| override fun onStopArrival(routeLegProgress: RouteLegProgress) { | ||
| // todo: no op |
There was a problem hiding this comment.
What does // todo: no op mean? Can we leave a clearer message? What's the TODO?
For now, maybe just // Empty because this callback's not needed in this example
| import com.mapbox.navigation.base.trip.model.RouteProgressState | ||
| import timber.log.Timber | ||
|
|
||
| /** |
There was a problem hiding this comment.
Let's remove the RouteUtils altogether for now. What do you think? Currently is never used.
There was a problem hiding this comment.
sounds good to me. I am kind of hesitate to so because we might are lacking some features somewhere which needs the rest of the methods.
but we could remove it now and bring it back when necessary.
| * the {@link DirectionsRoute}. The final destination is the same as the final | ||
| * {@link com.mapbox.api.directions.v5.models.DirectionsWaypoint} along the route. | ||
| */ | ||
| void onFinalDestinationArrival(); |
There was a problem hiding this comment.
Maybe separate discussion but @langsmith had some concerns with the current naming for ArrivalObserver's methods we might want to use this in there cc @kmadsen
There was a problem hiding this comment.
I think let's do that in a separate pr
There was a problem hiding this comment.
Feel free to cut a follow up ticket @langsmith so we don't forget 😅
There was a problem hiding this comment.
Suggest to use onFinalDestinationArrival over onRouteArrival? onStopArrival may not be best either.
Feel free to suggest, the effort was mostly on functionality. Have been thinking these could be better. But would like to see what others think too!
onWaypointDeparture
onFinalDestinationArrival
There was a problem hiding this comment.
yea, I had waypoint in mind as well because we refer to waypoints in a lot of nav sdk documentation rather than stops. Waypoints is the Directions API terminology too https://docs.mapbox.com/api/navigation/#waypoint-object
There was a problem hiding this comment.
Feel free to cut a follow up ticket @langsmith so we don't forget 😅
Will do
There was a problem hiding this comment.
Feel free to cut a follow up ticket @langsmith so we don't forget 😅
Codecov Report
@@ Coverage Diff @@
## master #2928 +/- ##
============================================
- Coverage 36.32% 36.30% -0.03%
+ Complexity 2194 2190 -4
============================================
Files 550 550
Lines 19887 19872 -15
Branches 1887 1885 -2
============================================
- Hits 7224 7214 -10
+ Misses 11832 11826 -6
- Partials 831 832 +1 |
5 participants
Description
Fix #2895
As #2855 has been merged into Core SDK, the UI SDK should adopt it and expose a way to the developer so they could benefit from it in the drop-in UI as well.
bug,feature,new API(s),SEMVER, etc.)Goal
Please describe the PR goals. Just the stuff needed to implement the fix / feature and a simple rationale. It could contain many check points if needed
Implementation
ArrivalObserverinto theNavigationViewOptions.RouteListener'sonArrivalandonFinalDestinationArrivalmethod which are replaced by theArrivalObserverRouteUtilsBuildingFootprintHighlightActivityKtScreenshots or Gifs
Please include all the media files to give some context about what's being implemented or fixed. It's not mandatory to upload screenshots or gifs, but for most of the cases it becomes really handy to get into the scope of the feature / bug being fixed and also it's REALLY useful for UI related PRs
Testing
Please describe the manual tests that you ran to verify your changes
SNAPSHOTupstream dependencies if needed) through testapp/demo app and run all activities to avoid regressionsChecklist
CHANGELOGincluding this PR