From c6a0dc533f50114999c4f70db7e74d8681710ddb Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Tue, 4 Oct 2022 18:01:59 -0700 Subject: [PATCH] Implement equals/hashcode for RequestOptions and TransitionOptions concrete classes. It's a little tricky to get this right because we've defined equals/hashcode in the base classes, but are implementing them in the concrete classes. I think this implementation is actually safe because the base classes are abstract. However making equals transitive requires the weird equals() only implementations in the classes that have no properties but are concrete implementations of the base classes. A safer way to do this would be to move the the equals/hashcode implementation out of the base class and re-implement it in each subclass. However that would require exposing a bunch of internal instance variables to subclasses. Given the existing structure and the public nature of the base classes, this seems like a reasonable compromise. Fixes #4916 --- integration/compose/build.gradle | 1 + .../integration/compose/GlideComposeTest.kt | 47 +++++++ .../glide/GenericTransitionOptions.java | 14 ++ .../com/bumptech/glide/RequestBuilder.java | 35 +++++ .../com/bumptech/glide/TransitionOptions.java | 17 +++ .../bitmap/BitmapTransitionOptions.java | 14 ++ .../drawable/DrawableTransitionOptions.java | 15 +++ .../glide/request/RequestOptions.java | 14 ++ .../bumptech/glide/RequestBuilderTest.java | 122 +++++++++++++++++- .../glide/request/RequestOptionsTest.java | 27 +++- 10 files changed, 300 insertions(+), 6 deletions(-) diff --git a/integration/compose/build.gradle b/integration/compose/build.gradle index a6a92f2397..633bb13f22 100644 --- a/integration/compose/build.gradle +++ b/integration/compose/build.gradle @@ -60,6 +60,7 @@ dependencies { androidTestImplementation "androidx.test.espresso:espresso-core:$ANDROID_X_TEST_ESPRESSO_VERSION" androidTestImplementation "androidx.test.espresso.idling:idling-concurrent:$ANDROID_X_TEST_ESPRESSO_VERSION" androidTestImplementation "androidx.test.ext:junit:$ANDROID_X_TEST_JUNIT_VERSION" + androidTestImplementation "androidx.compose.material:material:$ANDROID_X_COMPOSE_VERSION" } apply from: "${rootProject.projectDir}/scripts/upload.gradle" diff --git a/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideComposeTest.kt b/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideComposeTest.kt index e75dd7da73..3571cfeaee 100644 --- a/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideComposeTest.kt +++ b/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideComposeTest.kt @@ -5,8 +5,13 @@ package com.bumptech.glide.integration.compose import android.content.Context import android.graphics.drawable.BitmapDrawable import android.graphics.drawable.Drawable +import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.size import androidx.compose.runtime.MutableState +import androidx.compose.runtime.mutableStateOf +import androidx.compose.material.Text +import androidx.compose.material.TextButton +import androidx.compose.runtime.remember import androidx.compose.ui.Modifier import androidx.compose.ui.platform.LocalDensity import androidx.compose.ui.semantics.SemanticsPropertyKey @@ -14,6 +19,8 @@ import androidx.compose.ui.test.SemanticsMatcher import androidx.compose.ui.test.assert import androidx.compose.ui.test.junit4.createComposeRule import androidx.compose.ui.test.onNodeWithContentDescription +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick import androidx.compose.ui.unit.dp import androidx.test.core.app.ApplicationProvider import com.bumptech.glide.Glide @@ -21,6 +28,7 @@ import com.bumptech.glide.integration.ktx.InternalGlideApi import com.bumptech.glide.integration.ktx.Size import com.bumptech.glide.load.engine.executor.GlideIdlingResourceInit import java.util.concurrent.atomic.AtomicReference +import org.junit.After import org.junit.Before import org.junit.Rule import org.junit.Test @@ -34,6 +42,11 @@ class GlideComposeTest { GlideIdlingResourceInit.initGlide(composeRule) } + @After + fun tearDown() { + Glide.tearDown() + } + @Test fun glideImage_noModifierSize_resourceDrawable_displaysDrawable() { val description = "test" @@ -68,6 +81,40 @@ class GlideComposeTest { .assert(expectDisplayedDrawableSize(expectedSize)) } + @Test + fun glideImage_withChangingModel_refreshes() { + val description = "test" + + val firstDrawable: Drawable = context.getDrawable(android.R.drawable.star_big_off)!! + val secondDrawable: Drawable = context.getDrawable(android.R.drawable.star_big_on)!! + + composeRule.setContent { + val model = remember { mutableStateOf(firstDrawable) } + + fun swapModel() { + model.value = secondDrawable + } + + Column { + TextButton(onClick = ::swapModel) {Text(text="Swap")} + GlideImage( + model = model.value, + modifier = Modifier.size(100.dp), + contentDescription = description + ) + } + } + + composeRule.waitForIdle() + composeRule.onNodeWithText("Swap").performClick() + composeRule.waitForIdle() + + val fullsizeBitmap = (secondDrawable as BitmapDrawable).bitmap + composeRule + .onNodeWithContentDescription(description) + .assert(expectDisplayedDrawable(fullsizeBitmap) { (it as BitmapDrawable).bitmap }) + } + @Test fun glideImage_withSizeLargerThanImage_upscaleTransformSet_upscalesImage() { val viewDimension = 300 diff --git a/library/src/main/java/com/bumptech/glide/GenericTransitionOptions.java b/library/src/main/java/com/bumptech/glide/GenericTransitionOptions.java index ee9c684925..ca55aa2a6e 100644 --- a/library/src/main/java/com/bumptech/glide/GenericTransitionOptions.java +++ b/library/src/main/java/com/bumptech/glide/GenericTransitionOptions.java @@ -55,4 +55,18 @@ public static GenericTransitionOptions with( @NonNull TransitionFactory transitionFactory) { return new GenericTransitionOptions().transition(transitionFactory); } + + // Make sure that we're not equal to any other concrete implementation of TransitionOptions. + @Override + public boolean equals(Object o) { + return o instanceof GenericTransitionOptions && super.equals(o); + } + + // Our class doesn't include any additional properties, so we don't need to modify hashcode, but + // keep it here as a reminder in case we add properties. + @SuppressWarnings("PMD.UselessOverridingMethod") + @Override + public int hashCode() { + return super.hashCode(); + } } diff --git a/library/src/main/java/com/bumptech/glide/RequestBuilder.java b/library/src/main/java/com/bumptech/glide/RequestBuilder.java index 9341ad790e..69ecb3e32d 100644 --- a/library/src/main/java/com/bumptech/glide/RequestBuilder.java +++ b/library/src/main/java/com/bumptech/glide/RequestBuilder.java @@ -42,6 +42,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.Objects; import java.util.concurrent.Executor; /** @@ -80,6 +81,7 @@ public class RequestBuilder extends BaseRequestOptions) { + RequestBuilder that = (RequestBuilder) o; + return super.equals(that) + && Objects.equals(transcodeClass, that.transcodeClass) + && transitionOptions.equals(that.transitionOptions) + && Objects.equals(model, that.model) + && Objects.equals(requestListeners, that.requestListeners) + && Objects.equals(thumbnailBuilder, that.thumbnailBuilder) + && Objects.equals(errorBuilder, that.errorBuilder) + && Objects.equals(thumbSizeMultiplier, that.thumbSizeMultiplier) + && isDefaultTransitionOptionsSet == that.isDefaultTransitionOptionsSet + && isModelSet == that.isModelSet; + } + return false; + } + + @Override + public int hashCode() { + int hashCode = super.hashCode(); + hashCode = Util.hashCode(transcodeClass, hashCode); + hashCode = Util.hashCode(transitionOptions, hashCode); + hashCode = Util.hashCode(model, hashCode); + hashCode = Util.hashCode(requestListeners, hashCode); + hashCode = Util.hashCode(thumbnailBuilder, hashCode); + hashCode = Util.hashCode(errorBuilder, hashCode); + hashCode = Util.hashCode(thumbSizeMultiplier, hashCode); + hashCode = Util.hashCode(isDefaultTransitionOptionsSet, hashCode); + hashCode = Util.hashCode(isModelSet, hashCode); + return hashCode; + } } diff --git a/library/src/main/java/com/bumptech/glide/TransitionOptions.java b/library/src/main/java/com/bumptech/glide/TransitionOptions.java index 1a8d116d63..c8f4626609 100644 --- a/library/src/main/java/com/bumptech/glide/TransitionOptions.java +++ b/library/src/main/java/com/bumptech/glide/TransitionOptions.java @@ -7,10 +7,13 @@ import com.bumptech.glide.request.transition.ViewPropertyAnimationFactory; import com.bumptech.glide.request.transition.ViewPropertyTransition; import com.bumptech.glide.util.Preconditions; +import com.bumptech.glide.util.Util; /** * A base class for setting a transition to use on a resource when a load completes. * + *

Note: Implementations must implement equals/hashcode. + * * @param The implementation of this class to return to chain methods. * @param The type of resource that will be animated. */ @@ -97,4 +100,18 @@ final TransitionFactory getTransitionFactory() { private CHILD self() { return (CHILD) this; } + + @Override + public boolean equals(Object o) { + if (o instanceof TransitionOptions) { + TransitionOptions other = (TransitionOptions) o; + return Util.bothNullOrEqual(transitionFactory, other.transitionFactory); + } + return false; + } + + @Override + public int hashCode() { + return transitionFactory != null ? transitionFactory.hashCode() : 0; + } } diff --git a/library/src/main/java/com/bumptech/glide/load/resource/bitmap/BitmapTransitionOptions.java b/library/src/main/java/com/bumptech/glide/load/resource/bitmap/BitmapTransitionOptions.java index 1a2a3a7b2b..9ff5381c5d 100644 --- a/library/src/main/java/com/bumptech/glide/load/resource/bitmap/BitmapTransitionOptions.java +++ b/library/src/main/java/com/bumptech/glide/load/resource/bitmap/BitmapTransitionOptions.java @@ -125,4 +125,18 @@ public BitmapTransitionOptions transitionUsing( public BitmapTransitionOptions crossFade(@NonNull DrawableCrossFadeFactory.Builder builder) { return transitionUsing(builder.build()); } + + // Make sure that we're not equal to any other concrete implementation of TransitionOptions. + @Override + public boolean equals(Object o) { + return o instanceof BitmapTransitionOptions && super.equals(o); + } + + // Our class doesn't include any additional properties, so we don't need to modify hashcode, but + // keep it here as a reminder in case we add properties. + @SuppressWarnings("PMD.UselessOverridingMethod") + @Override + public int hashCode() { + return super.hashCode(); + } } diff --git a/library/src/main/java/com/bumptech/glide/load/resource/drawable/DrawableTransitionOptions.java b/library/src/main/java/com/bumptech/glide/load/resource/drawable/DrawableTransitionOptions.java index d05077a834..5922e2af8a 100644 --- a/library/src/main/java/com/bumptech/glide/load/resource/drawable/DrawableTransitionOptions.java +++ b/library/src/main/java/com/bumptech/glide/load/resource/drawable/DrawableTransitionOptions.java @@ -105,4 +105,19 @@ public DrawableTransitionOptions crossFade( public DrawableTransitionOptions crossFade(@NonNull DrawableCrossFadeFactory.Builder builder) { return crossFade(builder.build()); } + + // Make sure that we're not equal to any other concrete implementation of TransitionOptions. + @Override + public boolean equals(Object o) { + return o instanceof DrawableTransitionOptions && super.equals(o); + } + + // Our class doesn't include any additional properties, so we don't need to modify hashcode, but + // keep it here as a reminder in case we add properties. + @SuppressWarnings("PMD.UselessOverridingMethod") + @Override + public int hashCode() { + return super.hashCode(); + } } + diff --git a/library/src/main/java/com/bumptech/glide/request/RequestOptions.java b/library/src/main/java/com/bumptech/glide/request/RequestOptions.java index c2a1882a83..22dffcf3b3 100644 --- a/library/src/main/java/com/bumptech/glide/request/RequestOptions.java +++ b/library/src/main/java/com/bumptech/glide/request/RequestOptions.java @@ -279,4 +279,18 @@ public static RequestOptions noAnimation() { } return noAnimationOptions; } + + // Make sure that we're not equal to any other concrete implementation of RequestOptions. + @Override + public boolean equals(Object o) { + return o instanceof RequestOptions && super.equals(o); + } + + // Our class doesn't include any additional properties, so we don't need to modify hashcode, but + // keep it here as a reminder in case we add properties. + @SuppressWarnings("PMD.UselessOverridingMethod") + @Override + public int hashCode() { + return super.hashCode(); + } } diff --git a/library/test/src/test/java/com/bumptech/glide/RequestBuilderTest.java b/library/test/src/test/java/com/bumptech/glide/RequestBuilderTest.java index f8dfb9d9a9..6cb719852a 100644 --- a/library/test/src/test/java/com/bumptech/glide/RequestBuilderTest.java +++ b/library/test/src/test/java/com/bumptech/glide/RequestBuilderTest.java @@ -11,9 +11,12 @@ import static org.mockito.Mockito.when; import android.app.Application; +import android.net.Uri; import android.widget.ImageView; +import androidx.annotation.Nullable; import androidx.test.core.app.ApplicationProvider; import com.bumptech.glide.load.DataSource; +import com.bumptech.glide.load.engine.GlideException; import com.bumptech.glide.load.resource.SimpleResource; import com.bumptech.glide.request.Request; import com.bumptech.glide.request.RequestListener; @@ -23,6 +26,7 @@ import com.bumptech.glide.request.target.ViewTarget; import com.bumptech.glide.tests.BackgroundUtil.BackgroundTester; import com.bumptech.glide.tests.TearDownGlide; +import com.google.common.testing.EqualsTester; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -159,13 +163,129 @@ public void testListenerApiOverridesListeners() { .onResourceReady(any(), any(), isA(Target.class), isA(DataSource.class), anyBoolean()); } + @Test + public void testEquals() { + Object firstModel = new Object(); + Object secondModel = new Object(); + + RequestListener firstListener = new RequestListener<>() { + @Override + public boolean onLoadFailed(@Nullable GlideException e, Object model, Target target, + boolean isFirstResource) { + return false; + } + + @Override + public boolean onResourceReady(Object resource, Object model, Target target, + DataSource dataSource, boolean isFirstResource) { + return false; + } + }; + RequestListener secondListener = new RequestListener<>() { + @Override + public boolean onLoadFailed(@Nullable GlideException e, Object model, Target target, + boolean isFirstResource) { + return false; + } + + @Override + public boolean onResourceReady(Object resource, Object model, Target target, + DataSource dataSource, boolean isFirstResource) { + return false; + } + }; + + new EqualsTester() + .addEqualityGroup(new Object()) + .addEqualityGroup( + newRequestBuilder(Object.class), + newRequestBuilder(Object.class) + ) + .addEqualityGroup( + newRequestBuilder(Object.class).load((Object) null), + newRequestBuilder(Object.class).load((Object) null), + newRequestBuilder(Object.class).load((Uri) null) + ) + .addEqualityGroup( + newRequestBuilder(Object.class).load(firstModel), + newRequestBuilder(Object.class).load(firstModel) + ) + .addEqualityGroup( + newRequestBuilder(Object.class).load(secondModel), + newRequestBuilder(Object.class).load(secondModel) + ) + .addEqualityGroup( + newRequestBuilder(Object.class).load(Uri.EMPTY), + newRequestBuilder(Object.class).load(Uri.EMPTY) + ) + .addEqualityGroup( + newRequestBuilder(Uri.class).load(Uri.EMPTY), + newRequestBuilder(Uri.class).load(Uri.EMPTY) + ) + .addEqualityGroup( + newRequestBuilder(Object.class).centerCrop(), + newRequestBuilder(Object.class).centerCrop() + ) + .addEqualityGroup( + newRequestBuilder(Object.class).addListener(firstListener), + newRequestBuilder(Object.class).addListener(firstListener) + ) + .addEqualityGroup( + newRequestBuilder(Object.class).addListener(secondListener), + newRequestBuilder(Object.class).addListener(secondListener) + ) + .addEqualityGroup( + newRequestBuilder(Object.class).error(newRequestBuilder(Object.class)), + newRequestBuilder(Object.class).error(newRequestBuilder(Object.class)) + ) + .addEqualityGroup( + newRequestBuilder(Object.class).error(firstModel), + newRequestBuilder(Object.class).error(firstModel), + newRequestBuilder(Object.class).error(newRequestBuilder(Object.class).load(firstModel)) + ) + .addEqualityGroup( + newRequestBuilder(Object.class).error(secondModel), + newRequestBuilder(Object.class).error(secondModel), + newRequestBuilder(Object.class).error(newRequestBuilder(Object.class).load(secondModel)) + ) + .addEqualityGroup( + newRequestBuilder(Object.class) + .error(newRequestBuilder(Object.class).load(firstModel).centerCrop()), + newRequestBuilder(Object.class) + .error(newRequestBuilder(Object.class).load(firstModel).centerCrop()) + ) + .addEqualityGroup( + newRequestBuilder(Object.class) + .thumbnail(newRequestBuilder(Object.class).load(firstModel)), + newRequestBuilder(Object.class) + .thumbnail(newRequestBuilder(Object.class).load(firstModel)) + ) + .addEqualityGroup( + newRequestBuilder(Object.class) + .thumbnail(newRequestBuilder(Object.class).load(secondModel)), + newRequestBuilder(Object.class) + .thumbnail(newRequestBuilder(Object.class).load(secondModel)) + ) + .addEqualityGroup( + newRequestBuilder(Object.class) + .transition(new GenericTransitionOptions<>().dontTransition()), + newRequestBuilder(Object.class) + .transition(new GenericTransitionOptions<>().dontTransition()) + ) + .testEquals(); + } + private RequestBuilder getNullModelRequest() { + return newRequestBuilder(Object.class).load((Object) null); + } + + private RequestBuilder newRequestBuilder(Class modelClass) { when(glideContext.buildImageViewTarget(isA(ImageView.class), isA(Class.class))) .thenReturn(mock(ViewTarget.class)); when(glideContext.getDefaultRequestOptions()).thenReturn(new RequestOptions()); when(requestManager.getDefaultRequestOptions()).thenReturn(new RequestOptions()); when(requestManager.getDefaultTransitionOptions(any(Class.class))) .thenReturn(new GenericTransitionOptions<>()); - return new RequestBuilder<>(glide, requestManager, Object.class, context).load((Object) null); + return new RequestBuilder<>(glide, requestManager, modelClass, context); } } diff --git a/library/test/src/test/java/com/bumptech/glide/request/RequestOptionsTest.java b/library/test/src/test/java/com/bumptech/glide/request/RequestOptionsTest.java index 3eed10c228..8236ad05ed 100644 --- a/library/test/src/test/java/com/bumptech/glide/request/RequestOptionsTest.java +++ b/library/test/src/test/java/com/bumptech/glide/request/RequestOptionsTest.java @@ -559,7 +559,29 @@ public void testEqualsHashCode() { Drawable second = new GradientDrawable(); assertThat(first).isNotEqualTo(second); assertThat(Util.bothNullOrEqual(first, second)).isFalse(); + // Make sure we're not equal to any other subclass of RequestOptions. + class FakeOptions extends BaseRequestOptions{ + @Override + public boolean equals(Object o) { + return o instanceof FakeOptions && super.equals(o); + } + + // Our class doesn't include any additional properties, so we don't need to modify hashcode, but + // keep it here as a reminder in case we add properties. + @SuppressWarnings("PMD.UselessOverridingMethod") + @Override + public int hashCode() { + return super.hashCode(); + } + } new EqualsTester() + .addEqualityGroup( + new RequestOptions(), + new RequestOptions(), + new RequestOptions().skipMemoryCache(false), + new RequestOptions().onlyRetrieveFromCache(false), + new RequestOptions().useUnlimitedSourceGeneratorsPool(false)) + .addEqualityGroup(new FakeOptions(), new FakeOptions()) .addEqualityGroup( new RequestOptions().sizeMultiplier(.7f), new RequestOptions().sizeMultiplier(.7f)) .addEqualityGroup(new RequestOptions().sizeMultiplier(0.8f)) @@ -579,11 +601,6 @@ public void testEqualsHashCode() { .addEqualityGroup(new RequestOptions().fallback(second)) .addEqualityGroup( new RequestOptions().skipMemoryCache(true), new RequestOptions().skipMemoryCache(true)) - .addEqualityGroup( - new RequestOptions(), - new RequestOptions().skipMemoryCache(false), - new RequestOptions().onlyRetrieveFromCache(false), - new RequestOptions().useUnlimitedSourceGeneratorsPool(false)) .addEqualityGroup( new RequestOptions().override(100), new RequestOptions().override(100, 100)) .addEqualityGroup(