Feature - Add dynamic image (Coil, Glide, Picasso) #7
Conversation
TomasValenta
commented
Dec 20, 2020
TomasValenta
commented
- This is currently "only a preview" as I imagined this library.
- Please, look at it if it's okay from your opinion or if you would do it differently.
- Then I would test it and write some useful test ;)
dynamic/image-coil/src/main/java/com/twocoders/dynamic/image/coil/DynamicImage.kt
Outdated
Show resolved
Hide resolved
dynamic/image-coil/src/main/java/com/twocoders/dynamic/image/coil/DynamicImage.kt
Outdated
Show resolved
Hide resolved
dynamic/image/src/main/java/com/twocoders/dynamic/image/BaseDynamicImage.kt
Outdated
Show resolved
Hide resolved
dynamic/image/src/main/java/com/twocoders/dynamic/image/BaseDynamicImage.kt
Show resolved
Hide resolved
| private fun Drawable?.getParcelable(): Parcelable? = when (this) { | ||
| is BitmapDrawable -> bitmap | ||
| is Drawable -> { | ||
| logd("Unsupported $this in imageDrawable for parcel, only BitmapDrawable is supported now.") |
There was a problem hiding this comment.
hmm toto dost vela Drawable implementacii zahodi, co moze byt pre pouzivatelov celkom velka prekazka :-/
There was a problem hiding this comment.
Mno až tak černě bych to neviděl. Toto se reálně týká stavu, kdy máš naloadovanou nějakou instanci "Drawable", tedy nepracuješ s resourcem (imageRes: Int) a ani s uri (ať už remote nebo local). Tedy je tu velká šance, že tam budeš mít právě tu BitmapDrawable :-D ale i kdyby ne, tak ten parcel je tu aj tak spíše jako taková záchrana, aby jsi si to mohl posílat přes Bundle, ve finále nepředpokládám, že se to bude někdo reálně snažit parcelovat.
Ale ano, ten case nastat může. Můžeme k tomu dát do popisu ještě trochu info, ale není to až takový běžný use case, že tam budeš mít naloadovanou instanci např. VectorDrawable (pravdě podobně by jsi jí také už měl toho času jako BitmapDrawable),ale tu budeš mít pravdě podobně v té imageRes referenci ;)
A navýše to necrashne, "jen" se ti ta edge case Drawable při parcelu zahodí, protože to vrátí null ;)
There was a problem hiding this comment.
No ono prave ked naloadujes vector, tak budes mat vector drawable po cely cas zivota tej drawable, takze to moze byt cim dalej tym viac pripadov. Aj ked setnes drawable na view a potom nacitas spat z. view, sale to bude vector. To iste plati aj o Animated atd. Ale ano, crash to nebude, len sa to nevykona
There was a problem hiding this comment.
To co popisuješ ale není ten primární use case. I když máš např. vector, tak si ho pravděpodobně držíš v té imageRes nebo případně Uri referenci. Reálně se "naloaduje" až když s ním jde např. View pracovat a to si na něm v tu chvilku zavolá ten loadDrawableInto(), ale neznamená to, že ho naloaduje do té drawable v DynamicImage. Když to mezi tím pošleš někam jinam před bundle nebo dojde k re-create aktivity, tak se to znovu naloaduje z té původní reference. A na optimalizace je tu už pak konkrétní implementace té image knihovny (glide, coil, picasso), která si to ve své cache už najde, aby tu drawable interně nevytvářel nanovo.
Proto si myslím, že to o čem se tu bavíme, je opravdu jen "minoritní" use case, ale ano, je tam 😉
...ic/image-glide/src/main/java/com/twocoders/dynamic/image/glide/DynamicImageBindingAdapter.kt
Outdated
Show resolved
Hide resolved
In general it looks very good. I'm worried about "Parceling" drawables though. But not sure what can be done to improve that :-/ |
…ge in BaseDynamicImage and it's implementations. Fix project url. Improve DynamicImage class docs.
|
BTW ten base moze mat verziu a byt samostantne releasovany, skor mi islo o to meno. Ale v zasade to momentalne nie je take dolezite, ci je to project dependency alebo lib dependency |
|
Tak já zkusím co nejdříve doplnit testy a můžeme to pak mergnout 😉 |
