Include/exclude countries from the country selector list in phone authentication #1315
Conversation
| } | ||
|
|
||
| @Override | ||
| protected List<CountryInfo> doInBackground(Void... params) { | ||
| final List<CountryInfo> countryInfoList = new ArrayList<>(MAX_COUNTRIES); |
There was a problem hiding this comment.
@SUPERCILEX I rememeber last time you touched this code you had a reason to keep this list separate from the map in PhoneNumberUtils. Is it not exactly inverted?
There was a problem hiding this comment.
@lsirac btw holding my approval on this discussion.
There was a problem hiding this comment.
Ok I went through and actually just printed out the map in each case. And there is no diff in the map information:
http://www.mergely.com/MhLwmZdA/
| Collections.sort(countryInfoList); | ||
| return countryInfoList; | ||
| } | ||
|
|
||
| public List<CountryInfo> getAvailableCountryCodes() { | ||
| Map<String, Integer> countryInfoMap = PhoneNumberUtils.getImmutableCountryCodeMap(); | ||
| if (whitelistedCountryCodes == null && blacklistedCountryCodes == null) { |
There was a problem hiding this comment.
I think if you want this to be the default you should do this in the constructor and then let every other method in the class assume that the whitelist and blacklist are setup properly.
There was a problem hiding this comment.
Might also want to throw an IllegalStateException in the constructor if they're both non-null.
There was a problem hiding this comment.
I'm not sure if I agree with adding an IllegalStateException. I moved that bit to the constructor though.
| if (whitelistedCountryCodes == null) { | ||
| excludedCountries.addAll(blacklistedCountryCodes); | ||
| } else { | ||
| excludedCountries.addAll(countryInfoMap.keySet()); |
There was a problem hiding this comment.
Add some comments here, as the assumptions took me a minute to wrap my head around (I think you're saying that at this point you know only one of whitelist or blacklist will be non-null so you're just handling the two cases for whitelist and assuming the opposite about blacklist)
| Collections.sort(countryInfoList); | ||
| return countryInfoList; | ||
| } | ||
|
|
||
| public List<CountryInfo> getAvailableCountryCodes() { | ||
| Map<String, Integer> countryInfoMap = PhoneNumberUtils.getImmutableCountryCodeMap(); | ||
| if (whitelistedCountryCodes == null && blacklistedCountryCodes == null) { |
There was a problem hiding this comment.
Might also want to throw an IllegalStateException in the constructor if they're both non-null.
| @@ -134,6 +135,8 @@ public void onActivityCreated(@Nullable Bundle savedInstanceState) { | |||
| // It is assumed that the phone number that are being wired in via Credential Selector | |||
| // are e164 since we store it. | |||
| Bundle params = getArguments().getBundle(ExtraConstants.PARAMS); | |||
| List<String> whitelistedCountryCodes = null; | |||
There was a problem hiding this comment.
I think these two variables are not used?
There was a problem hiding this comment.
Removed them in my last commit.
| Bundle params = getArguments().getBundle(ExtraConstants.PARAMS); | ||
| if (params != null) { | ||
| mCountryListSpinner.setWhitelistedCountryCodes( | ||
| params.getStringArrayList(ExtraConstants.WHITELISTED_COUNTRY_CODES)); |
There was a problem hiding this comment.
What's the default for getStringArrayList, is it null?
There was a problem hiding this comment.
Yes, it returns null if the key isn't present.
|
@lsirac as we talked about in chat, let's use |
|
I just took a look and, amazingly, it doesn't look like this will create any conflicts with #1253 |
| mListener = listener; | ||
| countryInfoMap = PhoneNumberUtils.getImmutableCountryIsoMap(); |
There was a problem hiding this comment.
@lsirac would it mess you up if this work was moved to doInBackground? I think originally there was a concern that the construction of this map was expensive enough to be done in the background (motivates the whole existence of this class).
If you can move it to there, then I am confident this is a safe change.
There was a problem hiding this comment.
Looking around, I think that just means you will want to lazy-initialize the big static members of PhoneNumberUtils and expose getters for them.
No description provided.