-
Notifications
You must be signed in to change notification settings - Fork 319
Navigation Reroute controller V2 [In progress] #6765
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Left some comments but I like the approach from #6764 more for reasons specified in the ticket. |
| } | ||
| } | ||
|
|
||
| @JvmOverloads |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigation.kt
Outdated
Show resolved
Hide resolved
|
|
||
| @ExperimentalPreviewMapboxNavigationAPI | ||
| interface NavigationRerouteControllerV2 : NavigationRerouteController { | ||
| fun reroute(params: RerouteParameters, callback: NavigationRerouteController.RoutesCallback) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we deprecate the old reroute method? And the interface? Because now it's not very clear how the user would implement the old reroute method. They'll still have to search for relevant request data on their own. With the deprecation it becomes clear that the old method will not be called for NavigationRerouteControllerV2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SDK users may have mapboxNavigation.reroute() calls in their code. I don't know why they might want to call it, but they can, and if they switch to NavigationRerouteControllerV2 without implementing old reroute, this case may be broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MapboxNavigation#reroute is private, they can't invoke it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I meant mapboxNavigaiton.getRerouteController()?.reroute()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if they invoke it then they have it implemented. Nothing will break here.
However, why should all the users that don't invoke reroute directly implement it? If they use NavigationRerouteControllerV2 they will probably want to receive request data as arguments. What would they do for the old reroute method? Would they just ignore the case with switching to alternatives? Or get the data themselves via RouteProgressObserver? But if we deprecate it with a message that says that we won't invoke it, they won't have to implement it at all. If they invoke it somewhere in their code - let them implement it. I don't see what exactly would break here.
Codecov Report
@@ Coverage Diff @@
## main #6765 +/- ##
============================================
+ Coverage 72.49% 72.50% +0.01%
- Complexity 5522 5524 +2
============================================
Files 772 774 +2
Lines 29924 29949 +25
Branches 3535 3536 +1
============================================
+ Hits 21694 21716 +22
- Misses 6814 6816 +2
- Partials 1416 1417 +1
|
|
It feels that new reroute controller interface brings more confusion than value. |
This PR solves exactly the same problem as #6764
I think that the SDK should provide all the needed data inside the reroute event so that users don't need to combine route progress and reroute events.
To support reroute parameters, I had to introduce a new interface.