Complete rewrite/refactor of email sign in flow by SUPERCILEX · Pull Request #418 · firebase/FirebaseUI-Android

SUPERCILEX · 2016-11-24T03:46:11Z

I'm really excited about this one!!! The performance/ui/code is massively improved: we've gone from jumping through 3 activities to just saying on RegisterEmailActivity and everything regarding email flow is so much simpler and easier to understand now! Here are some of the important changes:

fixes Propagate display name from Smart Lock for Passwords to account creation screen #129
lets the user update their email in the RegisterEmailActivity
no more confusing activity jumping (except when they already have an account, but that's for another day)
performance improvements
less code, fewer tests, and more features = happiness

SUPERCILEX · 2016-11-24T03:50:42Z

auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java

+                                                                                       RC_WELCOME_BACK_IDP);
+                                            }
+                                        }
+                                    } // TODO: 11/23/2016 what happens if we fail?


Thoughts? We didn't do anything before, but I think we should display a message or at least do something.

Yeah not sure what the best course of action is here ... since we didn't do anything before let's leave this for another time.

SUPERCILEX · 2016-11-24T03:54:36Z

auth/src/main/res/values/strings.xml

    <string name="progress_dialog_sending">Sending…</string>
    <string name="progress_dialog_signing_in">Signing in…</string>
    <string name="progress_dialog_signing_up">Signing up…</string>
+    <string name="progress_dialog_checking_accounts">Checking for existing accounts…</string>


Does this sound good?

…factor # Conflicts: # auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java

…factor

SUPERCILEX · 2016-12-01T03:53:36Z

I've been off and on trying to figure out if a memory leak was caused by my changes in this PR and I finally figured it out! Turns out this PR wasn't the cause, but RegisterEmailActivity helped me figure it out because it creates an instance of SaveSmartLock.

Full explanation:

I've stubbed my toe several times now with setRetainInstance(true) and I think I've finally eradicated all memory leaks related to the smart lock stuff. (Yay! 😄) If you look at the commit fixing the smart lock leak, you'll notice that I was passing in Fragment#getContext() to BaseHelper's constructor which stores the context in mContext. (BaseHelper's subclass, FragmentHelper is initialized in Fragment#onCreate which only occurs once because of setRetainInstance). In this case, mContext is the activity associated with SaveSmartLock. This would work fine without setRetainInstance(true), however, because the fragment lives longer than its activity, we leak SaveSmartLock's activity aka mContext. A simple fix is to use Fragment#getContext()#getApplicationContext(). Unfortunately, ProgressDialogs don't work with getApplicationContext() so I'm using fragment.getContext() only when needed (to show the dialog).

TLDR

Keep an eye out for getContext() leaks in setRetainInstance(true) fragments!

SUPERCILEX · 2016-12-01T04:09:29Z

auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java

-        Button createButton = (Button) findViewById(R.id.button_create);
-        createButton.setOnClickListener(this);
+
+        showEmailAutoCompleteHint();


Me previously:

Why are we doing this only if smart lock is enabled? I think users should get autocomplete whether or not devs have an aversion to smart lock.

I went through with that idea so unless anyone has any objections, I really think autocomplete should be available whether or not smart lock is enabled.

Technically the email hint is provided by SmartLock, so this would make the API somewhat dishonest. Although I agree with you that enabling the hint for everyone may be a superior UX, I want to respect the intent of the developer when they say "disable smartlock".

Sounds fair, but still it makes me sad inside. 😄

…factor

samtstern

Mostly LGTM! Just double-checking that I understand the full scope here: rather than have SignInNoPasswordActivity which takes an email alone and checks for existing accounts, we are moving all of that into RegisterEmailActivity and doing the check right there behind a progress dialog.

Any other major changes?

samtstern · 2016-12-02T21:13:08Z

auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java

-        Button createButton = (Button) findViewById(R.id.button_create);
-        createButton.setOnClickListener(this);
+
+        showEmailAutoCompleteHint();


Technically the email hint is provided by SmartLock, so this would make the API somewhat dishonest. Although I agree with you that enabling the hint for everyone may be a superior UX, I want to respect the intent of the developer when they say "disable smartlock".

samtstern · 2016-12-02T21:35:33Z

auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java

+
+    @Override
+    public void onFocusChange(View view, boolean hasFocus) {
+        if (hasFocus) return;


Just a general style comment (first noticed it here) but let's add some more comments in meaty methods like this. For instance in this method the first line could use a comment like // Only consider fields losing focus.

samtstern · 2016-12-02T21:37:11Z

auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java

+                                    if (task.isSuccessful()) {
+                                        List<String> providers = task.getResult().getProviders();
+                                        if (providers != null && !providers.isEmpty()) {
+                                            // Account does exist


Recommended to expand comments to something like this:

// There is an account tied to this email. If only the email provider is associated with the account, direct the user // to the email sign in flow. Otherwise, direct the user to sign in with the IDP they previously selected.

samtstern · 2016-12-02T21:39:16Z

auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java

+                            new OnCompleteListener<ProviderQueryResult>() {
+                                @Override
+                                public void onComplete(@NonNull Task<ProviderQueryResult> task) {
+                                    if (task.isSuccessful()) {


nit: use separate success and failure listeners rather than checking task.isSuccessful()

samtstern · 2016-12-02T21:40:33Z

auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java

+                                                                                       RC_WELCOME_BACK_IDP);
+                                            }
+                                        }
+                                    } // TODO: 11/23/2016 what happens if we fail?


Yeah not sure what the best course of action is here ... since we didn't do anything before let's leave this for another time.

samtstern · 2016-12-02T21:41:50Z

auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java

                    }
                })
-                .addOnFailureListener(new OnFailureListener() {
+                .addOnFailureListener(this, new OnFailureListener() {


Any reason we're only tying the failure listener to the Activity and not the success listener?

Here was my logic:

If the request succeeds, we to continue whether or not the original activity is still there because mSaveSmartLock is already initialized and not tied to the activity lifecycles. That way, the activity will finish and the sign in will proceed.

If it fails, the only thing we are doing in the failure listener is updating views. I'm not sure how that works, but I would think something bad will happen if we try to update a view from a previous instance of an activity. Am I wrong?

Update:

I found RecoverEmailActivity doing the same thing, so I think my logic is correct.

samtstern · 2016-12-02T21:45:05Z

auth/src/main/res/values/strings.xml

    <string name="progress_dialog_sending">Sending…</string>
    <string name="progress_dialog_signing_in">Signing in…</string>
    <string name="progress_dialog_signing_up">Signing up…</string>
+    <string name="progress_dialog_checking_accounts">Checking for existing accounts…</string>


…factor

SUPERCILEX

Yup, this is mostly just unifying the email sign in experience with some small improvements peppered here and there.

SUPERCILEX · 2016-12-02T22:52:10Z

app/src/main/java/com/firebase/uidemo/auth/AuthUiActivity.java

    private static final String UNCHANGED_CONFIG_VALUE = "CHANGE-ME";
    private static final String GOOGLE_TOS_URL = "https://www.google.com/policies/terms/";
-    private static final String FIREBASE_TOS_URL = "https://www.firebase.com/terms/terms-of-service.html";
+    private static final String FIREBASE_TOS_URL = "https://firebase.google.com/terms/";


Just noticed this was using the old Firebase.

Wow, good catch :-)

SUPERCILEX · 2016-12-02T22:53:04Z

auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java

+                        .build()
+                        .launchUrl(
+                                RegisterEmailActivity.this,
+                                Uri.parse(mActivityHelper.getFlowParams().termsOfServiceUrl));


Since Facebook forces us to use CCT, I don't see why we shouldn't modernize.

Agreed, looks great in your screencast!

SUPERCILEX · 2016-12-02T22:54:57Z

auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java

-                startActivity(intent);
+                // Getting default color
+                TypedValue typedValue = new TypedValue();
+                getTheme().resolveAttribute(R.attr.colorPrimary, typedValue, true);


This works, but what if they don't have a colorPrimary. (Actually, is it even possible to not have a colorPrimary? I'm pretty sure AppCompatActivity uses that for all default theming)

I think this should be fine ... and if people see a crash I think saying "you must define colorPrimary" is a reasonable response so not too worried.

SUPERCILEX · 2016-12-02T22:57:27Z

auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java

                        } else if (e instanceof FirebaseAuthUserCollisionException) {
                            // Collision with existing user email
                            emailInput.setError(getString(R.string.error_user_collision));
+                            checkAccountExists(mEmailEditText.getText().toString());


Come to think of it, this could be our error handling if fetchProvidersForEmail fails: the user can't sign up if a user already exists and we will retry loading that user.

samtstern

LGTM! I think I will create a 1.1.0 branch for your three outstanding PRs.

samtstern · 2016-12-06T16:55:44Z

app/src/main/java/com/firebase/uidemo/auth/AuthUiActivity.java

    private static final String UNCHANGED_CONFIG_VALUE = "CHANGE-ME";
    private static final String GOOGLE_TOS_URL = "https://www.google.com/policies/terms/";
-    private static final String FIREBASE_TOS_URL = "https://www.firebase.com/terms/terms-of-service.html";
+    private static final String FIREBASE_TOS_URL = "https://firebase.google.com/terms/";


Wow, good catch :-)

samtstern · 2016-12-06T16:58:50Z

auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java

-                startActivity(intent);
+                // Getting default color
+                TypedValue typedValue = new TypedValue();
+                getTheme().resolveAttribute(R.attr.colorPrimary, typedValue, true);


I think this should be fine ... and if people see a crash I think saying "you must define colorPrimary" is a reasonable response so not too worried.

samtstern · 2016-12-06T16:59:00Z

auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java

+                        .build()
+                        .launchUrl(
+                                RegisterEmailActivity.this,
+                                Uri.parse(mActivityHelper.getFlowParams().termsOfServiceUrl));


Agreed, looks great in your screencast!

samtstern · 2016-12-06T16:59:50Z

auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java

                    }
                })
-                .addOnFailureListener(new OnFailureListener() {
+                .addOnFailureListener(this, new OnFailureListener() {


samtstern · 2016-12-06T17:09:26Z

I just tried to test all of your PRs together and there are some small lingering conflicts. So I made a branch called version-1.1.0-dev from master.

Let's move PRs #418, #420, and #426 to target that branch and then merge them into there one by one (fixing conflicts as we go). Then we can do some poke-around QA, make sure all dependencies are up to date, and cut a version 1.1.0 release. Sound good?

…factor

SUPERCILEX · 2016-12-06T17:24:16Z

@samtstern Sounds good, I changed the base for all my PRs and pulled from master.

SUPERCILEX added 3 commits November 23, 2016 19:30

Initial refactor

d515d5f

Fix broken tests

cf342ce

Cleanup

c8a0d26

SUPERCILEX commented Nov 24, 2016

View reviewed changes

SUPERCILEX mentioned this pull request Nov 24, 2016

Refactor container activities #374

Closed

SUPERCILEX added 6 commits November 28, 2016 11:31

Merge remote-tracking branch 'firebase/master' into register-email-re…

b7d9054

…factor # Conflicts: # auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java

Fix merge mistakes

0c5701a

Merge remote-tracking branch 'firebase/master' into register-email-re…

a793f8c

…factor

Remove unused resources

d7ebebc

Fix smart lock memory leak

78e83ca

Fix another memory leak

f304277

Cleanup

5aa595c

SUPERCILEX commented Dec 1, 2016

View reviewed changes

SUPERCILEX mentioned this pull request Dec 1, 2016

Fix leaks #429

Merged

Merge remote-tracking branch 'firebase/master' into register-email-re…

5080f82

…factor

samtstern reviewed Dec 2, 2016

View reviewed changes

SUPERCILEX added 2 commits December 2, 2016 14:50

Address review comments and add CCT

98ed856

Merge remote-tracking branch 'firebase/master' into register-email-re…

44408f1

…factor

SUPERCILEX commented Dec 2, 2016

View reviewed changes

Tie WelcomeBackPasswordPrompt.java failure listener to activity

8d162f5

samtstern approved these changes Dec 6, 2016

View reviewed changes

This was referenced Dec 6, 2016

Add and enforce checkstyle, lint, findbugs, and pmd #420

Merged

Differentiate between user cancellation and actual errors #426

Merged

Merge remote-tracking branch 'firebase/master' into register-email-re…

e51f561

…factor

SUPERCILEX changed the base branch from master to version-1.1.0-dev December 6, 2016 17:23

samtstern merged commit 5bb61d1 into firebase:version-1.1.0-dev Dec 6, 2016

SUPERCILEX deleted the register-email-refactor branch December 6, 2016 17:29

Conversation

SUPERCILEX commented Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX commented Dec 1, 2016

Full explanation:

TLDR

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samtstern left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Update:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samtstern left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samtstern commented Dec 6, 2016

Uh oh!

SUPERCILEX commented Nov 24, 2016 •

edited

Loading

SUPERCILEX Dec 2, 2016 •

edited

Loading

SUPERCILEX Dec 2, 2016 •

edited

Loading

SUPERCILEX Dec 2, 2016 •

edited

Loading