-
Notifications
You must be signed in to change notification settings - Fork 311
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
Update to Mapbox Maps v10.0.0-rc.8
, Navigation Native 66.0.0
and Common 18.0.0
.
#3342
Conversation
# Conflicts: # CHANGELOG.md # Cartfile # Cartfile.resolved # MapboxCoreNavigation.podspec # MapboxNavigation-Documentation.podspec # MapboxNavigation-SPM.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved # Tests/CocoaPodsTest/PodInstall/Podfile.lock
v10.0.0-rc.8
.v10.0.0-rc.8
, Mapbox Navigation Native 66.0.0
and Mapbox Common 18.0.0
.
v10.0.0-rc.8
, Mapbox Navigation Native 66.0.0
and Mapbox Common 18.0.0
.v10.0.0-rc.8
, Navigation Native 66.0.0
and Common 18.0.0
.
@@ -252,7 +252,11 @@ open class RouteController: NSObject { | |||
|
|||
rawLocation = location | |||
|
|||
locations.forEach { navigator.updateLocation(for: FixLocation($0)) } | |||
locations.forEach { | |||
navigator.updateLocation(for: FixLocation($0)) { _ in |
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.
Not sure that we have to do anything here. Additional logging might be helpful, but if there are a lot of locations it might clutter the output. I also think that this callback should be optional.
for location in locations { | ||
navigator.updateLocation(for: FixLocation(location)) | ||
navigator.updateLocation(for: FixLocation(location)) { success in | ||
completion?(success) |
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 call completion for every location in the array?
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.
Good point, probably should just call it once. Should it be considered successful only if all the updates succeed, or only if the one of them succeeds?
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.
To be serious I'm not sure what would be better here. Looks like completion only in case when all locations were processed would be preferable, as user might not really care about every location in particular. BTW this method is only called from two places: first one is CLLocationDelegate
and we do not use that completion at all there, second one is in PassiveLocationManager.updateLocation(_:completion:)
and we provide only one location there, instead of collection.
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.
OK, if we ever do need a PassiveLocationManager.updateLocations(_:completion:)
, then we can revisit this logic at that time.
MapboxNavigation-SPM.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
Outdated
Show resolved
Hide resolved
- parameter success: Boolean value, which is set to `true` in case if `RouteLeg` was | ||
successfully changed, otherwise `false`. | ||
*/ | ||
public typealias AdvanceLegCompletionHandler = (_ success: Bool) -> 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.
This is a breaking change, which affects both RouteController
and LegacyRouteController
. I'm also hesitant about introducing it: previously we did not provide any information to the user about success. Now it's available, which might be useful.
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.
Similar to #3342 (comment), it would be helpful to define this completion handler to accept a Result<Error, RouteProgress>
. Even though the navigator doesn’t provide us with an Error in the failure case, we can create a basic one on the spot. In the future, if the navigator does start providing more information, we can naturally pass it along without having to break the public API. But if we don’t go through that trouble now, we can introduce a parallel method and completion handler type when the time comes.
Remember to mention the change in the changelog.
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.
UpdateLocationCompletionHandler
and AdvanceLegCompletionHandler
were modified to use Result
instead of Boolean
. Error enums are internal, which gives us an opportunity to change them in future.
…tements while creating instance from `MapboxNavigationNative.MatchedRoadObjectLocation`.
….labelCurrentRoadFeature(at:)` for a better type-safety.
…atements while creating instance from `MapboxNavigationNative.RoadObjectDistance`.
…build failures due to deprecated APIs in Navigation Native.
- parameter success: Boolean value, which is set to `true` in case if `RouteLeg` was | ||
successfully changed, otherwise `false`. | ||
*/ | ||
public typealias AdvanceLegCompletionHandler = (_ success: Bool) -> 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.
Similar to #3342 (comment), it would be helpful to define this completion handler to accept a Result<Error, RouteProgress>
. Even though the navigator doesn’t provide us with an Error in the failure case, we can create a basic one on the spot. In the future, if the navigator does start providing more information, we can naturally pass it along without having to break the public API. But if we don’t go through that trouble now, we can introduce a parallel method and completion handler type when the time comes.
Remember to mention the change in the changelog.
# Conflicts: # Sources/MapboxCoreNavigation/EHorizon/RoadObjectLocation.swift
These tests are failing on the SPM build:
Zeroing in on one:
This test works by setting up a route request from 38.853108, -77.043331 (DCA) to 38.910736, -76.966906 (Arboretum) and running a full trace along that fixture. But the fixture looks pretty weird; I wonder if that is causing a reroute at a different time than expected: This reminds me of another test fixture generated at around the same time that also took weird jogs: #2412 (comment). |
…ProactiveRerouting`.
…dler` to return result instead of boolean value. Update changelog.
|
||
navigator.changeRouteLeg(forRoute: 0, leg: legIndex) { [weak self] success in |
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.
@@ -630,7 +630,7 @@ class NavigationServiceTests: TestCase { | |||
XCTAssertTrue(delegate.recentMessages.contains("navigationService(_:didArriveAt:)")) | |||
} | |||
|
|||
func testProactiveRerouting() { | |||
func disabled_testProactiveRerouting() { |
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.
Revival of several disabled tests will be tackled as a tail-work in this ticket: #3375.
Update Mapbox Navigation to Mapbox Maps
v10.0.0-rc.8
, Mapbox Navigation Native66.0.0
and Mapbox Common18.0.0
.