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

Support tint list on GifDrawable. #5341

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

Conversation

sumita12
Copy link

@sumita12 sumita12 commented Dec 4, 2023

Description

This PR implements GifDrawable#setTintList() and GifDrawable#setTintMode() to support tint list (e.g. android:tint).

Motivation and Context

Our Android app uses a tint list to change the drawable colors, but GifDrawable doesn't support tint thus it doesn't work well.

The Java doc of `Drawable#setColorFilter(ColorFilter)` says that "non-null
color filter disables tint.".
https://developer.android.com/reference/android/graphics/drawable/Drawable#setColorFilter(android.graphics.ColorFilter)

This change sets the tint color filter by `draw()` to align with the
document. This behavior is the same as the `ColorDrawable`.
@@ -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.

@@ -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


// 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

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

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.

verify(paint).setColorFilter(null);

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.

@sumita12 sumita12 force-pushed the master branch 2 times, most recently from 66a32ed to 6d23472 Compare December 18, 2023 07:20
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.

2 participants