Skip to content

Commit

Permalink
Experimental ModelLoader to load using Resources
Browse files Browse the repository at this point in the history
The new ModelLoader is guarded by an experiment. It should, in the long
run, replace ResourceLoader as the new way of loading resourceIds.
Instead of converting resourceIds to Uris and then loading the Uri with
ContentResolver, the new ModelLoader uses Resources.openRawResource
directly.

openRawResource, despite the name, works fine with normal Drawables as
well as raw resources. In addition, we can obtain a Resources object
that will use the Theme to pick the appropriate day or night mode
drawable. ContentResolver and resource Uris always use the default or
day mode Drawable.

The new ModelLoader should fix the most common use case for #3778 where
people pass in resource ids.

We'd need an additional change to handle cases where users pass in
resource Uris, since those still go through ContentResolver.
  • Loading branch information
sjudd committed Dec 14, 2022
1 parent 18bba92 commit 04f198e
Show file tree
Hide file tree
Showing 4 changed files with 270 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,15 @@
import com.bumptech.glide.test.ForceDarkOrLightModeActivity;
import com.google.common.base.Function;
import org.junit.Before;
import org.junit.Ignore;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;

@RunWith(AndroidJUnit4.class)
public class DarkModeTest {
private final Context context = ApplicationProvider.getApplicationContext();

@Rule
public final IdlingGlideRule idlingGlideRule =
IdlingGlideRule.newGlideRule(glideBuilder -> glideBuilder);
@Rule public final IdlingGlideRule idlingGlideRule =
IdlingGlideRule.newGlideRule(glideBuilder -> glideBuilder.useDirectResourceLoader(true));

@Before
public void before() {
Expand All @@ -62,7 +59,6 @@ public void before() {
// when it's specified by the user. Otherwise whether or not we'd obey dark mode would depend on
// the user also providing the theme from the activity. We'd want to try to make sure that doesn't
// leak the Activity.
// TODO(judds): Add tests for Fragments for load().
@Test
public void load_withDarkModeActivity_usesLightModeDrawable() {
runActivityTest(
Expand Down Expand Up @@ -95,7 +91,6 @@ public void load_withLightModeFragment_usesLightModeDrawable() {
fragment -> Glide.with(fragment).load(R.drawable.dog).override(Target.SIZE_ORIGINAL));
}

@Ignore("We do not asynchronously load resources correctly")
@Test
public void load_withDarkModeActivity_darkModeTheme_usesDarkModeDrawable() {
runActivityTest(
Expand All @@ -108,7 +103,6 @@ public void load_withDarkModeActivity_darkModeTheme_usesDarkModeDrawable() {
.theme(activity.getTheme()));
}

@Ignore("We do not asynchronously load resources correctly")
@Test
public void load_withDarkModeFragment_darkModeTheme_usesDarkModeDrawable() {
runFragmentTest(
Expand All @@ -121,6 +115,30 @@ public void load_withDarkModeFragment_darkModeTheme_usesDarkModeDrawable() {
.theme(fragment.requireActivity().getTheme()));
}

@Test
public void load_withApplicationContext_darkTheme_usesDarkModeDrawable() {
runActivityTest(
darkModeActivity(),
R.raw.dog_dark,
input ->
Glide.with(input.getApplicationContext())
.load(R.drawable.dog)
.override(Target.SIZE_ORIGINAL)
.theme(input.getTheme()));
}

@Test
public void load_withApplicationContext_lightTheme_usesLightModeDrawable() {
runActivityTest(
lightModeActivity(),
R.raw.dog_light,
input ->
Glide.with(input.getApplicationContext())
.load(R.drawable.dog)
.override(Target.SIZE_ORIGINAL)
.theme(input.getTheme()));
}

@Test
public void load_withLightModeActivity_lightModeTheme_usesLightModeDrawable() {
runActivityTest(
Expand Down
24 changes: 24 additions & 0 deletions library/src/main/java/com/bumptech/glide/GlideBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,23 @@ public GlideBuilder useLifecycleInsteadOfInjectingFragments(boolean isEnabled) {
return this;
}

/**
* Use {@link com.bumptech.glide.load.model.DirectResourceLoader} instead of {@link
* com.bumptech.glide.load.model.ResourceLoader} so that we load drawables asynchronously with
* the correc theme (ie light / dark mode).
*
* <p>This also removes support for loading resources as resource Uris and for loading
* {@link android.os.ParcelFileDescriptor}s from resource ids. I think neither of those are useful
* but if you have a use case or seem to find some test failing with this experiment enabled,
* please file an issue so we can investigate.
*
* <p>This flag is experimental and will be removed without notice in a future version.
*/
public GlideBuilder useDirectResourceLoader(boolean isEnabled) {
glideExperimentsBuilder.update(new UseDirectResourceLoader(), isEnabled);
return this;
}

void setRequestManagerFactory(@Nullable RequestManagerFactory factory) {
this.requestManagerFactory = factory;
}
Expand Down Expand Up @@ -621,4 +638,11 @@ public static final class LogRequestOrigins implements Experiment {}
* and activities.
*/
public static final class UseLifecycleInsteadOfInjectingFragments implements Experiment {}

/**
* Use {@link com.bumptech.glide.load.model.DirectResourceLoader} instead of
* {@link com.bumptech.glide.load.model.ResourceLoader} so that we load resources asynchronously
* with the correct theme.
*/
public static final class UseDirectResourceLoader implements Experiment {}
}
51 changes: 36 additions & 15 deletions library/src/main/java/com/bumptech/glide/RegistryFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import androidx.annotation.Nullable;
import androidx.tracing.Trace;
import com.bumptech.glide.GlideBuilder.EnableImageDecoderForBitmaps;
import com.bumptech.glide.GlideBuilder.UseDirectResourceLoader;
import com.bumptech.glide.gifdecoder.GifDecoder;
import com.bumptech.glide.load.ImageHeaderParser;
import com.bumptech.glide.load.ResourceDecoder;
Expand All @@ -25,9 +26,11 @@
import com.bumptech.glide.load.model.ByteBufferEncoder;
import com.bumptech.glide.load.model.ByteBufferFileLoader;
import com.bumptech.glide.load.model.DataUrlLoader;
import com.bumptech.glide.load.model.DirectResourceLoader;
import com.bumptech.glide.load.model.FileLoader;
import com.bumptech.glide.load.model.GlideUrl;
import com.bumptech.glide.load.model.MediaStoreFileLoader;
import com.bumptech.glide.load.model.ModelLoaderFactory;
import com.bumptech.glide.load.model.ResourceLoader;
import com.bumptech.glide.load.model.StreamEncoder;
import com.bumptech.glide.load.model.StringLoader;
Expand Down Expand Up @@ -177,13 +180,7 @@ private static void initializeDefaults(
}

ResourceDrawableDecoder resourceDrawableDecoder = new ResourceDrawableDecoder(context);
ResourceLoader.StreamFactory resourceLoaderStreamFactory =
new ResourceLoader.StreamFactory(resources);
ResourceLoader.UriFactory resourceLoaderUriFactory = new ResourceLoader.UriFactory(resources);
ResourceLoader.FileDescriptorFactory resourceLoaderFileDescriptorFactory =
new ResourceLoader.FileDescriptorFactory(resources);
ResourceLoader.AssetFileDescriptorFactory resourceLoaderAssetFileDescriptorFactory =
new ResourceLoader.AssetFileDescriptorFactory(resources);

BitmapEncoder bitmapEncoder = new BitmapEncoder(arrayPool);

BitmapBytesTranscoder bitmapBytesTranscoder = new BitmapBytesTranscoder();
Expand Down Expand Up @@ -274,15 +271,39 @@ Uri.class, Bitmap.class, new ResourceBitmapDecoder(resourceDrawableDecoder, bitm
registry.register(new ParcelFileDescriptorRewinder.Factory());
}


if (experiments.isEnabled(UseDirectResourceLoader.class)) {
ModelLoaderFactory<Integer, InputStream> inputStreamFactory =
DirectResourceLoader.inputStreamFactory(context);
ModelLoaderFactory<Integer, AssetFileDescriptor> assetFileDescriptorFactory =
DirectResourceLoader.assetFileDescriptorFactory(context);
registry
.append(int.class, InputStream.class, inputStreamFactory)
.append(Integer.class, InputStream.class, inputStreamFactory)
.append(int.class, AssetFileDescriptor.class, assetFileDescriptorFactory)
.append(Integer.class, AssetFileDescriptor.class, assetFileDescriptorFactory);
} else {
ResourceLoader.StreamFactory resourceLoaderStreamFactory =
new ResourceLoader.StreamFactory(resources);
ResourceLoader.UriFactory resourceLoaderUriFactory = new ResourceLoader.UriFactory(resources);
ResourceLoader.FileDescriptorFactory resourceLoaderFileDescriptorFactory =
new ResourceLoader.FileDescriptorFactory(resources);
ResourceLoader.AssetFileDescriptorFactory resourceLoaderAssetFileDescriptorFactory =
new ResourceLoader.AssetFileDescriptorFactory(resources);

registry
.append(int.class, InputStream.class, resourceLoaderStreamFactory)
.append(int.class, ParcelFileDescriptor.class, resourceLoaderFileDescriptorFactory)
.append(Integer.class, InputStream.class, resourceLoaderStreamFactory)
.append(Integer.class, ParcelFileDescriptor.class, resourceLoaderFileDescriptorFactory)
.append(Integer.class, Uri.class, resourceLoaderUriFactory)
.append(int.class, AssetFileDescriptor.class, resourceLoaderAssetFileDescriptorFactory)
.append(Integer.class, AssetFileDescriptor.class,
resourceLoaderAssetFileDescriptorFactory)
.append(int.class, Uri.class, resourceLoaderUriFactory);
}

registry
.append(int.class, InputStream.class, resourceLoaderStreamFactory)
.append(int.class, ParcelFileDescriptor.class, resourceLoaderFileDescriptorFactory)
.append(Integer.class, InputStream.class, resourceLoaderStreamFactory)
.append(Integer.class, ParcelFileDescriptor.class, resourceLoaderFileDescriptorFactory)
.append(Integer.class, Uri.class, resourceLoaderUriFactory)
.append(int.class, AssetFileDescriptor.class, resourceLoaderAssetFileDescriptorFactory)
.append(Integer.class, AssetFileDescriptor.class, resourceLoaderAssetFileDescriptorFactory)
.append(int.class, Uri.class, resourceLoaderUriFactory)
.append(String.class, InputStream.class, new DataUrlLoader.StreamFactory<String>())
.append(Uri.class, InputStream.class, new DataUrlLoader.StreamFactory<Uri>())
.append(String.class, InputStream.class, new StringLoader.StreamFactory())
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
package com.bumptech.glide.load.model;

import android.content.Context;
import android.content.res.AssetFileDescriptor;
import android.content.res.Resources;
import android.content.res.Resources.Theme;
import android.os.Build;
import android.os.Build.VERSION_CODES;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import com.bumptech.glide.Priority;
import com.bumptech.glide.load.DataSource;
import com.bumptech.glide.load.Options;
import com.bumptech.glide.load.data.DataFetcher;
import com.bumptech.glide.load.resource.drawable.ResourceDrawableDecoder;
import com.bumptech.glide.signature.ObjectKey;
import java.io.Closeable;
import java.io.IOException;
import java.io.InputStream;

/**
* Loads themed resource ids using {@link Resources#openRawResource(int)} or
* {@link Resources#openRawResourceFd(int)} using the theme from {@link
* ResourceDrawableDecoder#THEME} when it's available.
* @param <DataT> The type of data this {@code ModelLoader} will produce (e.g. {@link InputStream},
* {@link AssetFileDescriptor} etc).
*/
public final class DirectResourceLoader<DataT extends Closeable>
implements ModelLoader<Integer, DataT> {

private final Context context;
private final ResourceOpener<DataT> resourceOpener;

public static ModelLoaderFactory<Integer, InputStream> inputStreamFactory(Context context) {
return new InputStreamFactory(context);
}

public static ModelLoaderFactory<Integer, AssetFileDescriptor> assetFileDescriptorFactory(
Context context) {
return new AssetFileDescriptorFactory(context);
}

DirectResourceLoader(Context context, ResourceOpener<DataT> resourceOpener) {
this.context = context.getApplicationContext();
this.resourceOpener = resourceOpener;
}

@Override
public LoadData<DataT> buildLoadData(
@NonNull Integer resourceId, int width, int height, @NonNull Options options) {
Theme theme = options.get(ResourceDrawableDecoder.THEME);
Resources resources =
Build.VERSION.SDK_INT >= VERSION_CODES.LOLLIPOP && theme != null
? theme.getResources() : context.getResources();
return new LoadData<>(
// TODO(judds): We try to apply AndroidResourceSignature for caching in RequestBuilder.
// Arguably we should mix in that information here instead.
new ObjectKey(resourceId),
new ResourceDataFetcher<>(resources, resourceOpener, resourceId));
}

@Override
public boolean handles(@NonNull Integer integer) {
// We could check that this is in fact a resource ID, but doing so isn't free and in practice
// it doesn't seem to have been an issue historically.
return true;
}

private interface ResourceOpener<DataT> {
DataT open(Resources resources, int resourceId);
Class<DataT> getDataClass();
}

private static final class AssetFileDescriptorFactory
implements ModelLoaderFactory<Integer, AssetFileDescriptor>,
ResourceOpener<AssetFileDescriptor> {

private final Context context;

AssetFileDescriptorFactory(Context context) {
this.context = context;
}

@Override
public AssetFileDescriptor open(Resources resources, int resourceId) {
return resources.openRawResourceFd(resourceId);
}

@Override
public Class<AssetFileDescriptor> getDataClass() {
return AssetFileDescriptor.class;
}

@NonNull
@Override
public ModelLoader<Integer, AssetFileDescriptor> build(
@NonNull MultiModelLoaderFactory multiFactory) {
return new DirectResourceLoader<>(context, this);
}

@Override public void teardown() {}
}

private static final class InputStreamFactory
implements ModelLoaderFactory<Integer, InputStream>, ResourceOpener<InputStream> {

private final Context context;

InputStreamFactory(Context context) {
this.context = context;
}

@NonNull
@Override
public ModelLoader<Integer, InputStream> build(@NonNull MultiModelLoaderFactory multiFactory) {
return new DirectResourceLoader<>(context, this);
}

@Override
public InputStream open(Resources resources, int resourceId) {
return resources.openRawResource(resourceId);
}

@Override
public Class<InputStream> getDataClass() {
return InputStream.class;
}

@Override public void teardown() {}
}

private static final class ResourceDataFetcher<DataT extends Closeable>
implements DataFetcher<DataT> {

private final Resources resources;
private final ResourceOpener<DataT> resourceOpener;
private final int resourceId;
private @Nullable DataT data;

ResourceDataFetcher(Resources resources, ResourceOpener<DataT> resourceOpener, int resourceId) {
this.resources = resources;
this.resourceOpener = resourceOpener;
this.resourceId = resourceId;
}

@Override
public void loadData(
@NonNull Priority priority, @NonNull DataCallback<? super DataT> callback) {
try {
data = resourceOpener.open(resources, resourceId);
callback.onDataReady(data);
} catch (Resources.NotFoundException e) {
callback.onLoadFailed(e);
}
}

@Override
public void cleanup() {
DataT local = data;
if (local != null) {
try {
local.close();
} catch (IOException e) {
// Ignored.
}
}
}

@Override
public void cancel() {}

@NonNull
@Override
public Class<DataT> getDataClass() {
return resourceOpener.getDataClass();
}

@NonNull
@Override
public DataSource getDataSource() {
return DataSource.LOCAL;
}
}
}

0 comments on commit 04f198e

Please sign in to comment.