Conversation
hexCode63
requested review from
JeelDobariya38 and
kudanilll
and removed request for
JeelDobariya38
JeelDobariya38
force-pushed
the
hex-dev
branch
from
f77f20b to
8b0df00
Compare
|
🎉🎉🎉 First of all, a very big thank for submit a pr!!! & Very welcome, collaborator!! 🎉🎉🎉🎉 I was look into, your code changes it seem fine to me (on surface)... I will still relook into code once again after compile it and test autofill.... Overall it is okay, and seems to give a good foundation for autofill feature, but it seems to me, like your code is not backward compatible... I am not sure though, I will look in to it... it because your code implement new database, instead of use the one that app use... It would be better if you have use controller class. I will look into it and update you soon... Expect a proper reply from me, Tommorow... ==== Note I have not compile the code I just look the code and saying. So, I am not sure.. Also it hard to test autofill service in general as it required user data and setup on phone... It reason this delay & why i am currently put this pr on hold and not approve directly... |
|
Very welcome. Backwards compatible for what? Autofill was implemented in android 8 (API 26) but that is only support for it, the inline features were only supported from android 11 (API 30) When I pulled the repo, I didn't notice a preexisting database so I created a new local storage database. It's not a problem, I can either change it to incorporate the preexisting one, or keep it as I have set it up and remove the preexisting one as from what I saw, it had no implementation in the app anyway. As I said I during our chat, any bugs, let me so I can fix them. |
There was a problem hiding this comment.
Overview
I have review the code changes.. and overall it seem fine to me.
I have compile & run the app and looked at code changes.. App work fine on my device (samsungA14 android-15)..
Now hear me out, features like auto fill are very hard to test even on real phone and heavily rely on the android version used, according to me.. So, I have not test (or was not able to test) the auto-fill feature, but i end up, i have just testing app working and functionality. and it was fine... even, android detect passcodes as a auto-fill service aleast that what i have verify on my phone.
So, as said above I am accepting this pr as a solid foundation for autofill feature... and will make it as a preview feature for now...
TL;DR
Look good to me overall!! If I ignore the database thing. I am approving the pr... And feel free to proceed with the merge, and then i will takeover and make the database changes that I said Or if you wish, you can make changes suggested below.. and then ask review...
Suggestion
- Make the database changes suggested above... because we have one real user using the app in production @jainil63. and we can;t effort to lose data...
- Highlight in the readme of repository, that this features are in preview and are not intended to be use in production.. Also wrap this this feature inside a feature flag if possible. but i guess that would not be possible due to involvement of android manifest.
- And optionally write a good commit message and also make this pr such that it describe your changes & also the preview feature thing and not use in production....
feel free to ask if you get suck anywhere
|
|
||
| if (!username.isNullOrEmpty() && !password.isNullOrEmpty()) { | ||
| serviceScope.launch { | ||
| PasscodeDatabase.getDatabase(applicationContext).passcodeDao().insert( |
There was a problem hiding this comment.
Use the existing database. because app is in production and we can;t effort to lose users data.
refer:- Controller.kt interact with this class instead of master db directly
| import com.jeeldobariya.passcodes.R | ||
| import com.google.android.material.button.MaterialButton | ||
|
|
||
| class AutofillSettingsActivity : AppCompatActivity() { |
There was a problem hiding this comment.
I don;t understand what this is for... and where this activity will be call..
I mean, currently it not called from anywhere... It was probably intend to be call from setting activity.. I guess.
There was a problem hiding this comment.
Also, I don;t know why we require this file... I have seen the same over on keypass codebase..
What i mean is, if it just a empty file then, why we need this and what it is use for please explain.. if it use for configuration then how?
(I am new to android so don;t mind... I think it is require by android.. but i don't know it purpose..)
|
@kudanilll have look into the pr... and also test the app if possible on your device... because i try but I was not able to... because the autofill service is complicated to test and we can't say wether it work everywhere or not |
2 participants
Changes Made
This is a test push to verify push authentication due to permission errors. Docs will be provided later once verified.