New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support pre-Pie devices #8

Closed
wants to merge 5 commits into
base: master
from

Conversation

2 participants
@ksc91u
Copy link

ksc91u commented Jan 9, 2019

use androidx.biometrics to support pre-Pie devices

ksc91u added some commits Jan 8, 2019

@ksc91u ksc91u closed this Jan 9, 2019

@ksc91u ksc91u reopened this Jan 9, 2019

@ksc91u ksc91u closed this Jan 9, 2019

@ksc91u ksc91u reopened this Jan 9, 2019

@pwittchen
Copy link
Owner

pwittchen left a comment

Thank you for your contribution and PR.
I have suggestions for improvements and questions regarding your changes. Please, have a look at them. Moreover, I prefer to have all commits squashed into one in the Pull Requests, so it's easier to manage, review and navigate through the git history.

@@ -65,12 +64,4 @@ class AuthenticationTest {
// then
verify(emitter).tryOnError(any(AuthenticationError::class.java))
}

@Test fun shouldTryOnErrorOnAuthenticationHelp() {

This comment has been minimized.

@pwittchen

pwittchen Jan 9, 2019

Owner

Why this test is removed?

// return hasBiometricSupport(context).flatMap { it ->
// if (it) isAtLeastAndroidPie()
// else Single.just(false)
// }

This comment has been minimized.

@pwittchen

pwittchen Jan 9, 2019

Owner

Why this code is commented?

fun authenticate(
context: Context,
activity: FragmentActivity,

This comment has been minimized.

@pwittchen

pwittchen Jan 9, 2019

Owner

Why did you replaced Context with FragmentActivity? Now it's less flexible.

import android.os.CancellationSignal
import android.support.annotation.RequiresApi
import androidx.fragment.app.FragmentActivity

This comment has been minimized.

@pwittchen

pwittchen Jan 9, 2019

Owner

This import is not needed.

fun authenticate(activity: FragmentActivity): Completable {
return Completable.create { emitter ->
createPrompt(activity, emitter).authenticate(promptInfo)
}.subscribeOn(AndroidSchedulers.mainThread())

This comment has been minimized.

@pwittchen

pwittchen Jan 9, 2019

Owner

Why are you subscribing this Completable here?

@@ -75,7 +70,10 @@ class MainActivity : AppCompatActivity() {
is AuthenticationFail -> showMessage("fail")
is AuthenticationHelp -> showMessage("help")
is BiometricNotSupported -> showMessage("biometric not supported")
else -> showMessage("other error")
else -> {
it.printStackTrace()

This comment has been minimized.

@pwittchen

pwittchen Jan 9, 2019

Owner

There's no need to print stackTrace here.

@@ -38,7 +39,7 @@ import kotlinx.android.synthetic.main.content_main.button

class MainActivity : AppCompatActivity() {

private lateinit var disposable: Disposable
private var disposable: Disposable? = null

This comment has been minimized.

@pwittchen

pwittchen Jan 9, 2019

Owner

Why lateinit is removed? I prefer to avoid assigning nulls.

.build()
.authenticate(this)
}
disposable = RxBiometric

This comment has been minimized.

@pwittchen

pwittchen Jan 9, 2019

Owner

Why canHandleBiometric(...) method is removed? Not all devices supports biometric.

@@ -4,7 +4,7 @@ apply plugin: 'kotlin-android-extensions'

android {
compileSdkVersion rootProject.ext.compileSdkVersion
buildToolsVersion rootProject.ext.buildToolsVersion
buildToolsVersion '28.0.3'

This comment has been minimized.

@pwittchen

pwittchen Jan 9, 2019

Owner

It should be updated in top-level build.gradle file

implementation deps.design
implementation 'androidx.appcompat:appcompat:1.1.0-alpha01'
implementation 'androidx.constraintlayout:constraintlayout:2.0.0-alpha3'
implementation 'com.google.android.material:material:1.1.0-alpha02'

This comment has been minimized.

@pwittchen

pwittchen Jan 9, 2019

Owner

This dependencies should be added in the top-level build.gradle file and referenced here as other dependencies. I organized this project in such way and it should be consistent across all modules.

@ksc91u

This comment has been minimized.

Copy link

ksc91u commented Jan 9, 2019

Thanks for the review, I've created a new request here

#9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment