Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[ios] adapt Mapbox Streets–sourced layers for user preferred language #9582

Conversation

fabian-guerra
Copy link
Contributor

@fabian-guerra fabian-guerra commented Jul 21, 2017

Fixes #7031


- (void)updateLocalization
{
NSString *mapboxLanguage = [[NSUserDefaults standardUserDefaults] stringForKey:@"MGLMapboxLanguage"];
Copy link
Contributor

@friedbunny friedbunny Jul 21, 2017

Choose a reason for hiding this comment

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

I’m not sure NSUserDefaults is appropriate here — it’s possible that a user will load a map, we will localize it once, then the app will eventually be killed. When the app restarts, we would not relocalize the map because the key was never reset.

Instead, I think an ivar might work better.

mapboxLanguage = preferredLanguageCode;
[[NSUserDefaults standardUserDefaults] setObject:mapboxLanguage forKey:@"MGLMapboxLanguage"];
[self setLabelLanguage:mapboxLanguage];
[[NSNotificationCenter defaultCenter] postNotificationName:NSCurrentLocaleDidChangeNotification object:nil];
Copy link
Contributor

Choose a reason for hiding this comment

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

Posting this notification is the system’s responsibility, isn’t it? If we want to post a notification about the map’s language changing, it would need to be scoped to our framework.

Additionally, we may want to listen for this notification and adjust the map’s localization accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@friedbunny I agree on posting a Mapbox scoped notification. Changing the language preferences makes the OS to restart, we don't get this notification NSCurrentLocaleDidChangeNotification

Copy link
Contributor

Choose a reason for hiding this comment

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

I mentioned NSCurrentLocaleDidChangeNotification mainly because it’s possible to change the preferred languages without restarting on macOS. I figured this feature would go in the cross-platform code in MGLStyle rather than MGLMapView.

}
}

- (NSString *)preferredLanguageCode {
// Languages supported by Mapbox Streets v10.
Copy link
Contributor

Choose a reason for hiding this comment

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

streets-v10 is the style, but the deciding factor for language support is the vector tile source, which is (confusingly) called mapbox-streets-v7.

}
}

- (NSString *)preferredLanguageCode {
// Languages supported by Mapbox Streets v10.
NSSet *supportedLanguages = [NSSet setWithObjects:@"en", @"es", @"fr", @"de", @"ru", @"zh", nil];
Copy link
Contributor

@friedbunny friedbunny Jul 21, 2017

Choose a reason for hiding this comment

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

This is missing ar, pt, and zh-Hans. See the docs for mapbox-streets-v7 and this help doc for more information.

@friedbunny friedbunny added feature iOS Mapbox Maps SDK for iOS localization Human language support and internationalization labels Jul 21, 2017
1ec5
1ec5 previously requested changes Jul 23, 2017
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.

There’s nothing specific to iOS about this feature, so we should seek to implement it in cross-platform code if possible. I think -[MGLStyle initWithRawStyle:mapView:] would be a good entry point for this feature.

Incidentally, this feature is already implemented at the application level in macosapp. If you haven’t already taken a look at it, I’d encourage you to consider how it accounts for variables like non-Streets layers:

- (void)updateLabels {
MGLStyle *style = self.mapView.style;
NSString *preferredLanguage = _isLocalizingLabels ? [MGLVectorSource preferredMapboxStreetsLanguage] : nil;
NSMutableDictionary *localizedKeysByKeyBySourceIdentifier = [NSMutableDictionary dictionary];
for (MGLSymbolStyleLayer *layer in style.layers) {
if (![layer isKindOfClass:[MGLSymbolStyleLayer class]]) {
continue;
}
MGLVectorSource *source = (MGLVectorSource *)[style sourceWithIdentifier:layer.sourceIdentifier];
if (![source isKindOfClass:[MGLVectorSource class]] || !source.mapboxStreets) {
continue;
}
NSDictionary *localizedKeysByKey = localizedKeysByKeyBySourceIdentifier[layer.sourceIdentifier];
if (!localizedKeysByKey) {
localizedKeysByKey = localizedKeysByKeyBySourceIdentifier[layer.sourceIdentifier] = [source localizedKeysByKeyForPreferredLanguage:preferredLanguage];
}
NSString *(^stringByLocalizingString)(NSString *) = ^ NSString * (NSString *string) {
NSMutableString *localizedString = string.mutableCopy;
[localizedKeysByKey enumerateKeysAndObjectsUsingBlock:^(NSString * _Nonnull key, NSString * _Nonnull localizedKey, BOOL * _Nonnull stop) {
NSAssert([key isKindOfClass:[NSString class]], @"key is not a string");
NSAssert([localizedKey isKindOfClass:[NSString class]], @"localizedKey is not a string");
[localizedString replaceOccurrencesOfString:[NSString stringWithFormat:@"{%@}", key]
withString:[NSString stringWithFormat:@"{%@}", localizedKey]
options:0
range:NSMakeRange(0, localizedString.length)];
}];
return localizedString;
};
if ([layer.text isKindOfClass:[MGLConstantStyleValue class]]) {
NSString *textField = [(MGLConstantStyleValue<NSString *> *)layer.text rawValue];
layer.text = [MGLStyleValue<NSString *> valueWithRawValue:stringByLocalizingString(textField)];
}
else if ([layer.text isKindOfClass:[MGLCameraStyleFunction class]]) {
MGLCameraStyleFunction *function = (MGLCameraStyleFunction<NSString *> *)layer.text;
NSMutableDictionary *stops = function.stops.mutableCopy;
[stops enumerateKeysAndObjectsUsingBlock:^(NSNumber *zoomLevel, MGLConstantStyleValue<NSString *> *stop, BOOL *done) {
NSString *textField = stop.rawValue;
stops[zoomLevel] = [MGLStyleValue<NSString *> valueWithRawValue:stringByLocalizingString(textField)];
}];
function.stops = stops;
layer.text = function;
}
}
}

https://github.com/mapbox/mapbox-gl-native/blob/f053feb6d4aba1eaa44a35d050522b641c011b33/platform/macos/app/MGLVectorSource+MBXAdditions.m

Finally, there needs to be either an opt-in mechanism or an out-out mechanism for this feature. Sometimes a style is intentionally designed for a particular language. Where should such an option live?

continue;
}

if ([layer.text isKindOfClass:[MGLConstantStyleValue class]]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should only attempt to modify layers that draw from the Mapbox Streets source. A developer should be able to add a shape or vector source using their own custom data with a name field and not have MGLMapView attempt to “localize” it to a nonexistent name_fr field.

@@ -1128,9 +1128,68 @@ - (void)wakeGL:(__unused NSNotification *)notification
[self validateLocationServices];

[MGLMapboxEvents pushEvent:MGLEventTypeMapLoad withAttributes:@{}];

[self updateLocalization];
Copy link
Contributor

Choose a reason for hiding this comment

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

If the developer switches to a new style while the application is already in the foreground, the labels won’t be localized until the user switches away and back to the application. Instead, we should localize the labels in response to -mapView:didFinishLoadingStyle:, as MapDocument does in macosapp.

Additionally, if the developer adds a named symbol layer after loading the style, using the runtime styling API, we should localize it consistently. If we’re concerned about that possibility, we could require a change to either -[MGLSymbolStyleLayer setText:] or -[MGLStyle addLayer:].

mapboxLanguage = preferredLanguageCode;
[[NSUserDefaults standardUserDefaults] setObject:mapboxLanguage forKey:@"MGLMapboxLanguage"];
[self setLabelLanguage:mapboxLanguage];
[[NSNotificationCenter defaultCenter] postNotificationName:NSCurrentLocaleDidChangeNotification object:nil];
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn’t be posting a notification at all; instead, we should observe for NSCurrentLocaleDidChangeNotification.

mapboxLanguage = preferredLanguageCode;
[[NSUserDefaults standardUserDefaults] setObject:mapboxLanguage forKey:@"MGLMapboxLanguage"];
[self setLabelLanguage:mapboxLanguage];
[[NSNotificationCenter defaultCenter] postNotificationName:NSCurrentLocaleDidChangeNotification object:nil];
Copy link
Contributor

Choose a reason for hiding this comment

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

I mentioned NSCurrentLocaleDidChangeNotification mainly because it’s possible to change the preferred languages without restarting on macOS. I figured this feature would go in the cross-platform code in MGLStyle rather than MGLMapView.

@fabian-guerra fabian-guerra self-assigned this Jul 24, 2017
@fabian-guerra fabian-guerra force-pushed the fabian-adapt-user-language-7031 branch 2 times, most recently from a3b518a to cc69672 Compare July 24, 2017 22:52
Copy link
Contributor

@friedbunny friedbunny left a comment

Choose a reason for hiding this comment

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

  • I don’t see any way for the localization to be kept in sync with the platform language — is that coming or is there something about this implementation that makes that unnecessary?
  • This obviates the current example implementations in iosapp/macapp — let’s update those to use this.

it will look for the Preferred Language Order setting in the Language & Region section of Settings.
If it doesn't fing a match it will fall back to english localization.
*/
@property (nonatomic, getter=isLocalizingLabels) BOOL localizingLabels;
Copy link
Contributor

Choose a reason for hiding this comment

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

@1ec5 will have a better grasp of platform conventions, but the verb tense of localizingLabels feels off to me. More standard would be localizeLabels, I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

The correct tense here would be localizesLabels; there’s no need for a custom getter name in that case.

Set to true to determine the localization according to the languages
supported by <a href="https://www.mapbox.com/vector-tiles/mapbox-streets-v7/#overview">Mapbox Streets</a>,
it will look for the Preferred Language Order setting in the Language & Region section of Settings.
If it doesn't fing a match it will fall back to english localization.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good start. A few notes:

  • Wrap at 80 characters, if possible.
  • I’m not sure if we follow this everywhere, but to avoid the Swift/ObjC true/YES issue, we can choose different words, like “enable”.
  • I would make it more clear that we’re looking at the platform’s language setting — it might be good to toss in a “system’s” or “platform” qualifier somewhere. This is a darwin header, so it may be trickier.
  • Make it clear that the Mapbox Streets in this case is the vector tile source, not the style of the same name.
  • What happens if the style does not have the Mapbox Streets source?

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the style does not have the Mapbox Streets source?

None of the style layers would pass this test, so there would be no effect on the style.

@fabian-guerra
Copy link
Contributor Author

@friedbunny on iOS we can't get the notification when there is a change in the system's language preferences. The developer will have to implements this in didFinishLoadingStyle.

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.

Remember to update the iOS and macOS changelogs.

{
_localizeLabels = localizeLabels;

if (_localizeLabels) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Normally, a custom setter checks whether the old value is equal to the new value. If it is, the setter returns early because there’s nothing to do.

href="https://www.mapbox.com/vector-tiles/mapbox-streets-v7/#overview">
Mapbox Streets</a>, it will look for the Preferred Language Order setting in
the system's Language & Region section of Settings. If it doesn't find a
match it will fall back to english localization.
Copy link
Contributor

Choose a reason for hiding this comment

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

A Boolean value that determines whether the style attempts to localize labels in the style into the system’s preferred language.

When this property is enabled, the style automatically modifies the text property of any symbol style layer whose source is the Mapbox Streets source. On iOS, the user can set the system’s preferred language in Settings, General Settings, Language & Region. On macOS, the user can set the system’s preferred language in the Language & Region pane of System Preferences.

Copy link
Contributor

@friedbunny friedbunny Jul 26, 2017

Choose a reason for hiding this comment

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

This property will currently only take effect in/after -mapView:didFinishLoadingStyle: — if this remains a limitation, we need to make that clear in the docs.

layer.text = function;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the developer disables label localization when the layers are already localized? I think the developer would expect each layer to return to its original state as defined by the style JSON.

@friedbunny friedbunny added this to the ios-v3.6.1 milestone Jul 26, 2017
@"country-label-sm",
];
[self styleLabelLanguageForLayersNamed:labelLayers];
[self.mapView.style setLocalizeLabels:YES];
Copy link
Contributor

Choose a reason for hiding this comment

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

Along the lines of this comment, this should be converted to an on/off toggle.

the system's Language & Region section of Settings. If it doesn't find a
match it will fall back to english localization.
*/
@property (nonatomic, getter=isLocalizeLabels) BOOL localizeLabels;
Copy link
Contributor

Choose a reason for hiding this comment

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

isLocalizingLabels was probably OK here, if we care about grammar. 😆

}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also replace/remove -bestLanguageForUser?

Copy link
Contributor Author

@fabian-guerra fabian-guerra Jul 26, 2017

Choose a reason for hiding this comment

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

@friedbunny We could but in the MBXViewController we would have to explicitly import "MGLVectorSource+MBXAdditions.h" is that ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as -bestLanguageForUser matches the implementation of MGLVectorSource.preferredMapboxStreetsLanguage, there should be no need to make that header public to import it. Displaying the language that would be used is a pretty narrow use case that I wouldn’t expect any other application to need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@friedbunny @1ec5 it does matches the implementation so I don't think we need to get rid of this.

href="https://www.mapbox.com/vector-tiles/mapbox-streets-v7/#overview">
Mapbox Streets</a>, it will look for the Preferred Language Order setting in
the system's Language & Region section of Settings. If it doesn't find a
match it will fall back to english localization.
Copy link
Contributor

@friedbunny friedbunny Jul 26, 2017

Choose a reason for hiding this comment

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

This property will currently only take effect in/after -mapView:didFinishLoadingStyle: — if this remains a limitation, we need to make that clear in the docs.

@@ -610,6 +610,18 @@ MGL_EXPORT
*/
@property (nonatomic, strong) MGLLight *light;

#pragma mark Managing Style's localization
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m not sure how we landed on “Managing” everything. 🤔 Could we just call this “Localization”?

@friedbunny
Copy link
Contributor

@friedbunny on iOS we can't get the notification when there is a change in the system's language preferences. The developer will have to implements this in didFinishLoadingStyle.

This code also operates on macOS — does the notification appear on that platform?

@fabian-guerra
Copy link
Contributor Author

@friedbunny it does appear on MacOS. That could be tail work, now both platforms are consistent on localization update only by request.

@@ -17,6 +17,10 @@
1F7454971ECD450D00021D39 /* MGLLight_Private.h in Headers */ = {isa = PBXBuildFile; fileRef = 1F7454941ECD450D00021D39 /* MGLLight_Private.h */; };
1F7454A91ED08AB400021D39 /* MGLLightTest.mm in Sources */ = {isa = PBXBuildFile; fileRef = 1F7454A61ED08AB400021D39 /* MGLLightTest.mm */; };
1F95931D1E6DE2E900D5B294 /* MGLNSDateAdditionsTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 1F95931C1E6DE2E900D5B294 /* MGLNSDateAdditionsTests.mm */; };
1FDD9D6F1F26936400252B09 /* MGLVectorSource+MBXAdditions.h in Headers */ = {isa = PBXBuildFile; fileRef = 1FDD9D6D1F26936400252B09 /* MGLVectorSource+MBXAdditions.h */; };
Copy link
Contributor

Choose a reason for hiding this comment

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

The MBX class prefix is used for the example applications, while the MGL class prefix is used for the SDK itself.

}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as -bestLanguageForUser matches the implementation of MGLVectorSource.preferredMapboxStreetsLanguage, there should be no need to make that header public to import it. Displaying the language that would be used is a pretty narrow use case that I wouldn’t expect any other application to need.

@fabian-guerra fabian-guerra force-pushed the fabian-adapt-user-language-7031 branch from 9ebf34e to 5a72965 Compare July 27, 2017 16:52
Copy link
Contributor

@friedbunny friedbunny left a comment

Choose a reason for hiding this comment

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

LGTM, provided we ticket out tail work like delocalization and NSCurrentLocaleDidChangeNotification.

@fabian-guerra fabian-guerra force-pushed the fabian-adapt-user-language-7031 branch from 5a72965 to f10c9a6 Compare July 27, 2017 22:21
@@ -610,6 +610,21 @@ MGL_EXPORT
*/
@property (nonatomic, strong) MGLLight *light;

#pragma mark Style's localization
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency in jazzy documentation, marks in headers should be in title case and should describe a task:

Localizing Map Content

} else {
for (NSString *identifier in self.localizedLayersByIdentifier) {
MGLSymbolStyleLayer *layer = (MGLSymbolStyleLayer *)[self.mapView.style layerWithIdentifier:identifier];
NSDictionary *languages = [self.localizedLayersByIdentifier objectForKey:identifier];
Copy link
Contributor

Choose a reason for hiding this comment

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

We can avoid this lookup by calling -[NSDictionary enumerateKeysAndObjectsUsingBlock:] instead of using a for-in loop.

for (NSString *identifier in self.localizedLayersByIdentifier) {
MGLSymbolStyleLayer *layer = (MGLSymbolStyleLayer *)[self.mapView.style layerWithIdentifier:identifier];
NSDictionary *languages = [self.localizedLayersByIdentifier objectForKey:identifier];
NSString *originalLanguage = [languages allKeys].firstObject;
Copy link
Contributor

Choose a reason for hiding this comment

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

The NSDictionary.allKeys documentation states:

The order of the elements in the array is not defined.

Therefore this code makes an assumption that there’s only one key-value pair in the dictionary, of the form oldText = newText. If that fact ever changes, this code would have non-deterministic behavior.

NSString *textField = stop.rawValue;
NSString *localizingString = stringByLocalizingString(textField);
if (![textField isEqualToString:localizingString]) {
[self.localizedLayersByIdentifier setObject:@{ textField : localizingString } forKey:layer.identifier];
Copy link
Contributor

Choose a reason for hiding this comment

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

If there are multiple stops, each with a different value, only the last stop’s value would be associated with this layer in localizedLayersByIdentifier. If a style function has {name_en} at some zoom levels but {name_en} ({name}) at others, only one value will be remembered for when localization is disabled. This scenario could become a lot more common when expressions are implemented (#8074).

@@ -7,6 +7,7 @@ Mapbox welcomes participation and contributions from everyone. Please read [CONT
* Reduced the size of the dynamic framework by optimizing symbol visibility. ([#7604](https://github.com/mapbox/mapbox-gl-native/pull/7604))
* Fixed an issue where the attribution button would have its custom tint color reset when the map view received a tint color change notification, such as when an alert controller was presented. ([#9598](https://github.com/mapbox/mapbox-gl-native/pull/9598))
* Improved the behavior of zoom gestures when the map reaches the minimum zoom limit. ([#9626](https://github.com/mapbox/mapbox-gl-native/pull/9626))
* Added support to adapt Mapbox Streets–sourced layers for user preferred language. ([#9582](https://github.com/mapbox/mapbox-gl-native/pull/9582))
Copy link
Contributor

Choose a reason for hiding this comment

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

Let’s be a bit more specific:

Added an MGLStyle.localizesLabels property, off by default, that localizes any Mapbox Streets–sourced symbol layer into the user’s preferred language.

layer.text = function;
}
}
[self.mapView.style setLocalizesLabels:YES];
Copy link
Contributor

Choose a reason for hiding this comment

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

This method should account for _isLocalizingLabels. Otherwise, the View ‣ Labels In ‣ Local Language menu item has no effect.

@"country-label-sm",
];
[self styleLabelLanguageForLayersNamed:labelLayers];
[self.mapView.style setLocalizesLabels:YES];
Copy link
Contributor

Choose a reason for hiding this comment

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

This method should account for _usingLocaleBasedCountryLabels. Otherwise, the “Label Countries in Local Language” option has no effect. Also, we should rename the option to “Show Labels in ”, because MGLStyle.localizesLabels localizes everything, not just country labels.

@"country-label-sm",
];
[self styleLabelLanguageForLayersNamed:labelLayers];
[self.mapView.style setLocalizesLabels:YES];
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: use dot syntax consistently: self.mapView.style.localizesLabels = YES.

/**
Model class for localization changes.
*/
@interface __MGLTextLanguage: NSObject
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer not to introduce this convention since I don't think we've used it for any other similar cases.

Copy link
Contributor

@boundsj boundsj left a comment

Choose a reason for hiding this comment

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

Apart from the __ convention used to name the new internal class __MGLTextLanguage this looks good to me and I think this can be merged.

@fabian-guerra fabian-guerra force-pushed the fabian-adapt-user-language-7031 branch from 6abfbac to 7a07856 Compare August 1, 2017 17:48
@fabian-guerra fabian-guerra dismissed 1ec5’s stale review August 1, 2017 18:58

The comments were addressed in the last commit

@1ec5
Copy link
Contributor

1ec5 commented Apr 11, 2018

LGTM, provided we ticket out tail work like delocalization and NSCurrentLocaleDidChangeNotification.

#11660

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature iOS Mapbox Maps SDK for iOS localization Human language support and internationalization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants