Bug fixes and perf improvements for setting providers in AuthUI #591
Conversation
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
There was a problem hiding this comment.
@samtstern I feel like this is a good change, but since I didn't create an issue first I'd be fine with you telling me no.
|
|
||
| IdpConfig config = (IdpConfig) o; | ||
|
|
||
| return mProviderId.equals(config.mProviderId); |
There was a problem hiding this comment.
@samtstern Identifying providers by provider id only and completely ignoring scopes makes sense to me, do you agree?
| private int mLogo = NO_LOGO; | ||
| private int mTheme = getDefaultTheme(); | ||
| private LinkedHashSet<IdpConfig> mProviders = new LinkedHashSet<>(); | ||
| private List<IdpConfig> mProviders = new ArrayList<>(); |
There was a problem hiding this comment.
This is much better, we were converting this to a list anyway which seems kinda pointless.
| */ | ||
| public SignInIntentBuilder setProviders(@NonNull List<IdpConfig> idpConfigs) { | ||
| mProviders.clear(); | ||
| Set<String> configuredProviders = new HashSet<>(); |
There was a problem hiding this comment.
Less memory usage if you don't overlap providers! 😄
| return this; | ||
| } | ||
|
|
||
| private boolean isIdpAlreadyConfigured(@NonNull String providerId) { |
There was a problem hiding this comment.
I moved it to where it's being used (in the deprecated setProviders).
| assertTrue(emailScopes.size() == 2); | ||
| assertTrue(emailScopes.contains("A") && emailScopes.contains("B")); | ||
|
|
||
| assertTrue(providers.get(1).getProviderId().equals(AuthUI.GOOGLE_PROVIDER)); |
There was a problem hiding this comment.
@samtstern Did I get all the possible cases?
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
|
@SUPERCILEX what is the use case for specifying the same provider multiple times in the builder? I think throwing an Exception is good since I doubt the developer meant to specify duplicate providers and we are probably helping them catch a mistake. |
|
@samtstern If a user enables a setting in the setup of the app, they get logged in with different permissions than the standard setup. This means I have to take my default configuration, remove the |
|
@SUPERCILEX my personal opinion is that there will be more people saved from doing the wrong thing by the error-throwing behavior than there will be people who appreciate the merging of IdpConfig. |
# Conflicts: # auth/src/main/java/com/firebase/ui/auth/AuthUI.java
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
SUPERCILEX
changed the title
AuthUI.SignInIntentBuilder#setProviders(List<IdpConfig>) smarter
|
@samtstern Okay, that makes sense. I made some improvements to increase readability and reduce memory thrashing along the way so I'll leave this PR open and focus its changes on that instead. |
| public FlowParameters getFlowParams() { | ||
| if (mProviders.isEmpty()) { | ||
| mProviders.add(new IdpConfig.Builder(EMAIL_PROVIDER).build()); | ||
| } |
There was a problem hiding this comment.
This is why we should always do this kind of thing in the build method: if people call setProviders(Collections.emptyList()), we will start the auth flow with no providers. Now, we can guarantee that at least the email provider is provided.
|
@SUPERCILEX SGTM, we can move forward with the general improvements. |
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
SUPERCILEX
changed the base branch from
version-1.2.0-dev
to
version-2.0.0-dev
|
Forgot that you had updated this, but LGTM. Squashing and merging. |
|
@samtstern Oops, I forgot to ask you to wait for @amandle's PR to be merged since this one caused a bunch of merge conflicts. I created amandle#1 to fix the conflicts. Sorry about that! |
3 participants
@samtstern We currently throw an exception, but I think we should be smarter about this. Now, everything gets merged together into one unique set of providers and scopes.