Skip to content
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

feat(refactor): replace glide with coil image loading library (fast & small) #1368

Merged
merged 4 commits into from
Oct 26, 2024

Conversation

IndusAryan
Copy link
Contributor

@IndusAryan IndusAryan commented Oct 5, 2024

🔥
coil is native and faster
is small
no conditions for error placeholder visibility
unlike glide where multiplt instances with different contexts were used, we now use a global synchronized instance for all builders through application class
expect performance improvements and smaller apk

@fire-light42
Copy link
Collaborator

I can maybe understand why to use coil over glide. However I dont understand why you have to break the setImage API with some new API, as it was quite literally made to be replaceable???

@IndusAryan
Copy link
Contributor Author

IndusAryan commented Oct 5, 2024

I can maybe understand why to use coil over glide. However I dont understand why you have to break the setImage API with some new API, as it was quite literally made to be replaceable???

i have tested in many screens and many extensions, i have not encountered any breakpoint, the UIHelper was modified because loadImage is present in coil module so that we can stick to one instance and coil doesn't support drawable res, instead default input formats and presets

@IndusAryan IndusAryan changed the title feat(refactor): ditch glide and embrace coil feat(refactor): replace glide with coil image loading library (fast & small) Oct 6, 2024
@IndusAryan
Copy link
Contributor Author

i also reformatted to a proper text util and its location plus introduced a header object

@@ -1,17 +1,13 @@
package com.lagradost.cloudstream3.ui.result
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason behind this package move? It is a breaking change, so it needs to be a good reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not encountered any issue, the file name should be textutil instead of uitext as image functions have been removed, location was also in wrong package

Copy link
Collaborator

@fire-light42 fire-light42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can merge this, but some extensions may break due to UiText.

@IndusAryan
Copy link
Contributor Author

I can merge this, but some extensions may break due to UiText.

i think extension devs need to be pinged for changes immediately after merge so that they can update few

@fire-light42
Copy link
Collaborator

After testing this PR, I have to say that it is has several issues, but all of them stem from your weak typing of Any? and as such it fails to load on several places. Your removal of UiImage also prevents any strong typing making it impossible to use the type system correctly. Due to this I will fix this issues myself.

@IndusAryan
Copy link
Contributor Author

@fire-light42 can you specify certain location where loading fails ? the any supports all types we load if we look at the load function inside library

@fire-light42
Copy link
Collaborator

@fire-light42 can you specify certain location where loading fails ? the any supports all types we load if we look at the load function inside library

There is two places where the Any type caused problems, one was pluginIcon.loadImage(pluginIcon) where you give an ImageView as the image, and second the profileBg.loadImage(art) where you insert a list of integers as the image. The last thing this caused was with accounts, or more specifically the type got turned into any so it was laborious to use.

I dont see why you need to use Any when it just causes these types of bugs.

@fire-light42 fire-light42 merged commit 83318b0 into recloudstream:master Oct 26, 2024
2 checks passed
@fire-light42
Copy link
Collaborator

Lets hope this does not break stable or pre too hard.

@IndusAryan
Copy link
Contributor Author

IndusAryan commented Oct 26, 2024

Lets hope this does not break stable or pre too hard.

yeah,, can you tell the repo name in which you faced problem, type safety is good and robust but unfortunaetely i am just comparing latest pre and my debug hand to hand in same device and both account images in select activity , the bottom sheet, setting fragment are loading properly plus the repo images also !

@fire-light42
Copy link
Collaborator

This PR breaks the API in two ways, the first is that registerVideoClickAction(VideoClickAction) where VideoClickAction has abstract val name: UiText. Some providers may also use some custom settings to load loading images or do stuff in the custom action if TvType=CustomMedia.

@IndusAryan
Copy link
Contributor Author

understood now, thank you.

@rockhero1234
Copy link

In my case it is bit slower

@IndusAryan
Copy link
Contributor Author

old caches cleared, new disk caches, memory caches and http caches will be built slowly and size will be raised gradually, i forgot to include crossfade also so it looks like rapid load

@DZFOORT
Copy link

DZFOORT commented Dec 6, 2024

Screenshot_2024-12-06-18-05-49-451_com lagradost cloudstream3 prerelease

bug After this update

fire-light42 added a commit that referenced this pull request Dec 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants