Re-implement Request#pause() to allow partial clearing of requests.

RequestManager#pauseRequests expects to pause requests that are running without affecting requests that have completed. From a user perspective, pauseRequests should allow grey squares in a photo grid to remain grey, without affecting any images that have managed to successfully load. Without an interface change it's not possible to apply this behavior to thumbnail requests where the thumbnail(s) have completed but the full request has not because clear() applies to all parts of the request, regardless of whether or not they have completed. The pause() method added in this change allows us to clear() an in progress full request while leaving any completed thumbnails in place. As a result the user may see lower resolution thumbnails until RequestManager#resumeRequests is called, but they won't see the lower resolution thumbnails disappear as soon as RequestManager#pauseRequests is called. PiperOrigin-RevId: 260725883
bumptech · Jul 30, 2019 · 2f56153 · 2f56153
1 parent 7910f68
commit 2f56153
Show file tree

Hide file tree

Showing 10 changed files with 289 additions and 11 deletions.
diff --git a/instrumentation/src/androidTest/java/com/bumptech/glide/RequestTest.java b/instrumentation/src/androidTest/java/com/bumptech/glide/RequestTest.java
@@ -157,7 +157,7 @@ public void run() {
 
   /** Tests #2555. */
   @Test
-  public void onStop_withRequestWithOnlyFullInProgress_nullsOutDrawableInView() {
+  public void clear_withRequestWithOnlyFullInProgress_nullsOutDrawableInView() {
     final WaitModel<Integer> mainModel = WaitModelLoader.Factory.waitOn(ResourceIds.raw.canonical);
     concurrency.loadUntilFirstFinish(
         GlideApp.with(context)
@@ -174,7 +174,7 @@ public void onStop_withRequestWithOnlyFullInProgress_nullsOutDrawableInView() {
         new Runnable() {
           @Override
           public void run() {
-            GlideApp.with(context).onStop();
+            GlideApp.with(context).clear(imageView);
           }
         });
 
@@ -196,6 +196,46 @@ public void run() {
     mainModel.countDown();
   }
 
+  @Test
+  public void clear_withRequestWithOnlyFullInProgress_doesNotNullOutDrawableInView() {
+    final WaitModel<Integer> mainModel = WaitModelLoader.Factory.waitOn(ResourceIds.raw.canonical);
+    concurrency.loadUntilFirstFinish(
+        GlideApp.with(context)
+            .load(mainModel)
+            .listener(requestListener)
+            .thumbnail(
+                GlideApp.with(context)
+                    .load(ResourceIds.raw.canonical)
+                    .listener(requestListener)
+                    .override(100, 100)),
+        imageView);
+
+    concurrency.runOnMainThread(
+        new Runnable() {
+          @Override
+          public void run() {
+            GlideApp.with(context).onStop();
+          }
+        });
+
+    verify(requestListener, never())
+        .onResourceReady(
+            anyDrawable(),
+            any(),
+            anyDrawableTarget(),
+            eq(DataSource.DATA_DISK_CACHE),
+            anyBoolean());
+    verify(requestListener, never())
+        .onResourceReady(
+            anyDrawable(),
+            any(),
+            anyDrawableTarget(),
+            eq(DataSource.RESOURCE_DISK_CACHE),
+            anyBoolean());
+    assertThat(imageView.getDrawable()).isNotNull();
+    mainModel.countDown();
+  }
+
   @Test
   public void onStop_withRequestWithOnlyThumbnailInProgress_doesNotNullOutDrawableInView() {
     final WaitModel<Integer> thumbModel = WaitModelLoader.Factory.waitOn(ResourceIds.raw.canonical);

diff --git a/library/src/main/java/com/bumptech/glide/manager/RequestTracker.java b/library/src/main/java/com/bumptech/glide/manager/RequestTracker.java
@@ -93,7 +93,10 @@ public void pauseRequests() {
     isPaused = true;
     for (Request request : Util.getSnapshot(requests)) {
       if (request.isRunning()) {
-        request.clear();
+        // Avoid clearing parts of requests that may have completed (thumbnails) to avoid blinking
+        // in the UI, while still making sure that any in progress parts of requests are immediately
+        // stopped.
+        request.pause();
         pendingRequests.add(request);
       }
     }

diff --git a/library/src/main/java/com/bumptech/glide/request/ErrorRequestCoordinator.java b/library/src/main/java/com/bumptech/glide/request/ErrorRequestCoordinator.java
@@ -38,6 +38,12 @@ public void clear() {
     }
   }
 
+  @Override
+  public void pause() {
+    primary.pause();
+    error.pause();
+  }
+
   @Override
   public boolean isRunning() {
     return primary.isFailed() ? error.isRunning() : primary.isRunning();

diff --git a/library/src/main/java/com/bumptech/glide/request/Request.java b/library/src/main/java/com/bumptech/glide/request/Request.java
@@ -2,7 +2,6 @@
 
 /** A request that loads a resource for an {@link com.bumptech.glide.request.target.Target}. */
 public interface Request {
-
   /** Starts an asynchronous load. */
   void begin();
 
@@ -13,6 +12,17 @@ public interface Request {
    */
   void clear();
 
+  /**
+   * Similar to {@link #clear} for in progress requests (or portions of a request), but does nothing
+   * if the request is already complete.
+   *
+   * <p>Unlike {@link #clear()}, this method allows implementations to act differently on subparts
+   * of a request. For example if a Request has both a thumbnail and a primary request and the
+   * thumbnail portion of the request is complete, this method allows only the primary portion of
+   * the request to be paused without clearing the previously completed thumbnail portion.
+   */
+  void pause();
+
   /** Returns true if this request is running and has not completed or failed. */
   boolean isRunning();
 

diff --git a/library/src/main/java/com/bumptech/glide/request/SingleRequest.java b/library/src/main/java/com/bumptech/glide/request/SingleRequest.java
@@ -325,6 +325,13 @@ public synchronized void clear() {
     status = Status.CLEARED;
   }
 
+  @Override
+  public synchronized void pause() {
+    if (isRunning()) {
+      clear();
+    }
+  }
+
   private void releaseResource(Resource<?> resource) {
     engine.release(resource);
     this.resource = null;

diff --git a/library/src/main/java/com/bumptech/glide/request/ThumbnailRequestCoordinator.java b/library/src/main/java/com/bumptech/glide/request/ThumbnailRequestCoordinator.java
@@ -13,6 +13,7 @@ public class ThumbnailRequestCoordinator implements RequestCoordinator, Request
   private Request full;
   private Request thumb;
   private boolean isRunning;
+  private boolean isPaused;
 
   @VisibleForTesting
   ThumbnailRequestCoordinator() {
@@ -56,7 +57,7 @@ public boolean canNotifyStatusChanged(Request request) {
 
   @Override
   public boolean canNotifyCleared(Request request) {
-    return parentCanNotifyCleared() && request.equals(full);
+    return parentCanNotifyCleared() && request.equals(full) && !isPaused;
   }
 
   private boolean parentCanNotifyCleared() {
@@ -106,6 +107,7 @@ private boolean parentIsAnyResourceSet() {
   /** Starts first the thumb request and then the full request. */
   @Override
   public void begin() {
+    isPaused = false;
     isRunning = true;
     // If the request has completed previously, there's no need to restart both the full and the
     // thumb, we can just restart the full.
@@ -119,11 +121,20 @@ public void begin() {
 
   @Override
   public void clear() {
+    isPaused = false;
     isRunning = false;
     thumb.clear();
     full.clear();
   }
 
+  @Override
+  public void pause() {
+    isPaused = true;
+    isRunning = false;
+    thumb.pause();
+    full.pause();
+  }
+
   /** Returns true if the full request is still running. */
   @Override
   public boolean isRunning() {
@@ -133,7 +144,7 @@ public boolean isRunning() {
   /** Returns true if the full request is complete. */
   @Override
   public boolean isComplete() {
-    return full.isComplete() || thumb.isComplete();
+    return full.isComplete();
   }
 
   @Override

diff --git a/library/test/src/test/java/com/bumptech/glide/manager/RequestTrackerTest.java b/library/test/src/test/java/com/bumptech/glide/manager/RequestTrackerTest.java
@@ -117,18 +117,40 @@ public void pauseRequest_withRunningRequest_pausesRequest() {
 
     tracker.pauseRequests();
 
-    assertThat(request.isCleared()).isTrue();
+    assertThat(request.isPaused()).isTrue();
   }
 
   @Test
-  public void pauseRequests_withCompletedRequest_doesNotClearRequest() {
+  public void pauseRequests_withCompletedRequest_doesNotPauseRequest() {
     FakeRequest request = new FakeRequest();
     tracker.addRequest(request);
 
     request.setIsComplete();
     tracker.pauseRequests();
 
-    assertThat(request.isCleared()).isFalse();
+    assertThat(request.isPaused()).isFalse();
+  }
+
+  @Test
+  public void pauseRequests_withFailedRequest_doesNotPauseRequest() {
+    FakeRequest request = new FakeRequest();
+    tracker.addRequest(request);
+
+    request.setIsFailed();
+    tracker.pauseRequests();
+
+    assertThat(request.isPaused()).isFalse();
+  }
+
+  @Test
+  public void pauseRequests_withClearedRequest_doesNotPauseRequest() {
+    FakeRequest request = new FakeRequest();
+    tracker.addRequest(request);
+
+    request.clear();
+    tracker.pauseRequests();
+
+    assertThat(request.isPaused()).isFalse();
   }
 
   @Test
@@ -351,6 +373,27 @@ public void testReturnsTrueFromIsPausedWhenPaused() {
     assertTrue(tracker.isPaused());
   }
 
+  @Test
+  public void pauseRequests_pausesRunningRequest() {
+    FakeRequest request = new FakeRequest();
+    request.setIsRunning();
+    tracker.addRequest(request);
+    tracker.pauseRequests();
+
+    assertThat(request.isCleared()).isTrue();
+  }
+
+  @Test
+  public void pauseRequest_doesNotPauseCompletedRequest() {
+    FakeRequest request = new FakeRequest();
+    request.setIsComplete();
+    tracker.addRequest(request);
+    tracker.pauseRequests();
+
+    assertThat(request.isComplete()).isTrue();
+    assertThat(request.isCleared()).isFalse();
+  }
+
   @Test
   public void testReturnsFalseFromIsPausedWhenResumed() {
     tracker.resumeRequests();
@@ -381,6 +424,17 @@ public void resumeRequests_afterRequestIsPausedViaPauseAllRequests_resumesReques
   }
 
   private static final class FakeRequest implements Request {
+
+    private boolean isPaused;
+
+    @Override
+    public void pause() {
+      isPaused = true;
+      if (isRunning) {
+        clear();
+      }
+    }
+
     private boolean isRunning;
     private boolean isFailed;
     private boolean isCleared;
@@ -407,6 +461,10 @@ boolean isRecycled() {
       return isRecycled;
     }
 
+    boolean isPaused() {
+      return isPaused;
+    }
+
     @Override
     public void begin() {
       if (isRunning) {

diff --git a/library/test/src/test/java/com/bumptech/glide/request/ErrorRequestCoordinatorTest.java b/library/test/src/test/java/com/bumptech/glide/request/ErrorRequestCoordinatorTest.java
@@ -76,6 +76,72 @@ public void clear_whenErrorIsRunning_clearsError() {
     verify(error).clear();
   }
 
+  @Test
+  public void pause_whenPrimaryIsRunning_pausesPrimary() {
+    when(primary.isRunning()).thenReturn(true);
+    coordinator.pause();
+
+    verify(primary).pause();
+  }
+
+  // Rely on the underlying implementation to ignore the pause call. It's somewhat more efficient
+  // because we don't need an additional lock.
+  @Test
+  public void pause_whenPrimaryIsComplete_doesNotPausePrimary() {
+    when(primary.isComplete()).thenReturn(true);
+    coordinator.pause();
+
+    verify(primary).pause();
+  }
+
+  // Rely on the underlying implementation to ignore the pause call. It's somewhat more efficient
+  // because we don't need an additional lock.
+  @Test
+  public void pause_whenPrimaryIsFailed_pausesPrimary() {
+    when(primary.isFailed()).thenReturn(true);
+    coordinator.pause();
+
+    verify(primary).pause();
+  }
+
+  // Rely on the underlying implementation to ignore the pause call. It's somewhat more efficient
+  // because we don't need an additional lock.
+  @Test
+  public void pause_whenErrorIsNotRunning_pausesError() {
+    when(error.isRunning()).thenReturn(false);
+    coordinator.pause();
+
+    verify(error).pause();
+  }
+
+  // Rely on the underlying implementation to ignore the pause call. It's somewhat more efficient
+  // because we don't need an additional lock.
+  @Test
+  public void pause_whenErrorIsComplete_pausesError() {
+    when(error.isComplete()).thenReturn(true);
+    coordinator.pause();
+
+    verify(error).pause();
+  }
+
+  // Rely on the underlying implementation to ignore the pause call. It's somewhat more efficient
+  // because we don't need an additional lock.
+  @Test
+  public void pause_whenErrorIsFailed_pausesError() {
+    when(error.isFailed()).thenReturn(true);
+    coordinator.pause();
+
+    verify(error).pause();
+  }
+
+  @Test
+  public void pause_whenErrorIsRunning_pausesError() {
+    when(error.isRunning()).thenReturn(true);
+    coordinator.pause();
+
+    verify(error).pause();
+  }
+
   @Test
   public void isRunning_primaryNotFailed_primaryNotRunning_returnsFalse() {
     assertThat(coordinator.isRunning()).isFalse();