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.

Sign up for GitHub

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

Jump to bottom

Map labels are no longer blank and using China Map Style URLs if the application is configured to use Mapbox China APIs #1558

Merged
merged 11 commits into from
Jul 25, 2018
Merged

Map labels are no longer blank and using China Map Style URLs if the application is configured to use Mapbox China APIs #1558

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
34f90ac
Solution for issue#https://github.com/mapbox/mapbox-navigation-ios/is…
m-stephen Jul 16, 2018
cc9286a
Modify for #PR https://github.com/mapbox/mapbox-navigation-ios/pull/1558
m-stephen Jul 17, 2018
87bfdea
Update
m-stephen Jul 17, 2018
d681921
Update
m-stephen Jul 18, 2018
d3664e5
Update for MGLExtension
m-stephen Jul 20, 2018
2ee884c
Change the default URL of .cn map
m-stephen Jul 24, 2018
f701580
Documentation and move style constants into MGLStyle extension.
m-stephen Jul 25, 2018
3688c3b
Add documentation full stop.
m-stephen Jul 25, 2018
d59a428
Modify documentation.
m-stephen Jul 25, 2018
a064cc8
Update
m-stephen Jul 25, 2018
cfe9473
Adding change log : Map labels are no longer blank and using China Ma…
m-stephen Jul 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions MapboxNavigation.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@
C5EF397520599120009A2C50 /* straight-line.json in Resources */ = {isa = PBXBuildFile; fileRef = C5EF397420599120009A2C50 /* straight-line.json */; };
C5F2DCA0206DBF5E002F99F6 /* Sequence.swift in Sources */ = {isa = PBXBuildFile; fileRef = C5F2DC9F206DBF5E002F99F6 /* Sequence.swift */; };
C5F2DCA1206DBF5E002F99F6 /* Sequence.swift in Sources */ = {isa = PBXBuildFile; fileRef = C5F2DC9F206DBF5E002F99F6 /* Sequence.swift */; };
CFD47D9020FD85EC00BC1E49 /* NetworkConfiguartion.swift in Sources */ = {isa = PBXBuildFile; fileRef = CFD47D8F20FD85EC00BC1E49 /* NetworkConfiguartion.swift */; };
DA23C9611F4FC05C00BA9522 /* MGLMapView+MGLNavigationAdditions.h in Headers */ = {isa = PBXBuildFile; fileRef = 35D825F91E6A2DBE0088F83B /* MGLMapView+MGLNavigationAdditions.h */; settings = {ATTRIBUTES = (Public, ); }; };
DA3525702010A5210048DDFC /* Localizable.stringsdict in Resources */ = {isa = PBXBuildFile; fileRef = DA35256E2010A5200048DDFC /* Localizable.stringsdict */; };
DAAE5F301EAE4C4700832871 /* Localizable.strings in Resources */ = {isa = PBXBuildFile; fileRef = DAAE5F321EAE4C4700832871 /* Localizable.strings */; };
Expand Down Expand Up @@ -663,6 +664,7 @@
C5E7A31B1F4F6828001CB015 /* NavigationRouteOptions.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NavigationRouteOptions.swift; sourceTree = "<group>"; };
C5EF397420599120009A2C50 /* straight-line.json */ = {isa = PBXFileReference; lastKnownFileType = text.json; path = "straight-line.json"; sourceTree = "<group>"; };
C5F2DC9F206DBF5E002F99F6 /* Sequence.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Sequence.swift; sourceTree = "<group>"; };
CFD47D8F20FD85EC00BC1E49 /* NetworkConfiguartion.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = NetworkConfiguartion.swift; sourceTree = "<group>"; };
DA1811FD20128B0900C91918 /* he */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = he; path = he.lproj/Main.strings; sourceTree = "<group>"; };
DA1811FE20128B0900C91918 /* he */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = he; path = he.lproj/Navigation.strings; sourceTree = "<group>"; };
DA18120120128B7B00C91918 /* he */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = he; path = he.lproj/Localizable.strings; sourceTree = "<group>"; };
Expand Down Expand Up @@ -915,6 +917,7 @@
356B7D8A1EE166E100FE5B89 /* scripts */,
351BEC201E5BD4DC006FE110 /* Supporting files */,
C51923B51EA55CD4002AF9E1 /* Views */,
CFD47D8F20FD85EC00BC1E49 /* NetworkConfiguartion.swift */,
3EA93A10227A7DAF1861D9F5 /* Cache.swift */,
160D8278205996DA00D278D6 /* DataCache.swift */,
35726EE71F0856E900AFA1B6 /* DayStyle.swift */,
Expand Down Expand Up @@ -1856,6 +1859,7 @@
35ECAF2D2092275100DC3BC3 /* UIImage.swift in Sources */,
351BEC051E5BCC6C006FE110 /* LaneView.swift in Sources */,
C5A7EC5C1FD610A80008B9BA /* VisualInstructionComponent.swift in Sources */,
CFD47D9020FD85EC00BC1E49 /* NetworkConfiguartion.swift in Sources */,
351BEC0D1E5BCC72006FE110 /* Bundle.swift in Sources */,
8DF399B21FB257B30034904C /* UIGestureRecognizer.swift in Sources */,
35B7837E1F9547B300291F9A /* Transitioning.swift in Sources */,
Expand Down
4 changes: 4 additions & 0 deletions MapboxNavigation/NavigationMapView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -946,6 +946,10 @@ open class NavigationMapView: MGLMapView, UIGestureRecognizerDelegate {
to call this method on the value of `NavigationViewController.mapView`.
*/
@objc public func localizeLabels() {
guard NetworkConfiguration.sharedConfiguration.isChinaMap == false else{
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there ever any instances where a user on .cn would want to see non-Chinese labels? If the application is set to the .cn endpoint but the user’s preferred language is English, should we make sure to show English labels?

Another consideration is that the code below tries to always show local names for roads. Unlike most maps, navigation maps have to match the guidance. The Directions API currently shows the local names in all cases, never localized names: Project-OSRM/osrm-backend#4561. But hopefully we can assume that if the application is using the .cn Maps API endpoint, then it’s also using the .cn Directions API endpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question. In China, we can only provide Chinese version map via .cn endpoint(That is depends on EMG map, they do not have any other languages map).

So if a customer use a .cn map, it means their customers can only use Chinese version map, otherwise they should buy our .com maps(It means they want to freely switch the language by settings)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For another question, if a customer use a .cn map, our navigation SDK should automatically switch the Directions API endpoint to.cn. That's what my next step should do and why I need make this module, to easily switch the endpoints and make a default map style use for China navigation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Everything already respects MGLMapboxAccessToken in Info.plist, so we should have them respect the MGLMapboxAPIBaseURL key as well: mapbox/MapboxGeocoder.swift#107. The Directions and SpeechSynthesizer initializers do allow the developer to override both settings, but Info.plist should be the central place for configuration changes. (If we need a central place for configuration changes that can change at runtime, then that should be UserDefaults.)

return
}

guard let style = style else {
return
}
Expand Down
31 changes: 31 additions & 0 deletions MapboxNavigation/NetworkConfiguartion.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import Foundation

class NetworkConfiguration : NSObject{
Copy link
Contributor

Choose a reason for hiding this comment

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

Per mapbox/mapbox-gl-native#12417, it seems like the hasChinaAPIBaseURL property should go on an extension of MGLAccountManager instead of this singleton.

@objc static let sharedConfiguration = NetworkConfiguration()

// The current API base URL of map
private var apiBaseURL : String?

// The PRC base URL for Mapbox APIs other than the telemetry API.
private let mapboxChinaBaseAPIURL = "https://api.mapbox.cn"

// The base URL host for Mapbox China
public let mapboxChinaBaseURLHost = "api.mapbox.cn"
Copy link
Contributor

Choose a reason for hiding this comment

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

MGLNetworkConfiguration is a private class in the map SDK. Is there a need to make these symbols public in the navigation SDK?

Copy link
Contributor Author

@m-stephen m-stephen Jul 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

If it can make public, that is really helpful for only change the map endpoint. But for a further consideration, I wanna to make this module easily change the map endpoint, api-directions endpoint and integrate Chinese TTS service.


// The URL String of China map style.
public let mapboxChinaStyleURL = "mapbox://styles/mapbox/streets-zh-v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

This property is currently unused. Let’s leave it out until we need it. Normally it’s the responsibility of the map SDK to handle style URLs anyways.

Copy link
Contributor Author

@m-stephen m-stephen Jul 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This property is used for my next PR. I'll change the codes in `MGLStyle.swift' sth like:

  (NetworkConfiguration.sharedConfiguration.hasChinaBaseURL ? 
  NetworkConfiguration.sharedConfiguration.mapboxChinaStyleURL : 
  "mapbox://styles/mapbox/navigation-guidance-day-v2")

It will be the default style of .cn map for China navigation.


// Value of whether the map is China map or not
public var isChinaMap : Bool!{
Copy link
Contributor

Choose a reason for hiding this comment

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

The property name is a bit misleading. If the developer sets the style to a third-party URL, the map isn’t necessarily a China map. Conversely, the same URL in mapboxChinaStyleURL resolves to an already localized map even for the .com API endpoint.

Currently, in the map SDK, MGLNetworkConfiguration is an internal implementation detail, but MGLAccountManager is public. Access tokens and API endpoints always go hand-in-hand, so I think it would make sense to move this property to an extension on MGLAccountManager. We can name it something like hasChinaBaseURL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good recommendation of name. hasChinaBaseURL is a better name.

I don't think it should be a extension of MGLAccountManager. For my next step, I wanna to use this module to control the whole endpoints switch like Direction APIs, TTS engine. It should be automatically when our customers use our .cn map.

Actually, if it is make by Objective-C, this file should be internal and only used for navigation SDK.

Copy link
Contributor

@1ec5 1ec5 Jul 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

An access token is meaningless without an API endpoint. Up to now, the API endpoint has been assumed to be .com, so the access tokens are .com access tokens. I think it would be helpful to keep the options for access tokens and API endpoints as close as possible.

For my next step, I wanna to use this module to control the whole endpoints switch like Direction APIs, TTS engine. It should be automatically when our customers use our .cn map.

The map SDK, MapboxDirections.swift, and Mapbox Speech for Swift all respect the MGLMapboxAccessToken Info.plist key, so we should have all three libraries respect the MGLMapboxAPIBaseURL key as well: mapbox/MapboxGeocoder.swift#107.

Copy link
Contributor

Choose a reason for hiding this comment

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

Normally there’s no reason to declare a computed property as having an implicitly unwrapped optional (!) type. Bool is correct, because the return value is never nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I'll correct that.

guard apiBaseURL == mapboxChinaBaseAPIURL else {
return false
}
return true
Copy link
Contributor

Choose a reason for hiding this comment

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

The guard statement is unnecessary; just return the result of the equality comparison.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll change that.

}

private override init() {
super.init()
apiBaseURL = Bundle.main.object(forInfoDictionaryKey:"MGLMapboxAPIBaseURL") as? String
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to store this base URL upfront? I think it would be more straightforward to execute this code on demand, as part of hasChinaBaseURL. It’s likely that Bundle already caches these info dictionary values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you deep diving into our map SDK, this key have already load in +(void)load function in MGLNetworkConfiguration.m
image
So that's NP we write here. Of course, if our map SDK didn't do this, we should execute this code on demand.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, the value doesn’t change, but my suggestion is to call object(forInfoDictionaryKey:) at the point of use anyways, because it shouldn’t affect performance significantly even if we call it several times in a row.

Copy link
Contributor

Choose a reason for hiding this comment

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

This can be a local variable.

}

}