Skip to content

Commit 690f815

Browse files
sjuddglide-copybara-robot
sjudd
authored and
glide-copybara-robot
committedOct 2, 2019
Distinguish resource set from request complete in Glide's RequestCoordinators
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
·
v5.0.0-rc01compose-alpha6
1 parent 5c20f1a commit 690f815

9 files changed

+70
-24
lines changed
 

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

+8-6
Original file line numberDiff line numberDiff line change
@@ -144,15 +144,10 @@ private boolean isValidRequest(Request request) {
144144
@Override
145145
public boolean isAnyResourceSet() {
146146
synchronized (requestLock) {
147-
return parentIsAnyResourceSet() || isComplete();
147+
return primary.isAnyResourceSet() || error.isAnyResourceSet();
148148
}
149149
}
150150

151-
@GuardedBy("requestLock")
152-
private boolean parentIsAnyResourceSet() {
153-
return parent != null && parent.isAnyResourceSet();
154-
}
155-
156151
@Override
157152
public void onRequestSuccess(Request request) {
158153
synchronized (requestLock) {
@@ -186,4 +181,11 @@ public void onRequestFailed(Request request) {
186181
}
187182
}
188183
}
184+
185+
@Override
186+
public RequestCoordinator getRoot() {
187+
synchronized (requestLock) {
188+
return parent != null ? parent.getRoot() : this;
189+
}
190+
}
189191
}

‎library/src/main/java/com/bumptech/glide/request/Request.java

+6
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,12 @@ public interface Request {
3232
/** Returns true if the request has been cleared. */
3333
boolean isCleared();
3434

35+
/**
36+
* Returns true if a resource is set, even if the request is not yet complete or the primary
37+
* request has failed.
38+
*/
39+
boolean isAnyResourceSet();
40+
3541
/**
3642
* Returns {@code true} if this {@link Request} is equivalent to the given {@link Request} (has
3743
* all of the same options and sizes).

‎library/src/main/java/com/bumptech/glide/request/RequestCoordinator.java

+3
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,9 @@ public interface RequestCoordinator {
4444
/** Must be called when a {@link Request} coordinated by this object fails. */
4545
void onRequestFailed(Request request);
4646

47+
/** Returns the top most parent {@code RequestCoordinator}. */
48+
RequestCoordinator getRoot();
49+
4750
/** A simple state enum to keep track of the states of individual subrequests. */
4851
enum RequestState {
4952
RUNNING(false),

‎library/src/main/java/com/bumptech/glide/request/SingleRequest.java

+8-1
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,13 @@ public boolean isCleared() {
357357
}
358358
}
359359

360+
@Override
361+
public boolean isAnyResourceSet() {
362+
synchronized (requestLock) {
363+
return status == Status.COMPLETE;
364+
}
365+
}
366+
360367
@GuardedBy("requestLock")
361368
private Drawable getErrorDrawable() {
362369
if (errorDrawable == null) {
@@ -493,7 +500,7 @@ private boolean canNotifyStatusChanged() {
493500

494501
@GuardedBy("requestLock")
495502
private boolean isFirstReadyResource() {
496-
return requestCoordinator == null || !requestCoordinator.isAnyResourceSet();
503+
return requestCoordinator == null || !requestCoordinator.getRoot().isAnyResourceSet();
497504
}
498505

499506
@GuardedBy("requestLock")

‎library/src/main/java/com/bumptech/glide/request/ThumbnailRequestCoordinator.java

+7-11
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ private boolean parentCanSetImage() {
6060
@Override
6161
public boolean canNotifyStatusChanged(Request request) {
6262
synchronized (requestLock) {
63-
return parentCanNotifyStatusChanged() && request.equals(full) && !isResourceSet();
63+
return parentCanNotifyStatusChanged() && request.equals(full) && !isAnyResourceSet();
6464
}
6565
}
6666

@@ -84,7 +84,7 @@ private boolean parentCanNotifyStatusChanged() {
8484
@Override
8585
public boolean isAnyResourceSet() {
8686
synchronized (requestLock) {
87-
return parentIsAnyResourceSet() || isResourceSet();
87+
return thumb.isAnyResourceSet() || full.isAnyResourceSet();
8888
}
8989
}
9090

@@ -123,9 +123,11 @@ public void onRequestFailed(Request request) {
123123
}
124124
}
125125

126-
@GuardedBy("requestLock")
127-
private boolean parentIsAnyResourceSet() {
128-
return parent != null && parent.isAnyResourceSet();
126+
@Override
127+
public RequestCoordinator getRoot() {
128+
synchronized (requestLock) {
129+
return parent != null ? parent.getRoot() : this;
130+
}
129131
}
130132

131133
/** Starts first the thumb request and then the full request. */
@@ -189,12 +191,6 @@ public boolean isComplete() {
189191
}
190192
}
191193

192-
private boolean isResourceSet() {
193-
synchronized (requestLock) {
194-
return fullState == RequestState.SUCCESS || thumbState == RequestState.SUCCESS;
195-
}
196-
}
197-
198194
@Override
199195
public boolean isCleared() {
200196
synchronized (requestLock) {

‎library/test/src/test/java/com/bumptech/glide/manager/RequestTrackerTest.java

+5
Original file line numberDiff line numberDiff line change
@@ -416,6 +416,11 @@ public boolean isCleared() {
416416
return isCleared;
417417
}
418418

419+
@Override
420+
public boolean isAnyResourceSet() {
421+
return isComplete;
422+
}
423+
419424
@Override
420425
public boolean isEquivalentTo(Request other) {
421426
throw new UnsupportedOperationException();

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

+10-4
Original file line numberDiff line numberDiff line change
@@ -341,6 +341,7 @@ public void isAnyResourceSet_primaryNotSet_nullParent_returnsFalse() {
341341

342342
@Test
343343
public void isAnyResourceSet_primarySet_nullParent_returnsTrue() {
344+
when(primary.isAnyResourceSet()).thenReturn(true);
344345
coordinator.onRequestSuccess(primary);
345346
assertThat(coordinator.isAnyResourceSet()).isTrue();
346347
}
@@ -349,6 +350,7 @@ public void isAnyResourceSet_primarySet_nullParent_returnsTrue() {
349350
public void isAnyResourceSet_primarySet_parentResourceNotSet_returnsTrue() {
350351
coordinator = newCoordinator(parent);
351352
coordinator.setRequests(primary, error);
353+
when(primary.isAnyResourceSet()).thenReturn(true);
352354
coordinator.onRequestSuccess(primary);
353355

354356
assertThat(coordinator.isAnyResourceSet()).isTrue();
@@ -358,24 +360,26 @@ public void isAnyResourceSet_primarySet_parentResourceNotSet_returnsTrue() {
358360
public void isAnyResourceSet_primarySet_parentSet_returnsTrue() {
359361
coordinator = newCoordinator(parent);
360362
coordinator.setRequests(primary, error);
363+
when(primary.isAnyResourceSet()).thenReturn(true);
361364
coordinator.onRequestSuccess(primary);
362365
when(parent.isAnyResourceSet()).thenReturn(true);
363366

364367
assertThat(coordinator.isAnyResourceSet()).isTrue();
365368
}
366369

367370
@Test
368-
public void isAnyResourceSet_parentSet_returnsTrue() {
371+
public void isAnyResourceSet_parentSet_returnsFalse() {
369372
coordinator = newCoordinator(parent);
370373
coordinator.setRequests(primary, error);
371374
when(parent.isAnyResourceSet()).thenReturn(true);
372375

373-
assertThat(coordinator.isAnyResourceSet()).isTrue();
376+
assertThat(coordinator.isAnyResourceSet()).isFalse();
374377
}
375378

376379
@Test
377380
public void isAnyResourceSet_errorSet_failedPrimary_nullParent_returnsTrue() {
378381
coordinator.onRequestFailed(primary);
382+
when(error.isAnyResourceSet()).thenReturn(true);
379383
coordinator.onRequestSuccess(error);
380384
assertThat(coordinator.isAnyResourceSet()).isTrue();
381385
}
@@ -385,6 +389,7 @@ public void isAnyResourceSet_errorSet_failedPrimary_nonNullParentNotSet_returnsT
385389
coordinator = newCoordinator(parent);
386390
coordinator.setRequests(primary, error);
387391
coordinator.onRequestFailed(primary);
392+
when(error.isAnyResourceSet()).thenReturn(true);
388393
coordinator.onRequestSuccess(error);
389394

390395
assertThat(coordinator.isAnyResourceSet()).isTrue();
@@ -395,6 +400,7 @@ public void isAnyResourceSet_errorSet_nonNullParentSet_returnsTrue() {
395400
coordinator = newCoordinator(parent);
396401
coordinator.setRequests(primary, error);
397402
when(parent.isAnyResourceSet()).thenReturn(true);
403+
when(error.isAnyResourceSet()).thenReturn(true);
398404
coordinator.onRequestSuccess(error);
399405

400406
assertThat(coordinator.isAnyResourceSet()).isTrue();
@@ -409,13 +415,13 @@ public void isAnyResourceSet_primaryNotSet_errorNotSet_nonNullParentNotSet_retur
409415
}
410416

411417
@Test
412-
public void isAnyResourceSet_primaryNotSet_errorNotSet_nonNullParentSet_returnsTrue() {
418+
public void isAnyResourceSet_primaryNotSet_errorNotSet_nonNullParentSet_returnsFalse() {
413419
coordinator = newCoordinator(parent);
414420
coordinator.setRequests(primary, error);
415421

416422
when(parent.isAnyResourceSet()).thenReturn(true);
417423

418-
assertThat(coordinator.isAnyResourceSet()).isTrue();
424+
assertThat(coordinator.isAnyResourceSet()).isFalse();
419425
}
420426

421427
@Test

‎library/test/src/test/java/com/bumptech/glide/request/SingleRequestTest.java

+17
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,7 @@ public void clear_doesNotNotifyTarget_ifRequestCoordinatorReturnsFalseForCanClea
167167
@Test
168168
public void testResourceIsNotCompleteWhenAskingCoordinatorIfCanSetImage() {
169169
RequestCoordinator requestCoordinator = mock(RequestCoordinator.class);
170+
when(requestCoordinator.getRoot()).thenReturn(requestCoordinator);
170171
doAnswer(
171172
new Answer() {
172173
@Override
@@ -625,6 +626,21 @@ public void testRequestListenerIsCalledWithFirstImageIfRequestCoordinatorReturns
625626
eq(builder.result), any(Number.class), isAListTarget(), isADataSource(), eq(false));
626627
}
627628

629+
@Test
630+
public void
631+
testRequestListenerIsCalledWithNotIsFirstRequestIfRequestCoordinatorParentReturnsResourceSet() {
632+
SingleRequest<List> request = builder.addRequestListener(listener1).build();
633+
RequestCoordinator rootRequestCoordinator = mock(RequestCoordinator.class);
634+
when(rootRequestCoordinator.isAnyResourceSet()).thenReturn(true);
635+
when(builder.requestCoordinator.isAnyResourceSet()).thenReturn(false);
636+
when(builder.requestCoordinator.getRoot()).thenReturn(rootRequestCoordinator);
637+
request.onResourceReady(builder.resource, DataSource.DATA_DISK_CACHE);
638+
639+
verify(listener1)
640+
.onResourceReady(
641+
eq(builder.result), any(Number.class), isAListTarget(), isADataSource(), eq(false));
642+
}
643+
628644
@Test
629645
public void testTargetIsCalledWithAnimationFromFactory() {
630646
SingleRequest<List> request = builder.build();
@@ -903,6 +919,7 @@ static final class SingleRequestBuilder {
903919
private final Map<Class<?>, Transformation<?>> transformations = new HashMap<>();
904920

905921
SingleRequestBuilder() {
922+
when(requestCoordinator.getRoot()).thenReturn(requestCoordinator);
906923
when(requestCoordinator.canSetImage(any(Request.class))).thenReturn(true);
907924
when(requestCoordinator.canNotifyCleared(any(Request.class))).thenReturn(true);
908925
when(requestCoordinator.canNotifyStatusChanged(any(Request.class))).thenReturn(true);

‎library/test/src/test/java/com/bumptech/glide/request/ThumbnailRequestCoordinatorTest.java

+6-2
Original file line numberDiff line numberDiff line change
@@ -208,12 +208,14 @@ public void testCanNotNotifyStatusChangedIfThumb() {
208208

209209
@Test
210210
public void canNotNotifyStatusChanged_forFull_whenFullComplete_isFalse() {
211+
when(full.isAnyResourceSet()).thenReturn(true);
211212
coordinator.onRequestSuccess(full);
212213
assertFalse(coordinator.canNotifyStatusChanged(full));
213214
}
214215

215216
@Test
216217
public void canNotNotifyStatusChanged_forFull_whenIfThumbComplete_isFalse() {
218+
when(thumb.isAnyResourceSet()).thenReturn(true);
217219
coordinator.onRequestSuccess(thumb);
218220
assertFalse(coordinator.canNotifyStatusChanged(full));
219221
}
@@ -249,24 +251,26 @@ public void isAnyResourceSet_withIncompleteThumbAndFull_isFalse() {
249251

250252
@Test
251253
public void isAnyResourceSet_withCompleteFull_isTrue() {
254+
when(full.isAnyResourceSet()).thenReturn(true);
252255
coordinator.onRequestSuccess(full);
253256
assertTrue(coordinator.isAnyResourceSet());
254257
}
255258

256259
@Test
257260
public void isAnyResourceSet_withCompleteThumb_isTrue() {
261+
when(thumb.isAnyResourceSet()).thenReturn(true);
258262
coordinator.onRequestSuccess(thumb);
259263
assertTrue(coordinator.isAnyResourceSet());
260264
}
261265

262266
@Test
263-
public void isAnyResourceSet_withParentResourceSet_isTrue() {
267+
public void isAnyResourceSet_withParentResourceSet_isFalse() {
264268
coordinator = newCoordinator(parent);
265269
coordinator.setRequests(full, thumb);
266270

267271
when(parent.isAnyResourceSet()).thenReturn(true);
268272

269-
assertTrue(coordinator.isAnyResourceSet());
273+
assertThat(coordinator.isAnyResourceSet()).isFalse();
270274
}
271275

272276
@Test

0 commit comments

Comments
 (0)
Please sign in to comment.