Android icon indicating copy to clipboard operation
Android copied to clipboard

[Code Improvement] Kotlin Delegates are not null safe and contain deprecated OnLifecycleEvent.

Open AndroidPoet opened this issue 3 years ago • 3 comments

Describe the bug

Currant kotlin delegates are not null safe and also accessing methods using getMethod("bind") is not very good practice, even though there will be no problem using these. There is also deprecated claas "@OnLifecycleEvent" This annotation required the usage of code generation or reflection, which should be avoided, alternative "DefaultLifecycleObserver" can be used. We can also eliminate typecasting.

Get Method

get method

Not null safe (can be fixed but lot of boilerplate code)

nullunsafe

Deprecated OnLifecycleEvent

deprecated

Soultion

I have added a link for the gist , which can be used to eliminate all the above issues. I have recently migrated most of the classes from the kotlin extension and findViewById to ViewBinding.I would love to implement this kotlin delegate with those classes also there is not much refactoring we have to replace only one line in every fragment or activity

Even though the current solution is ok and works fine, it might create issues in the future, with updated delegates code will become much cleaner and soild.

I need approval from the internal team to change these delegates.

Gist:https://gist.github.com/AndroidPoet/907a7c39c6312ab4d6db6b2581d8a0a1

soultion

How to use

Fragment

fragment

Activity

activity

DialogFragment

DialogFragment

AndroidPoet avatar Jun 06 '22 09:06 AndroidPoet

Hi, @aitorvs please check these once and Let me know your thoughts on this, as we discussed about memory leaks, those are bit untraceable but with this delegate, we can make sure that there is no memory leak as for as view binding is concerned.

AndroidPoet avatar Jun 08 '22 15:06 AndroidPoet

Hey @AndroidPoet. Thanks for opening this issue and the time spent caring about the code base.

Unless there's something fundamentally wrong with the current code and/or we find and are able to consistently repro a bug eg. memory leak in the viewbinding delegate logic, I'd personally not be very supportive of changing it just because.

aitorvs avatar Jun 09 '22 08:06 aitorvs

@aitorvs I agree with you completely! the problem is as of now only a few activities or fragments are using view binding. I worked on view binding PR it will be reviewed, but there was a problem with DialogFragment view binding delegate, we are using the same view binding for fragment and DialogFragmen, but it will crash when we override "onCreateDialog" and show "AlertDialog" I know it is working in "DaxDialog but" we are using it normally we are not showing "AlertDialog". That might be fixed in the PR I have made,I am using a different delegate for DialogFragment because they have a different lifecycle compared to normal fragments.

Let's see any memory leaks after the whole app view binding migration.

AndroidPoet avatar Jun 09 '22 09:06 AndroidPoet