Reimplement label localization atop Expression (redux)#3330
Conversation
There was a problem hiding this comment.
I consolidated this operation with the block above. I’m pretty sure all of these style customizations need to be reapplied every time the style changes.
There was a problem hiding this comment.
Road labels are untouched and remain in English if the original style had them in English, as does the default Mapbox Navigation Day/Night v1 style. This is due to a missing special case upstream: mapbox/mapbox-maps-ios#653. It’s a regression from v1.x.
There was a problem hiding this comment.
Localization is currently broken for most Chinese locales, including Simplified Chinese and Hong Kong Traditional Chinese: mapbox/mapbox-maps-ios#652.
There was a problem hiding this comment.
I worked around this issue for now by ensuring that only supported locales make it through to the map SDK.
There was a problem hiding this comment.
The map SDK throws a fatal error rather if we pass in an unsupported Locale: mapbox/mapbox-maps-ios#655. This is problematic because the user could have set the system language preference to some language that the Streets source doesn’t support, triggering a crash. If this issue isn’t fixed upstream, we’ll have to hard-code the same array of supported languages in the navigation SDK, making it even more difficult for the server team to add support for new language.
There was a problem hiding this comment.
If this issue isn’t fixed upstream, we’ll have to hard-code the same array of supported languages in the navigation SDK, making it even more difficult for the server team to add support for new language.
I wound up implementing this workaround for now. We can easily remove the workaround once the map SDK is fixed.
There was a problem hiding this comment.
@mapbox/navigation-ios do we know if it is safe to change to onEvery?
There was a problem hiding this comment.
As of #3332, the only thing left using .onNext will be the call to localizeLabels(), which does need to be reapplied every time the style changes.
There was a problem hiding this comment.
I'm also not sure that we should apply this change. Basically now any changes to the NavigationMapView.mapView.showsTraffic, which were made by the end user will be ignored.
There was a problem hiding this comment.
Any time the style changes, their change to showsTraffic would get blown away anyways. If the developer manually makes any runtime styling change to the style, they need to either respond to StyleManagerDelegate methods or use onEvery(_:handler:) – that would include showsTraffic.
There was a problem hiding this comment.
I ended up porting +[MGLVectorTileSource preferredMapboxStreetsLanguageForPreferences:] from map SDK v6.x to work around mapbox/mapbox-maps-ios#652 and mapbox/mapbox-maps-ios#655. I would very much like not to have to hard-code the supported Mapbox Streets source locales in this codebase, but this is the only way to work around the blanking and crashing issues for now.
There was a problem hiding this comment.
If this issue isn’t fixed upstream, we’ll have to hard-code the same array of supported languages in the navigation SDK, making it even more difficult for the server team to add support for new language.
I wound up implementing this workaround for now. We can easily remove the workaround once the map SDK is fixed.
There was a problem hiding this comment.
I worked around this issue for now by ensuring that only supported locales make it through to the map SDK.
There was a problem hiding this comment.
I'm also not sure that we should apply this change. Basically now any changes to the NavigationMapView.mapView.showsTraffic, which were made by the end user will be ignored.
1ec5
force-pushed
the
1ec5-label-l10n-upstream
branch
from
a390b3d to
6add4de
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Shall we also update changelog to mention that |
We should mention it in the next release candidate’s release notes but not the running changelog for v2.0.0, because it was already working in v1.x. |
1ec5
force-pushed
the
1ec5-label-l10n-upstream
branch
from
048c9f3 to
655735b
Compare
Ported language preference selection logic from MGLVectorTileSource in map SDK v6.x. Used this logic to ensure that the map SDK is only ever asked to localize labels into a language it supports.
1ec5
force-pushed
the
1ec5-label-l10n-upstream
branch
from
655735b to
54031cf
Compare
4 participants
Restored the
NavigationMapView.localizeLabels()implementation for automatically localizing map labels into the user’s language. The previous implementation was removed as part of upgrading to map SDK v10 in #2808, because it relied on an NSExpression-based localization feature in map SDK v6.x. This implementation continues to be based on the map SDK’s built-in label localization functionality rather than reinventing the wheel as in #2933./cc @mapbox/navigation-ios @neelmistry94 @tobrun