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

[ios] Switching between label locales can make “null” labels appear #11719

Closed
friedbunny opened this issue Apr 17, 2018 · 11 comments
Closed
Labels
bug Core The cross-platform C++ core, aka mbgl localization Human language support and internationalization runtime styling
Milestone

Comments

@friedbunny
Copy link
Contributor

friedbunny commented Apr 17, 2018

Mapbox SDK version: ios-v4.0.0-rc.1

Steps to trigger behavior

  1. Use -[MGLStyle localizeLabelsIntoLocale:] to set a locale — e.g., nil or [NSLocale localeWithLocaleIdentifier:@"mul"].
  2. Later, use -[MGLStyle localizeLabelsIntoLocale:] to set a different label locale.

Expected behavior

All labels will be localized into the desired locale. If no label should exist, none will exist.

Actual behavior

After switching to the second locale, some map features that shouldn’t be labeled gain “null” labels.

simulator_screen_shot_-iphone_x-_2018-04-17_at_18_13_38
This road at Tokyo Station.

Workaround

Call -[MGLMapView reloadStyle:] before switching locales, then set the new locale in -mapView:didFinishLoadingStyle:.

/cc @1ec5

@friedbunny friedbunny added bug iOS Mapbox Maps SDK for iOS localization Human language support and internationalization labels Apr 17, 2018
@1ec5
Copy link
Contributor

1ec5 commented Apr 17, 2018

These ways lack name tags in OpenStreetMap and therefore lack name properties in the Streets source. However, I can’t reproduce the issue in macosapp – these roads continue to have no label. Is there a particular sequence of locale codes that leads to this issue?

@friedbunny
Copy link
Contributor Author

friedbunny commented Apr 17, 2018

Is there a particular sequence of locale codes that leads to this issue?

I saw this in treble when switching either way between nil (with a German system locale) and mul.

@1ec5
Copy link
Contributor

1ec5 commented Apr 17, 2018

The issue reproduces when using en in the second step, but not when using any other locale code, such as es. The following code path does allow for the return value of +[MGLVectorTileSource(Private) preferredMapboxStreetsLanguageForPreferences:] to be nil, but the relevant call site should already guard against that:

if ([mostSpecificLanguage isEqualToString:@"mul"]) {
return acceptsEnglish ? @"en" : nil;
}

NSString *preferredLanguage = [MGLVectorTileSource preferredMapboxStreetsLanguageForPreferences:preferences];
if (preferredLanguage) {
localizedKeyPath = [NSString stringWithFormat:@"name_%@", preferredLanguage];
}

@1ec5
Copy link
Contributor

1ec5 commented Apr 17, 2018

The layer in question is road-label-large in the Streets v10 style, which starts out as "text-field": "{name_en}", which the text getter translates to the expression name_en. Localizing to nil resolves to an expression like name_es, which translates to the JSON ["get", "name_es"]. But the text getter now returns CAST(name_es, "NSString").

Then, localizing to en produces an expression like CAST(name_en, "NSString"), which translates to ["to-string", ["get", "name_en"]]. The line feature lacks a name_en property, so that resolves to ["to-string", null] or "null".

@1ec5
Copy link
Contributor

1ec5 commented Apr 18, 2018

I’ve confirmed that a to-string expression is coming out of the getter before replacing tokens with key paths here:

return expression.mgl_expressionByReplacingTokensWithKeyPaths;

So it’s very likely that this cast is being introduced in mbgl.

/cc @anandthakker

@1ec5 1ec5 added Core The cross-platform C++ core, aka mbgl and removed iOS Mapbox Maps SDK for iOS labels Apr 18, 2018
@anandthakker
Copy link
Contributor

which starts out as "text-field": "{name_en}", which the text getter translates to the expression name_en

I think this is probably the best place to fix this: can we translate "{name_en}" to (the NSExpression equivalent of) ["coalesce", ["get", "name_en"], ""]?

@1ec5
Copy link
Contributor

1ec5 commented Apr 18, 2018

Yes, that would be a possibility. However, more generally, it might be unexpected to a developer that ["concat", ["get", "name_en"], " (", ["get", "name"], ")"] would yield “null (null)” for features like the ones in the screenshot above. We may want to document the need to always coalesce any string-typed key path with an empty string, just as we’re documenting the need to cast key paths in predicates.

/cc @jmkiley @captainbarbosa @nickidlugash

@1ec5 1ec5 added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS documentation and removed Core The cross-platform C++ core, aka mbgl labels Apr 18, 2018
@anandthakker
Copy link
Contributor

Hm, good point. Would it be more appropriate for ["to-string", null] to yield "" instead of "null"?

@friedbunny friedbunny added this to the ios-v4.0.1 milestone Apr 18, 2018
@1ec5 1ec5 added Core The cross-platform C++ core, aka mbgl runtime styling and removed documentation iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS labels Apr 18, 2018
@ChrisLoer
Copy link
Contributor

Is this the right ticket to track porting mapbox/mapbox-gl-js#6534 to gl-native? Since I don't see another ticket, I'll use this one for the native ignore until we make the corresponding native change to to-string.

@1ec5
Copy link
Contributor

1ec5 commented Apr 24, 2018

Yes, this ticket tracks a port of mapbox/mapbox-gl-js#6534.

@1ec5
Copy link
Contributor

1ec5 commented May 14, 2018

Fixed in #11904 on the release-boba branch for iOS map SDK v4.0.1 and macOS map SDK v0.7.1.

@1ec5 1ec5 closed this as completed May 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Core The cross-platform C++ core, aka mbgl localization Human language support and internationalization runtime styling
Projects
None yet
Development

No branches or pull requests

4 participants