Handle dynamic list of identity providers #1799
Conversation
The Facebook Android SDK link is quite old. We should point to the official Facebook Android SDK changelog in Github: https://github.com/facebook/facebook-android-sdk/blob/master/CHANGELOG.md
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
@googlebot I signed it! |
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
|
@googlebot I fixed it. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
@robapaul88 thanks for this contribution! BTW I changed the target branch to |
| View loginButton = findViewById(buttonId); | ||
| handleSignInOperation(idpConfig, loginButton); | ||
| } | ||
| //hide custom layout buttons that don't have their identity provider set | ||
| for (String providerBtnId : providerButtonIds.keySet()) { | ||
| if (providerBtnId == null) continue; |
There was a problem hiding this comment.
nit: we try not to use single-line if statements
| if (providerBtnId == null) continue; | |
| if (providerBtnId == null) { | |
| continue; | |
| } |
| } | ||
| if (!hasProvider) { | ||
| Integer resId = providerButtonIds.get(providerBtnId); | ||
| if (resId == null) continue; |
There was a problem hiding this comment.
| if (resId == null) continue; | |
| if (resId == null) { | |
| continue; | |
| } |
| } | ||
| if (!hasProvider) { | ||
| Integer resId = providerButtonIds.get(providerBtnId); | ||
| if (resId == null) continue; |
There was a problem hiding this comment.
Is this null condition even possible? How would we get into this state?
There was a problem hiding this comment.
Well, it's an unboxing, In theory I know that map should only have provider ids and layout res ids. But since that Map is passed around by reference, placing an Integer in it that's later nullified, you can end up with a null here. Better safe than sorry.
|
Created #1800 to have this pushed to the right branch, redirecting this from master seems to also affect some other files (probably reverting some other commits from 6.3.0-dev). |
This will take care of also hiding buttons from the custom layout when adding them for all possible identity providers, but the actual list is dynamic so some won't be available.
Issue: #1775