Distinguish resource set from request complete in Glide's RequestCoor…

…dinators A request might have it's thumbnail complete, but not it's full. It's not complete, but it has a resource set. If it has a resource set, and a parent's request fails, we shouldn't replace the thumbnail with the error placeholder. Complicating matters is that we have another way in which we want to know if a resource is set. SingleRequest wants to know if its the first request in the set so it can tell its RequestListener. That requires walking up the request coordinator tree and then back down. To solve the first issue, we check the requests explicitly and walk down the tree to see if any has set a resource, regardless of its state. To solve the second issue, we introduce a getRoot() method on RequestCoordinator that can be used by SingleRequest to walk down the entire tree. PiperOrigin-RevId: 272448635
bumptech · Oct 2, 2019 · 690f815 · 690f815
1 parent 5c20f1a
commit 690f815
Show file tree

Hide file tree

Showing 9 changed files with 70 additions and 24 deletions.
diff --git a/library/src/main/java/com/bumptech/glide/request/ErrorRequestCoordinator.java b/library/src/main/java/com/bumptech/glide/request/ErrorRequestCoordinator.java
@@ -144,15 +144,10 @@ private boolean isValidRequest(Request request) {
   @Override
   public boolean isAnyResourceSet() {
     synchronized (requestLock) {
-      return parentIsAnyResourceSet() || isComplete();
+      return primary.isAnyResourceSet() || error.isAnyResourceSet();
     }
   }
 
-  @GuardedBy("requestLock")
-  private boolean parentIsAnyResourceSet() {
-    return parent != null && parent.isAnyResourceSet();
-  }
-
   @Override
   public void onRequestSuccess(Request request) {
     synchronized (requestLock) {
@@ -186,4 +181,11 @@ public void onRequestFailed(Request request) {
       }
     }
   }
+
+  @Override
+  public RequestCoordinator getRoot() {
+    synchronized (requestLock) {
+      return parent != null ? parent.getRoot() : this;
+    }
+  }
 }
diff --git a/library/src/main/java/com/bumptech/glide/request/Request.java b/library/src/main/java/com/bumptech/glide/request/Request.java
@@ -32,6 +32,12 @@ public interface Request {
   /** Returns true if the request has been cleared. */
   boolean isCleared();
 
+  /**
+   * Returns true if a resource is set, even if the request is not yet complete or the primary
+   * request has failed.
+   */
+  boolean isAnyResourceSet();
+
   /**
    * Returns {@code true} if this {@link Request} is equivalent to the given {@link Request} (has
    * all of the same options and sizes).

diff --git a/library/src/main/java/com/bumptech/glide/request/RequestCoordinator.java b/library/src/main/java/com/bumptech/glide/request/RequestCoordinator.java
@@ -44,6 +44,9 @@ public interface RequestCoordinator {
   /** Must be called when a {@link Request} coordinated by this object fails. */
   void onRequestFailed(Request request);
 
+  /** Returns the top most parent {@code RequestCoordinator}. */
+  RequestCoordinator getRoot();
+
   /** A simple state enum to keep track of the states of individual subrequests. */
   enum RequestState {
     RUNNING(false),

diff --git a/library/src/main/java/com/bumptech/glide/request/SingleRequest.java b/library/src/main/java/com/bumptech/glide/request/SingleRequest.java
@@ -357,6 +357,13 @@ public boolean isCleared() {
     }
   }
 
+  @Override
+  public boolean isAnyResourceSet() {
+    synchronized (requestLock) {
+      return status == Status.COMPLETE;
+    }
+  }
+
   @GuardedBy("requestLock")
   private Drawable getErrorDrawable() {
     if (errorDrawable == null) {
@@ -493,7 +500,7 @@ private boolean canNotifyStatusChanged() {
 
   @GuardedBy("requestLock")
   private boolean isFirstReadyResource() {
-    return requestCoordinator == null || !requestCoordinator.isAnyResourceSet();
+    return requestCoordinator == null || !requestCoordinator.getRoot().isAnyResourceSet();
   }
 
   @GuardedBy("requestLock")

diff --git a/library/src/main/java/com/bumptech/glide/request/ThumbnailRequestCoordinator.java b/library/src/main/java/com/bumptech/glide/request/ThumbnailRequestCoordinator.java
@@ -60,7 +60,7 @@ private boolean parentCanSetImage() {
   @Override
   public boolean canNotifyStatusChanged(Request request) {
     synchronized (requestLock) {
-      return parentCanNotifyStatusChanged() && request.equals(full) && !isResourceSet();
+      return parentCanNotifyStatusChanged() && request.equals(full) && !isAnyResourceSet();
     }
   }
 
@@ -84,7 +84,7 @@ private boolean parentCanNotifyStatusChanged() {
   @Override
   public boolean isAnyResourceSet() {
     synchronized (requestLock) {
-      return parentIsAnyResourceSet() || isResourceSet();
+      return thumb.isAnyResourceSet() || full.isAnyResourceSet();
     }
   }
 
@@ -123,9 +123,11 @@ public void onRequestFailed(Request request) {
     }
   }
 
-  @GuardedBy("requestLock")
-  private boolean parentIsAnyResourceSet() {
-    return parent != null && parent.isAnyResourceSet();
+  @Override
+  public RequestCoordinator getRoot() {
+    synchronized (requestLock) {
+      return parent != null ? parent.getRoot() : this;
+    }
   }
 
   /** Starts first the thumb request and then the full request. */
@@ -189,12 +191,6 @@ public boolean isComplete() {
     }
   }
 
-  private boolean isResourceSet() {
-    synchronized (requestLock) {
-      return fullState == RequestState.SUCCESS || thumbState == RequestState.SUCCESS;
-    }
-  }
-
   @Override
   public boolean isCleared() {
     synchronized (requestLock) {

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
@@ -416,6 +416,11 @@ public boolean isCleared() {
       return isCleared;
     }
 
+    @Override
+    public boolean isAnyResourceSet() {
+      return isComplete;
+    }
+
     @Override
     public boolean isEquivalentTo(Request other) {
       throw new UnsupportedOperationException();

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
@@ -341,6 +341,7 @@ public void isAnyResourceSet_primaryNotSet_nullParent_returnsFalse() {
 
   @Test
   public void isAnyResourceSet_primarySet_nullParent_returnsTrue() {
+    when(primary.isAnyResourceSet()).thenReturn(true);
     coordinator.onRequestSuccess(primary);
     assertThat(coordinator.isAnyResourceSet()).isTrue();
   }
@@ -349,6 +350,7 @@ public void isAnyResourceSet_primarySet_nullParent_returnsTrue() {
   public void isAnyResourceSet_primarySet_parentResourceNotSet_returnsTrue() {
     coordinator = newCoordinator(parent);
     coordinator.setRequests(primary, error);
+    when(primary.isAnyResourceSet()).thenReturn(true);
     coordinator.onRequestSuccess(primary);
 
     assertThat(coordinator.isAnyResourceSet()).isTrue();
@@ -358,24 +360,26 @@ public void isAnyResourceSet_primarySet_parentResourceNotSet_returnsTrue() {
   public void isAnyResourceSet_primarySet_parentSet_returnsTrue() {
     coordinator = newCoordinator(parent);
     coordinator.setRequests(primary, error);
+    when(primary.isAnyResourceSet()).thenReturn(true);
     coordinator.onRequestSuccess(primary);
     when(parent.isAnyResourceSet()).thenReturn(true);
 
     assertThat(coordinator.isAnyResourceSet()).isTrue();
   }
 
   @Test
-  public void isAnyResourceSet_parentSet_returnsTrue() {
+  public void isAnyResourceSet_parentSet_returnsFalse() {
     coordinator = newCoordinator(parent);
     coordinator.setRequests(primary, error);
     when(parent.isAnyResourceSet()).thenReturn(true);
 
-    assertThat(coordinator.isAnyResourceSet()).isTrue();
+    assertThat(coordinator.isAnyResourceSet()).isFalse();
   }
 
   @Test
   public void isAnyResourceSet_errorSet_failedPrimary_nullParent_returnsTrue() {
     coordinator.onRequestFailed(primary);
+    when(error.isAnyResourceSet()).thenReturn(true);
     coordinator.onRequestSuccess(error);
     assertThat(coordinator.isAnyResourceSet()).isTrue();
   }
@@ -385,6 +389,7 @@ public void isAnyResourceSet_errorSet_failedPrimary_nonNullParentNotSet_returnsT
     coordinator = newCoordinator(parent);
     coordinator.setRequests(primary, error);
     coordinator.onRequestFailed(primary);
+    when(error.isAnyResourceSet()).thenReturn(true);
     coordinator.onRequestSuccess(error);
 
     assertThat(coordinator.isAnyResourceSet()).isTrue();
@@ -395,6 +400,7 @@ public void isAnyResourceSet_errorSet_nonNullParentSet_returnsTrue() {
     coordinator = newCoordinator(parent);
     coordinator.setRequests(primary, error);
     when(parent.isAnyResourceSet()).thenReturn(true);
+    when(error.isAnyResourceSet()).thenReturn(true);
     coordinator.onRequestSuccess(error);
 
     assertThat(coordinator.isAnyResourceSet()).isTrue();
@@ -409,13 +415,13 @@ public void isAnyResourceSet_primaryNotSet_errorNotSet_nonNullParentNotSet_retur
   }
 
   @Test
-  public void isAnyResourceSet_primaryNotSet_errorNotSet_nonNullParentSet_returnsTrue() {
+  public void isAnyResourceSet_primaryNotSet_errorNotSet_nonNullParentSet_returnsFalse() {
     coordinator = newCoordinator(parent);
     coordinator.setRequests(primary, error);
 
     when(parent.isAnyResourceSet()).thenReturn(true);
 
-    assertThat(coordinator.isAnyResourceSet()).isTrue();
+    assertThat(coordinator.isAnyResourceSet()).isFalse();
   }
 
   @Test

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
@@ -167,6 +167,7 @@ public void clear_doesNotNotifyTarget_ifRequestCoordinatorReturnsFalseForCanClea
   @Test
   public void testResourceIsNotCompleteWhenAskingCoordinatorIfCanSetImage() {
     RequestCoordinator requestCoordinator = mock(RequestCoordinator.class);
+    when(requestCoordinator.getRoot()).thenReturn(requestCoordinator);
     doAnswer(
             new Answer() {
               @Override
@@ -625,6 +626,21 @@ public void testRequestListenerIsCalledWithFirstImageIfRequestCoordinatorReturns
             eq(builder.result), any(Number.class), isAListTarget(), isADataSource(), eq(false));
   }
 
+  @Test
+  public void
+      testRequestListenerIsCalledWithNotIsFirstRequestIfRequestCoordinatorParentReturnsResourceSet() {
+    SingleRequest<List> request = builder.addRequestListener(listener1).build();
+    RequestCoordinator rootRequestCoordinator = mock(RequestCoordinator.class);
+    when(rootRequestCoordinator.isAnyResourceSet()).thenReturn(true);
+    when(builder.requestCoordinator.isAnyResourceSet()).thenReturn(false);
+    when(builder.requestCoordinator.getRoot()).thenReturn(rootRequestCoordinator);
+    request.onResourceReady(builder.resource, DataSource.DATA_DISK_CACHE);
+
+    verify(listener1)
+        .onResourceReady(
+            eq(builder.result), any(Number.class), isAListTarget(), isADataSource(), eq(false));
+  }
+
   @Test
   public void testTargetIsCalledWithAnimationFromFactory() {
     SingleRequest<List> request = builder.build();
@@ -903,6 +919,7 @@ static final class SingleRequestBuilder {
     private final Map<Class<?>, Transformation<?>> transformations = new HashMap<>();
 
     SingleRequestBuilder() {
+      when(requestCoordinator.getRoot()).thenReturn(requestCoordinator);
       when(requestCoordinator.canSetImage(any(Request.class))).thenReturn(true);
       when(requestCoordinator.canNotifyCleared(any(Request.class))).thenReturn(true);
       when(requestCoordinator.canNotifyStatusChanged(any(Request.class))).thenReturn(true);

diff --git a/library/test/src/test/java/com/bumptech/glide/request/ThumbnailRequestCoordinatorTest.java b/library/test/src/test/java/com/bumptech/glide/request/ThumbnailRequestCoordinatorTest.java
@@ -208,12 +208,14 @@ public void testCanNotNotifyStatusChangedIfThumb() {
 
   @Test
   public void canNotNotifyStatusChanged_forFull_whenFullComplete_isFalse() {
+    when(full.isAnyResourceSet()).thenReturn(true);
     coordinator.onRequestSuccess(full);
     assertFalse(coordinator.canNotifyStatusChanged(full));
   }
 
   @Test
   public void canNotNotifyStatusChanged_forFull_whenIfThumbComplete_isFalse() {
+    when(thumb.isAnyResourceSet()).thenReturn(true);
     coordinator.onRequestSuccess(thumb);
     assertFalse(coordinator.canNotifyStatusChanged(full));
   }
@@ -249,24 +251,26 @@ public void isAnyResourceSet_withIncompleteThumbAndFull_isFalse() {
 
   @Test
   public void isAnyResourceSet_withCompleteFull_isTrue() {
+    when(full.isAnyResourceSet()).thenReturn(true);
     coordinator.onRequestSuccess(full);
     assertTrue(coordinator.isAnyResourceSet());
   }
 
   @Test
   public void isAnyResourceSet_withCompleteThumb_isTrue() {
+    when(thumb.isAnyResourceSet()).thenReturn(true);
     coordinator.onRequestSuccess(thumb);
     assertTrue(coordinator.isAnyResourceSet());
   }
 
   @Test
-  public void isAnyResourceSet_withParentResourceSet_isTrue() {
+  public void isAnyResourceSet_withParentResourceSet_isFalse() {
     coordinator = newCoordinator(parent);
     coordinator.setRequests(full, thumb);
 
     when(parent.isAnyResourceSet()).thenReturn(true);
 
-    assertTrue(coordinator.isAnyResourceSet());
+    assertThat(coordinator.isAnyResourceSet()).isFalse();
   }
 
   @Test