From 6d23472c835c6e5ad234fbcb8cc5db5684338a14 Mon Sep 17 00:00:00 2001 From: Hiroshi Sumita Date: Mon, 4 Dec 2023 12:19:04 +0900 Subject: [PATCH 1/2] Support tint list on GifDrawable. 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`. --- .../glide/load/resource/gif/GifDrawable.java | 61 ++++++++++++++++++- .../load/resource/gif/GifDrawableTest.java | 54 ++++++++++++++-- 2 files changed, 107 insertions(+), 8 deletions(-) diff --git a/library/src/main/java/com/bumptech/glide/load/resource/gif/GifDrawable.java b/library/src/main/java/com/bumptech/glide/load/resource/gif/GifDrawable.java index d62a48437c..eec7369bf3 100644 --- a/library/src/main/java/com/bumptech/glide/load/resource/gif/GifDrawable.java +++ b/library/src/main/java/com/bumptech/glide/load/resource/gif/GifDrawable.java @@ -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; @@ -66,6 +70,9 @@ public class GifDrawable extends Drawable private boolean applyGravity; private Paint paint; + private ColorStateList tint; + private PorterDuff.Mode tintMode; + private ColorFilter tintFilter; private Rect destRect; /** Callbacks to notify loop completion of a gif, where the loop count is explicitly specified. */ @@ -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 @@ -298,7 +315,47 @@ public void setAlpha(int i) { @Override public void setColorFilter(ColorFilter colorFilter) { - getPaint().setColorFilter(colorFilter); + if (getColorFilter() != colorFilter) { + 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() { diff --git a/library/test/src/test/java/com/bumptech/glide/load/resource/gif/GifDrawableTest.java b/library/test/src/test/java/com/bumptech/glide/load/resource/gif/GifDrawableTest.java index 015cc786e3..a286fc59e5 100644 --- a/library/test/src/test/java/com/bumptech/glide/load/resource/gif/GifDrawableTest.java +++ b/library/test/src/test/java/com/bumptech/glide/load/resource/gif/GifDrawableTest.java @@ -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; @@ -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; @@ -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; @@ -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); + + assertThat(drawable.setState(new int[] {android.R.attr.state_pressed})).isTrue(); + drawable.draw(new Canvas()); + + // 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 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 From 8ef3767b5f65b613b862ca71714ac0cfda7ca519 Mon Sep 17 00:00:00 2001 From: Hiroshi Sumita Date: Mon, 18 Dec 2023 16:23:52 +0900 Subject: [PATCH 2/2] Address review comments. --- .../glide/load/resource/gif/GifDrawable.java | 31 +++--- .../load/resource/gif/GifDrawableTest.java | 102 ++++++++++++++---- 2 files changed, 96 insertions(+), 37 deletions(-) diff --git a/library/src/main/java/com/bumptech/glide/load/resource/gif/GifDrawable.java b/library/src/main/java/com/bumptech/glide/load/resource/gif/GifDrawable.java index eec7369bf3..8eab773a90 100644 --- a/library/src/main/java/com/bumptech/glide/load/resource/gif/GifDrawable.java +++ b/library/src/main/java/com/bumptech/glide/load/resource/gif/GifDrawable.java @@ -49,6 +49,8 @@ public class GifDrawable extends Drawable private static final int GRAVITY = Gravity.FILL; + private static final PorterDuff.Mode DEFAULT_TINT_MODE = PorterDuff.Mode.SRC_IN; + private final GifState state; /** True if the drawable is currently animating. */ private boolean isRunning; @@ -70,8 +72,9 @@ public class GifDrawable extends Drawable private boolean applyGravity; private Paint paint; + private ColorFilter colorFilter; private ColorStateList tint; - private PorterDuff.Mode tintMode; + private PorterDuff.Mode tintMode = DEFAULT_TINT_MODE; private ColorFilter tintFilter; private Rect destRect; @@ -296,16 +299,15 @@ public void draw(@NonNull Canvas canvas) { Bitmap currentFrame = state.frameLoader.getCurrentFrame(); Paint paint = getPaint(); - ColorFilter colorFilter = paint.getColorFilter(); - if (colorFilter != null || tintFilter == null) { + if (colorFilter != 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); + } else if (tintFilter != null) { + paint.setColorFilter(tintFilter); + } else { + paint.setColorFilter(null); } + canvas.drawBitmap(currentFrame, null, getDestRect(), paint); } @Override @@ -315,10 +317,8 @@ public void setAlpha(int i) { @Override public void setColorFilter(ColorFilter colorFilter) { - if (getColorFilter() != colorFilter) { - getPaint().setColorFilter(colorFilter); - invalidateSelf(); - } + this.colorFilter = colorFilter; + invalidateSelf(); } @Override @@ -342,7 +342,7 @@ public void setTintMode(PorterDuff.Mode tintMode) { @Override protected boolean onStateChange(int[] stateSet) { - if (tint != null && tintMode != null) { + if (tint != null) { updateTintFilter(); return true; } @@ -350,9 +350,10 @@ protected boolean onStateChange(int[] stateSet) { } private void updateTintFilter() { - if (tint != null && tintMode != null) { + if (tint != null) { int color = tint.getColorForState(getState(), Color.TRANSPARENT); - tintFilter = new PorterDuffColorFilter(color, tintMode); + PorterDuff.Mode mode = tintMode != null ? tintMode : DEFAULT_TINT_MODE; + tintFilter = new PorterDuffColorFilter(color, mode); } else { tintFilter = null; } diff --git a/library/test/src/test/java/com/bumptech/glide/load/resource/gif/GifDrawableTest.java b/library/test/src/test/java/com/bumptech/glide/load/resource/gif/GifDrawableTest.java index a286fc59e5..31058ca967 100644 --- a/library/test/src/test/java/com/bumptech/glide/load/resource/gif/GifDrawableTest.java +++ b/library/test/src/test/java/com/bumptech/glide/load/resource/gif/GifDrawableTest.java @@ -6,7 +6,6 @@ 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; @@ -42,6 +41,7 @@ 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; @@ -567,55 +567,113 @@ public void testSetAlphaSetsAlphaOnPaint() { @Test public void testSetColorFilterSetsColorFilterOnPaint() { + // Use a real Paint object, as this test depends on Paint#(get|set)ColorFilter. + drawable = new GifDrawable(frameLoader, new Paint()); ColorFilter colorFilter = new PorterDuffColorFilter(Color.RED, Mode.ADD); drawable.setColorFilter(colorFilter); - verify(paint).setColorFilter(eq(colorFilter)); + Canvas canvas = mock(Canvas.class); + drawable.draw(canvas); + + ArgumentCaptor captor = ArgumentCaptor.forClass(Paint.class); + verify(canvas).drawBitmap(isA(Bitmap.class), isNull(), isA(Rect.class), captor.capture()); + assertThat(captor.getValue().getColorFilter()).isEqualTo(colorFilter); } @Config(sdk = Build.VERSION_CODES.LOLLIPOP) @Test - public void testDrawSetsTintListColorFilterOnPaint() { + public void testSetTintListSetsColorFilterOnPaint() { + // Use a real Paint object, as this test depends on Paint#(get|set)ColorFilter. + Paint paint = new Paint(); + drawable = new GifDrawable(frameLoader, paint); 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()); + Canvas canvas = mock(Canvas.class); + drawable.draw(canvas); - // draw() temporary sets tint filter then restore. - verify(paint).setColorFilter(eq(new PorterDuffColorFilter(Color.GREEN, Mode.ADD))); - verify(paint).setColorFilter(null); + ArgumentCaptor captor = ArgumentCaptor.forClass(Paint.class); + verify(canvas).drawBitmap(isA(Bitmap.class), isNull(), isA(Rect.class), captor.capture()); + assertThat(captor.getValue().getColorFilter()).isEqualTo(new PorterDuffColorFilter(Color.GREEN, Mode.SRC_IN)); + } + @Config(sdk = Build.VERSION_CODES.LOLLIPOP) + @Test + public void testSetTintListSetsColorFilterForPressedStateOnPaint() { + // Use a real Paint object, as this test depends on Paint#(get|set)ColorFilter. + drawable = new GifDrawable(frameLoader, new Paint()); + 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); assertThat(drawable.setState(new int[] {android.R.attr.state_pressed})).isTrue(); - drawable.draw(new Canvas()); + Canvas canvas = mock(Canvas.class); + drawable.draw(canvas); - // 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); + ArgumentCaptor captor = ArgumentCaptor.forClass(Paint.class); + verify(canvas).drawBitmap(isA(Bitmap.class), isNull(), isA(Rect.class), captor.capture()); + assertThat(captor.getValue().getColorFilter()).isEqualTo(new PorterDuffColorFilter(Color.RED, Mode.SRC_IN)); } @Config(sdk = Build.VERSION_CODES.LOLLIPOP) @Test - public void testDrawUsesColorFilterInsteadOfTintList() { + public void testSetTintModeSetsColorFilterOnPaint() { + // Use a real Paint object, as this test depends on Paint#(get|set)ColorFilter. + drawable = new GifDrawable(frameLoader, new Paint()); 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); + Canvas canvas = mock(Canvas.class); + drawable.draw(canvas); + + ArgumentCaptor captor = ArgumentCaptor.forClass(Paint.class); + verify(canvas).drawBitmap(isA(Bitmap.class), isNull(), isA(Rect.class), captor.capture()); + assertThat(captor.getValue().getColorFilter()).isEqualTo(new PorterDuffColorFilter(Color.GREEN, Mode.ADD)); + } + + @Config(sdk = Build.VERSION_CODES.LOLLIPOP) + @Test + public void testNullTintModeFallsBackToDefaultTintMode() { + // Use a real Paint object, as this test depends on Paint#(get|set)ColorFilter. + drawable = new GifDrawable(frameLoader, new Paint()); + 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(null); + Canvas canvas = mock(Canvas.class); + drawable.draw(canvas); + + ArgumentCaptor captor = ArgumentCaptor.forClass(Paint.class); + verify(canvas).drawBitmap(isA(Bitmap.class), isNull(), isA(Rect.class), captor.capture()); + assertThat(captor.getValue().getColorFilter()).isEqualTo(new PorterDuffColorFilter(Color.GREEN, Mode.SRC_IN)); + } + + @Config(sdk = Build.VERSION_CODES.LOLLIPOP) + @Test + public void testSetColorFilterIsPrioritizedThanSetTintList() { + // Use a real Paint object, as this test depends on Paint#(get|set)ColorFilter. + drawable = new GifDrawable(frameLoader, new Paint()); 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()); + 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); + Canvas canvas = mock(Canvas.class); + drawable.draw(canvas); - // ColorFilter disables tint list, so draw() should not invoke setColorFilter() any more. - verify(paint).setColorFilter(any()); + ArgumentCaptor captor = ArgumentCaptor.forClass(Paint.class); + verify(canvas).drawBitmap(isA(Bitmap.class), isNull(), isA(Rect.class), captor.capture()); + assertThat(captor.getValue().getColorFilter()).isEqualTo(colorFilter); } @Test