Skip to content

Comments

Use Locale for deciding current language code#187

Merged
bsudekum merged 4 commits intomasterfrom
voice
May 3, 2017
Merged

Use Locale for deciding current language code#187
bsudekum merged 4 commits intomasterfrom
voice

Conversation

@bsudekum
Copy link
Contributor

Closes: #183

Bundle.main.preferredLocalizations.first only would specify en vs with Locale.preferredLanguages which produces en-us. We use the same lookup for Polly voices.

/cc @incanus


// Only localized languages will have a proper fallback voice
utterance.voice = AVSpeechSynthesisVoice(language: Bundle.main.preferredLocalizations.first)
utterance.voice = AVSpeechSynthesisVoice(language: Locale.preferredLanguages.first!)
Copy link
Contributor

@1ec5 1ec5 Apr 29, 2017

Choose a reason for hiding this comment

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

Bundle.main.preferredLocalizations.first is the localization used by the current application (taking into account the Preferred Languages in Settings), whereas Locale.preferredLanguages are the locales listed under Preferred Languages in Settings (regardless of the available localizations).

For example, if the application is localized into only English and Simplified Chinese, but the user's preferred languages are Spanish and English (in that order), this new code will cause the application to read Spanish instructions (because OSRM Text Instructions are available in Spanish) with an English voice, spliced into English format strings. You'd hear, "In 3 kilómetros, gire a la izquierda, then ir por 10 kilómetros" – all with an English voice. The old code would've given English instructions in English, consistent with an interface already in English. If the application wants to support Spanish, it needs to provide a Spanish localization.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this change, I suspect this line that determines the Polly voice needs to be switched from Locale to Bundle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@1ec5 how can we get even more specific localization than just en. If I set my region to en-us, Bundle.main.preferredLocalizations.first will only return en.

Copy link
Contributor

Choose a reason for hiding this comment

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

en should be good enough for AVSpeechSynthesizer. If it isn’t specific enough for Polly, use Locale.current.regionCode. (regionCode is only available on iOS 10+, but you can use Locale.object(forKey:) with NSLocale.Key.countryCode to get the same information.

@1ec5
Copy link
Contributor

1ec5 commented May 1, 2017

While we’re at it, we should also be using the Amy or Emma voice for British English.

@bsudekum
Copy link
Contributor Author

bsudekum commented May 1, 2017

@1ec5 I updated this in 328c0fa but I'm still getting meters when the language country code of en-fr. See anything that I'm missing?

@1ec5
Copy link
Contributor

1ec5 commented May 1, 2017

I'm still getting meters when the language country code of en-fr. See anything that I'm missing?

That’s the expected behavior, since France uses the metric system.

extension Locale {
static var preferredLocalLanguageCountryCode: String {
let bundleLanguage = Bundle.main.preferredLocalizations.first!
let bundleLanguages = bundleLanguage.components(separatedBy: "-")
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn’t a list of languages, it’s a list of components detailing a single locale. See this article for more information.

return bundleLanguage
}

let countryCode = NSLocale.Key.countryCode
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for a separate variable for this constant.

}

let countryCode = NSLocale.Key.countryCode
let locale = NSLocale.current as NSLocale
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for a separate variable for the result of this cast. Surround it with parentheses and inline it below.

let bundleLanguage = Bundle.main.preferredLocalizations.first!
let bundleLanguages = bundleLanguage.components(separatedBy: "-")

if bundleLanguages.count == 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

It’s possible for the host application to have a localization whose locale code contains more than two components, for instance zh-Hant-HK for Traditional Chinese in Hong Kong versus zh-Hant-TW for the same in Taiwan.

super.viewDidLoad()
automaticallyAdjustsScrollViewInsets = false

distanceFormatter.numberFormatter.locale = Locale.init(identifier: Locale.preferredLocalLanguageCountryCode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: drop .init.

override public init() {
super.init()
maneuverVoiceDistanceFormatter.unitStyle = .long
maneuverVoiceDistanceFormatter.numberFormatter.locale = Locale.init(identifier: Locale.preferredLocalLanguageCountryCode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: drop .init.


turnArrowView.step = step
titleLabel.text = routeStepFormatter.string(for: step)
distanceFormatter.numberFormatter.locale = Locale.init(identifier: Locale.preferredLocalLanguageCountryCode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Given how often we have to write this, it might be more convenient to add another variable to Locale called nationalizedCurrent that returns the same Locale as this line is creating.

@bsudekum
Copy link
Contributor Author

bsudekum commented May 2, 2017

@1ec5 updated

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Looks good. Just a couple stylistic comments below.

return firstBundleLocale
}

let countryCode = NSLocale.Key.countryCode
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: inline this expression. It’s a constant, so there’s no advantage to breaking it out. In fact, since NSLocale.object(forKey:) takes an NSLocale.Key, which is an enumeration, you can pass in just .countryCode.

super.viewDidLoad()
automaticallyAdjustsScrollViewInsets = false

distanceFormatter.numberFormatter.locale = Locale.nationalizedCurrent
Copy link
Contributor

Choose a reason for hiding this comment

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

You might be able to say just .nationalizedCurrent here, but I’m not sure.

@bsudekum bsudekum merged commit 0da6378 into master May 3, 2017
@bsudekum bsudekum deleted the voice branch May 3, 2017 16:54
wishtrip-dev pushed a commit to wishtrip-dev/mapbox-navigation-ios that referenced this pull request Feb 28, 2018
@1ec5 1ec5 mentioned this pull request Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants