[iOS] Add fallbacks for name fields#12387
Conversation
1ec5
left a comment
There was a problem hiding this comment.
Thanks, this is a much more elegant approach to implementing v8 support than what I had been working on for #11867. (#11867 covers some additional things like VoiceOver that are unrelated to localization.)
In addition to the feedback below, we should figure out the edge case mentioned in #12164 (comment), that the same expression may be localized more than once. When localizing a function expression, perhaps we check whether the function is mgl_coalesce: and the argument array consists entirely of name_ key path expressions; if so, then we can replace the coalescing function instead of localizing each argument as we normally do.
| } | ||
| return [NSExpression expressionForKeyPath:localizedKeyPath]; | ||
| // If the keypath is `name` or `name_en`, no need to fallback | ||
| if ([localizedKeyPath isEqualToString:@"name"] || [localizedKeyPath isEqualToString:@"name_en"]) { |
There was a problem hiding this comment.
As this documentation states, name_en can be null in Streets source v8, in which case we need to fall back to name.
| // If the keypath is `name_zh-Hans`, fallback to `name_zh`. If `name_zh` is empty, fallback to `name` | ||
| // The `name_zh-Hans` field was added since Mapbox Streets v7 | ||
| // See the documentation of name fields for detail https://www.mapbox.com/vector-tiles/mapbox-streets-v7/#overview | ||
| if ([localizedKeyPath isEqualToString:@"name_zh-Hans"]) { |
There was a problem hiding this comment.
Should we also have name_zh-Hant attempt to fall back to name_zh-Hans and vice versa, for Streets source v8?
| // If the keypath is `name_zh-Hans`, fallback to `name_zh`. If `name_zh` is empty, fallback to `name` | ||
| // The `name_zh-Hans` field was added since Mapbox Streets v7 | ||
| // See the documentation of name fields for detail https://www.mapbox.com/vector-tiles/mapbox-streets-v7/#overview | ||
| if ([localizedKeyPath isEqualToString:@"name_zh-Hans"]) { |
There was a problem hiding this comment.
Apparently the same mapbox.mapbox-streets-v7 tileset on api.mapbox.cn uses name_zh-CN instead of name_zh-Hans: mapbox/mapbox-navigation-ios#1554. Could we address that here by opportunistically falling back to name_zh-CN if name_zh-Hans is nil?
/cc @m-stephen
| * Added the `collator` and `resolved-locale` expression operators to more precisely compare strings in style JSON. A subset of this functionality is available through predicate options when creating an `NSPredicate`. ([#11869](https://github.com/mapbox/mapbox-gl-native/pull/11869)) | ||
| * Fixed a crash when trying to parse expressions containing legacy filters. ([#12263](https://github.com/mapbox/mapbox-gl-native/pull/12263)) | ||
| * Fixed a crash that occurred when creating an `MGL_MATCH` expression using non-expressions as arguments. ([#12332](https://github.com/mapbox/mapbox-gl-native/pull/12332)) | ||
| * Fixed an issue that no localized low-zoom labels when system language is Simplified Chinese ([#12164](https://github.com/mapbox/mapbox-gl-native/issues/12164)) |
There was a problem hiding this comment.
Fixed an issue causing country and ocean labels to disappear after calling
-[MGLStyle localizeLabelsIntoLocale:]when the system language is set to Simplified Chinese.
| return [NSExpression expressionWithFormat:@"mgl_coalesce(%@)", | ||
| @[[NSExpression expressionForKeyPath:localizedKeyPath], | ||
| [NSExpression expressionForKeyPath:@"name_zh"], | ||
| [NSExpression expressionForKeyPath:@"name"],]]; |
There was a problem hiding this comment.
This can be written more succinctly as:
[NSExpression expressionWithFormat:@"mgl_coalesce({%K, name_zh, name})", localizedKeyPath]| // CN tiles might using `name_zh-CN` for Simplified Chinese | ||
| if ([localizedKeyPath isEqualToString:@"name_zh-Hans"]) { | ||
| return [NSExpression expressionWithFormat:@"mgl_coalesce({%K, %K, %K, %K})", | ||
| localizedKeyPath, @"name_zh-CN", @"name_zh", @"name"]; |
There was a problem hiding this comment.
Of these four key paths, only name_zh-CN really needs to be escaped using %K, due to the hyphen. But this is better because it preserves the correct fallback order in the code.
A more generic approach would be to build up an array of key path expressions, then create a coalescing expression in only one place. Something like this:
NSArray *keyPaths;
if (…) {
keyPaths = @[localizedKeyPath, @"name_zh-CN", @"name_zh", @"name"];
} else if (…) {
…
}
NSMutableArray *keyPathExpressions = [NSMutableArray array];
for (NSString *keyPath in keyPaths) {
[keyPathExpressions addObject:[NSExpression expressionForKeyPath:keyPath]];
}
return [NSExpression expressionWithFormat:@"mgl_coalesce(%@)", keyPathExpressions];This might make it easier to address the edge case in #12387 (review) with the solution in #12164 (comment):
[keyPathExpressions addObject:
[NSExpression expressionForKeyPath:@"com.mapbox.expressions.localized"]];Given the complexity of handling the relocalization case, we can address it in a subsequent PR if you think it’s out of scope for this PR.
There was a problem hiding this comment.
@1ec5 Good suggestion, but I think it might happen in another PR.
| * Added the `collator` and `resolved-locale` expression operators to more precisely compare strings in style JSON. A subset of this functionality is available through predicate options when creating an `NSPredicate`. ([#11869](https://github.com/mapbox/mapbox-gl-native/pull/11869)) | ||
| * Fixed a crash when trying to parse expressions containing legacy filters. ([#12263](https://github.com/mapbox/mapbox-gl-native/pull/12263)) | ||
| * Fixed a crash that occurred when creating an `MGL_MATCH` expression using non-expressions as arguments. ([#12332](https://github.com/mapbox/mapbox-gl-native/pull/12332)) | ||
| * Fixed an issue causing country and ocean labels to disappear after calling -[MGLStyle localizeLabelsIntoLocale:] when the system language is set to Simplified Chinese. ([#12164](https://github.com/mapbox/mapbox-gl-native/issues/12164)) |
There was a problem hiding this comment.
Surround -[MGLStyle localizeLabelsIntoLocale:] in backticks so that jazzy will automatically link it to the method’s documentation.
| return [NSExpression expressionWithFormat:@"mgl_coalesce({%K, %K, %K, %K})", | ||
| localizedKeyPath, @"name_zh-CN", @"name_zh", @"name"]; | ||
| } | ||
| // Mapbox Streets v8 has `name_zh-Hant`, we should fallback to Simplified Chinese if the filed has no value |
There was a problem hiding this comment.
Note that label localization is currently disabled for Streets source v8 because this code only recognizes v6 and v7:
But this fallback is good to have in preparation for #11867 anyhow.
b11c139 to
70cac0e
Compare
70cac0e to
601f0b7
Compare
Aiming to fix this issue #12164 and compatible with name fields of Mapbox Streets v8.