bumptech / glide

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

Tinted drawable into ImageView only works with custom Target that does nothing #3426

Open d4rken opened 5 years ago

d4rken commented 5 years ago

Glide Version: 4.8.0

Device/Android Version: Pixel2@Android 9

Issue details / Repro steps / Use case background: Trying to tint a drawable within my custom decoder.

Decoder snippet

OwnerInfoDecoder implements ResourceDecoder<OwnerInfo, Drawable>
//
AnimationDrawable animation = new AnimationDrawable();
animation.setOneShot(false);
Drawable drawable = ContextCompat.getDrawable(context, R.drawable.ic_ghost_white_24dp);
drawable = DrawableCompat.wrap(drawable);
DrawableCompat.setTint(drawable.mutate(), ContextCompat.getColor(context, R.color.teal));
animation.addFrame(drawable, FRAME_DURATION);
return new AnimationDrawableResource(bitmapPool, animation);

Glide call:

GlideApp.with(getContext())
    .load(item.getOwnerInfos())
    .listener(new PlaceHolderRequestListener(ownerIcon))
    .into(ownerIcon);

This doesn't work, no tint is applied.

Now I created a custom target which currently does NOTHING:

public class TintTarget extends ImageViewTarget<Drawable> {
    public TintTarget(ImageView view) {
        super(view);
    }

    public TintTarget(ImageView view, boolean waitForLayout) {
        super(view, waitForLayout);
    }

    @Override
    protected void setResource(@Nullable Drawable resource) {
        view.setImageDrawable(resource);
    }
}

and use .into(new TintTarget(ownerIcon)); and now it works, without any other changes.

When I look at DrawableImageViewTarget which gets automatically created by Glide when just using into(ImageView), then it doesn't work, even though my TintTarget is same thing?

What am I missing here?

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had activity in the last seven days. It will be closed if no further activity occurs within the next seven days. Thank you for your contributions.

d4rken commented 5 years ago

:man_shrugging:

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had activity in the last seven days. It will be closed if no further activity occurs within the next seven days. Thank you for your contributions.

sjudd commented 5 years ago

Why are you wrapping it in an AnimatableDrawable? Also why are you mutating the drawable before you set the tint, but not referencing the returned Drawable? mutate() often returns a copy of the original Drawable, specifically so you can apply thins like tints to some copies of the image but not others while still storing only one copy of the image bytes in memory.

Otherwise you can compare the difference between your custom Target and DrawableImageViewTarget. The only one I can think of off the top of my head that might be relevant is that DrawableImageViewTarget will start animated drawables if they implement Animatable. Maybe the animated drawable ignores your tint

d4rken commented 5 years ago

Why are you wrapping it in an AnimatableDrawable?

That code was simplified, the actual code loops through multiple Android packages, gets their icons, creates an animation with one icon per frame and tints each frame.

Also why are you mutating the drawable before you set the tint, but not referencing the returned Drawable? mutate() often returns a copy of the original Drawable, specifically so you can apply thins like tints to some copies of the image but not others while still storing only one copy of the image bytes in memory.

Documentation leads me to believe that .mutate() affects the drawable itself, with mutate() returning itself as code sugar.

Otherwise you can compare the difference between your custom Target and DrawableImageViewTarget. The only one I can think of off the top of my head that might be relevant is that DrawableImageViewTarget will start animated drawables if they implement Animatable. Maybe the animated drawable ignores your tint

Tried it but it also doesn't work with just this:

        Drawable drawable = ContextCompat.getDrawable(context, R.drawable.ic_ghost_white_24dp);
        drawable = DrawableCompat.wrap(drawable);
        drawable = drawable.mutate();
        DrawableCompat.setTint(drawable, ContextCompat.getColor(context, R.color.state_m3));
        return new SimpleResource<>(drawable);

Here is a minimal example project:

https://github.com/d4rken/android-kotlin-starter/compare/pr-glidedemo

d4rken commented 5 years ago

It works with the DrawableImageViewTarget.

So it seems related to public ViewTarget<ImageView, TranscodeType> into(@NonNull ImageView view)

d4rken commented 5 years ago

This is causing it, no idea why though, but if I overwrite the requestOptions via debugger with dontTransform() then it works. Hm, thoughts @sjudd ?

    RequestOptions requestOptions = this.requestOptions;
    if (!requestOptions.isTransformationSet()
        && requestOptions.isTransformationAllowed()
        && view.getScaleType() != null) {
      // Clone in this method so that if we use this RequestBuilder to load into a View and then
      // into a different target, we don't retain the transformation applied based on the previous
      // View's scale type.
      switch (view.getScaleType()) {
        case CENTER_CROP:
          requestOptions = requestOptions.clone().optionalCenterCrop();
          break;
        case CENTER_INSIDE:
          requestOptions = requestOptions.clone().optionalCenterInside();
          break;
        case FIT_CENTER:
        case FIT_START:
        case FIT_END:
          requestOptions = requestOptions.clone().optionalFitCenter();
          break;
        case FIT_XY:
          requestOptions = requestOptions.clone().optionalCenterInside();
          break;
        case CENTER:
        case MATRIX:
        default:
          // Do nothing.
      }
    }