Deduplicate status and cleared calls in ErrorRequestCoordinator

Previously we'd let both the the primary and the error request notify when the status of a request changes (ie cleared -> running). That would lead to duplicate running calls, once for the primary request, then when it failed, again for the error request.

This change also relaxes the requirements for canSetImage to be either request because we only ever run one at a time. It also simplifies canNotifyCleared to just the primary. For notifyCleared we just need one or the other request to do it, it doesn't particularly matter which one. There's no defined agreement over whether the primary's placehodler will be used, or the error if both have placeholders. Using the primary's always seems more coherent.

library/src/main/java/com/bumptech/glide/request/ErrorRequestCoordinator.java

@@ -102,7 +102,9 @@ public boolean isEquivalentTo(Request o) {
   @Override
   public boolean canSetImage(Request request) {
     synchronized (requestLock) {
-      return parentCanSetImage() && isValidRequest(request);
+      // Only one of primary or error runs at a time, so if we've reached this point and nothing
+      // else is broken, we should have nothing else to enforce.
+      return parentCanSetImage();
     }
   }
 
@@ -114,14 +116,14 @@ private boolean parentCanSetImage() {
   @Override
   public boolean canNotifyStatusChanged(Request request) {
     synchronized (requestLock) {
-      return parentCanNotifyStatusChanged() && isValidRequest(request);
+      return parentCanNotifyStatusChanged() && isValidRequestForStatusChanged(request);
     }
   }
 
   @Override
   public boolean canNotifyCleared(Request request) {
     synchronized (requestLock) {
-      return parentCanNotifyCleared() && isValidRequest(request);
+      return parentCanNotifyCleared() && request.equals(primary);
     }
   }
 
@@ -136,9 +138,17 @@ private boolean parentCanNotifyStatusChanged() {
   }
 
   @GuardedBy("requestLock")
-  private boolean isValidRequest(Request request) {
-    return request.equals(primary)
-        || (primaryState == RequestState.FAILED && request.equals(error));
+  private boolean isValidRequestForStatusChanged(Request request) {
+    if (primaryState != RequestState.FAILED) {
+      return request.equals(primary);
+    } else {
+      return request.equals(error)
+          // We don't want to call onLoadStarted once for the primary request and then again
+          // if it fails and the error request starts. It's already running, so we might as well
+          // avoid the duplicate notification by only notifying about the error state when it's
+          // final.
+          && (errorState == RequestState.SUCCESS || errorState == RequestState.FAILED);
+    }
   }
 
   @Override

library/test/src/test/java/com/bumptech/glide/request/ErrorRequestCoordinatorTest.java

@@ -234,11 +234,6 @@ public void canSetImage_withNotFailedPrimary_andNullParent_returnsTrue() {
     assertThat(coordinator.canSetImage(primary)).isTrue();
   }
 
-  @Test
-  public void canSetImage_withError_andNullParent_andNotFailedPrimary_returnsFalse() {
-    assertThat(coordinator.canSetImage(error)).isFalse();
-  }
-
   @Test
   public void canSetImage_withNotFailedPrimary_parentCanSetImage_returnsTrue() {
     coordinator = newCoordinator(parent);
@@ -309,9 +304,17 @@ public void canNotifyStatusChanged_withError_notFailedPrimary_nullParent_returns
   }
 
   @Test
-  public void canNotifyStatusChanged_withError_failedPrimary_nullParent_returnsTrue() {
+  public void canNotifyStatusChanged_withErrorRequest_failedPrimary_nullParent_errorIsNotFailed_returnsFalse() {
     coordinator.onRequestFailed(primary);
 
+    assertThat(coordinator.canNotifyStatusChanged(error)).isFalse();
+  }
+
+  @Test
+  public void canNotifyStatusChanged_withErrorRequest_failedPrimary_nullParent_failedError_returnsTrue() {
+    coordinator.onRequestFailed(primary);
+    coordinator.onRequestFailed(error);
+
     assertThat(coordinator.canNotifyStatusChanged(error)).isTrue();
   }
 
@@ -325,15 +328,27 @@ public void canNotifyStatusChanged_withError_failedPrimary_nonNullParentCantNoti
   }
 
   @Test
-  public void canNotifyStatusChanged_withError_failedPrimary_nonNullParentCanNotify_returnsTrue() {
+  public void canNotifyStatusChanged_withError_failedPrimary_notFailedError_nonNullParentCanNotify_returnsFalse() {
     coordinator = newCoordinator(parent);
     coordinator.setRequests(primary, error);
     coordinator.onRequestFailed(primary);
     when(parent.canNotifyStatusChanged(coordinator)).thenReturn(true);
 
-    assertThat(coordinator.canNotifyStatusChanged(primary)).isTrue();
+    assertThat(coordinator.canNotifyStatusChanged(error)).isFalse();
+  }
+
+  @Test
+  public void canNotifyStatusChanged_withError_failedPrimary_failedError_nonNullParentCanNotify_returnsTrue() {
+    coordinator = newCoordinator(parent);
+    coordinator.setRequests(primary, error);
+    coordinator.onRequestFailed(primary);
+    when(parent.canNotifyStatusChanged(coordinator)).thenReturn(true);
+    coordinator.onRequestFailed(error);
+
+    assertThat(coordinator.canNotifyStatusChanged(error)).isTrue();
   }
 
+
   @Test
   public void isAnyResourceSet_primaryNotSet_nullParent_returnsFalse() {
     assertThat(coordinator.isAnyResourceSet()).isFalse();
@@ -532,9 +547,19 @@ public void canNotifyCleared_errorRequest_nullParent_returnsFalse() {
   }
 
   @Test
-  public void canNotifyCleared_errorRequest_primaryFailed_nullParent_returnsTrue() {
+  public void canNotifyCleared_errorRequest_primaryFailed_nullParent_returnsFalse() {
     coordinator.onRequestFailed(primary);
-    assertThat(coordinator.canNotifyCleared(error)).isTrue();
+    assertThat(coordinator.canNotifyCleared(error)).isFalse();
+  }
+
+  @Test
+  public void canNotifyCleared_primaryRequest_primaryFailed_nonNullParentCanNotNotify_returnsFalse() {
+    coordinator = newCoordinator(parent);
+    coordinator.setRequests(primary, error);
+    when(parent.canNotifyCleared(coordinator)).thenReturn(false);
+    coordinator.onRequestFailed(primary);
+
+    assertThat(coordinator.canNotifyCleared(primary)).isFalse();
   }
 
   @Test
@@ -548,13 +573,23 @@ public void canNotifyCleared_errorRequest_primaryFailed_nonNullParentCanNotNotif
   }
 
   @Test
-  public void canNotifyCleared_errorRequest_primaryFailed_nonNullParentCanNotify_returnsTrue() {
+  public void canNotifyCleared_errorRequest_primaryFailed_nonNullParentCanNotify_returnsFalse() {
     coordinator = newCoordinator(parent);
     coordinator.setRequests(primary, error);
     when(parent.canNotifyCleared(coordinator)).thenReturn(true);
     coordinator.onRequestFailed(primary);
 
-    assertThat(coordinator.canNotifyCleared(error)).isTrue();
+    assertThat(coordinator.canNotifyCleared(error)).isFalse();
+  }
+
+  @Test
+  public void canNotifyCleared_primaryRequest_primaryFailed_nonNullParentCanNotify_returnsTrue() {
+    coordinator = newCoordinator(parent);
+    coordinator.setRequests(primary, error);
+    when(parent.canNotifyCleared(coordinator)).thenReturn(true);
+    coordinator.onRequestFailed(primary);
+
+    assertThat(coordinator.canNotifyCleared(primary)).isTrue();
   }
 
   private static ErrorRequestCoordinator newCoordinator() {