Skip to content

Commit

Permalink
Implement equals/hashcode for RequestOptions and TransitionOptions co…
Browse files Browse the repository at this point in the history
…ncrete 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
  • Loading branch information
sjudd committed Oct 7, 2022
1 parent 4152c04 commit f3d6ff7
Show file tree
Hide file tree
Showing 10 changed files with 300 additions and 6 deletions.
1 change: 1 addition & 0 deletions integration/compose/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,30 @@ 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
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
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
Expand All @@ -34,6 +42,11 @@ class GlideComposeTest {
GlideIdlingResourceInit.initGlide(composeRule)
}

@After
fun tearDown() {
Glide.tearDown()
}

@Test
fun glideImage_noModifierSize_resourceDrawable_displaysDrawable() {
val description = "test"
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,18 @@ public static <TranscodeType> GenericTransitionOptions<TranscodeType> with(
@NonNull TransitionFactory<? super TranscodeType> transitionFactory) {
return new GenericTransitionOptions<TranscodeType>().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();
}
}
35 changes: 35 additions & 0 deletions library/src/main/java/com/bumptech/glide/RequestBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -80,6 +81,7 @@ public class RequestBuilder<TranscodeType> extends BaseRequestOptions<RequestBui
@Nullable private Float thumbSizeMultiplier;
private boolean isDefaultTransitionOptionsSet = true;
private boolean isModelSet;

private boolean isThumbnailBuilt;

// We only override the method to change the return type, not the functionality.
Expand Down Expand Up @@ -1251,4 +1253,37 @@ private Request obtainRequest(
transitionOptions.getTransitionFactory(),
callbackExecutor);
}

@Override
public boolean equals(Object o) {
if (o instanceof RequestBuilder<?>) {
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;
}
}
17 changes: 17 additions & 0 deletions library/src/main/java/com/bumptech/glide/TransitionOptions.java
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
* <p>Note: Implementations must implement equals/hashcode.
*
* @param <CHILD> The implementation of this class to return to chain methods.
* @param <TranscodeType> The type of resource that will be animated.
*/
Expand Down Expand Up @@ -97,4 +100,18 @@ final TransitionFactory<? super TranscodeType> 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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
Loading

0 comments on commit f3d6ff7

Please sign in to comment.