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

m-stephen · 2018-07-16T07:42:16Z

Issue here: #1554

After thinking of our navigation SDK for supporting .cn map, and I think there should be a .cn network module like our map SDK(Refer to MGLNetworkConfiguration.h & MGLNetworkConfiguration.m).

So this PR is make a extension of MGLAccountManager for reading configurations of base api URLs.

And another feature is to change the style URLs when the endpoint is switched to .cn map.

bsudekum

@m-stephen Looks good, just have a few minor requests.

bsudekum · 2018-07-16T15:34:16Z

MapboxNavigation/NetworkConfiguartion.swift

+    }
+
+    // Return of whether the map is China map or not
+    public func isChinaMap() -> Bool{


Minor enhancement, you can change this to a computed variable, something like: var isChinaMap: Bool {...

bsudekum · 2018-07-16T15:34:47Z

MapboxNavigation/NavigationMapView.swift

@@ -946,6 +946,10 @@ open class NavigationMapView: MGLMapView, UIGestureRecognizerDelegate {
     to call this method on the value of `NavigationViewController.mapView`.
     */
    @objc public func localizeLabels() {
+        if(NetworkConfiguration.sharedConfiguration.isChinaMap()){


Use a guard statement here.

bsudekum · 2018-07-16T15:36:35Z

MapboxNavigation/NetworkConfiguartion.swift

+
+    // Return of whether the map is China map or not
+    public func isChinaMap() -> Bool{
+        guard apiBaseURL != nil, apiBaseURL == mapboxChinaBaseAPIURL else {


I think guard apiBaseURL == mapboxChinaBaseAPIURL else { should be enough here.

m-stephen · 2018-07-17T02:11:30Z

I've modified the codes and one question for you @bsudekum.

I saw that api-directions' host is in MBDirection.swift and if we wanna build .cn navigation we should change that. So this file NetworkConfiguration.swift should be used in MBDirection module as well.

What's your recommendation for the dependency of this file? Or I should say, which part or where should this file at?

1ec5 · 2018-07-17T04:54:12Z

MapboxNavigation/NetworkConfiguartion.swift

+    private let mapboxChinaBaseAPIURL = "https://api.mapbox.cn"
+
+    // The base URL host for Mapbox China
+    public let mapboxChinaBaseURLHost = "api.mapbox.cn"


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

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.

1ec5 · 2018-07-17T05:01:58Z

MapboxNavigation/NetworkConfiguartion.swift

+    public let mapboxChinaBaseURLHost = "api.mapbox.cn"
+
+    // The URL String of China map style.
+    public let mapboxChinaStyleURL = "mapbox://styles/mapbox/streets-zh-v1"


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.

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.

1ec5 · 2018-07-17T05:08:21Z

MapboxNavigation/NetworkConfiguartion.swift

+        guard apiBaseURL == mapboxChinaBaseAPIURL else {
+            return false
+        }
+        return true


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

OK, I'll change that.

1ec5 · 2018-07-17T05:09:11Z

MapboxNavigation/NetworkConfiguartion.swift

+    public let mapboxChinaStyleURL = "mapbox://styles/mapbox/streets-zh-v1"
+
+    // Value of whether the map is China map or not
+    public var isChinaMap : Bool!{


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.

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.

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.

1ec5 · 2018-07-17T05:09:57Z

MapboxNavigation/NetworkConfiguartion.swift

+    public let mapboxChinaStyleURL = "mapbox://styles/mapbox/streets-zh-v1"
+
+    // Value of whether the map is China map or not
+    public var isChinaMap : Bool!{


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.

Yep, I'll correct that.

1ec5 · 2018-07-17T05:11:54Z

MapboxNavigation/NetworkConfiguartion.swift

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


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.

If you deep diving into our map SDK, this key have already load in +(void)load function in MGLNetworkConfiguration.m

So that's NP we write here. Of course, if our map SDK didn't do this, we should execute this code on demand.

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.

1ec5 · 2018-07-17T05:17:09Z

MapboxNavigation/NavigationMapView.swift

@@ -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{


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.

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)

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.

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.)

1ec5 · 2018-07-17T17:04:54Z

MapboxNavigation/NetworkConfiguartion.swift

        }
-        return true
+        return false


return apiBaseURL == mapboxChinaBaseAPIURL

1ec5 · 2018-07-18T07:36:44Z

MapboxNavigation/NetworkConfiguartion.swift

+
+    // Value of whether the map is China map or not
+    public var hasChinaBaseURL : Bool{
+        apiBaseURL = Bundle.main.object(forInfoDictionaryKey:"MGLMapboxAPIBaseURL") as? String


This can be a local variable.

1ec5 · 2018-07-18T07:39:34Z

MapboxNavigation/NetworkConfiguartion.swift

@@ -0,0 +1,23 @@
+import Foundation
+
+class NetworkConfiguration : NSObject{


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

1ec5

Getting close!

1ec5 · 2018-07-24T14:09:47Z

MapboxNavigation/MGLStyle.swift

 extension MGLStyle {
    // Returns the URL to the current version of the Mapbox Navigation Guidance Day style.
    @objc
-    public class var navigationGuidanceDayStyleURL: URL { get { return URL(string: "mapbox://styles/mapbox/navigation-guidance-day-v2")! } }
+    public class var navigationGuidanceDayStyleURL: URL { get { return URL(string: (MGLAccountManager.hasChinaBaseURL ? MGLAccountManager.mapboxChinaDayStyleURL : "mapbox://styles/mapbox/navigation-guidance-day-v2"))! } }


I agree with using constants for the China URLs since each gets reused. However, consider turning them into URL-typed static constants within MGLStyle. (There’s no need for string constants if we have constant URLs.)

extension MGLStyle { static let streetsChineseURL = URL(string: "mapbox://styles/mapbox/streets-zh-v1")! }

1ec5 · 2018-07-24T14:14:55Z

MapboxNavigation/MGLStyle.swift


    // Returns the URL to the current version of the Mapbox Navigation Guidance Night style.
    @objc
-    public class var navigationGuidanceNightStyleURL: URL { get { return URL(string: "mapbox://styles/mapbox/navigation-guidance-night-v2")! } }
+    public class var navigationGuidanceNightStyleURL: URL { get { return URL(string: (MGLAccountManager.hasChinaBaseURL ? MGLAccountManager.mapboxChinaNightStyleURL : "mapbox://styles/mapbox/navigation-guidance-night-v2"))! } }


For readability, put this code on multiple lines instead of using a ternary operator. (×4)

1ec5 · 2018-07-24T14:18:36Z

MapboxNavigation/MGLStyle.swift


    // Returns the URL to the current version of the Mapbox Navigation Guidance Night style.
    @objc
-    public class var navigationGuidanceNightStyleURL: URL { get { return URL(string: "mapbox://styles/mapbox/navigation-guidance-night-v2")! } }
+    public class var navigationGuidanceNightStyleURL: URL { get { return URL(string: (MGLAccountManager.hasChinaBaseURL ? MGLAccountManager.mapboxChinaNightStyleURL : "mapbox://styles/mapbox/navigation-guidance-night-v2"))! } }

    @objc
    // Returns the URL to the given version of the navigation guidance style. Available version are 1, 2, and 3.


Add a note that the version is ignored when the API base URL is api.mapbox.cn. (×2)

Also, while you’re here, would you mind turning all four of these comments into proper documentation comments using the /** syntax? Thanks!

1ec5 · 2018-07-24T14:19:26Z

MapboxNavigation/MGLAccountManager.swift

+    static let mapboxChinaDayStyleURL = "mapbox://styles/mapbox/streets-zh-v1"
+    static let mapboxChinaNightStyleURL = "mapbox://styles/mapbox/dark-zh-v1"
+
+    // Value of whether the map is China map or not


Use the /** syntax, so that Quick Help and jazzy can pick this up as a documentation comment.

1ec5 · 2018-07-24T14:21:02Z

MapboxNavigation/MGLAccountManager.swift

+    static let mapboxChinaNightStyleURL = "mapbox://styles/mapbox/dark-zh-v1"
+
+    // Value of whether the map is China map or not
+    public class var hasChinaBaseURL : Bool{


Since this is public, let’s make it available to Objective-C code using @objc.

frederoni · 2018-07-24T15:26:51Z

MapboxNavigation/MGLAccountManager.swift

+
+    static let mapboxChinaBaseAPIURL = "https://api.mapbox.cn"
+    static let mapboxChinaBaseURLHost = "api.mapbox.cn"
+    static let mapboxChinaDayStyleURL = "mapbox://styles/mapbox/streets-zh-v1"


Can we move the style URLs to the MGLStyle extension?

Actually u r right. I'll modify that.

m-stephen · 2018-07-25T02:10:44Z

@1ec5 I have updated documentation and moved China style constants into MGLStyle extension.

1ec5

Looks good! Before merging, please update the PR description to reflect the final changes. Thanks!

1ec5 · 2018-07-25T02:24:49Z

MapboxNavigation/MGLAccountManager.swift


-    // Value of whether the map is China map or not
+    /**
+     Returns true if the map's endpoint is China.


Returns true if the application is configured to connect to Mapbox China APIs.

1ec5 · 2018-07-25T02:25:40Z

MapboxNavigation/MGLAccountManager.swift

    static let mapboxChinaBaseAPIURL = "https://api.mapbox.cn"
+
+    //Mapbox China base URL host.


Nit: Use /** syntax (or /// syntax) any time a comment describes a non-local variable, property, method, class, etc. Even though the jazzy documentation omits private constants like this one, Quick Help does show it to people who are working on this SDK.

1ec5 · 2018-07-25T02:27:07Z

MapboxNavigation/MGLStyle.swift

    @objc
-    public class var navigationGuidanceDayStyleURL: URL { get { return URL(string: (MGLAccountManager.hasChinaBaseURL ? MGLAccountManager.mapboxChinaDayStyleURL : "mapbox://styles/mapbox/navigation-guidance-day-v2"))! } }
+    public class var navigationGuidanceDayStyleURL: URL {
+        get {


Nit: the get { can be omitted for a read-only property; only one level of curly braces is required.

1ec5 · 2018-07-25T02:27:48Z

MapboxNavigation/MGLStyle.swift

-    public class var navigationGuidanceDayStyleURL: URL { get { return URL(string: (MGLAccountManager.hasChinaBaseURL ? MGLAccountManager.mapboxChinaDayStyleURL : "mapbox://styles/mapbox/navigation-guidance-day-v2"))! } }
+    public class var navigationGuidanceDayStyleURL: URL {
+        get {
+            if(MGLAccountManager.hasChinaBaseURL){


Nit: the parentheses are unnecessary.

1ec5 · 2018-07-25T02:30:45Z

Oh, one more thing: would you mind adding this to the changelog?

Map labels are no longer blank if the application is configured to use Mapbox China APIs. (#1558)

m-stephen · 2018-07-25T02:34:47Z

Definitely.

Oh, one more thing: would you mind adding this to the changelog?

…p Style URLs if the application is configured to use Mapbox China APIs. [#1558](#1558)

* master: (1385 commits) Update CHANGELOG.md Adding change log : Map labels are no longer blank and using China Map Style URLs if the application is configured to use Mapbox China APIs. [mapbox#1558](mapbox#1558) Update Modify documentation. Add documentation full stop. Documentation and move style constants into MGLStyle extension. Update CHANGELOG.md Update CHANGELOG.md Change the default URL of .cn map comment Use weak self in MapboxVoiceController Fix missing instruction on first step Nil out audioPlayer delegate when deiniting Update for MGLExtension Ensure build phase inserts access token Upgrade jazzy before generating documentation re-test Remove unused properties on RouteVoiceController fix Update maps sdk, use new camera fitting api ... # Conflicts: # MapboxNavigation/Resources/Base.lproj/Navigation.storyboard

Solution for issue##1554

34f90ac

m-stephen requested review from bsudekum and 1ec5 July 16, 2018 07:42

m-stephen self-assigned this Jul 16, 2018

m-stephen added the topic: localization label Jul 16, 2018

m-stephen mentioned this pull request Jul 16, 2018

Road and POI labels text cannot display when switch endpoint to .cn #1554

Closed

bsudekum reviewed Jul 16, 2018

View reviewed changes

Modify for #PR #1558

cc9286a

1ec5 reviewed Jul 17, 2018

View reviewed changes

Update

87bfdea

1ec5 reviewed Jul 17, 2018

View reviewed changes

MapboxNavigation/NetworkConfiguartion.swift Outdated

}

return true

return false

Copy link

Contributor

1ec5 Jul 17, 2018

Choose a reason for hiding this comment

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

return apiBaseURL == mapboxChinaBaseAPIURL

Update

d681921

1ec5 reviewed Jul 18, 2018

View reviewed changes

m-stephen added 2 commits July 20, 2018 11:21

Update for MGLExtension

d3664e5

Change the default URL of .cn map

2ee884c

m-stephen mentioned this pull request Jul 24, 2018

Default to available styles on api.mapbox.cn endpoint #1564

Closed

1ec5 reviewed Jul 24, 2018

View reviewed changes

frederoni reviewed Jul 24, 2018

View reviewed changes

Documentation and move style constants into MGLStyle extension.

f701580

m-stephen added 3 commits July 25, 2018 10:16

Add documentation full stop.

3688c3b

Modify documentation.

d59a428

Update

a064cc8

1ec5 approved these changes Jul 25, 2018

View reviewed changes

m-stephen changed the title ~~Fix road and POI labels text cannot display when switch endpoint to .cn~~ Map labels are no longer blank if the application is configured to use Mapbox China APIs and use China Map Style URLs Jul 25, 2018

m-stephen changed the title ~~Map labels are no longer blank if the application is configured to use Mapbox China APIs and use China Map Style URLs~~ Map labels are no longer blank and use China Map Style URLs if the application is configured to use Mapbox China APIs Jul 25, 2018

m-stephen changed the title ~~Map labels are no longer blank and use China Map Style URLs if the application is configured to use Mapbox China APIs~~ Map labels are no longer blank and using China Map Style URLs if the application is configured to use Mapbox China APIs Jul 25, 2018

Adding change log : Map labels are no longer blank and using China Ma…

cfe9473

…p Style URLs if the application is configured to use Mapbox China APIs. [#1558](#1558)

m-stephen merged commit 65b8238 into master Jul 25, 2018

m-stephen deleted the stephen-cn-labels-cannot-display-1554 branch July 25, 2018 03:08

		@@ -0,0 +1,23 @@
		import Foundation

		class NetworkConfiguration : NSObject{

		static let mapboxChinaBaseAPIURL = "https://api.mapbox.cn"

		//Mapbox China base URL host.

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

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

Conversation

m-stephen commented Jul 16, 2018 • edited Loading

bsudekum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsudekum Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-stephen commented Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

m-stephen Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-stephen Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-stephen commented Jul 25, 2018 • edited Loading

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 commented Jul 25, 2018

m-stephen commented Jul 25, 2018 • edited Loading

m-stephen commented Jul 16, 2018 •

edited

Loading

bsudekum Jul 16, 2018 •

edited

Loading

m-stephen commented Jul 17, 2018 •

edited

Loading

m-stephen Jul 17, 2018 •

edited

Loading

m-stephen Jul 17, 2018 •

edited

Loading

1ec5 Jul 17, 2018 •

edited

Loading

m-stephen commented Jul 25, 2018 •

edited

Loading

m-stephen commented Jul 25, 2018 •

edited

Loading