Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Localization plugin #74

Merged
merged 3 commits into from
Feb 21, 2018
Merged

Localization plugin #74

merged 3 commits into from
Feb 21, 2018

Conversation

langsmith
Copy link
Contributor

@langsmith langsmith commented Aug 7, 2017

This plugin resolves #36 and enables a developer to localize the map text to the language set on the Android device.

The plugin also has individual methods for converting the map to each of the languages that we support. Anyways, here it is.

ezgif com-resize 2

ezgif com-resize

cc @tobrun @1ec5 @jcsg

@langsmith langsmith requested a review from zugaldia August 7, 2017 19:36
@langsmith
Copy link
Contributor Author

And yes, I will create a test class too

@langsmith
Copy link
Contributor Author

FYI, here's why the test is failing 👇
screen shot 2017-08-09 at 1 32 29 pm

@tobrun
Copy link
Member

tobrun commented Aug 10, 2017

@langsmith if you aren't adding any instrumentation tests, feel free to remove that example file.

String deviceLanguage = Locale.getDefault().getLanguage();
for (Layer layer : listOfMapLayers) {
// TODO: Fix if() statement? What should if statement be looking for in layer ids?
if (layer.getId().contains("")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Briefly looking over this, I think you are looking to change all Symbol layers? For that you can do if(layer instanceof SymbolLayer) {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -0,0 +1,25 @@
# Add project specific ProGuard rules here.
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't look like this ProGuard files needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@@ -0,0 +1,3 @@
<resources>
<string name="app_name">localization</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@langsmith
Copy link
Contributor Author

Note: Cameron's review requests have been addressed.

@1ec5
Copy link
Contributor

1ec5 commented Aug 31, 2017

I added an individual method for converting the map to each of the languages that we support. Maybe they don't belong in this plugin.

There does need to be a list hard-coded somewhere, until mapbox/tilejson-spec#14 is implemented. (Someday…) Note that the canonical list of available languages is in the Mapbox Streets source v7 documentation.

public LocalizationPlugin(@NonNull final MapboxMap mapboxMap) {
listOfMapLayers = mapboxMap.getLayers();
String deviceLanguage = Locale.getDefault().getLanguage();
for (Layer layer : listOfMapLayers) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the style includes layers that use a source other than Mapbox Streets? For example, there may be a layer based on a custom Studio dataset that also has a {name} property but no corresponding {name_*} properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now checking for Mapbox-only sources with this commit

0314d08

Copy link
Contributor

Choose a reason for hiding this comment

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

It turns out that commit doesn’t actually filter by source. Instead, it reruns the code once for each source, meaning that each layer gets processed one time per source. The Android map SDK apparently lacks a way to determine the source that a layer is using: mapbox/mapbox-gl-native#12691.

String deviceLanguage = Locale.getDefault().getLanguage();
for (Layer layer : listOfMapLayers) {
if (layer instanceof SymbolLayer) {
layer.setProperties(PropertyFactory.textField(String.format("{name_%s}", deviceLanguage)));
Copy link
Contributor

@1ec5 1ec5 Aug 31, 2017

Choose a reason for hiding this comment

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

What if the symbol layer had no textField to begin with, or what if it was set to {ref}? Would highway shields start displaying road names instead of route numbers?

Note that, as a cartographic choice, most Mapbox-designed styles use {abbr} at low zoom levels. However, {abbr} is only available in English, so it should also be replaced by {name_*} when the language isn’t English. (See mapbox/mapbox-gl-native#9902 for the corresponding iOS/macOS issue.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can definitely be cleaned up, but here's mytextField check for now 😕 :

if (layer instanceof SymbolLayer) {
if (((SymbolLayer) layer).getTextField() != null) {
if (((SymbolLayer) layer).getTextField().getValue() != null &&
!((SymbolLayer) layer).getTextField().getValue().isEmpty()) {
if (((SymbolLayer) layer).getTextField().getValue().contains("name")) {
layer.setProperties(textField(String.format("{name_%s}", getDeviceLanguage())));
}

{abbr} check and adjustment:

if (((SymbolLayer) layer).getTextField().getValue().contains("abbr") &&
!getDeviceLanguage().equals("en")) {
layer.setProperties(textField(String.format("{name_%s}", getDeviceLanguage())));
}

/**
* Sets map text to Traditional Chinese.
*/
public void setMapTextToTraditionalChinese() {
Copy link
Contributor

Choose a reason for hiding this comment

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

{name_zh} is Chinese in general, not necessarily Traditional Chinese. For example, country labels are in Simplified Chinese, as are places in 🇨🇳.

Copy link
Contributor Author

@langsmith langsmith Aug 31, 2017

Choose a reason for hiding this comment

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

Cool. Changed the method name to setMapTextToChinese() and added explanatory javadocs for both methods. Now reads as 👇

/**
   * Sets map text to Chinese.
   *
   * This method uses simplified Chinese characters for custom label layers: #country_label, #state_label,
   * and #marine_label. All other label layers are sourced from OpenStreetMap and may contain one of several dialects
   * and either simplified or traditional Chinese characters in the {name_zh} field.
   */
  public void setMapTextToChinese() {
    setMapTextLanguage("zh");
  }

  /**
   * Sets map text to Simplified Chinese.
   *
   * Using this method is similar to setMapTextToChinese() (see above), except any Traditional Chinese
   * characters are automatically transformed to Simplified Chinese.
   */
  public void setMapTextToSimplifiedChinese() {
    setMapTextLanguage("zh-Hans");
  }

*/
public LocalizationPlugin(@NonNull final MapboxMap mapboxMap) {
listOfMapLayers = mapboxMap.getLayers();
String deviceLanguage = Locale.getDefault().getLanguage();
Copy link
Contributor

@1ec5 1ec5 Aug 31, 2017

Choose a reason for hiding this comment

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

According to this documentation, getLanguage() returns deprecated ISO 639 codes, such as iw for Hebrew instead of the current standard he. For forward-compatibility, consider using one of the other available methods.

Additionally, will this method return zh-Hans when the user chooses Chinese (Simplified) in their device settings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this commit which checks for and adjusts known deprecated ISO 639 codes. After lots of research, it doesn't seem like there's a single method that covers the backward and forwards compatibility that @1ec5 mentions above. Let's let my commit ✅ this comment for now so that this isn't a blocker for getting this plugin released.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This commit addresses simplified/traditional Chinese selection

Copy link
Contributor

Choose a reason for hiding this comment

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

How about toLanguageTag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, looked into it. For some reason, I think I read that it wouldn't work both ways, but I don't remember exactly why now 😵 . That link you just gave above, does sound like it would work for covering back and forwards compatibility. Will look back into it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using toLanguageTag() if api level is above 21.

Both issues brought up in OP have been addressed.

@langsmith
Copy link
Contributor Author

Thanks @1ec5 . Lots more to learn/fix 👍

There does need to be a list hard-coded somewhere, until mapbox/tilejson-spec#14 is >implemented. (Someday…) Note that the canonical list of available languages is in the Mapbox >Streets source v7 documentation.

Are there specific changes that I need to make related to ☝️ or was that just a comment? Couldn't tell

@1ec5
Copy link
Contributor

1ec5 commented Aug 31, 2017

Note that the canonical list of available languages is in the Mapbox >Streets source v7 documentation.

Are there specific changes that I need to make related to ☝️ or was that just a comment? Couldn't tell

No changes needed for now. The canonical list is maintained by the cartography team and is more likely to be kept up-to-date than the /help document.

/**
* Sets map text to Simplified Chinese.
*
* Using his method is similar to setMapTextToChinese() (see above), except any Traditional Chinese
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: his method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

/**
* Sets map text to Simplified Chinese.
*
* Using his method is similar to setMapTextToChinese() (see above), except any Traditional Chinese
Copy link
Contributor

Choose a reason for hiding this comment

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

(see above)

Doesn’t JavaDoc autolink method references? That would make glosses like this unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


<item
android:id="@+id/chinese"
android:title="Traditional Chinese"
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace with “Chinese”.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

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 to me (a total nondroid).

@cammace
Copy link
Contributor

cammace commented Dec 1, 2017

I cleaned up a bunch of the code and moved things around to reflect the latest changes made in master. Would be good to continue working on this and merging it.

@langsmith
Copy link
Contributor Author

Thanks @cammace

@zugaldia zugaldia requested review from osana and removed request for zugaldia December 23, 2017 17:47
@langsmith
Copy link
Contributor Author

After lots of refactoring, this is what the test app activity is looking like
ezgif com-resize 2

*/
private boolean sourceIsFromMapbox(Source singleSource) {
return singleSource instanceof VectorSource
&& ((VectorSource) singleSource).getUrl().contains("mapbox://mapbox.mapbox");
Copy link
Contributor

@1ec5 1ec5 Jan 10, 2018

Choose a reason for hiding this comment

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

Many Mapbox-designed styles – indeed, many styles designed in Studio – use a single composite source whose URL is mapbox://mapbox.mapbox-terrain-v2,mapbox.mapbox-streets-v7. By only checking if the URL begins with mapbox://mapbox.mapbox, this code may inadvertently snag sources that consist of mapbox.satellite or mapbox.terrain composited with a Studio-managed dataset.

The iOS/macOS map SDK’s implementation considers the source to be the Mapbox Streets source – the only one this plugin can reliably localize – if both of the following conditions hold:

  1. The URL scheme is mapbox; that is, the URL begins with mapbox://.
  2. The URL’s hostname is a comma-delimited list containing either mapbox.mapbox-streets-v7 or mapbox.mapbox-streets-v6. (The latter doesn’t matter so much at this point.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @1ec5 . Will adjust based on your comments. 🚀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although I don't create String arrays like the iOS/macOS map SDK’s implementation, I believe that the way I've handled scheme/hostname check will work just fine:

&& ((VectorSource) singleSource).getUrl().substring(0, 9).equals("mapbox://")
&& (((VectorSource) singleSource).getUrl().contains("mapbox.mapbox-streets-v7")
|| ((VectorSource) singleSource).getUrl().contains("mapbox.mapbox-streets-v6"));

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think it’s robust enough. It’s unlikely that someone would create an account named likemapbox and upload a dataset named mapbox-streets-v71 to it. It’s also going to be awhile before the cartography team gets a chance to release Mapbox Streets source v60.

@langsmith langsmith added the ready for review When your PR has been personally reviewed, its time for an external contributors to approve label Jan 12, 2018
@langsmith langsmith self-assigned this Jan 12, 2018
layer.setProperties(textField(String.format("{name_%s}", languageToSetMapTo)));
}
if (((SymbolLayer) layer).getTextField().getValue().contains("abbr")
&& !getDeviceLanguage().equals("en")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This assumes the style was set to use name_en in the first place, which is true about most Mapbox-designed styles like Mapbox Streets. However, it’d be totally reasonable for a style to use name (local language) right out of the gate.

For reference, the iOS and macOS map SDKs preserve the style’s original language (e.g., English) unless the localizesLabels feature is enabled. Once the feature is enabled, it changes every name and name_* to the device language, falling back to name_en if the device language is unsupported. So whether the style originally used name_en, name, or name_zh, the localization feature will still take effect.

(The Mapbox GL Language plugin for GL JS has a defaultLanguage option instead of falling back to English.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the && !getDeviceLanguage().equals("en") assumption in this commit 86be093

The comment below addresses the checking and fallback functions that I've built in.

Toast.makeText(this, R.string.map_not_localized, Toast.LENGTH_SHORT).show();
mapIsLocalized = false;
} else {
localizationPlugin.setMapTextLanguage(localizationPlugin.getDeviceLanguage());
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 device language happens to be a language that the Mapbox Streets source doesn’t support, such as Thai, this plugin will end up changing all the layers to {name_th}, producing undesired results. This plugin needs to check that the device language is supported, falling back to either English or the local language. (The iOS and macOS map SDKs fall back to English, whereas the Mapbox GL Language plugin for GL JS falls back to a default language that the developer specifies as an option.)

Copy link
Contributor Author

@langsmith langsmith Feb 5, 2018

Choose a reason for hiding this comment

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

I believe that af59d95 now fixes this. I've added a preferred backup language parameter for the initialization of the plugin.

public LocalizationPlugin(@NonNull MapboxMap mapboxMap, @NonNull String backupLanguage) {

I also added a check to make sure that a language is supported by Mapbox Streets style source

private boolean openStreetMapSupportsLanguage(String languageTag) {
return languageTag.equals("ar")
|| languageTag.equals("en")
|| languageTag.equals("es")
|| languageTag.equals("fr")
|| languageTag.equals("de")
|| languageTag.equals("pt")
|| languageTag.equals("ru")
|| languageTag.equals("zh")
|| languageTag.equals("zh-Hans");
}

If the check detects that the device language isn't supported, then the backup language is checked and set. English is the final fall back if the backup language isn't supported by Mapbox Streets style source either

public void setMapTextLanguage(String language) {
if (openStreetMapSupportsLanguage(getDeviceLanguage())) {
adjustMapTextLayers(language);
} else if (openStreetMapSupportsLanguage(selectedBackupLanguage)) {
adjustMapTextLayers(selectedBackupLanguage);
} else {
adjustMapTextLayers("en");
}
}

@langsmith langsmith removed the ready for review When your PR has been personally reviewed, its time for an external contributors to approve label Feb 5, 2018
* @param mapboxMap the MapboxMap to apply the localization plugin to
* @param mapboxMap the MapboxMap to apply the localization plugin to
* @param backupLanguage the default language for the map to fall back to if the device language is not
* supported by Open Street Maps.
Copy link
Contributor

@1ec5 1ec5 Feb 6, 2018

Choose a reason for hiding this comment

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

Nit: OpenStreetMap is capable of providing names in every language that has an ISO 639 code. (Coverage is uneven from one language to another.) It’s the Mapbox Streets source that’s currently limited to a handful of languages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks

if (((SymbolLayer) layer).getTextField().getValue().contains("abbr")
&& !getDeviceLanguage().equals("en")) {
if (((SymbolLayer) layer).getTextField().getValue().contains("name")
|| ((SymbolLayer) layer).getTextField().getValue().equals("abbr")) {
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 language is English and the text field is {abbr}, we should use {abbr}, because that field is written in English.

@langsmith
Copy link
Contributor Author

langsmith commented Feb 6, 2018

Per @1ec5 's suggestion, let's add installation instructions to this help page before releasing this plugin. I will handle this.

cc @colleenmcginnis

if (((SymbolLayer) layer).getTextField().getValue().contains("{name")
|| !getDeviceLanguage().equals("en") && ((SymbolLayer) layer).getTextField().getValue().contains("{abbr}")
) {
layer.setProperties(textField(String.format("{name_%s}", languageToSetMapTo)));
Copy link
Contributor

Choose a reason for hiding this comment

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

If text-field was originally set to something like {name_en} ({name}), this would replace it with just {name_en} for English speakers. I think it would be better to get the text field value as a string, then find and replace {name} and {name_*} with {name_xy} (where xy is languageToSetMapTo), then set the modified string as the text field.

Copy link
Contributor

Choose a reason for hiding this comment

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

I addressed this issue by using a regrex and replacing only the {name_*} part in textfields.

private boolean layerHasAdjustableTextField(Layer singleLayer) {
return singleLayer instanceof SymbolLayer && (((SymbolLayer) singleLayer).getTextField() != null
&& (((SymbolLayer) singleLayer).getTextField().getValue() != null
&& !(((SymbolLayer) singleLayer).getTextField().getValue().isEmpty())));
Copy link
Contributor

@1ec5 1ec5 Feb 7, 2018

Choose a reason for hiding this comment

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

As tail work, consider making this plugin also affect layers whose text-field property is set to a function. In the Mapbox Streets style, for example, U.S. state labels are a function with stops for either {name_en} or {abbr} based on the zoom level.

Without accounting for functions, there will be some inconsistency in state and country labels at low zoom levels. However, expressions will be available in the next release, so it might not be worth spending much effort accommodating functions in the meantime.

/ref mapbox/mapbox-gl-native#10713 mapbox/mapbox-gl-native#10944

Copy link
Contributor

Choose a reason for hiding this comment

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

However, expressions will be available in the next release, so it might be worth spending much effort accommodating functions in the meantime.

I'd prefer merging this PR and fixing this in a different branch in the future when we switch to 6.0.0.

Copy link
Member

Choose a reason for hiding this comment

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

@cammace Is there a ticket in this repo where we're tracking this tail work?

Copy link
Contributor

Choose a reason for hiding this comment

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

I ticketed this out in #301

@cammace cammace merged commit 401b669 into master Feb 21, 2018
@cammace cammace deleted the ls-localization-plugin branch February 22, 2018 15:46
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.

Localization plugin
6 participants