Auth code cleanup #376

SUPERCILEX · 2016-10-23T02:29:17Z

For the removed todo, see http://stackoverflow.com/questions/2643949/can-i-use-a-static-variable-from-a-java-class-in-res-values-styles-xml.

I added RTL support, a wee bit of accessibility, and removed unused resources.

For other stuff, see the review below:

SUPERCILEX

Cleaner code! 😄

SUPERCILEX · 2016-10-23T02:29:45Z

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

    public static final int RC_SIGN_IN = 16;
-    public static final List<Integer> REQUEST_CODES = Arrays.asList(
+    private static final List<Integer> REQUEST_CODES = Arrays.asList(


Was there a reason for keeping this stuff public?

Nope this LGTM, not meant to be part of the public API/

SUPERCILEX · 2016-10-23T02:31:57Z

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

@@ -80,30 +80,22 @@ private void startEmailHandler(String email, List<String> providers) {
            mActivityHelper.startActivityForResult(registerIntent, RC_REGISTER_ACCOUNT);
        } else {
            // account does exist
-            for (String provider : providers) {


Here's the most important change in this PR: the for loop doesn't loop! Is my change appropriate or should we be doing something else here?

Can we be sure that if the email provider is in the providers collection that it will be at index 0? I agree the loop was not needed but not sure that get(0) will always be equivalent.

List<String> providers so isn't the first item in the for loop always going to be at index 0?

SUPERCILEX · 2016-10-23T02:32:59Z

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

@@ -76,8 +78,7 @@ public CredentialRequest createCredentialRequest(List<IdpConfig> providers) {
                    @Override
                    protected void process(
                            final GoogleApiClient client,
-                            final TaskCompletionSource<CredentialRequestResult> source)
-                            throws Exception {


I removed all the throws exception here. Did they serve a purpose?

I guess not? @amandle see anything?

I'm not sure why these are throwing Exception, @iainmcgin might know

SUPERCILEX · 2016-10-23T02:34:33Z

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

+
+    <!-- accessibility -->
+    <string name="accessibility_logo">App logo</string>
+    <string name="accessibility_toggle">Toggle password visibility</string>


Are these descriptions adequate?

SUPERCILEX · 2016-10-23T02:36:48Z

library/src/main/res/drawable/facebook_button.xml

@@ -1,19 +0,0 @@
-<?xml version="1.0" encoding="utf-8"?>


The rest is all unused layouts and resources.

SUPERCILEX · 2016-10-23T02:38:03Z

Lint and Intellij are mostly happy now.

SUPERCILEX · 2016-10-23T07:09:45Z

I also updated the travis file to make builds faster. We're now at around 3 minutes!

SUPERCILEX · 2016-10-23T07:15:38Z

@samtstern could you merge master into dev or should I just add it here since it only has 2 commits?

samtstern · 2016-10-23T23:21:52Z

@SUPERCILEX merged master into 1.0.0-dev

SUPERCILEX · 2016-10-23T23:50:59Z

@samtstern Awesome, we're all green now!

samtstern · 2016-10-24T00:03:18Z

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

    public static final int RC_SIGN_IN = 16;
-    public static final List<Integer> REQUEST_CODES = Arrays.asList(
+    private static final List<Integer> REQUEST_CODES = Arrays.asList(


Nope this LGTM, not meant to be part of the public API/

samtstern · 2016-10-24T00:05:32Z

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

-                    mActivityHelper.getApplicationContext(), SignInActivity.class);
-            signInIntent.putExtra(ExtraConstants.EXTRA_EMAIL, email);
-            mActivityHelper.startActivityForResult(signInIntent, RC_SIGN_IN);
+            Intent intent = WelcomeBackIdpPrompt.createIntent(


This seems to be a behavior change from SignInActivity to WelcomeBackIDPPrompt.

No, the git diff just got really messed up. See the old vs the new:

samtstern · 2016-10-24T00:06:10Z

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

@@ -80,30 +80,22 @@ private void startEmailHandler(String email, List<String> providers) {
            mActivityHelper.startActivityForResult(registerIntent, RC_REGISTER_ACCOUNT);
        } else {
            // account does exist
-            for (String provider : providers) {


Can we be sure that if the email provider is in the providers collection that it will be at index 0? I agree the loop was not needed but not sure that get(0) will always be equivalent.

samtstern · 2016-10-24T00:13:29Z

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

@@ -76,8 +78,7 @@ public CredentialRequest createCredentialRequest(List<IdpConfig> providers) {
                    @Override
                    protected void process(
                            final GoogleApiClient client,
-                            final TaskCompletionSource<CredentialRequestResult> source)
-                            throws Exception {


I guess not? @amandle see anything?

samtstern · 2016-10-24T00:14:27Z

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

+
+    <!-- accessibility -->
+    <string name="accessibility_logo">App logo</string>
+    <string name="accessibility_toggle">Toggle password visibility</string>


samtstern · 2016-10-24T00:15:01Z

auth/src/test/java/com/firebase/ui/auth/test_helpers/FacebookProviderShadow.java

@@ -44,8 +41,6 @@ public FacebookProviderShadow() {
        }
    }

-    public void __constructor__(Activity activity, List<String> scopes) {}


Are we sure none of the tests are implicitly using this? Android Studio would not be able to tell if this was unused.

From what I've read (see Shadowing Constructors at the bottom of the page), this would be used to collect variables from the constructor, but it was empty. If you believe it serves a purpose, I will add it back.

This is actually used to prevent the real constructor from being called. Should have left a comment, sorry.

@amandle Do you want me to add it back? Why aren't the tests failing then?

SUPERCILEX

Addressed your comments.

SUPERCILEX · 2016-10-24T00:23:47Z

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

@@ -80,30 +80,22 @@ private void startEmailHandler(String email, List<String> providers) {
            mActivityHelper.startActivityForResult(registerIntent, RC_REGISTER_ACCOUNT);
        } else {
            // account does exist
-            for (String provider : providers) {


List<String> providers so isn't the first item in the for loop always going to be at index 0?

SUPERCILEX · 2016-10-24T00:27:58Z

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

-                    mActivityHelper.getApplicationContext(), SignInActivity.class);
-            signInIntent.putExtra(ExtraConstants.EXTRA_EMAIL, email);
-            mActivityHelper.startActivityForResult(signInIntent, RC_SIGN_IN);
+            Intent intent = WelcomeBackIdpPrompt.createIntent(


No, the git diff just got really messed up. See the old vs the new:

SUPERCILEX · 2016-10-24T00:36:40Z

auth/src/test/java/com/firebase/ui/auth/test_helpers/FacebookProviderShadow.java

@@ -44,8 +41,6 @@ public FacebookProviderShadow() {
        }
    }

-    public void __constructor__(Activity activity, List<String> scopes) {}


From what I've read (see Shadowing Constructors at the bottom of the page), this would be used to collect variables from the constructor, but it was empty. If you believe it serves a purpose, I will add it back.

SUPERCILEX

Updated dependencies and reorganized auth ones to follow a more conventional style: support/firebase -> third party -> test.

SUPERCILEX · 2016-10-24T02:01:40Z

auth/build.gradle

    compile "com.android.support:design:${project.ext.support_library_version}"

    compile "com.google.firebase:firebase-auth:${project.ext.firebase_version}"
    compile "com.google.android.gms:play-services-auth:${project.ext.firebase_version}"
+
+    compile 'com.facebook.android:facebook-android-sdk:4.16.1'


old: v4.14.1

SUPERCILEX · 2016-10-24T02:01:51Z

auth/build.gradle

    compile "com.android.support:design:${project.ext.support_library_version}"

    compile "com.google.firebase:firebase-auth:${project.ext.firebase_version}"
    compile "com.google.android.gms:play-services-auth:${project.ext.firebase_version}"
+
+    compile 'com.facebook.android:facebook-android-sdk:4.16.1'
+    compile("com.twitter.sdk.android:twitter:2.1.1@aar") {


SUPERCILEX · 2016-10-24T02:02:04Z

auth/build.gradle

+    }
+
+    testCompile 'junit:junit:4.12'
+    testCompile 'org.mockito:mockito-core:2.2.6'


SUPERCILEX · 2016-10-24T02:03:53Z

build.gradle

        classpath 'com.google.gms:google-services:3.0.0'
-        classpath 'io.fabric.tools:gradle:1.21.7'
+        classpath 'io.fabric.tools:gradle:1.+'


Twitter recommends using open ended versions: https://fabric.io/kits/android/twitterkit/install

The Fabric Gradle plugin uses an open ended version to react quickly to Android tooling updates

SUPERCILEX · 2016-10-24T23:57:46Z

common/constants.gradle

@@ -1,4 +1,4 @@
-project.ext.firebase_version = '9.6.1'
+project.ext.firebase_version = '9.8.00'


@samtstern (or any other firebase dev) Why the extra zero? It won't compile with just one zero.

@samtstern That's really weird. It won't compile on my machine without the extra zero and it won't compile with the extra zero on Travis. I'll have to try clearing the cache and such.

SUPERCILEX · 2016-10-25T01:37:55Z

I upgraded the EditTexts in android.support.design.widget.TextInputLayout to android.support.design.widget.TextInputEditText. See http://stackoverflow.com/questions/35775919/edittext-added-is-not-a-textinputedittext-please-switch-to-using-that-class-ins. Apparently, it makes things prettier in landscape mode.

I also made the xml style more consistent. (any objections to the style I chose?)

Removed duplicate code and cleaned stuff up.

SUPERCILEX · 2016-10-25T02:11:48Z

I did some final cleanup.

Sorry for making this a big PR in the end. However, I do think it really cleans things up nicely.

# Conflicts: # auth/src/main/java/com/firebase/ui/auth/ui/AcquireEmailHelper.java

SUPERCILEX · 2016-10-27T23:37:25Z

@samtstern @amandle I added back the constructors for the tests and merged v1.0.0. We should be good to go now!

amandle · 2016-10-27T23:59:42Z

@SUPERCILEX cool, will take a look. Just a heads up, we're nearing 1.0 release and trying to stabilize things at the moment, so it may make sense to hold off on merging this for the time being.

SUPERCILEX · 2016-10-28T01:04:18Z

@amandle Awesome, can't wait for 1.0! We'll finally have official Twitter support and keeping track of changes between master and v1.0.0-dev will no longer be a thing.

For this PR, there shouldn't be an behavioral changes. Just code cleanup.

# Conflicts: # auth/src/main/java/com/firebase/ui/auth/ui/idp/IdpSignInContainerActivity.java

# Conflicts: # common/constants.gradle

samtstern · 2016-11-17T19:54:47Z

@SUPERCILEX just reviewed all of this again, it all LGTM. I believe merging your other CL caused a conflict but once that's fixed I will merge this as well.

Again sorry for the delay here, after 1.0 we had to go tend to other projects which we had neglected due to the 1.0 release 😄 there's never enough time to go around!

# Conflicts: # auth/build.gradle # auth/src/main/AndroidManifest.xml # auth/src/main/res/layout/auth_method_picker_layout.xml

SUPERCILEX · 2016-11-18T01:57:13Z

@samtstern Sounds busy! 🏗
Things should be ready to go now as I just finished merging.

samtstern · 2016-11-18T18:00:30Z

Squashed and merged! Thanks @SUPERCILEX

SUPERCILEX added 2 commits October 22, 2016 18:21

cleanup

6ecc551

cleanup

00a59cd

SUPERCILEX commented Oct 23, 2016

View reviewed changes

Update .travis.yml

bb352e9

SUPERCILEX changed the title ~~Auh code cleanup~~ Auth code cleanup Oct 23, 2016

Merge remote-tracking branch 'firebase/version-1.0.0-dev' into cleanup

d8f1e1a

samtstern reviewed Oct 24, 2016

View reviewed changes

SUPERCILEX commented Oct 24, 2016

View reviewed changes

SUPERCILEX added 2 commits October 23, 2016 17:45

More dependency updates

e733913

Reorganize dependencies

9ab7dd4

SUPERCILEX commented Oct 24, 2016

View reviewed changes

SUPERCILEX added 2 commits October 23, 2016 19:08

Cleanup

b5fe9f1

Update firebase

7a05e46

SUPERCILEX commented Oct 24, 2016

View reviewed changes

SUPERCILEX and others added 4 commits October 24, 2016 17:00

Update constants.gradle

51dc270

Remove unnecessary app dependencies

90f2c56

Style consistency in xml files

1234066

Upgrade EditTexts to android.support.design.widget.TextInputEditText

575cb9b

Final cleanup

34a7800

Removed duplicate code and cleaned stuff up.

SUPERCILEX added 4 commits October 24, 2016 19:29

Even more xml refactoring. Extracted FirebaseUI.EditText.EmailField

5e7c955

app dependency updates

97bc852

Massively cleaned up gradle ext variables.

41c5ddd

Merge remote-tracking branch 'firebase/version-1.0.0-dev' into cleanup

e666a15

# Conflicts: # auth/src/main/java/com/firebase/ui/auth/ui/AcquireEmailHelper.java

Address review comments and fix mistakes

6798d92

SUPERCILEX added 12 commits October 27, 2016 19:18

Turns out braces are unnecessary in gradle

afd6fc9

Merge remote-tracking branch 'firebase/version-1.0.0-dev' into cleanup

1cdf4f2

# Conflicts: # auth/src/main/java/com/firebase/ui/auth/ui/idp/IdpSignInContainerActivity.java

Fix merge mistakes

2b2da92

Cleanup SmartLock.java

9d9b7f5

Fix nits

47b204e

Merge remote-tracking branch 'firebase/version-1.0.0-dev' into cleanup

6e35dcb

# Conflicts: # common/constants.gradle

Move AcquireEmailHelper.java so as to narrow down access to RC_SIGN_IN

18e969f

Cleanup

a183c7f

Merge remote-tracking branch 'firebase/master' into cleanup

26343b4

Cleanup

2ac59d9

Cleanup

8925f51

Cleanup

9603d9a

SUPERCILEX changed the base branch from version-1.0.0-dev to master November 5, 2016 05:40

SUPERCILEX and others added 4 commits November 4, 2016 22:44

Update README.md

2e9f399

More xml cleanup

f60c448

Merge remote-tracking branch 'origin/cleanup' into cleanup

0406cd8

Update AuthMethodPickerActivity.java

0c85d63

SUPERCILEX added 3 commits November 17, 2016 17:28

Merge remote-tracking branch 'firebase/master' into cleanup

7b95605

# Conflicts: # auth/build.gradle # auth/src/main/AndroidManifest.xml # auth/src/main/res/layout/auth_method_picker_layout.xml

Fix merge mistakes

5610ad1

Merge remote-tracking branch 'origin/cleanup' into cleanup

af34fab

samtstern merged commit 42fa51c into firebase:master Nov 18, 2016

SUPERCILEX deleted the cleanup branch November 18, 2016 18:00

SUPERCILEX mentioned this pull request Nov 28, 2016

Update AcquireEmailHelper.java #425

Closed

		@@ -1,4 +1,4 @@
		project.ext.firebase_version = '9.6.1'
		project.ext.firebase_version = '9.8.00'

Auth code cleanup #376

Auth code cleanup #376

Uh oh!

Conversation

SUPERCILEX commented Oct 23, 2016

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!

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 commented Oct 23, 2016

Uh oh!

SUPERCILEX commented Oct 23, 2016

Uh oh!

SUPERCILEX commented Oct 23, 2016

Uh oh!

samtstern commented Oct 23, 2016

Uh oh!

SUPERCILEX commented Oct 23, 2016

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!

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 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!

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

SUPERCILEX Oct 25, 2016 •

edited

Loading

SUPERCILEX commented Oct 25, 2016 •

edited

Loading