-
Notifications
You must be signed in to change notification settings - Fork 310
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
Native Route Objects #3824
Native Route Objects #3824
Conversation
Breaking Changes in MapboxCoreNavigationBreaking API Changes
|
29cdf38
to
1d3acf1
Compare
Breaking Changes in MapboxCoreNavigationBreaking API Changes
|
Breaking API bot is pointing towards a method which got additional parameter with a default value. This will not result in breaking changes |
Breaking Changes in MapboxCoreNavigationBreaking API Changes
|
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.
Some stylistic comments, but it looks like you have things under control here.
@@ -88,16 +101,20 @@ public class NavigationSettings { | |||
fall back to the `NavigationSettings.directions` by default. | |||
- tileStoreConfiguration: Options for configuring how map and navigation tiles are stored on the device. See | |||
`TileStoreConfiguration` for more details. | |||
- routingProviderSource: Configures the type of routing to be used by various SDK objects when providing route calculations. |
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.
(This change is mentioned in the changelog blurb being added in #3754.)
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.
True, but without #3754 this change is unused. I've added it in this PR though to avoid conflicts between RerouteController and Alternative routes integrations.
typealias SetMainRouteHandler = (RouteInterface?, _ legIndex: UInt32, _ completion: @escaping (Result<RouteInfo, Error>) -> Void) -> Void | ||
typealias SetAlternativeRoutesHandler = ([RouteInterface], _ completion: @escaping (Result<[RouteAlternative], Error>) -> Void) -> Void |
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.
While we’re at it, we should give these type aliases less method-like names so that variables of this type can have less awkward names. How about MainRouteSelectionHandler
and AlternativeRoutesAvailabilityHandler
?
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.
I feel it's better to have both main and alternative handlers to share similar naming pattern because they are doing similar action.
Breaking Changes in MapboxCoreNavigationBreaking API Changes
|
…; refactored RouteController to set full routeResponse data to Navigator; added setting and configuring Navigator router; added RerouteController stub for future native Reroute and Alternatives integration.
…; renamed RouteController updateNavigator argument
c5467e3
to
2831b6c
Compare
Breaking Changes in MapboxCoreNavigationBreaking API Changes
|
lazy var configHandle: ConfigHandle = { | ||
let customConfig = UserDefaults.standard.dictionary(forKey: customConfigKey) ?? [:] | ||
let historyAutorecordingConfig = [ |
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.
It's not about auto-recording anymore. What if we call it defaultConfig
or something like that?
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.
You are right. It was like this, but seems to be overwritten during rebase.
uuid: sessionUUID, | ||
legIndex: UInt32(legIndex)) { [weak self] result in | ||
self?.sharedNavigator.setAlternativeRoutes(routes, | ||
completion: { _ in }) | ||
completion?(result) |
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.
Is it okay that we don't wait until alternative routes are set and call this callback here?
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.
It is. completion
reports info about the main route, while setting of alternatives is irrelevant to it. It is also does not change the flow if alternatives set failed for some reason.
Breaking Changes in MapboxCoreNavigationBreaking API Changes
|
Resolves #3689
This PR implements setting full route response data to NN.Router by setting alternative routes too. It also adds
RerouteController
and Navigator router setup which is a common starting point for future NN features integrations like Rerouting and Alternative routes.Omitting changelog entry since this change should be completely transparent for now.