Migration: com.passwordmanager to com.jeeldobariya.passcodes#17
Migration: com.passwordmanager to com.jeeldobariya.passcodes#17JeelDobariya38 merged 26 commits intomainfrom
com.passwordmanager to com.jeeldobariya.passcodes#17Conversation
waring: changes are not yet tested
compileSdk = 36 (android 13) targetSdk = 34 (android 14)
warn: commit consist of code which is not test, this commit alone itself is irreversible.
Use Gemini 2.5 Flash
|
@kudanilll please, through review the code and point any mistakes.. even if seems small as this is a major changes in project... |
fix: the bug of database operation on main thread bug was introduce in 1fbcc9b This commit is not stand alone all previous commit are part of it. after bug are part of it. be very careful revert any of this commit as standalone chore(depsfix): fix a issue in build.gradle refactor: ViewPassword.kt to use coroutines refactor: UpdatePassword.kt to use coroutines refactor: SavePassword.kt to use coroutines refactor: LoadPassword.kt to use coroutines refactor: update the controller to use suspend function the commit is not complete as a whole feat: make every db operation a suspend function and use flows the commit is not complete as a whole chore(deps): add dependency for coroutines chore(deps): improve the build.gradle and make morder
b9d521d to
d5617ef
Compare
|
Most code changes have been done!!. I am still make sense of codebase and also of test. (As this migration was mostly assisted by gemini. And also I am new to kotlin ecosystem)
@kudanilll You also check the codebase!! And it better to review code in this branch as whole rather then this pr itself. |
GitHub URL & README are still left.
f950ed5 to
5553e7e
Compare
|
@kudanilll 🥳🥳 Finally, migration is completed. Now, I will go through entire kotlin codebase and try make sense of it... |
JeelDobariya38
left a comment
There was a problem hiding this comment.
I have thoroughly review the code and understand it and want to point out something to you @kudanilll that stick out to me while review the code...
Also, As I understand code, I am marking pr as ready to merge.... meaning undrafted.... and If everything is fine, Don't wait for me danil just proceed merge it....
Please provide your valuable feedback!! Thanks for your time...
| binding = ActivityUpdatePasswordBinding.inflate(layoutInflater) | ||
| setContentView(binding.root) | ||
|
|
||
| val intent = intent |
There was a problem hiding this comment.
I don't know, why gemini just did this, he/we initialized intent and then use it direct;y in next line... Do you @kudanilll, know any specific reason why this might be done?
Or is it a good practice in android community to first take the indent in variable and then do, stuff with it?
Similar changes are also found in app/src/main/kotlin/com/jeeldobariya/passcodes/ui/ViewPasswordActivity.kt
| class PasswordAdapter( | ||
| private val context: Context, | ||
| private var passwordList: List<Password> // Use mutable list if you need to update it | ||
| ) : BaseAdapter() { |
There was a problem hiding this comment.
I am as Android Beginner, not know of this Adaptor stuff, But still Accept this changes!! I will later, try to learn it...
I can sort of make sense of it as fragment.. but, I am not sure!! And still learn....
I hope @kudanilll you will take care of this code, if any issue occur until I fully learn this stuff!!
| private fun addOnClickListenerOnButton() { | ||
| binding.passwordList.setOnItemClickListener { _, _, position, _ -> |
There was a problem hiding this comment.
Important
POTENTIAL BUG!!
But, It's not yet encountered by me in any debug build that, I made or share with fellow friend/tester, @jainil63.
This code to me seems to be buggy, I don't why? and I am not sure is it really?
I guess, It because, I see three thing that i like to point out...
passwordAdapteris a lateinit var, Which mean it value can be change later, and could be potential null at first. until we initialize it, in this case, this happen incollectPasswordList()when password are found.. or not. in both case this is initialized..- We are in
addOnClickListenerOnButton()function, where we just add a event listener, Without do any prior null checks. Note that, InonCreate().addOnClickListenerOnButton()is called beforecollectPasswordList(). - Also, the initialization happen in specifically
collect()of Flow. Which mean it could change over time. andpasswordAdaptercould be reinitialized, and there in that new instance there be no onclicklistener on it, As we call the method only once inonCreate().
Anyways, Whatever the thing is, we will discuss it and probably, make work around it. But not in this PR [considering, there are already very many major change in this PR and it already review by danill and me, for multiple times, cost time & effort] Further changes on this, will be made in a new PR or under explicitly github issue (for bug specific)...
Please Provide Further Information @kudanilll.
| private fun addOnClickListenerOnButton() { | |
| binding.passwordList.setOnItemClickListener { _, _, position, _ -> | |
| /* | |
| TODO: | |
| - A Null check & Throw explicit error for null values. | |
| - Call this function every time on reinitialization of passwordAdapter. | |
| - Make this Function idempotent. | |
| */ | |
| private fun addOnClickListenerOnButton() { | |
| binding.passwordList.setOnItemClickListener { _, _, position, _ -> |
There was a problem hiding this comment.
Changes in this file are also unfamiliar to me...
I am still learning permissions as android beginner...
But accept this changes as this is not my code and also, app don't use this stuff!!
com.passwordmanager to com.jeeldobariya.passcodes
Important
Attention! Attention! Major architecture changes!!
Changes Made
com.passwordmanagertocom.jeeldobariya.passcodes.Gradle 7.6 -> 8.14.3.Groovy (old) -> Kotlin DSL.SDK 36and MoveTarget SDK from 33 -> 34.java to kotlin. [I have use Gemini 2.5 extensively just to migrate it faster I am still unfamiliar with code base and make sense of it. once i complete make sense of it will mark this pr as not drafted mean read to merge @kudanilll]SQLiteOpenHelpertoRoomLibrary.Kotlin Flowlibrary.Room DatabasePassword ManagertoPasscodeswhere possible. [meaning, url are still not updated]com.passwordmanagertocom.jeeldobariya.passcodes#14 and resolves ⁉️Doubt: Common Package Name⁉️ #12TODO: yet left changes