Skip to content

Commit

Permalink
Avoid assertions on model type in .error()
Browse files Browse the repository at this point in the history
It's too complicated to figure out if models are likely compatible or incompatible. The current logic is broken if we have three classes, Parent, Child1 and Child2 where both the Child classes extend parent. We want Child1 and Child2 to be compatible, but they won't be because Child2 is not assignable to Child1. Anything we do that's more sophisticated is likely to be heuristic anyway.

Ideally we'd just unapply the automatically applied options from the original model and apply the correct automatic options (if any) for the new model. Keeping track of which options can be undone and which options should be re-applied is quite difficult, in part because they automatic options could also be deliberately applied.

Rather than do anything intelligent, we'll rely on callers to use the API appropriately and update the javadoc to warn more. In general I expect the common case to be matching models and there's a reasonable fallback if people have trouble. Unfortunately I expect this won't be perfect, there will be some amount of debugging caused by someone expecting or not expecting a default option :/. I think the tradeoff of not having to create a new request builder and duplicate options for that request builder is probably worth the cost.

PiperOrigin-RevId: 345085610
  • Loading branch information
sjudd authored and glide-copybara-robot committed Dec 1, 2020
1 parent b8570b1 commit e442557
Showing 1 changed file with 10 additions and 15 deletions.
25 changes: 10 additions & 15 deletions library/src/main/java/com/bumptech/glide/RequestBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -228,31 +228,26 @@ public RequestBuilder<TranscodeType> error(@Nullable RequestBuilder<TranscodeTyp
/**
* Identical to calling {@link #error(RequestBuilder)} where the {@code RequestBuilder} is the
* result of calling {@link #clone()} and removing any existing thumbnail and error {@code
* RequestBuilders}
*
* <p>You can only call this method on a {@code RequestBuilder} that has previously had {@code
* load()} called on it with a non-null model. .
* RequestBuilders}.
*
* <p>Other than thumbnail and error {@code RequestBuilder}s, which are removed, all other options
* are retained from the primary request. However, <b>order matters!</b> Any options applied after
* this method is called will not be applied to the error {@code RequestBuilder}. We should move
* this method is called will not be applied to the error {@code RequestBuilder}.
*
* <p>WARNING: Calling this method with a {@code model} whose type does not match the type of the
* model passed to {@code load()} may be dangerous! Any options that were applied by the various
* type specific {@code load()} methods, like {@link #load(byte[])} will be copied to the error
* request here even if the {@code model} you pass to this method doesn't match. Similary, options
* that would be normally applied by type specific {@code load()} methods will <em>not</em> be
* applied to this request. If this behavior is confusing or unexpected, use {@link
* #error(RequestBuilder)} instead.
*/
@NonNull
@CheckResult
public RequestBuilder<TranscodeType> error(Object model) {
if (model == null) {
return error((RequestBuilder<TranscodeType>) null);
}
if (this.model == null) {
throw new IllegalArgumentException(
"Call this method after calling #load() with a non-null" + " model.");
}
if (!this.model.getClass().isAssignableFrom(model.getClass())) {
throw new IllegalArgumentException(
"You can only call #error(Object) with the same type of"
+ " model that you provided to #load(). If you need to load a different type, use the"
+ " somewhat more verbose #error(RequestBuilder) method instead of this shortcut");
}
return error(cloneWithNullErrorAndThumbnail().load(model));
}

Expand Down

0 comments on commit e442557

Please sign in to comment.