Fix broken play services checks #462

SUPERCILEX · 2016-12-20T00:45:59Z

Hey @samtstern,
While trying to find a device with play services 10.0, I noticed that our checks were broken: the dialog wouldn't display itself and I kept getting weird errors:

12-19 17:39:04.726 4123-4123/com.firebase.uidemo E/WindowManager: Activity com.firebase.ui.auth.KickoffActivity has leaked window com.android.internal.policy.impl.PhoneWindow$DecorView{a59865e8 V.E..... R.....I. 0,0-1368,683} that was originally added here
                                                                  android.view.WindowLeaked: Activity com.firebase.ui.auth.KickoffActivity has leaked window com.android.internal.policy.impl.PhoneWindow$DecorView{a59865e8 V.E..... R.....I. 0,0-1368,683} that was originally added here
                                                                      at android.view.ViewRootImpl.<init>(ViewRootImpl.java:345)
                                                                      at android.view.WindowManagerGlobal.addView(WindowManagerGlobal.java:239)
                                                                      at android.view.WindowManagerImpl.addView(WindowManagerImpl.java:69)
                                                                      at android.app.Dialog.show(Dialog.java:281)
                                                                      at com.firebase.ui.auth.util.PlayServicesHelper.makePlayServicesAvailable(PlayServicesHelper.java:93)
                                                                      at com.firebase.ui.auth.util.signincontainer.SignInDelegate.onCreate(SignInDelegate.java:89)
                                                                      at android.support.v4.app.Fragment.performCreate(Fragment.java:2172)
                                                                      at android.support.v4.app.FragmentManagerImpl.moveToState(FragmentManager.java:1243)
                                                                      at android.support.v4.app.FragmentManagerImpl.moveFragmentsToInvisible(FragmentManager.java:2323)
                                                                      at android.support.v4.app.FragmentManagerImpl.executeOpsTogether(FragmentManager.java:2136)
                                                                      at android.support.v4.app.FragmentManagerImpl.optimizeAndExecuteOps(FragmentManager.java:2092)
                                                                      at android.support.v4.app.FragmentManagerImpl.execPendingActions(FragmentManager.java:1998)
                                                                      at android.support.v4.app.FragmentController.execPendingActions(FragmentController.java:388)
                                                                      at android.support.v4.app.FragmentActivity.onStart(FragmentActivity.java:607)
                                                                      at android.support.v7.app.AppCompatActivity.onStart(AppCompatActivity.java:181)
                                                                      at android.app.Instrumentation.callActivityOnStart(Instrumentation.java:1171)
                                                                      at android.app.Activity.performStart(Activity.java:5143)
                                                                      at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2184)
                                                                      at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2261)
                                                                      at android.app.ActivityThread.access$600(ActivityThread.java:141)
                                                                      at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1256)
                                                                      at android.os.Handler.dispatchMessage(Handler.java:99)
                                                                      at android.os.Looper.loop(Looper.java:137)
                                                                      at android.app.ActivityThread.main(ActivityThread.java:5103)
                                                                      at java.lang.reflect.Method.invokeNative(Native Method)
                                                                      at java.lang.reflect.Method.invoke(Method.java:525)
                                                                      at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:737)
                                                                      at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553)
                                                                      at dalvik.system.NativeStart.main(Native Method)

I have no idea what caused the errors, but I rewrote our entry checks and everything is working now. The new code also improves the "S" in SOLID for SignInDelegate because it no longer has to care about network or play services issues. Cheers! 😄

PS: I think there is a bug in play services, but I couldn't find anywhere to report it so I thought maybe someone from the Firebase team could pass this on to the right person.

I tried using makeGooglePlayServicesAvailable, but it crashes the app if the dialog is dismissed twice.

GoogleApiAvailability.getInstance()
        .makeGooglePlayServicesAvailable(this)
        .addOnSuccessListener(new OnSuccessListener<Void>() {
            @Override
            public void onSuccess(Void aVoid) {
                SignInDelegate.delegate(KickoffActivity.this,
                                        mActivityHelper.getFlowParams());
            }
        });

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2016-12-20T00:48:01Z

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

@@ -169,9 +168,7 @@ private void finish() {
    public void saveCredentialsOrFinish(FirebaseUser firebaseUser,
                                        @Nullable String password,
                                        @Nullable IdpResponse response) {
-        if (!mHelper.getFlowParams().smartLockEnabled
-                || !PlayServicesHelper.getInstance(getContext()).isPlayServicesAvailable()


@samtstern Is there a reason for keeping this here? Since KickOffActivity acts as the gatekeeper, shouldn't play services always be available when we get here?

That should be true, fine with your new change.

…lability

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2016-12-20T23:53:35Z

This PR now includes #447 because there were some conflicts with PlayServicesUtil and that PR.

Signed-off-by: Alex Saveau <[email protected]>

…ailability

Signed-off-by: Alex Saveau <[email protected]>

samtstern

This is a good simplification! Good to keep all of these checks in KickoffActivity. Just a few comments.

samtstern · 2016-12-22T22:09:03Z

auth/src/main/java/com/firebase/ui/auth/KickoffActivity.java

+                return;
+            }
+
+            GoogleApiAvailability apiAvailability = GoogleApiAvailability.getInstance();


Although you're getting rid of PlayServicesHelper I'd still like to centralize all Play services availability logic somewhere. Even if the class is just a host for a singleton of GoogleApiAvailability that's a win for future testing. Play services has given me a lot of trouble in testing (although you seem to have simplified a lot of it here).

samtstern · 2016-12-22T22:10:22Z

auth/src/main/java/com/firebase/ui/auth/KickoffActivity.java

        }
    }

    @Override
    public void onSaveInstanceState(Bundle outState) {
        // It doesn't matter what we put here, we just don't want outState to be empty
-        outState.putBoolean(ExtraConstants.HAS_EXISTING_INSTANCE, true);
+        outState.putBoolean(ExtraConstants.HAS_EXISTING_INSTANCE, !mIsWaitingForPlayServices);


The use of HAS_EXISTING _INSTANCE to store the inverse of mIsWaitingForPlayServices is a little confusing.

Can we do two separate extras? One which is always true like before and one which stores the state of mIsWaitingForPlayServices.

samtstern · 2016-12-22T22:12:04Z

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

@@ -169,9 +168,7 @@ private void finish() {
    public void saveCredentialsOrFinish(FirebaseUser firebaseUser,
                                        @Nullable String password,
                                        @Nullable IdpResponse response) {
-        if (!mHelper.getFlowParams().smartLockEnabled
-                || !PlayServicesHelper.getInstance(getContext()).isPlayServicesAvailable()


That should be true, fine with your new change.

…ailability # Conflicts: # auth/src/main/java/com/firebase/ui/auth/util/signincontainer/SaveSmartLock.java # auth/src/test/java/com/firebase/ui/auth/ui/email/RegisterEmailActivityTest.java

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2016-12-22T23:23:14Z

Woah! We have a net negative of 529 removed lines of code!

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2016-12-22T23:26:24Z

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

        }
    }

+    private PendingIntent getEmailHintIntent() {


Do we want to move this to PlayServicesHelper or leave it here?

This can stay

SUPERCILEX · 2016-12-22T23:27:39Z

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

-    }
-
-    @Override
-    public PendingIntent getEmailHintIntent(FragmentActivity fragmentActivity) {


I'm amazed that nobody noticed: getEmailHintIntent was the only method being used in this entire class!!!

That is hilarious. I always hated this class!

samtstern · 2016-12-22T23:39:40Z

I love this CL, merging!

P.S. I don't think any of the repo owners will be in the office next week so this will be the last CL for a while! First week of Jan we will QA and release 1.1.0. Happy holidays!

SUPERCILEX · 2016-12-22T23:41:10Z

@samtstern Awesome! Enjoy your break and happy holidays!

SUPERCILEX added 2 commits December 19, 2016 16:21

Fix play services check

a59b111

Signed-off-by: Alex Saveau <[email protected]>

Fix rotation bugs

89add90

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX commented Dec 20, 2016

View reviewed changes

SUPERCILEX changed the title ~~Api availability~~ Fix broken play services checks Dec 20, 2016

SUPERCILEX added 2 commits December 20, 2016 15:46

Merge remote-tracking branch 'samtstern/email-refactor' into api-avai…

2361932

…lability

Fix merge mistakes

a57fdf3

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX added 3 commits December 20, 2016 16:12

Fix more conflicts

a289a01

Signed-off-by: Alex Saveau <[email protected]>

Merge remote-tracking branch 'firebase/version-1.1.0-dev' into api-av…

53a99f7

…ailability

Finish with an IdpResponse

6bf3220

Signed-off-by: Alex Saveau <[email protected]>

samtstern reviewed Dec 22, 2016

View reviewed changes

SUPERCILEX added 3 commits December 22, 2016 14:34

Merge remote-tracking branch 'firebase/version-1.1.0-dev' into api-av…

c056b0f

…ailability # Conflicts: # auth/src/main/java/com/firebase/ui/auth/util/signincontainer/SaveSmartLock.java # auth/src/test/java/com/firebase/ui/auth/ui/email/RegisterEmailActivityTest.java

Address review comments

e2b195d

Signed-off-by: Alex Saveau <[email protected]>

Cleanup

659bc5f

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX changed the title ~~Fix broken play services checks~~ [DON'T MERGE YET] Fix broken play services checks Dec 22, 2016

Remove a truck load of unused code

c05964e

Signed-off-by: Alex Saveau <[email protected]>

Fix typo

ca699bb

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX commented Dec 22, 2016

View reviewed changes

SUPERCILEX changed the title ~~[DON'T MERGE YET] Fix broken play services checks~~ Fix broken play services checks Dec 22, 2016

samtstern merged commit 63e3f95 into firebase:version-1.1.0-dev Dec 22, 2016

SUPERCILEX deleted the api-availability branch December 22, 2016 23:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix broken play services checks #462

Fix broken play services checks #462

SUPERCILEX commented Dec 20, 2016

SUPERCILEX Dec 20, 2016 •

edited

Loading

samtstern Dec 22, 2016

SUPERCILEX commented Dec 20, 2016

samtstern left a comment

samtstern Dec 22, 2016

samtstern Dec 22, 2016

samtstern Dec 22, 2016

SUPERCILEX commented Dec 22, 2016

SUPERCILEX Dec 22, 2016

samtstern Dec 22, 2016

SUPERCILEX Dec 22, 2016 •

edited

Loading

samtstern Dec 22, 2016

samtstern commented Dec 22, 2016

SUPERCILEX commented Dec 22, 2016

Fix broken play services checks #462

Fix broken play services checks #462

Conversation

SUPERCILEX commented Dec 20, 2016

SUPERCILEX Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

samtstern Dec 22, 2016

Choose a reason for hiding this comment

SUPERCILEX commented Dec 20, 2016

samtstern left a comment

Choose a reason for hiding this comment

samtstern Dec 22, 2016

Choose a reason for hiding this comment

samtstern Dec 22, 2016

Choose a reason for hiding this comment

samtstern Dec 22, 2016

Choose a reason for hiding this comment

SUPERCILEX commented Dec 22, 2016

SUPERCILEX Dec 22, 2016

Choose a reason for hiding this comment

samtstern Dec 22, 2016

Choose a reason for hiding this comment

SUPERCILEX Dec 22, 2016 • edited Loading

Choose a reason for hiding this comment

samtstern Dec 22, 2016

Choose a reason for hiding this comment

samtstern commented Dec 22, 2016

SUPERCILEX commented Dec 22, 2016

SUPERCILEX Dec 20, 2016 •

edited

Loading

SUPERCILEX Dec 22, 2016 •

edited

Loading