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

Compose GlideImage set loading placeholder with @Composable Painter bugs #5224

Closed
Caij opened this issue Jul 29, 2023 · 7 comments · Fixed by #5296
Closed

Compose GlideImage set loading placeholder with @Composable Painter bugs #5224

Caij opened this issue Jul 29, 2023 · 7 comments · Fixed by #5296
Labels

Comments

@Caij
Copy link

Caij commented Jul 29, 2023

Glide Version: 4.15.1

Integration libraries:
com.github.bumptech.glide:compose:1.0.0-alpha.3

When setting a placeholder of type Painter, LazyColumn scrolling through the list triggers image redraws, causing visual flickering of the image displayed on the screen.
If a ColorDrawable is set as the placeholder, the display appears normal without any flickering issues.

@OptIn(ExperimentalGlideComposeApi::class)
@Composable
actual fun PlatformImage(
    model: Any?,
    contentDescription: String?,
    modifier: Modifier,
    alignment: Alignment,
    contentScale: ContentScale,
    alpha: Float,
    colorFilter: ColorFilter?,
    filterQuality: FilterQuality,
    placeholderPainter: Painter?
) {
    GlideImage(
        model = model,
        contentDescription = null,
        modifier = modifier,
        alignment = alignment,
        alpha = alpha,
        colorFilter = colorFilter,
        contentScale = contentScale,
//        loading = placeholder(ColorDrawable(0xff000000.toInt()))
    loading = placeholder { placeholderPainter }
    )
}
@sjudd
Copy link
Collaborator

sjudd commented Aug 3, 2023

Recomposition is expensive :/. I do think the capability is useful for cases where you don't have tons of images, but I agree that in a scrolling list you should use a normal placeholder.

I'm re-writing how we render images to switch from painters to modifiers. Let's see how this makes sense when that's done.

@sjudd sjudd added the Compose label Aug 3, 2023
@sjudd
Copy link
Collaborator

sjudd commented Aug 13, 2023

Ok so we support resource ids or drawables as placeholders. Do you have a painter that you'd like to use as a placeholder that is not from a resource or drawable? If so, where are you getting the painter?

@joerogers
Copy link

joerogers commented Aug 21, 2023

To answer a specific case for a painter:

It feels like the "material" way to get resources is to use the Material Icons. An example would be: Icons.Default.AccountCircle which is of type ImageVector. I use it with the Icon composable function which uses a rememberVectorPainter under the covers. Seems like Glide could accept an ImageVector in addition to a resourceId and drawable and under the covers use the vectorPainter as well.

If the api really needs a painter, then I would pass: rememberVectorPainter(Icons.Default.AccountCircle)

I looked at the new GlideImage and GlideSubcomposition apis. Are you implying that if I pass a resourceId/drawable (although I would really love ImageVector to also be supported), I get a "loading/failure" placeholder for free (or cheaper than the GlideSubcomposition variation). Would that remain even if the image fails to load?

I get the enter recomposition problem. However, the use case I'm more interested in is the failure case. I could possibly live with a blank while loading, but if the network fails, I'd really love my placeholder to show instead of a blank.

@sjudd
Copy link
Collaborator

sjudd commented Aug 22, 2023

Are you implying that if I pass a resourceId/drawable (although I would really love ImageVector to also be supported), I get a "loading/failure" placeholder for free

Yup! There's no expensive recomposition if you use GlideImage's loading / failure APIs (both of which accept a Placeholder).

I think failure is less likely, but there's still a degenerate case where you have a large scrolling list of images that require network access. If you recompose each time the image fails, it'll still be janky. Granted it's already going to be suboptimal since the images aren't loading, but at least they can smoothly not load...

I'm not fundamentally opposed to adding more Placeholder types if they're sufficiently generic. However we'd just convert ImageVector into a painter to draw it anyway. It does sound like we should accept Painter, we could optionally do the VectorImage -> Painter conversion as well if that's a common type.

@joerogers
Copy link

joerogers commented Aug 22, 2023

I'll let the original reporter chime in on his Painter request. I think I'd be happy enough with the 'VectorImage' -> 'Painter' conversion. While you can still use XML resources, in my new app where I'm playing with compose a lot I'm mostly using VectorImages instead since it is essentially the same as a vector drawable for the material icon, but very Compose like. I love that I don't have to import them into the project every time I want to use them.

FYI in the last beta, I used the loading/failure as a composable only to load an Icon to load the VectorImage. Since the composable is expensive I wanted to make sure I stopped doing that but realize I now need to go back to the vector drawable resource.

However, I could see someone wanting a custom Painter as well, but I have a harder time coming up with that use case.

@Caij
Copy link
Author

Caij commented Sep 13, 2023

Ok so we support resource ids or drawables as placeholders. Do you have a painter that you'd like to use as a placeholder that is not from a resource or drawable? If so, where are you getting the painter?

compose multiplatform, Painter used in a large number of scenes, eg:https://github.com/icerockdev/moko-resources#compose-multiplatform-1 image resource.

And https://github.com/coil-kt/coil/blob/main/coil-compose-base/src/main/java/coil/compose/AsyncImage.kt placeholder is Painter

@joerogers
Copy link

In thinking about this since I first responded. I generally think a Painter as a placeholder removes the burden from Glide from having to support each different thing a developer would want like drawables/resources/ImageVectors. I think the ability to use an ImageVector as a fallback/placeholder is a must for version 1. However, if you simplify the api to accept a Painter for the placeholder, then Glide would be able to use an ImageVector for free. You already convert resources and drawables to a Painter internally so it would almost be a simplification of the current api.

If Glide wants to continue to provide a Painter for the Android concepts like drawables and image resources, that would be up to you. Seems like a handy thing to help migration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants