[Localization] Add new param to allow users localize selected layers. #461
Conversation
| */ | ||
| fun StyleInterface.localizeLabels(locale: Locale) { | ||
| setMapLanguage(locale, this) | ||
| fun StyleInterface.localizeLabels(locale: Locale, layerIds: List<String>? = null) { |
There was a problem hiding this comment.
Perhaps use emptyList() as default parameter and make layerIds not nullable then? imho it will work the same but code will be cleaner
There was a problem hiding this comment.
As shown in this line a nullable list can make the code more cleaner.
There was a problem hiding this comment.
Wouldn't layerIds.contains(layer.id) work the same here? 🤔
There was a problem hiding this comment.
I see now, I guess not.
My only concern - will the code work OK if user would pass emptyList() explicitly?
There was a problem hiding this comment.
This list contains the layers they want to localize, an empty list means no layers will be localized and the filter will filter out all the layers.
There was a problem hiding this comment.
So having an empty list is equivalent to passing null. In that case I'd make this parameter not nullable if it acts the same as null and pass empty list as default.
There was a problem hiding this comment.
If I understand correctly, passing empty list will result in no localization at any layer at all, please correct me if I'm wrong @Chaoba
There was a problem hiding this comment.
I see that iOS is also using null by default, ok let's leave it this way to be aligned. Although passing empty list basically meaning that function will not work at all seems pretty useless to me.
There was a problem hiding this comment.
Passing null will localize all feasible layers, which is the default action, while an empty list will not localize any layer.
| .forEach { layer -> | ||
| val symbolLayer = style.getLayerAs<SymbolLayer>(layer.id) | ||
| symbolLayer.textFieldAsExpression?.let { textFieldExpression -> | ||
| if (BuildConfig.DEBUG) { |
There was a problem hiding this comment.
I don't remember using that anywhere else in project. Do we really need that logging? I'd remove it, we can always add it manually if we really need it.
There was a problem hiding this comment.
Actually, I have added and removed the log output many times and these logs are really important for developing. Is there any particular reason we can't use this?
There was a problem hiding this comment.
I don't have any particular reasons.
@mapbox/maps-android WDYT?
There was a problem hiding this comment.
If this message isn't printed frequently, I'd keep the log. And probably we need a centralised solution to filter out log messages for specific plugins.
There was a problem hiding this comment.
There would be about a dozen of log output as we have several symbol layers in the style.
There was a problem hiding this comment.
I'm fine with using BuildConfig.DEBUG, users won't ever hit this. The filter log feature is being developed upstream atm.
extension-localization/src/main/java/com/mapbox/maps/extension/localization/Localization.kt
Show resolved
Hide resolved
PRs must be submitted under the terms of our Contributor License Agreement CLA.
Fixes: < Link to related issues that will be fixed by this pull request, if they exist >
Pull request checklist:
mapbox-maps-androidchangelog:<changelog>Add new param to allow users localize selected layers.</changelog>.Summary of changes
This pr add a new param
layerIdsfor APIlocalizeLabels,layerIdsis a list, user can put all the layers they want to localize in this list, other layers not in this list will not be localized.The default value of
layerIdsis null which means localize all the feasible layers.device-2021-06-30-161749.mp4
User impact (optional)