Add method to obtain native string resource in localization plugin. #24575
Conversation
…localization plugin.
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| String parts[] = localeString.split("-", -1); | ||
| String languageCode = parts[0]; | ||
| String countryCode = parts.length > 1 ? parts[1] : null; | ||
| String scriptCode = parts.length > 2 ? parts[2] : null; |
There was a problem hiding this comment.
I believe scriptCode comes before countrycode in locale strings. Though I'd have to double check to see the exact spec. It should be something like country/region code is 2char, while scriptcode is 4char. In most instances, the scriptcode has come first, eg, zh-Hans-cn
There is also divergent behavior with using - and _. I'd replace one to the other to handle both for robustness.
There was a problem hiding this comment.
You're right, I got confused by the order of the country code in the constructor.
Fixed this as suggested!
|
This needs tests! |
|
Added tests, PTAL, thanks! |
| stringToReturn = localContext.getResources().getString(resId); | ||
| } | ||
|
|
||
| if (localeString != null && Build.VERSION.SDK_INT < Build.VERSION_CODES.JELLY_BEAN_MR1) { |
There was a problem hiding this comment.
nit, comment the API number for Build.VERSION_CODES.JELLY_BEAN_MR1
| if (localeString != null) { | ||
| Locale locale = localeFromString(localeString); | ||
|
|
||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN_MR1) { |
There was a problem hiding this comment.
nit, comment the API number for Build.VERSION_CODES.JELLY_BEAN_MR1, same as below.
shell/platform/android/io/flutter/plugin/localization/LocalizationPlugin.java
Show resolved
Hide resolved
| Locale locale = localeFromString(localeString); | ||
|
|
||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN_MR1) { | ||
| Configuration conf = new Configuration(context.getResources().getConfiguration()); |
There was a problem hiding this comment.
nit, spell out config or configuration.
There was a problem hiding this comment.
Done. I went with config since because this file has already used config.
| */ | ||
| public interface LocalizationMessageHandler { | ||
| /** | ||
| * The Flutter application would like to obtain the string resource of given {@code key} in |
There was a problem hiding this comment.
Rephrase to be less passive/third person.
perhaps Obtains the string resource of the provided {@code key} ...
There was a problem hiding this comment.
I copied the tone from PlatformChannel.java.
The way I understand it is this comment is meant for the implementor of this interface.
What do you think?
|
Updated according to reviews :) |
| @VisibleForTesting | ||
| public static Locale localeFromString(String localeString) { | ||
| // Use Locale.forLanguageTag if available (API 21+). | ||
| if (false && Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.LOLLIPOP) { |
There was a problem hiding this comment.
Any chance you remember why this if block exist?
There was a problem hiding this comment.
Most likely a mistake. The false && was probably added to temporarily test the else branch and I forgot to remove that.
Do you want me to send a PR to undo this?
Engine side implementation for go/flutter-locale-split.
Only Android implementation for now just for review. After this is reviewed I'll work on the iOS implementation in a separate PR.
Tests will follow.