From c38ce36cd6827251fbc5e49b79d4e83dec8d71be Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Tue, 16 Aug 2022 13:42:46 -0700 Subject: [PATCH] Notify RequestCoordinator before Targets/Listeners when loads finish. This allows callers (ideally only Glide's framework) to reason about the state of the load based on the Request object. Prior to this change, the Request object is not updated for loads involving thumbnails or error request builders until after the Target/RequestListener is called. When RequestListeners/Targets check the state of the Request, it still has the previous state that doesn't match the callback the RequestListener/Target just received. PiperOrigin-RevId: 468013881 --- .../com/bumptech/glide/MultiRequestTest.java | 163 ++++++++++++++++++ .../bumptech/glide/request/SingleRequest.java | 10 +- .../glide/request/SingleRequestTest.java | 85 +++++++++ 3 files changed, 254 insertions(+), 4 deletions(-) create mode 100644 instrumentation/src/androidTest/java/com/bumptech/glide/MultiRequestTest.java diff --git a/instrumentation/src/androidTest/java/com/bumptech/glide/MultiRequestTest.java b/instrumentation/src/androidTest/java/com/bumptech/glide/MultiRequestTest.java new file mode 100644 index 0000000000..b04cc220fe --- /dev/null +++ b/instrumentation/src/androidTest/java/com/bumptech/glide/MultiRequestTest.java @@ -0,0 +1,163 @@ +package com.bumptech.glide; + +import static com.google.common.truth.Truth.assertThat; + +import android.content.Context; +import android.graphics.Bitmap; +import android.graphics.Bitmap.CompressFormat; +import android.graphics.Bitmap.Config; +import android.graphics.Canvas; +import android.graphics.Color; +import android.graphics.drawable.Drawable; +import androidx.annotation.NonNull; +import androidx.annotation.Nullable; +import androidx.test.core.app.ApplicationProvider; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import com.bumptech.glide.load.DataSource; +import com.bumptech.glide.load.engine.GlideException; +import com.bumptech.glide.load.engine.executor.GlideExecutor; +import com.bumptech.glide.request.Request; +import com.bumptech.glide.request.RequestListener; +import com.bumptech.glide.request.target.CustomTarget; +import com.bumptech.glide.request.target.Target; +import com.bumptech.glide.request.transition.Transition; +import com.bumptech.glide.test.ModelGeneratorRule; +import com.bumptech.glide.testutil.ConcurrencyHelper; +import com.bumptech.glide.testutil.TearDownGlide; +import java.io.BufferedOutputStream; +import java.io.File; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.OutputStream; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.junit.runner.RunWith; + +@RunWith(AndroidJUnit4.class) +public class MultiRequestTest { + private final Context context = ApplicationProvider.getApplicationContext(); + @Rule public final TearDownGlide tearDownGlide = new TearDownGlide(); + @Rule public final ModelGeneratorRule modelGeneratorRule = new ModelGeneratorRule(); + @Rule public final TemporaryFolder temporaryFolder = new TemporaryFolder(); + private final ConcurrencyHelper concurrency = new ConcurrencyHelper(); + + @Test + public void thumbnail_onResourceReady_forPrimary_isComplete_whenRequestListenerIsCalled() + throws IOException, InterruptedException { + + // Make sure the requests complete in the same order + Glide.init( + context, + new GlideBuilder() + .setSourceExecutor(GlideExecutor.newSourceBuilder().setThreadCount(1).build())); + + AtomicBoolean isPrimaryRequestComplete = new AtomicBoolean(false); + CountDownLatch countDownLatch = new CountDownLatch(1); + + RequestBuilder request = + Glide.with(context) + .load(newImageFile()) + .thumbnail(Glide.with(context).load(newImageFile())) + .listener( + new RequestListener<>() { + @Override + public boolean onLoadFailed( + @Nullable GlideException e, + Object model, + Target target, + boolean isFirstResource) { + return false; + } + + @Override + public boolean onResourceReady( + Drawable resource, + Object model, + Target target, + DataSource dataSource, + boolean isFirstResource) { + isPrimaryRequestComplete.set(target.getRequest().isComplete()); + countDownLatch.countDown(); + return false; + } + }); + concurrency.runOnMainThread(() -> request.into(new DoNothingTarget())); + + assertThat(countDownLatch.await(3, TimeUnit.SECONDS)).isTrue(); + assertThat(isPrimaryRequestComplete.get()).isTrue(); + } + + @Test + public void thumbnail_onLoadFailed_forPrimary_isNotRunningOrComplete_whenRequestListenerIsCalled() + throws IOException, InterruptedException { + + // Make sure the requests complete in the same order + Glide.init( + context, + new GlideBuilder() + .setSourceExecutor(GlideExecutor.newSourceBuilder().setThreadCount(1).build())); + + AtomicBoolean isNeitherRunningNorComplete = new AtomicBoolean(false); + CountDownLatch countDownLatch = new CountDownLatch(1); + + int missingResourceId = 123; + RequestBuilder requestBuilder = + Glide.with(context) + .load(missingResourceId) + .thumbnail(Glide.with(context).load(newImageFile())) + .listener( + new RequestListener<>() { + @Override + public boolean onLoadFailed( + @Nullable GlideException e, + Object model, + Target target, + boolean isFirstResource) { + Request request = target.getRequest(); + isNeitherRunningNorComplete.set(!request.isComplete() && !request.isRunning()); + countDownLatch.countDown(); + return false; + } + + @Override + public boolean onResourceReady( + Drawable resource, + Object model, + Target target, + DataSource dataSource, + boolean isFirstResource) { + return false; + } + }); + concurrency.runOnMainThread(() -> requestBuilder.into(new DoNothingTarget())); + + assertThat(countDownLatch.await(3, TimeUnit.SECONDS)).isTrue(); + assertThat(isNeitherRunningNorComplete.get()).isTrue(); + } + + private File newImageFile() throws IOException { + Bitmap bitmap = Bitmap.createBitmap(100, 100, Config.ARGB_8888); + Canvas canvas = new Canvas(bitmap); + canvas.drawColor(Color.RED); + File result = temporaryFolder.newFile(); + try (OutputStream os = new BufferedOutputStream(new FileOutputStream(result))) { + bitmap.compress(CompressFormat.JPEG, 75, os); + } + return result; + } + + // We don't store or do anything with the resource, so we don't need to do anything to release it + // in onLoadCleared. + private static final class DoNothingTarget extends CustomTarget { + @Override + public void onResourceReady( + @NonNull Drawable resource, @Nullable Transition transition) {} + + @Override + public void onLoadCleared(@Nullable Drawable placeholder) {} + } +} diff --git a/library/src/main/java/com/bumptech/glide/request/SingleRequest.java b/library/src/main/java/com/bumptech/glide/request/SingleRequest.java index e59bd259d7..839a4bd778 100644 --- a/library/src/main/java/com/bumptech/glide/request/SingleRequest.java +++ b/library/src/main/java/com/bumptech/glide/request/SingleRequest.java @@ -523,14 +523,14 @@ private boolean isFirstReadyResource() { } @GuardedBy("requestLock") - private void notifyLoadSuccess() { + private void notifyRequestCoordinatorLoadSucceeded() { if (requestCoordinator != null) { requestCoordinator.onRequestSuccess(this); } } @GuardedBy("requestLock") - private void notifyLoadFailed() { + private void notifyRequestCoordinatorLoadFailed() { if (requestCoordinator != null) { requestCoordinator.onRequestFailed(this); } @@ -639,6 +639,8 @@ private void onResourceReady( + " ms"); } + notifyRequestCoordinatorLoadSucceeded(); + isCallingCallbacks = true; try { boolean anyListenerHandledUpdatingTarget = false; @@ -660,7 +662,6 @@ private void onResourceReady( isCallingCallbacks = false; } - notifyLoadSuccess(); GlideTrace.endSectionAsync(TAG, cookie); } @@ -694,6 +695,8 @@ private void onLoadFailed(GlideException e, int maxLogLevel) { loadStatus = null; status = Status.FAILED; + notifyRequestCoordinatorLoadFailed(); + isCallingCallbacks = true; try { // TODO: what if this is a thumbnail request? @@ -715,7 +718,6 @@ private void onLoadFailed(GlideException e, int maxLogLevel) { isCallingCallbacks = false; } - notifyLoadFailed(); GlideTrace.endSectionAsync(TAG, cookie); } } diff --git a/library/test/src/test/java/com/bumptech/glide/request/SingleRequestTest.java b/library/test/src/test/java/com/bumptech/glide/request/SingleRequestTest.java index 123f38bcb2..16c8169e35 100644 --- a/library/test/src/test/java/com/bumptech/glide/request/SingleRequestTest.java +++ b/library/test/src/test/java/com/bumptech/glide/request/SingleRequestTest.java @@ -33,6 +33,7 @@ import com.bumptech.glide.load.engine.Engine; import com.bumptech.glide.load.engine.GlideException; import com.bumptech.glide.load.engine.Resource; +import com.bumptech.glide.request.target.CustomTarget; import com.bumptech.glide.request.target.SizeReadyCallback; import com.bumptech.glide.request.target.Target; import com.bumptech.glide.request.transition.Transition; @@ -46,6 +47,7 @@ import java.util.List; import java.util.Map; import java.util.concurrent.Executor; +import java.util.concurrent.atomic.AtomicBoolean; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -649,6 +651,89 @@ public void testRequestListenerIsCalledWithFirstImageIfRequestCoordinatorReturns eq(builder.result), any(Number.class), isAListTarget(), isADataSource(), eq(false)); } + @Test + public void onResourceReady_notifiesRequestCoordinator_beforeCallingRequestListeners() { + AtomicBoolean isRequestCoordinatorVerified = new AtomicBoolean(); + SingleRequest request = + builder + .setTarget(new DoNothingTarget()) + .addRequestListener( + new RequestListener<>() { + @Override + public boolean onLoadFailed( + @Nullable GlideException e, + Object model, + Target target, + boolean isFirstResource) { + return false; + } + + @Override + public boolean onResourceReady( + List resource, + Object model, + Target target, + DataSource dataSource, + boolean isFirstResource) { + verify(builder.requestCoordinator).onRequestSuccess(target.getRequest()); + isRequestCoordinatorVerified.set(true); + return false; + } + }) + .build(); + builder.target.setRequest(request); + request.onResourceReady( + builder.resource, DataSource.DATA_DISK_CACHE, /* isLoadedFromAlternateCacheKey= */ false); + + assertThat(isRequestCoordinatorVerified.get()).isTrue(); + } + + @Test + public void onLoadFailed_notifiesRequestCoordinator_beforeCallingRequestListeners() { + AtomicBoolean isRequestCoordinatorVerified = new AtomicBoolean(); + SingleRequest request = + builder + .setTarget(new DoNothingTarget()) + .addRequestListener( + new RequestListener<>() { + @Override + public boolean onLoadFailed( + @Nullable GlideException e, + Object model, + Target target, + boolean isFirstResource) { + verify(builder.requestCoordinator).onRequestFailed(target.getRequest()); + isRequestCoordinatorVerified.set(true); + return false; + } + + @Override + public boolean onResourceReady( + List resource, + Object model, + Target target, + DataSource dataSource, + boolean isFirstResource) { + return false; + } + }) + .build(); + builder.target.setRequest(request); + request.onLoadFailed(new GlideException("test")); + + assertThat(isRequestCoordinatorVerified.get()).isTrue(); + } + + // We don't need to clear a resource since we're not using it to being with. + private static final class DoNothingTarget extends CustomTarget { + @Override + public void onResourceReady( + @NonNull List resource, @Nullable Transition transition) {} + + @Override + public void onLoadCleared(@Nullable Drawable placeholder) {} + } + @Test public void testRequestListenerIsCalledWithNotIsFirstRequestIfRequestCoordinatorParentReturnsResourceSet() {