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

[ios, macos] Fix MGLSymbolStyleLayer.text localization issue. #14405

Merged
merged 3 commits into from
Apr 15, 2019

Conversation

fabian-guerra
Copy link
Contributor

@fabian-guerra fabian-guerra commented Apr 11, 2019

Fixes #14393 introduced in #14094

Localization was not performed due to the introduction of MGLAttributedExpression object. At localization time there was not a proper converter for format expressions, returning the same expression thus avoiding localization.

This is how it looks now on iOS

german

This is how it looks now on macOS

germanmacos

@fabian-guerra fabian-guerra added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS localization Human language support and internationalization labels Apr 11, 2019
@fabian-guerra fabian-guerra added this to the release-liquid milestone Apr 11, 2019
@fabian-guerra fabian-guerra requested review from 1ec5, julianrex and a team April 11, 2019 21:53
@fabian-guerra fabian-guerra self-assigned this Apr 11, 2019
@friedbunny
Copy link
Contributor

friedbunny commented Apr 11, 2019

We have MGLExpressionTests.testLocalization — I think we should:

  1. Add a new test for the MGLAttributedExpression type.
  2. Make the tests fail (or have some sort of warning, somewhere) if an unexpected type has localization attempted on it.

@friedbunny friedbunny added the release blocker Blocks the next final release label Apr 11, 2019
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.

Per #14393 (comment), let’s make sure our unit tests cover the cases of localizing both unformatted and formatted expressions. (They can be unit tests, because what’s important is the structure of the resulting expression, not so much the text that comes out of Streets source vector tiles.)

Suggested changelog entry:

  • Fixed an issue where the -[MGLStyle localizeLabelsIntoLocale:] and -[NSExpression mgl_expressionLocalizedIntoLocale:] methods had no effect. (#14405)

@@ -1471,6 +1471,12 @@ - (NSExpression *)mgl_expressionLocalizedIntoLocale:(nullable NSLocale *)locale
if (localizedValues != self.constantValue) {
return [NSExpression expressionForConstantValue:localizedValues];
}
} else if ([self.constantValue isKindOfClass:[MGLAttributedExpression class]]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What’s our theory for how this caused #14393? I was able to reproduce the issue in the standard Streets v10 and v11 styles. Is it because the MGLSymbolStyleLayer.text setter always returns an attributed expression now, even when the style JSON contains only a constant string value? If so, this may be indicative of a backwards-incompatible change that took place with the introduction of formatted expressions: a developer might reasonably expect to get an ordinary string out of the text property, just as we apparently did.

@1ec5
Copy link
Contributor

1ec5 commented Apr 11, 2019

Make the tests fail (or have some sort of warning, somewhere) if an unexpected type has localization attempted on it.

-[MGLStyle localizeLabelsIntoLocale:] works by attempting to localize everything (the text of every MGLSymbolStyleLayer). It’s designed to fail silently so that developers can set it and forget it. But these tests should assert that text’s getter returns the expected expression type. It’s a good idea in general for unit tests to spell out their assumed preconditions.

@fabian-guerra
Copy link
Contributor Author

Will followup with a style localization test.

@fabian-guerra fabian-guerra force-pushed the fabian-localize-14393 branch from 45adc40 to cb4dfd9 Compare April 15, 2019 17:55
@fabian-guerra fabian-guerra changed the title [ios, macos] Fix a localization issue. [ios, macos] Fix MGLSymbolStyleLayer.text localization issue. Apr 15, 2019
@fabian-guerra fabian-guerra merged commit a8526b4 into master Apr 15, 2019
fabian-guerra added a commit that referenced this pull request Apr 15, 2019
Fixed an MGLSymbolStyleLayer.text localization bug caused by the introduction of MGLAttributedExpression object. The localization parsing was ignoring the latter.

* [ios, macos] Fix a localization issue.

* [ios, macos] Add formating expressions localization test.

* [ios, macos] Update MGLSymbolStyleLayer.text documentation.
fabian-guerra added a commit that referenced this pull request Apr 15, 2019
Fixed an MGLSymbolStyleLayer.text localization bug caused by the introduction of MGLAttributedExpression object. The localization parsing was ignoring the latter.

* [ios, macos] Fix a localization issue.

* [ios, macos] Add formating expressions localization test.

* [ios, macos] Update MGLSymbolStyleLayer.text documentation.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
iOS Mapbox Maps SDK for iOS localization Human language support and internationalization macOS Mapbox Maps SDK for macOS release blocker Blocks the next final release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to localize labels
4 participants