From 5c232dd12b5710c9bb8d940313e77351a39bc6a6 Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Mon, 15 Aug 2022 19:14:55 -0700 Subject: [PATCH] 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. --- .../request/ErrorRequestCoordinator.java | 22 +++++-- .../request/ErrorRequestCoordinatorTest.java | 59 +++++++++++++++---- 2 files changed, 63 insertions(+), 18 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 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() {