diff --git a/library/src/main/java/com/bumptech/glide/request/ErrorRequestCoordinator.java b/library/src/main/java/com/bumptech/glide/request/ErrorRequestCoordinator.java index 4c112357f4..851acf2cdf 100644 --- a/library/src/main/java/com/bumptech/glide/request/ErrorRequestCoordinator.java +++ b/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 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 index fc0643b6aa..79eb10b5cf 100644 --- a/library/test/src/test/java/com/bumptech/glide/request/ErrorRequestCoordinatorTest.java +++ b/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() {