Skip to content

Commit

Permalink
Pass a non-application Context into Glide’s Requests.
Browse files Browse the repository at this point in the history
We can use the default themes from Activities and/or Context wrappers
to obtain placeholder Drawables, which can be more efficient than
forcing callers to call getDrawable when building their request and more
powerful/neater than having callers pass in a Theme.

This change does NOT pass through the Context or Themes to the decoder
that loads non-Bitmap drawables on a background thread to avoid memory
leaks.

Fixes #1267.
  • Loading branch information
sjudd committed Oct 18, 2017
1 parent 702386e commit bbb25af
Show file tree
Hide file tree
Showing 12 changed files with 89 additions and 32 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -359,14 +359,17 @@ private List<MethodSpec> generateConstructors() {
.addStatement("super($N, $N)", "transcodeClass", "other")
.build();

ClassName context = ClassName.get("android.content", "Context");
ClassName glide = ClassName.get("com.bumptech.glide", "Glide");
ClassName requestManager = ClassName.get("com.bumptech.glide", "RequestManager");
MethodSpec secondConstructor =
MethodSpec.constructorBuilder()
.addParameter(glide, "glide")
.addParameter(requestManager, "requestManager")
.addParameter(classOfTranscodeType, "transcodeClass")
.addStatement("super($N, $N ,$N)", "glide", "requestManager", "transcodeClass")
.addParameter(context, "context")
.addStatement(
"super($N, $N ,$N, $N)", "glide", "requestManager", "transcodeClass", "context")
.build();
return ImmutableList.of(firstConstructor, secondConstructor);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ final class RequestManagerFactoryGenerator {
"com.bumptech.glide.manager.RequestManagerRetriever.RequestManagerFactory";
private static final String REQUEST_MANAGER_QUALIFIED_NAME =
"com.bumptech.glide.RequestManager";
private static final ClassName CONTEXT_CLASS_NAME =
ClassName.get("android.content", "Context");

static final String GENERATED_REQUEST_MANAGER_FACTORY_PACKAGE_NAME =
"com.bumptech.glide";
Expand Down Expand Up @@ -79,8 +81,9 @@ TypeSpec generate(String generatedCodePackageName, TypeSpec generatedRequestMana
.addParameter(ClassName.get(glideType), "glide")
.addParameter(ClassName.get(lifecycleType), "lifecycle")
.addParameter(ClassName.get(requestManagerTreeNodeType), "treeNode")
.addParameter(CONTEXT_CLASS_NAME, "context")
.addStatement(
"return new $T(glide, lifecycle, treeNode)",
"return new $T(glide, lifecycle, treeNode, context)",
ClassName.get(generatedCodePackageName, generatedRequestManagerSpec.name))
.build()
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ final class RequestManagerGenerator {
"com.bumptech.glide.manager.RequestManagerTreeNode";
private static final ClassName CHECK_RESULT_CLASS_NAME =
ClassName.get("android.support.annotation", "CheckResult");
private static final ClassName CONTEXT_CLASS_NAME =
ClassName.get("android.content", "Context");

private static final String GENERATED_REQUEST_MANAGER_SIMPLE_NAME =
"GlideRequests";
Expand Down Expand Up @@ -128,7 +130,8 @@ private MethodSpec generateCallSuperConstructor() {
.addParameter(ClassName.get(glideType), "glide")
.addParameter(ClassName.get(lifecycleType), "lifecycle")
.addParameter(ClassName.get(requestManagerTreeNodeType), "treeNode")
.addStatement("super(glide, lifecycle, treeNode)")
.addParameter(CONTEXT_CLASS_NAME, "context")
.addStatement("super(glide, lifecycle, treeNode, context)")
.build();
}

Expand All @@ -150,7 +153,7 @@ private MethodSpec generateAsMethod(String generatedCodePackageName, TypeSpec re
.addParameter(classOfResouceType, "resourceClass")
.addAnnotation(AnnotationSpec.builder(CHECK_RESULT_CLASS_NAME).build())
.returns(requestBuilderOfResourceType)
.addStatement("return new $T<>(glide, this, resourceClass)",
.addStatement("return new $T<>(glide, this, resourceClass, context)",
this.generatedRequestBuilderClassName)
.build();
}
Expand Down
20 changes: 12 additions & 8 deletions library/src/main/java/com/bumptech/glide/RequestBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import static com.bumptech.glide.request.RequestOptions.signatureOf;

import android.content.Context;
import android.net.Uri;
import android.support.annotation.CheckResult;
import android.support.annotation.DrawableRes;
Expand Down Expand Up @@ -41,11 +42,12 @@ public class RequestBuilder<TranscodeType> implements Cloneable {
new RequestOptions().diskCacheStrategy(DiskCacheStrategy.DATA).priority(Priority.LOW)
.skipMemoryCache(true);

private final GlideContext context;
private final Context context;
private final RequestManager requestManager;
private final Class<TranscodeType> transcodeClass;
private final RequestOptions defaultRequestOptions;
private final Glide glide;
private final GlideContext glideContext;

@NonNull protected RequestOptions requestOptions;

Expand All @@ -65,18 +67,19 @@ public class RequestBuilder<TranscodeType> implements Cloneable {
private boolean isThumbnailBuilt;

protected RequestBuilder(Glide glide, RequestManager requestManager,
Class<TranscodeType> transcodeClass) {
Class<TranscodeType> transcodeClass, Context context) {
this.glide = glide;
this.requestManager = requestManager;
this.context = glide.getGlideContext();
this.transcodeClass = transcodeClass;
this.defaultRequestOptions = requestManager.getDefaultRequestOptions();
this.context = context;
this.transitionOptions = requestManager.getDefaultTransitionOptions(transcodeClass);
this.requestOptions = defaultRequestOptions;
this.glideContext = glide.getGlideContext();
}

protected RequestBuilder(Class<TranscodeType> transcodeClass, RequestBuilder<?> other) {
this(other.glide, other.requestManager, transcodeClass);
this(other.glide, other.requestManager, transcodeClass, other.context);
model = other.model;
isModelSet = other.isModelSet;
requestOptions = other.requestOptions;
Expand Down Expand Up @@ -554,7 +557,7 @@ public Target<TranscodeType> into(ImageView view) {
}
}

return into(context.buildImageViewTarget(view, transcodeClass), requestOptions);
return into(glideContext.buildImageViewTarget(view, transcodeClass), requestOptions);
}

/**
Expand Down Expand Up @@ -607,10 +610,10 @@ public FutureTarget<TranscodeType> submit() {
*/
public FutureTarget<TranscodeType> submit(int width, int height) {
final RequestFutureTarget<TranscodeType> target =
new RequestFutureTarget<>(context.getMainHandler(), width, height);
new RequestFutureTarget<>(glideContext.getMainHandler(), width, height);

if (Util.isOnBackgroundThread()) {
context.getMainHandler().post(new Runnable() {
glideContext.getMainHandler().post(new Runnable() {
@Override
public void run() {
if (!target.isCancelled()) {
Expand Down Expand Up @@ -839,6 +842,7 @@ private Request obtainRequest(Target<TranscodeType> target,
int overrideWidth, int overrideHeight) {
return SingleRequest.obtain(
context,
glideContext,
model,
transcodeClass,
requestOptions,
Expand All @@ -848,7 +852,7 @@ private Request obtainRequest(Target<TranscodeType> target,
target,
requestListener,
requestCoordinator,
context.getEngine(),
glideContext.getEngine(),
transitionOptions.getTransitionFactory());
}
}
24 changes: 17 additions & 7 deletions library/src/main/java/com/bumptech/glide/RequestManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ public class RequestManager implements LifecycleListener {
.skipMemoryCache(true);

protected final Glide glide;
protected final Context context;
@Synthetic final Lifecycle lifecycle;
private final RequestTracker requestTracker;
private final RequestManagerTreeNode treeNode;
Expand All @@ -67,8 +68,15 @@ public void run() {
@NonNull
private RequestOptions requestOptions;

public RequestManager(Glide glide, Lifecycle lifecycle, RequestManagerTreeNode treeNode) {
this(glide, lifecycle, treeNode, new RequestTracker(), glide.getConnectivityMonitorFactory());
public RequestManager(
Glide glide, Lifecycle lifecycle, RequestManagerTreeNode treeNode, Context context) {
this(
glide,
lifecycle,
treeNode,
new RequestTracker(),
glide.getConnectivityMonitorFactory(),
context);
}

// Our usage is safe here.
Expand All @@ -78,16 +86,18 @@ public RequestManager(Glide glide, Lifecycle lifecycle, RequestManagerTreeNode t
Lifecycle lifecycle,
RequestManagerTreeNode treeNode,
RequestTracker requestTracker,
ConnectivityMonitorFactory factory) {
ConnectivityMonitorFactory factory,
Context context) {
this.glide = glide;
this.lifecycle = lifecycle;
this.treeNode = treeNode;
this.requestTracker = requestTracker;

final Context context = glide.getGlideContext().getBaseContext();
this.context = context;

connectivityMonitor =
factory.build(context, new RequestManagerConnectivityListener(requestTracker));
factory.build(
context.getApplicationContext(),
new RequestManagerConnectivityListener(requestTracker));

// If we're the application level request manager, we may be created on a background thread.
// In that case we cannot risk synchronously pausing or resuming requests, so we hack around the
Expand Down Expand Up @@ -393,7 +403,7 @@ public RequestBuilder<File> asFile() {
*/
@CheckResult
public <ResourceType> RequestBuilder<ResourceType> as(Class<ResourceType> resourceClass) {
return new RequestBuilder<>(glide, this, resourceClass);
return new RequestBuilder<>(glide, this, resourceClass, context);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,13 @@ private RequestManager getApplicationManager(Context context) {
// ApplicationLifecycle.

// TODO(b/27524013): Factor out this Glide.get() call.
Glide glide = Glide.get(context);
Glide glide = Glide.get(context.getApplicationContext());
applicationManager =
factory.build(glide, new ApplicationLifecycle(), new EmptyRequestManagerTreeNode());
factory.build(
glide,
new ApplicationLifecycle(),
new EmptyRequestManagerTreeNode(),
context.getApplicationContext());
}
}
}
Expand Down Expand Up @@ -338,7 +342,8 @@ private RequestManager fragmentGet(Context context, android.app.FragmentManager
// TODO(b/27524013): Factor out this Glide.get() call.
Glide glide = Glide.get(context);
requestManager =
factory.build(glide, current.getGlideLifecycle(), current.getRequestManagerTreeNode());
factory.build(
glide, current.getGlideLifecycle(), current.getRequestManagerTreeNode(), context);
current.setRequestManager(requestManager);
}
return requestManager;
Expand Down Expand Up @@ -369,7 +374,8 @@ private RequestManager supportFragmentGet(Context context, FragmentManager fm,
// TODO(b/27524013): Factor out this Glide.get() call.
Glide glide = Glide.get(context);
requestManager =
factory.build(glide, current.getGlideLifecycle(), current.getRequestManagerTreeNode());
factory.build(
glide, current.getGlideLifecycle(), current.getRequestManagerTreeNode(), context);
current.setRequestManager(requestManager);
}
return requestManager;
Expand Down Expand Up @@ -406,14 +412,17 @@ public boolean handleMessage(Message message) {
*/
public interface RequestManagerFactory {
RequestManager build(
Glide glide, Lifecycle lifecycle, RequestManagerTreeNode requestManagerTreeNode);
Glide glide,
Lifecycle lifecycle,
RequestManagerTreeNode requestManagerTreeNode,
Context context);
}

private static final RequestManagerFactory DEFAULT_FACTORY = new RequestManagerFactory() {
@Override
public RequestManager build(Glide glide, Lifecycle lifecycle,
RequestManagerTreeNode requestManagerTreeNode) {
return new RequestManager(glide, lifecycle, requestManagerTreeNode);
RequestManagerTreeNode requestManagerTreeNode, Context context) {
return new RequestManager(glide, lifecycle, requestManagerTreeNode, context);
}
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -653,6 +653,16 @@ public RequestOptions error(@DrawableRes int resourceId) {
* for resource ids provided via {@link #error(int)}, {@link #placeholder(int)}, and
* {@link #fallback(Drawable)}.
*
* <p>The theme is <em>NOT</em> applied in the decoder that will attempt to decode a given
* resource id model on Glide's background threads. The theme is used exclusively on the main
* thread to obtain placeholder/error/fallback drawables to avoid leaking Activities.
*
* <p>If the {@link android.content.Context} of the {@link android.app.Fragment} or
* {@link android.app.Activity} used to start this load has a different
* {@link android.content.res.Resources.Theme}, the {@link android.content.res.Resources.Theme}
* provided here will override the {@link android.content.res.Resources.Theme} of the
* {@link android.content.Context}.
*
* @param theme The theme to use when loading Drawables.
* @return this request builder.
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package com.bumptech.glide.request;

import android.content.Context;
import android.content.res.Resources.Theme;
import android.graphics.drawable.Drawable;
import android.support.annotation.DrawableRes;
import android.support.annotation.Nullable;
Expand Down Expand Up @@ -84,6 +86,7 @@ private enum Status {
private final StateVerifier stateVerifier = StateVerifier.newInstance();

private RequestCoordinator requestCoordinator;
private Context context;
private GlideContext glideContext;
@Nullable
private Object model;
Expand All @@ -107,6 +110,7 @@ private enum Status {
private int height;

public static <R> SingleRequest<R> obtain(
Context context,
GlideContext glideContext,
Object model,
Class<R> transcodeClass,
Expand All @@ -125,6 +129,7 @@ public static <R> SingleRequest<R> obtain(
request = new SingleRequest<>();
}
request.init(
context,
glideContext,
model,
transcodeClass,
Expand All @@ -146,6 +151,7 @@ public static <R> SingleRequest<R> obtain(
}

private void init(
Context context,
GlideContext glideContext,
Object model,
Class<R> transcodeClass,
Expand All @@ -158,6 +164,7 @@ private void init(
RequestCoordinator requestCoordinator,
Engine engine,
TransitionFactory<? super R> animationFactory) {
this.context = context;
this.glideContext = glideContext;
this.model = model;
this.transcodeClass = transcodeClass;
Expand All @@ -181,6 +188,7 @@ public StateVerifier getVerifier() {
@Override
public void recycle() {
assertNotCallingCallbacks();
context = null;
glideContext = null;
model = null;
transcodeClass = null;
Expand Down Expand Up @@ -379,7 +387,9 @@ private Drawable getFallbackDrawable() {
}

private Drawable loadDrawable(@DrawableRes int resourceId) {
return DrawableDecoderCompat.getDrawable(glideContext, resourceId, requestOptions.getTheme());
Theme theme = requestOptions.getTheme() != null
? requestOptions.getTheme() : context.getTheme();
return DrawableDecoderCompat.getDrawable(glideContext, resourceId, theme);
}

private void setErrorPlaceholder() {
Expand Down
2 changes: 1 addition & 1 deletion library/src/test/java/com/bumptech/glide/GlideTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ public Boolean answer(InvocationOnMock invocation) throws Throwable {

Lifecycle lifecycle = mock(Lifecycle.class);
RequestManagerTreeNode treeNode = mock(RequestManagerTreeNode.class);
requestManager = new RequestManager(Glide.get(getContext()), lifecycle, treeNode);
requestManager = new RequestManager(Glide.get(getContext()), lifecycle, treeNode, getContext());
requestManager.resumeRequests();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import android.app.Application;
import android.widget.ImageView;
import com.bumptech.glide.request.Request;
import com.bumptech.glide.request.RequestOptions;
Expand All @@ -30,11 +31,13 @@ public class RequestBuilderTest {
@Mock GlideContext glideContext;
@Mock RequestManager requestManager;
private Glide glide;
private Application context;

@Before
public void setUp() {
MockitoAnnotations.initMocks(this);
glide = Glide.get(RuntimeEnvironment.application);
context = RuntimeEnvironment.application;
}

@After
Expand All @@ -44,7 +47,7 @@ public void tearDown() {

@Test(expected = NullPointerException.class)
public void testThrowsIfContextIsNull() {
new RequestBuilder<>(null /*context*/, requestManager, Object.class);
new RequestBuilder<>(null /*context*/, requestManager, Object.class, context);
}

@Test(expected = NullPointerException.class)
Expand Down Expand Up @@ -119,7 +122,7 @@ private RequestBuilder<Object> getNullModelRequest() {
.thenReturn(new RequestOptions());
when(requestManager.getDefaultTransitionOptions(any(Class.class)))
.thenReturn(new GenericTransitionOptions<>());
return new RequestBuilder<>(glide, requestManager, Object.class)
return new RequestBuilder<>(glide, requestManager, Object.class, context)
.load((Object) null);
}
}
Loading

0 comments on commit bbb25af

Please sign in to comment.