bumptech / glide

An image loading and caching library for Android focused on smooth scrolling
https://bumptech.github.io/glide/
Other
34.64k stars 6.12k forks source link

Compose: Unable to easily tint placeholder in GlideImage #5301

Open joerogers opened 1 year ago

joerogers commented 1 year ago

Glide Version: 5.0.0-rc01

Integration libraries: Compose: 1.0.0-beta01

Device/Android Version: All versions supporting dark themes/dynamic themes most impacted

Issue details / Repro steps / Use case background:

I am creating a list of items, showing an image of the artist, plus the title and artist name. If the artist image is downloaded successfully it will show a jpg image cropped into a circle. That is working fine.

However, if the image fails to download and potentially while loading, I want to show an icon in its place AND be properly tinted whether the user has a light or dark theme enabled. I'm also using a dynamic theme so the actual colors may have a slight tint to them based on theme.

Here is a basic look/feel for what I expect:

Expected result

Here is the look/feel for what actually happens:

Actual result

Note the dark theme is not tinted to the expected color of the text. The light theme is using pure black vs a slightly elevated black as well, but it is subtile to the naked eye.

Note: This is reproducible with resources, drawables and a painter using an ImageVector.

This actually makes sense. In Compose Image() lets you pass two items to help color an image: an alpha value and a colorFilter. The drawback is if I passed them, then the would also apply to the raw image rendering it into a colored circle which I do not want.

What would be handy is if I could provide an alternate colorFilter/alpha for the placeholders. I could see adding this information in two different ways:

1) an optional placeholderAlpha, placeholderColorFilter parameter on GlideImage itself. Defaults would be the same as alpha/colorFilter

2) providing this information in the placeHolder() function. The advantage of this, is the tint could be different for both loading/failure cases, but I realize this is the more messy form as you have to pass a placeholder item, alpha and colorFilter for each potential placeholder.

I would be fine with option 1 because it seems reasonable for any place holders to have the same tinting, but it is possible someone has a need for option 2.

The workarounds are not great options:

1) Use GlideSubcomposition and render my own placeholders using the proper tint. Relatively easy, but now always in the sub-composition flow which sounds worse using it in a list. 2) Jump through a hoop to get a drawable to load and then apply the appropriate tint before handing to Glide. It is a bigger hoop to use the compose based material icons. 3) Switch to a solid color placeholder instead

Layout code:

   val painter = rememberVectorPainter(image = Icons.Rounded.AccountCircle)

   GlideImage(
            model = hostImageUri,
            contentDescription = null,
            modifier = adjustedModifier.clip(CircleShape),
            alignment = Alignment.Center,
            contentScale = ContentScale.Crop,
            failure = placeholder(painter)
        ) 

No error see images.

sjudd commented 1 year ago

Can you apply a tint to the Painter? https://developer.android.com/reference/kotlin/androidx/compose/ui/graphics/vector/package-summary#rememberVectorPainter(androidx.compose.ui.unit.Dp,androidx.compose.ui.unit.Dp,kotlin.Float,kotlin.Float,kotlin.String,androidx.compose.ui.graphics.Color,androidx.compose.ui.graphics.BlendMode,kotlin.Boolean,kotlin.Function2)

sjudd commented 1 year ago

It might also be an issue with rememberVectorPainter, it should obey light / dark mode automatically if the underlying vector resource does?

joerogers commented 1 year ago

@sjudd Are you saying that the developer must set any tint placeholder before handing to GlideImage? If so, then the bug/enhancement is this requirement should be clearly stated in the documentation for GlideImage and the placeholder() functions themselves. It was a bit of a shock to think things were working and find out it fell apart on dark mode.

Also, I recommend you should remove support for a resource like R.drawable.ic_placeholder. That will never work unless the developer is maintaining a standard Android theme in the host Activity that matches the Compose theme in all instances.

Here are examples:

For a resource it is better to load the drawable manually like this, where color is derived or obtained from the Compose theme. This defeats the lazy loading feature of passing the resource directly, but it will work better if dynamic tinting is required.

    val drawable = LocalContext.current.getDrawable(R.drawable.ic_launcher_foreground)
    drawable?.setTint(color.toArgb())

For an image vector you have to do something like this. I didn't see this function somehow so that is easier than what I did do. (Creating a custom rememberVectorPainter, taking the ImageVector and color would at least hide this logic away)

    val image = Icons.Rounded.AccountCircle
     val painter = rememberVectorPainter(
            defaultWidth = image.defaultWidth,
            defaultHeight = image.defaultHeight,
            viewportWidth = image.viewportWidth,
            viewportHeight = image.viewportHeight,
            name = image.name,
            tintColor = color,
            tintBlendMode = image.tintBlendMode,
            autoMirror = image.autoMirror) {
           _, _ -> RenderVectorGroup(group = image.root)
        }
sjudd commented 1 year ago

You shouldn't have to manually tint resources to match dark / light mode so long as those resources are properly defined. There shouldn't be anything compose specific here, if the resources have different dark / light mode definitions that work in the normal Android view system, they should also work in Compose. Are you saying that's not working? So if you were to just load the resoure in a normal Compose Image it would be tinted correctly, but if you use it as a placeholder in Glide's Compose integration it's not? If so that sounds like a bug we should look into.

If instead you're saying that you have always been manually changing the tint to match dark / light mode and are asking for a better way to do it in Glide's compose integration, that's a separate question.

joerogers commented 1 year ago

So in Compose you set up a theme that works specifically with compose. The Android view system has no visibility into that theme.  When you load a resource from a context, that context is using the Activity theme or the base App theme. If you are only using compose in the app, there really isn't a need to set up that theme for compose to work correctly. If you are using Dynamic colors, you have to do more "non-compose" things in order to get the OS to apply the dynamic colors to the activity theme separately from the Compose theme.  Essentially you would have to maintain two theme definitions, one for compose and one for the Activity/App that at least matches "color" logic for loading system resources.  

I was able to confirm this, because the default Material3 theme I was given when I setup the project was just a Light theme. I never noticed this because I was using a MaterialTheme light/dark dynamic color theme created in Compose and everything was working.

When I updated the app theme to a basic light/dark theme it worked better at loading resources (provided at a minimum I edited a resource to use themed colors for the tint vs hard coded colors). However, even then what was displayed wasn't exactly using the same exact "tint" compose is using when you use a color meter because I set up Dynamic Colors in my compose theme. I didn't go so far to get Dynamic colors working in the Android theme system because using resources/drawables isn't my primary use case. I just wanted to see if resources loaded by GlideImage also had the same issue as the vectorPainter and went far enough to prove it.

For me personally I'm trying not to have duplicate resource assets in the system for icons. Instead I'm using the compose Material Icon library instead. That is why I am using the VectorPainter. I wasn't really able to test this until beta01 with the enhancements you made to support painters. Prior to it I used ColorDrawable I manually created in code as a stand in for failure which bypassed this problem because I had supplied my own tint when I created the drawable.

Like you I was surprised it didn't just work. Turns out that Material Icons use a default tint defined in the icon and it isn't easy to change. I suspect the intent was to utilize the tinting mechanisms provided by compose Image or Icon, etc. For example using Icon, the tinting works automatically using LocalContentColor.current as the default tint, but a developer may provide an override value.

For Image however, no tinting is applied because the most likely use case is to display a bitmap and not an icon. However, the api did provide an override via the more advanced colorFilter and alpha parameters to use in different situations. However because the GlideImage is wrapping Image and may load a bitmap (via url or system resource) and an icon via a painter, resource or drawable, it has no real way to utilize the alpha/colorFilter setting except for all images currently. What I was proposing at minimum was to allow the developer to provide an additional optional colorFilter/alpha for any placeholder provided. Since a dev may use a bitmap as a placeholder not providing a filter for the placeholder is also a requirement.

As for a work around, you did point out the other rememberVectorPainter method as an easier way to tint the vector painter correctly which is way easier than what I actually got working in a prototype. In my original prototype, I created my own custom ImageVector which seemed fragile as I'm no longer tied to the Material Icon to pick up fixes/improvements. I'd also have to do it for every icon placeholder I planned to use which seemed like a lot of extra work. That is what prompted me to create this ticket. It felt easier if I could just tell Glide to use a colorFilter for the placeholder especially since a developer may have a mismatch between the compose theme and the android theme hosted by the activity.

For now I'm going to create a custom "rememberVectorPainter(ImageVector, Color)" as a work around and hide the tinting logic away so I can progress.