Refactor chooseAccountActivity #357

SUPERCILEX · 2016-10-16T05:37:33Z

#254

TODO @samtstern:

merge Refactor SaveCredentialsActivity #343 first so that the diff is more readable

TODO me:

Check auto sign is still working
Check hideProgress is working with auto sign in
Fix tests
Fix onActivityResult not fired! #182
Fix roadblock: on low memory devices, AuthUI.AuthUIResult will become null and a null pointer will ensue. How to get rid of AuthUI.AuthUIResult. Any ideas?
Check SaveSmartLock behavior to make sure configuration changes don't cause a null pointer exception.
check for differences in https://github.com/firebase/FirebaseUI-Android/blob/master/auth/src/main/java/com/firebase/ui/auth/ui/ChooseAccountActivity.java
Do another round of testing
Do a leak check

Bugs fixed in this PR:

Check mEmail for empty String to prevent "credential identifier cannot be empty" error #152
Smart Lock does not retrieve saved Facebook credential #171
AuthMethodPickerActivity is started multiple times #354
Part 2 of Refactor SaveCredentialsActivity #254
Fixed onActivityResult not fired! #182 (comment) in the sense that there are seperate callbacks for failures and successes meaning that developers can rely on those instead. Activity lifecycle events and auth state listeners will still always take precedence though.

won't compile

…ctivities

SUPERCILEX

This PR includes #343 so that should be merged first.

Since I deleted everything, I'll go through and explain the changes I made for you guys, but first I have a road block I need you guys to make a decision on before I continue. Other than that, things are blazing fast and sleek!

SUPERCILEX · 2016-10-16T05:47:46Z

auth/src/main/java/com/firebase/ui/auth/util/SignInDelegate.java

+        if (status.getStatusCode() == CommonStatusCodes.RESOLUTION_REQUIRED) {
+            try {
+                // TODO check the mActivity stuff
+                status.startResolutionForResult(mActivity, RC_CREDENTIALS_READ);


Here's where I'm stuck. There are two options: either people using the library would have to call onActivityResult for us, or I would have to change SignInDelegate to an AppCompatFragment which would mean people using the library have to pass in an AppCompatActivity instead of being able to pass in any activity. I'm in favor of the latter because 99.9% of people are probably already using AppCompatActivity anyway since it is necessary for the Toolbar (added in API v21).

I decided to go ahead with the AppCompatActivity thing because it is a direct subclass of Activity: https://developer.android.com/reference/android/support/v7/app/AppCompatActivity.html so there isn't really a tradeoff to the developer if they are using Activity (aka they wouldn't lose anything by switching).

SUPERCILEX · 2016-11-23T04:28:35Z

auth/src/main/java/com/firebase/ui/auth/ui/account_link/WelcomeBackIdpPrompt.java

@@ -176,7 +176,6 @@ public void onComplete(@NonNull Task<AuthResult> task) {
                                        TAG, "Error signing in with previous credential"))
                                .addOnCompleteListener(new FinishListener(newIdpResponse));
                    } else {
-                        mActivityHelper.dismissDialog();


I removed unnecessary calls to dismissDialog() with an immediate successive finish()

SUPERCILEX · 2016-11-23T05:36:25Z

auth/src/main/java/com/firebase/ui/auth/util/signincontainer/SmartLock.java

+        GoogleApiClient.OnConnectionFailedListener {
+    private static final String TAG = "SmartLockBase";
+
+    protected GoogleApiClient mGoogleApiClient;


Yes below:

public void cleanup() { if (mGoogleApiClient != null) { mGoogleApiClient.disconnect(); } }

SUPERCILEX · 2016-11-23T06:02:37Z

@samtstern Unless you have more comments, this should be ready to go.

I added RecoveryEmailSentDialog to fix the dialog being dismissed on rotation.

SUPERCILEX · 2016-11-23T06:33:55Z

I just fixed a leak so now it should be ready to go. 😄

Do we want to keep LeakCanary or should I remove it? I'm pro keeping it since it would help prevent cases like #396 where leak canary could have helped us catch those issues earlier.

SUPERCILEX · 2016-11-23T08:25:43Z

STOPSHIP: I'm pretty sure I should be requesting the permissions specified in IdpConfig here: https://github.com/firebase/FirebaseUI-Android/pull/357/files#diff-741502abb522ecd56e7c30231e015bdbR112

samtstern · 2016-11-23T17:35:44Z

app/build.gradle

@@ -46,6 +46,9 @@ dependencies {
    compile 'pub.devrel:easypermissions:0.2.1'
    compile 'com.jakewharton:butterknife:8.4.0'
    apt 'com.jakewharton:butterknife-compiler:8.4.0'
+    debugCompile 'com.squareup.leakcanary:leakcanary-android:1.5'


+1 to keeping LeakCanary around

samtstern · 2016-11-23T17:43:06Z

@SUPERCILEX regarding your 'STOPSHIP' comment you only need to configure the GoogleSignInOptions with scopes if you are going to use that GoogleApiClient to use the GoogleSignInApi. From what I can tell you're only using it for the Credentials API, which means you can actually omit the addApi(GOOGLE_SIGN_IN_API) and the GoogleSignInOptions altogether.

Other than that I agree, this is looking good to go!

…nage

SUPERCILEX · 2016-11-23T19:54:45Z

@samtstern I think we're ready to roll!

samtstern · 2016-11-23T19:56:58Z

@SUPERCILEX I agree, I'll wait for Travis' green light. This has been a marathon! Thank you so much for seeing it through.

samtstern · 2016-11-23T20:08:06Z

Hate to be the bearer of bad news but found one more error! Did some manual testing and got a crash here:

SignInDelegate

    private void redirectToIdpSignIn(String email, String accountType) {
        if (accountType.equals(IdentityProviders.GOOGLE)
                || accountType.equals(IdentityProviders.FACEBOOK)
                || accountType.equals(IdentityProviders.TWITTER)) {
            IdpSignInContainer.signIn(

When I select an email credential from SmartLock, accountType == null so we get a NullPointerException.

SUPERCILEX · 2016-11-23T20:34:20Z

@samtstern I'm not seeing the crash. Also, the only way it could have happened is if the password from the email you selected in smart lock was null which would be weird:

if (!TextUtils.isEmpty(email)) {
    if (TextUtils.isEmpty(password)) {
        // log in with id/provider
        redirectToIdpSignIn(email, getAccountTypeFromCredential());
    } else {
        // Sign in with the email/password retrieved from SmartLock
        signInWithEmailAndPassword(email, password);
    }
}

Do you think there is something else going on?

samtstern · 2016-11-23T20:44:57Z

Here's the call stack:

Attempt to invoke virtual method 'boolean java.lang.String.equals(java.lang.Object)' on a null object reference
                                                       at com.firebase.ui.auth.util.signincontainer.SignInDelegate.redirectToIdpSignIn(SignInDelegate.java:332)
                                                       at com.firebase.ui.auth.util.signincontainer.SignInDelegate.handleCredential(SignInDelegate.java:241)
                                                       at com.firebase.ui.auth.util.signincontainer.SignInDelegate.onActivityResult(SignInDelegate.java:178)

So that would imply that the password is actually empty. I believe this is a "hint" credential. The SmartLock picker sometimes shows "hints" which contain email addresses that you have used before but no passwords, basically as an advanced sort of autofill.

samtstern · 2016-11-23T20:50:26Z

I'm not sure exactly why that Credential was in there, but I think we should be defensive and handle null accountType in that code branch (just fall back to email sign in).

SUPERCILEX · 2016-11-23T20:50:49Z

Ah, I get it. My commit fixes the crash, but I still need to figure out the flow.

samtstern · 2016-11-23T20:52:56Z

@SUPERCILEX thanks for the commit to fix it. I am going to merge this PR now! That seems like a separate issue.

So which issues can I mark as "fix-implemented" now? And hopefully I will do a release with this issue on Monday (don't want to do it today in case something goes wrong, everyone is out for Thanksgiving)

samtstern · 2016-11-23T20:54:50Z

Also 114 commits! That has to be some kind of record. This was a great effort, part of making this library a lot leaner. Excited to build on this.

SUPERCILEX · 2016-11-23T21:10:20Z

Whew! We did it! I'll look for the issues that can be closed and I also created #416 to fix the smart lock email flow.

SUPERCILEX added 21 commits October 6, 2016 21:36

temporary: saving to work on another computer

05ce3b5

won't compile

Merge remote-tracking branch 'firebase/master' into refactor-zombie-a…

2391efb

…ctivities

Merge remote-tracking branch 'firebase/master' into refactor-zombie-a…

05a476a

…ctivities

Code compiles

2c0eaf5

Cleanup

8adeee1

Cleanup

11f3c2b

Cleanup

cb826f4

Update SmartLock.java with new fragment logic

3b788f5

Fix bug in SmartLock.java

9eaa4cd

Cleanup

3b76ac6

Cleanup

86d58a9

Fix broken tests

6ad0021

Cleanup

550acc9

Cleanup

b9d3ca5

Cleanup

a8b5cf5

Refactor SaveSmartLock.java

83e13d8

Initial refactor

c529171

Cleanup

5142dec

Cleanup

361d308

Bug fix

9a4921e

Cleanup

6af7674

SUPERCILEX commented Oct 16, 2016

View reviewed changes

SUPERCILEX added 8 commits October 15, 2016 22:53

Cleanup

35d94bb

Cleanup and bug fixes

64728b2

Cleanup

dde4160

Cleanup

2d5dfdc

Fix intellij messing up documentation.

f3bd0f1

Cleanup

f06845b

Cleanup

9782793

Remove unused strings

7d1d1c6

SUPERCILEX added 2 commits November 22, 2016 21:22

Add leakcanary and update dependencies

be698da

Fix recovery dialog being dismissed on screen rotation

ac0a78e

SUPERCILEX commented Nov 23, 2016

View reviewed changes

SUPERCILEX added 2 commits November 22, 2016 21:52

Rename SmartLock to SmartLockBase.java

6cce271

Fix typo

6870518

Enable auto manage to help prevent memory leaks

bb09653

samtstern reviewed Nov 23, 2016

View reviewed changes

SUPERCILEX added 3 commits November 23, 2016 11:37

Fix #348 and IllegalStateException in SaveSmartLock because of automa…

151b8d3

…nage

Refactor GoogleApiClient auto manage constants

707f51a

Remove unnecessary sign in options.

6b678e4

Fix possible null pointer

52d27a3

samtstern merged commit 225dffd into firebase:master Nov 23, 2016

SUPERCILEX deleted the refactor-ChooseAccountActivity branch November 23, 2016 21:01

SUPERCILEX mentioned this pull request Nov 23, 2016

WelcomeBackIDPPrompt cancels sign in prematurely #348

Closed

SUPERCILEX mentioned this pull request Jan 3, 2017

Fix create account title not showing up (and cleanup strings) #484

Merged

samtstern mentioned this pull request Jan 5, 2017

Make KickoffActivity truly transparent #495

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor chooseAccountActivity #357

Refactor chooseAccountActivity #357

SUPERCILEX commented Oct 16, 2016 •

edited

Loading

SUPERCILEX left a comment

SUPERCILEX Oct 16, 2016

SUPERCILEX Oct 16, 2016

SUPERCILEX Nov 23, 2016

SUPERCILEX Nov 23, 2016

SUPERCILEX commented Nov 23, 2016

SUPERCILEX commented Nov 23, 2016

SUPERCILEX commented Nov 23, 2016 •

edited

Loading

samtstern Nov 23, 2016

SUPERCILEX Nov 23, 2016

samtstern commented Nov 23, 2016

SUPERCILEX commented Nov 23, 2016

samtstern commented Nov 23, 2016

samtstern commented Nov 23, 2016 •

edited

Loading

SUPERCILEX commented Nov 23, 2016

samtstern commented Nov 23, 2016

samtstern commented Nov 23, 2016

SUPERCILEX commented Nov 23, 2016 •

edited

Loading

samtstern commented Nov 23, 2016

samtstern commented Nov 23, 2016

SUPERCILEX commented Nov 23, 2016

Refactor chooseAccountActivity #357

Refactor chooseAccountActivity #357

Conversation

SUPERCILEX commented Oct 16, 2016 • edited Loading

SUPERCILEX left a comment

Choose a reason for hiding this comment

SUPERCILEX Oct 16, 2016

Choose a reason for hiding this comment

SUPERCILEX Oct 16, 2016

Choose a reason for hiding this comment

SUPERCILEX Nov 23, 2016

Choose a reason for hiding this comment

SUPERCILEX Nov 23, 2016

Choose a reason for hiding this comment

SUPERCILEX commented Nov 23, 2016

SUPERCILEX commented Nov 23, 2016

SUPERCILEX commented Nov 23, 2016 • edited Loading

samtstern Nov 23, 2016

Choose a reason for hiding this comment

SUPERCILEX Nov 23, 2016

Choose a reason for hiding this comment

samtstern commented Nov 23, 2016

SUPERCILEX commented Nov 23, 2016

samtstern commented Nov 23, 2016

samtstern commented Nov 23, 2016 • edited Loading

SUPERCILEX commented Nov 23, 2016

samtstern commented Nov 23, 2016

samtstern commented Nov 23, 2016

SUPERCILEX commented Nov 23, 2016 • edited Loading

samtstern commented Nov 23, 2016

samtstern commented Nov 23, 2016

SUPERCILEX commented Nov 23, 2016

SUPERCILEX commented Oct 16, 2016 •

edited

Loading

SUPERCILEX commented Nov 23, 2016 •

edited

Loading

samtstern commented Nov 23, 2016 •

edited

Loading

SUPERCILEX commented Nov 23, 2016 •

edited

Loading