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.

Sign up for GitHub

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

Jump to bottom

Support tint list on GifDrawable. #5341

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Support tint list on GifDrawable. #5341

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d23472
Support tint list on GifDrawable.
sumita12 Dec 4, 2023
8ef3767
Address review comments.
sumita12 Dec 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
61 changes: 59 additions & 2 deletions library/src/main/java/com/bumptech/glide/load/resource/gif/GifDrawable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,16 @@
import static com.bumptech.glide.gifdecoder.GifDecoder.TOTAL_ITERATION_COUNT_FOREVER;

import android.content.Context;
import android.content.res.ColorStateList;
import android.content.res.Resources;
import android.graphics.Bitmap;
import android.graphics.Canvas;
import android.graphics.Color;
import android.graphics.ColorFilter;
import android.graphics.Paint;
import android.graphics.PixelFormat;
import android.graphics.PorterDuff;
import android.graphics.PorterDuffColorFilter;
import android.graphics.Rect;
import android.graphics.drawable.Animatable;
import android.graphics.drawable.Drawable;
Expand Down Expand Up @@ -66,6 +70,9 @@ public class GifDrawable extends Drawable

private boolean applyGravity;
private Paint paint;
private ColorStateList tint;
private PorterDuff.Mode tintMode;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Default tintMode is documented as SRC_IN

https://developer.android.com/reference/android/graphics/drawable/Drawable#setTintMode(android.graphics.PorterDuff.Mode)

The Android Drawable implementations also imply that this should not be settable to null but null should set back to the default (this behavior is not documented and is not marked as nullable, but the base Drawable compat implementation treats null as default values)

Copy link
Author

Choose a reason for hiding this comment

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

null should set back to the default

You are right. I updated this PR to use the default mode if null.

The Android Drawable implementations also imply that this should not be settable to null

FYI, Drawable#setTintMode() accept null.
https://cs.android.com/android/platform/superproject/+/master:frameworks/base/graphics/java/android/graphics/drawable/Drawable.java;l=692?q=drawable.java

private ColorFilter tintFilter;
private Rect destRect;

/** Callbacks to notify loop completion of a gif, where the loop count is explicitly specified. */
Expand Down Expand Up @@ -288,7 +295,17 @@ public void draw(@NonNull Canvas canvas) {
}

Bitmap currentFrame = state.frameLoader.getCurrentFrame();
canvas.drawBitmap(currentFrame, null, getDestRect(), getPaint());
Paint paint = getPaint();
ColorFilter colorFilter = paint.getColorFilter();
if (colorFilter != null || tintFilter == null) {
// ColorFilter disables tint list. See Drawable#setColorFilter().
canvas.drawBitmap(currentFrame, null, getDestRect(), paint);
} else {
// Temporary set a tint filter then restore.
paint.setColorFilter(tintFilter);
canvas.drawBitmap(currentFrame, null, getDestRect(), paint);
paint.setColorFilter(colorFilter);
}
}

@Override
Expand All @@ -298,7 +315,47 @@ public void setAlpha(int i) {

@Override
public void setColorFilter(ColorFilter colorFilter) {
getPaint().setColorFilter(colorFilter);
if (getColorFilter() != colorFilter) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why use reference equality instead of Objects.equals?

and why does this specific method check for equality but the other methods not? I guess this is trying to handle the case where colorFilter is null to prevent it from clearing the tint? If that's the case please can you make the code explicit (preferred) or add a comment to say it is, because otherwise it's not at all obvious from the code that this is the intent.

Copy link
Author

Choose a reason for hiding this comment

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

I added the check because GradientDrawable does the same thing.
However, I think it is not directly related to this PR, so please let me revert this change to simplify this PR.

getPaint().setColorFilter(colorFilter);
invalidateSelf();
}
}

@Override
public ColorFilter getColorFilter() {
return getPaint().getColorFilter();
}

@Override
public void setTintList(ColorStateList tint) {
this.tint = tint;
updateTintFilter();
invalidateSelf();
}

@Override
public void setTintMode(PorterDuff.Mode tintMode) {
this.tintMode = tintMode;
updateTintFilter();
invalidateSelf();
}

@Override
protected boolean onStateChange(int[] stateSet) {
if (tint != null && tintMode != null) {
updateTintFilter();
return true;
}
return false;
}

private void updateTintFilter() {
if (tint != null && tintMode != null) {
int color = tint.getColorForState(getState(), Color.TRANSPARENT);
tintFilter = new PorterDuffColorFilter(color, tintMode);
} else {
tintFilter = null;
}
}

private Rect getDestRect() {
Expand Down
54 changes: 48 additions & 6 deletions library/test/src/test/java/com/bumptech/glide/load/resource/gif/GifDrawableTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.ArgumentMatchers.isA;
import static org.mockito.ArgumentMatchers.isNull;
Expand All @@ -16,6 +17,7 @@
import static org.mockito.Mockito.when;

import android.app.Application;
import android.content.res.ColorStateList;
import android.graphics.Bitmap;
import android.graphics.Canvas;
import android.graphics.Color;
Expand All @@ -40,7 +42,6 @@
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.ArgumentCaptor;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
import org.robolectric.RobolectricTestRunner;
Expand Down Expand Up @@ -568,12 +569,53 @@ public void testSetAlphaSetsAlphaOnPaint() {
public void testSetColorFilterSetsColorFilterOnPaint() {
ColorFilter colorFilter = new PorterDuffColorFilter(Color.RED, Mode.ADD);
drawable.setColorFilter(colorFilter);
verify(paint).setColorFilter(eq(colorFilter));
}

@Config(sdk = Build.VERSION_CODES.LOLLIPOP)
@Test
public void testDrawSetsTintListColorFilterOnPaint() {
ColorStateList tint =
new ColorStateList(
new int[][] {new int[] {android.R.attr.state_pressed}, new int[0]},
new int[] {Color.RED, Color.GREEN});
drawable.setTintList(tint);
drawable.setTintMode(Mode.ADD);
when(paint.getColorFilter()).thenReturn(null);
drawable.draw(new Canvas());

// draw() temporary sets tint filter then restore.
verify(paint).setColorFilter(eq(new PorterDuffColorFilter(Color.GREEN, Mode.ADD)));
verify(paint).setColorFilter(null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this test is not testing drawable behavior but internal implementation details of the draw method, what we care about is that the bitmap was drawn with a paint configured with the tint filter, not that paint was interacted with.

Copy link
Author

@sumita12 sumita12 Dec 18, 2023 •
edited

Choose a reason for hiding this comment

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

Thank you for pointing it out!
Previously I followed the test code of setColorFilter().

Now, I updated the test code of both setColorFilter() and setTintList() to check the Paint object which is used by Canvas#drawBitmap().
PTAL?

(Ideally it's better to check the drawn bitmap colors, but we cannot do it on Robolectric tests. Instrumentation tests may work, but I think the current Robolectric test is enough.)

Copy link
Author

@sumita12 sumita12 Dec 18, 2023 •
edited

Choose a reason for hiding this comment

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

Note:
I updated the behavior of GifDrawable#draw() to make it test-friendly.

We had the following code inside GifDrawable#draw().

// Temporary set a tint filter then restore.
paint.setColorFilter(tintFilter);
canvas.drawBitmap(currentFrame, null, getDestRect(), paint);
paint.setColorFilter(colorFilter);

With this code, we cannot verify whether canvas#drawBitmap() uses the tintFilter or not with ArgumentCaptor, because paint.setColorFilter(colorFilter) is called before ArgumentCaptor#capture(). The new code doesn't call paint.setColorFilter(colorFilter) to avoid the issue.


assertThat(drawable.setState(new int[] {android.R.attr.state_pressed})).isTrue();
drawable.draw(new Canvas());
Copy link
Collaborator

Choose a reason for hiding this comment

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

performing multiple acts in a test implies that there should be two tests, please split this into a separate test that verifies the state is reflected in the filter.

Copy link
Author

Choose a reason for hiding this comment

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

Done.


// Pressed state. draw() temporary sets a red color filter.
verify(paint).setColorFilter(eq(new PorterDuffColorFilter(Color.RED, Mode.ADD)));
verify(paint, times(2)).setColorFilter(null);
}

@Config(sdk = Build.VERSION_CODES.LOLLIPOP)
@Test
public void testDrawUsesColorFilterInsteadOfTintList() {
ColorStateList tint =
new ColorStateList(
new int[][] {new int[] {android.R.attr.state_pressed}, new int[0]},
new int[] {Color.RED, Color.GREEN});
drawable.setTintList(tint);
drawable.setTintMode(Mode.ADD);
ColorFilter colorFilter = new PorterDuffColorFilter(Color.BLUE, Mode.ADD);
drawable.setColorFilter(colorFilter);
verify(paint).setColorFilter(eq(colorFilter));
when(paint.getColorFilter()).thenReturn(colorFilter);

drawable.draw(new Canvas());
drawable.onStateChange(new int[] {android.R.attr.state_pressed});
drawable.draw(new Canvas());

// Use ArgumentCaptor instead of eq() due to b/73121412 where ShadowPorterDuffColorFilter.equals
// uses a method that can't be found (PorterDuffColorFilter.getColor).
ArgumentCaptor<ColorFilter> captor = ArgumentCaptor.forClass(ColorFilter.class);
verify(paint).setColorFilter(captor.capture());
assertThat(captor.getValue()).isSameInstanceAs(colorFilter);
// ColorFilter disables tint list, so draw() should not invoke setColorFilter() any more.
verify(paint).setColorFilter(any());
}

@Test
Expand Down