Skip to content
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

MapboxMaps v10.0.0, MapboxDirections v2.0.0-rc.4, MapboxNavigationNative v69.0.0 #3413

Merged
merged 15 commits into from
Oct 14, 2021

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Sep 29, 2021

Upgraded the MapboxMaps dependency to mapbox/mapbox-maps-ios@312a5f9 in anticipation of the final release of v10.0.0.

This upgrade introduces two @_spi(Restricted) imports to account for mapbox/mapbox-maps-ios#708. The map SDK’s default marker image has been vendored into the MapboxNavigation asset catalog, now that mapbox/mapbox-maps-ios#707 has removed the default annotation image option from the map SDK.

  • Upgrade to MapboxMaps v10.0.0
  • Upgrade to MapboxDirections v2.0.0-rc.4
  • Upgrade to MapboxNavigationNative v69.0.0
  • Remove redundant direct dependency on Turf

Depends on mapbox/mapbox-maps-ios#741 and mapbox/mapbox-directions-swift#608. Hold until the final release of v10.0.0 is published.

/cc @mapbox/navigation-ios

@1ec5 1ec5 added build Issues related to builds and dependency management. blocked Blocked by dependency or unclarity. labels Sep 29, 2021
@1ec5 1ec5 added this to the v2.0.0 (GA) milestone Sep 29, 2021
@1ec5 1ec5 self-assigned this Sep 29, 2021
@@ -902,7 +903,7 @@ open class NavigationMapView: UIView {
let triangleImage = Bundle.mapboxNavigation.image(named: "triangle")?.withRenderingMode(.alwaysTemplate) else { return }

do {
try mapView.mapboxMap.style.addImage(triangleImage, id: NavigationMapView.ImageIdentifier.arrowImage)
try mapView.mapboxMap.style.addImage(triangleImage, id: NavigationMapView.ImageIdentifier.arrowImage, stretchX: [], stretchY: [])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mapbox/mapbox-maps-ios#720 proposes giving these parameters default arguments so they can be omitted again.

@1ec5
Copy link
Contributor Author

1ec5 commented Sep 29, 2021

#3409 incorporates changes needed for mapbox/mapbox-maps-ios#715.

@1ec5 1ec5 changed the title MapboxMaps v10.0.0-rc.10 MapboxMaps v10.0.0 Oct 4, 2021
@1ec5 1ec5 added the release blocker Needs to be resolved before the release. label Oct 6, 2021
@1ec5
Copy link
Contributor Author

1ec5 commented Oct 6, 2021

As of mapbox/mapbox-maps-ios#732 mapbox/mapbox-core-maps-ios#39, the map SDK now requires MapboxCommon v20.0.0, whereas the latest MapboxNavigationNative requires v19.x. That blocks us from upgrading to a newer commit on main that incorporates mapbox/mapbox-maps-ios#741, which needs to happen in tandem with incorporating mapbox/mapbox-directions-swift#608.

navigationMapView.mapView.ornaments.options.logo._visibility = .hidden
navigationMapView.mapView.ornaments.options.attributionButton._visibility = .hidden
navigationMapView.mapView.ornaments.options.logo.visibility = .hidden
navigationMapView.mapView.ornaments.options.attributionButton.visibility = .hidden
Copy link
Contributor Author

@1ec5 1ec5 Oct 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We’re going through some lengths to keep hiding attribution on CarPlay, even though the visibility property is now Restricted SPI, not intended for customer use. Unfortunately, we don’t really have a choice, because the map SDK only provides an attribution control that requires interactivity. We can’t hard-code the attribution in the navigation SDK, because a given style could be using an arbitrary collection of tile sets with unpredictable data sources.

mapbox/mapbox-maps-ios#745 proposes an option to add an unobtrusive textual attribution label to the corner of the map. In the meantime, the user can still access attribution through the map view on the phone screen, which would be visible beforehand or simultaneously, assuming the application configures CarPlay navigation in the usual way.

@MaximAlien MaximAlien marked this pull request as ready for review October 8, 2021 19:40
@MaximAlien MaximAlien removed the blocked Blocked by dependency or unclarity. label Oct 8, 2021
@@ -76,7 +76,7 @@ private final class ProductionBillingService: BillingService {
}

func getSKUTokenIfValid(for sessionType: BillingHandler.SessionType) -> String {
TokenGenerator.getSKUTokenIfValid(for: tripSku(for: sessionType))
MapboxCommon_Private.BillingService.getSessionSKUTokenIfValid(for: tripSku(for: sessionType))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @S2Ler, TokenGenerator APIs were deprecated. Replaced with BillingService.

@MaximAlien
Copy link
Contributor

Looks like MapboxCoreNavigationTests.testOrderOfExecution(), NavigationServiceTests.testMultiLegRoute() and NavigationServiceTests.testLocationCourseShouldNotChange() have similar root cause (navigator reports incorrect status, which contains unexpected statusIndex).

@@ -37,7 +37,7 @@ class SKUTests: TestCase {

func testSKUTokensMatch() {
BillingHandler.shared.beginBillingSession(for: .freeDrive, uuid: .init())
let skuToken = TokenGenerator.getSKUToken(for: .nav2SesTrip)
let skuToken = MapboxCommon_Private.BillingService.getUserSKUToken(for: .nav2SesTrip)
Copy link
Contributor

@MaximAlien MaximAlien Oct 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@S2Ler, did you follow Common APIs rework related to billing? Right now BillingService.getUserSKUToken returns empty skuToken, it was not the case previously.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@S2Ler
Copy link
Contributor

S2Ler commented Oct 11, 2021

Looks like MapboxCoreNavigationTests.testOrderOfExecution(), NavigationServiceTests.testMultiLegRoute() and NavigationServiceTests.testLocationCourseShouldNotChange() have similar root cause (navigator reports incorrect status, which contains unexpected statusIndex).

I merged this branch into my branch with fixes: https://github.com/mapbox/mapbox-navigation-ios/tree/feature/async-updates-maps-10

I see that completeRoute still fails, but other tests seem to be fine.

Comment on lines +23 to +24
static let defaultFramesPerSecond = 30
static let pluggedInFramesPerSecond = 60
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like mapbox/mapbox-maps-ios#735 conditionally introduced usage of CAFrameRateRange on iOS 15. Should we follow suit?

@S2Ler
Copy link
Contributor

S2Ler commented Oct 12, 2021

⚠️ I rebased on top of main and force pushed ⚠️ @1ec5 @MaximAlien

@S2Ler
Copy link
Contributor

S2Ler commented Oct 12, 2021

I will try to fix tests

The issue seem to be with fast location replays. So the fix is to give
it a bit more room.
The token was incorrectly fetched.
github "mapbox/mapbox-directions-swift" "v2.0.0-rc.2"
github "mapbox/mapbox-events-ios" "v1.0.4"
github "mapbox/turf-swift" "v2.0.0-rc.1"
github "apple/swift-argument-parser" "1.0.1"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mapbox/mapbox-directions-swift#612 tracks removing this dependency from Carthage builds.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we maybe move CLI tool to a subfolder? So that our users won't need this dependency.

@1ec5 1ec5 changed the title MapboxMaps v10.0.0 MapboxMaps v10.0.0, MapboxDirections v2.0.0-rc.4, MapboxNavigationNative v69.0.0 Oct 14, 2021
@1ec5 1ec5 merged commit 028d44a into main Oct 14, 2021
@1ec5 1ec5 deleted the 1ec5-maps-v10.0.0-rc.10 branch October 14, 2021 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues related to builds and dependency management. release blocker Needs to be resolved before the release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants