Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix vector/shape drawables not loading with DirectResourceLoader. #4999

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix vector/shape drawables not loading with DirectResourceLoader. #4999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions instrumentation/src/androidTest/java/com/bumptech/glide/DarkModeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import android.content.Context;
import android.content.res.Resources;
import android.graphics.Bitmap;
import android.graphics.Canvas;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Structure Issue: The import 'android.graphics.Canvas' is added but not used in any of the modified code. It's good practice to remove unused imports to keep the codebase clean.
Fix: Remove the unused import to clean up the code.
Code Suggestion:

-import android.graphics.Canvas;

import android.graphics.drawable.BitmapDrawable;
import android.graphics.drawable.Drawable;
import android.net.Uri;
Expand Down Expand Up @@ -51,6 +52,45 @@ public void before() {
assumeTrue(VERSION.SDK_INT >= VERSION_CODES.Q);
}

@Test
public void load_withDarkModeActivity_vectorDrawable_usesDarkModeColor() {
try (ActivityScenario<FragmentActivity> scenario = darkModeActivity()) {
scenario.onActivity(
activity -> {
ViewGroup container = findContainer(activity);
ImageView imageView = newFixedSizeImageView(activity);
container.addView(imageView);

Glide.with(activity)
.load(R.drawable.vector_drawable)
.override(Target.SIZE_ORIGINAL)
.into(imageView);
});

onIdle();
scenario.onActivity(
activity -> {
ViewGroup container = findContainer(activity);
ImageView imageView = (ImageView) container.getChildAt(0);
Bitmap result = drawToBitmap(imageView.getDrawable());
Bitmap expected = drawToBitmap(activity.getDrawable(R.drawable.vector_drawable_dark));
assertThat(result).sameAs(expected);
});
}
}
Comment on lines +55 to +80
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimization Issue: The method 'load_withDarkModeActivity_vectorDrawable_usesDarkModeColor' performs a drawable to bitmap conversion inside the activity's onActivity method. This process is not optimized for performance. Converting a drawable to a bitmap is a CPU-intensive operation and doing it on the main thread can lead to frame drops, especially if the drawable is complex or the device is low-end.
Fix: To optimize this process, consider moving the drawable to bitmap conversion into a background thread or using Glide's built-in asynchronous loading and decoding capabilities. Glide already has efficient mechanisms for loading and decoding images in the background, which can significantly improve the performance of image loading and rendering.
Code Suggestion:

+  new Thread(() -> {
+    Bitmap result = drawToBitmap(imageView.getDrawable());
+    Bitmap expected = drawToBitmap(activity.getDrawable(R.drawable.vector_drawable_dark));
+    runOnUiThread(() -> assertThat(result).sameAs(expected));
+  }).start();


private static Bitmap drawToBitmap(Drawable drawable) {
int width = drawable.getIntrinsicWidth();
int height = drawable.getIntrinsicHeight();

Bitmap result = Bitmap.createBitmap(width, height, Bitmap.Config.ARGB_8888);
Canvas canvas = new Canvas(result);
drawable.setBounds(0, 0, width, height);
drawable.draw(canvas);
canvas.setBitmap(null);
return result;
}
Comment on lines +55 to +92
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Consider breaking down the test method load_withDarkModeActivity_vectorDrawable_usesDarkModeColor into smaller, more focused test cases. This can improve test readability and make it easier to identify specific points of failure.
Code Suggestion:

+ // Suggestion to split into smaller tests if applicable
+ @Test public void loadVectorDrawable_inDarkModeActivity_checksDarkModeColor() {

Comment on lines +82 to +92
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance Issue: The method 'drawToBitmap' creates a new Bitmap and Canvas every time it's called. This can be highly inefficient and memory-intensive, especially for large numbers of drawables or high-resolution images. Reusing bitmaps or using a bitmap pool could significantly improve memory usage and performance.
Fix: Consider using a Bitmap pool to reuse Bitmap objects rather than creating a new one each time. Glide provides a BitmapPool interface for this purpose. Alternatively, if the drawable sizes are known and fixed, preallocate a single Bitmap and Canvas and reuse them for each drawable.
Code Suggestion:

+  private static Bitmap reusableBitmap;
+  private static Canvas reusableCanvas;

   private static Bitmap drawToBitmap(Drawable drawable) {
     int width = drawable.getIntrinsicWidth();
     int height = drawable.getIntrinsicHeight();

+    if (reusableBitmap == null || reusableBitmap.getWidth() != width || reusableBitmap.getHeight() != height) {
+      reusableBitmap = Bitmap.createBitmap(width, height, Bitmap.Config.ARGB_8888);
+      reusableCanvas = new Canvas(reusableBitmap);
+    }
+    reusableCanvas.setBitmap(reusableBitmap);
     drawable.setBounds(0, 0, width, height);
     drawable.draw(reusableCanvas);
+    reusableCanvas.setBitmap(null);
     return reusableBitmap;
   }


@Test
public void load_withDarkModeActivity_useDarkModeDrawable() {
runActivityTest(
Expand Down
18 changes: 14 additions & 4 deletions instrumentation/src/androidTest/java/com/bumptech/glide/NonBitmapDrawableResourcesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@
import android.graphics.drawable.Drawable;
import android.net.Uri;
import androidx.test.core.app.ApplicationProvider;
import androidx.test.ext.junit.runners.AndroidJUnit4;
import com.bumptech.glide.load.resource.bitmap.RoundedCorners;
import com.bumptech.glide.test.ResourceIds;
import com.bumptech.glide.testutil.TearDownGlide;
Comment on lines 20 to 22
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Structure Issue: The import 'com.google.common.collect.ImmutableList' is added, which introduces a new dependency. Ensure this dependency is necessary and properly documented.
Fix: If 'ImmutableList' is essential for the test, keep the import and document its usage. Otherwise, consider using native Java or Android SDK alternatives.
Code Suggestion:

-import com.google.common.collect.ImmutableList;
+// Consider using native Java or Android SDK alternatives

import com.google.common.collect.ImmutableList;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
Expand All @@ -31,19 +31,29 @@
import org.junit.function.ThrowingRunnable;
import org.junit.rules.TestName;
import org.junit.runner.RunWith;
import org.mockito.MockitoAnnotations;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameter;
import org.junit.runners.Parameterized.Parameters;

@RunWith(AndroidJUnit4.class)
@RunWith(Parameterized.class)
public class NonBitmapDrawableResourcesTest {
@Rule public final TestName testName = new TestName();
@Rule public final TearDownGlide tearDownGlide = new TearDownGlide();

@Parameter public boolean useDirectResourceLoader;

@Parameters(name = "useDirectResourceLoader = {0}")
public static ImmutableList<Boolean> parameters() {
return ImmutableList.of(true, false);
}
Comment on lines +43 to +48
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Ensure that the parameterized test setup with useDirectResourceLoader is thoroughly validated across different test scenarios to confirm that both true and false states behave as expected without unintended side effects.
Code Suggestion:

+ // Additional validation or setup might be required for comprehensive testing


private Context context;

@Before
public void setUp() {
MockitoAnnotations.initMocks(this);
context = ApplicationProvider.getApplicationContext();

Glide.init(context, new GlideBuilder().useDirectResourceLoader(useDirectResourceLoader));
}

@Test
Expand Down
4 changes: 3 additions & 1 deletion instrumentation/src/main/AndroidManifest.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
package="com.bumptech.glide.instrumentation">
<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" />
<uses-permission android:name="android.permission.SYSTEM_ALERT_WINDOW" />
<application tools:ignore="MissingApplicationIcon" >
<application
tools:ignore="MissingApplicationIcon"
android:theme="@style/AppTheme">
Comment on lines +6 to +8
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Structure Issue: The 'android:theme' attribute is added to the 'application' tag, potentially overriding themes set in individual activities or fragments.
Fix: Ensure that setting a global theme at the application level does not unintentionally override local themes set on activities or fragments.
Code Suggestion:

<application
    tools:ignore="MissingApplicationIcon"
    android:theme="@style/AppTheme">
    <!-- Ensure individual activities or fragments can override this theme if needed -->
</application>

<activity
android:name="com.bumptech.glide.test.GlideWithBeforeSuperOnCreateActivity"
android:exported="false" />
Expand Down
2 changes: 1 addition & 1 deletion instrumentation/src/main/res/drawable/vector_drawable.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
android:viewportHeight="24.0"
android:viewportWidth="24.0">
<path
android:fillColor="#f9b840"
android:fillColor="?attr/colorControlNormal"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Ensure that using "?attr/colorControlNormal" for the fillColor in vector_drawable.xml aligns with the intended color theme across different app states and Android versions.
Code Suggestion:

- android:fillColor="?attr/colorControlNormal"
+ android:fillColor="@color/specificColor" // Ensure this color aligns with your color theme.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimization Issue: The fillColor attribute of the vector drawable uses a hard-coded color value. This approach is not theme-friendly and can lead to issues with dark mode or when trying to change the app's theme dynamically.
Fix: Use theme attributes or color resources to reference colors in vector drawables. This change will make it easier to support different themes, including dark mode, by allowing the color to change based on the current theme.
Code Suggestion:

-    android:fillColor="#f9b840"
+    android:fillColor="?attr/colorControlNormal"

android:pathData="M15.5,5.5c1.1,0 2,-0.9 2,-2s-0.9,-2 -2,-2 -2,0.9
-2,2 0.9,2 2,2zM5,12c-2.8,0 -5,2.2 -5,5s2.2,5 5,5 5,-2.2 5,-5 -2.2,
-5 -5,-5zM5,20.5c-1.9,0 -3.5,-1.6 -3.5,-3.5s1.6,-3.5 3.5,-3.5 3.5,
Expand Down
18 changes: 18 additions & 0 deletions instrumentation/src/main/res/drawable/vector_drawable_dark.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?xml version="1.0" encoding="utf-8"?>
<vector xmlns:android="http://schemas.android.com/apk/res/android"
android:width="64dp"
android:height="64dp"
android:viewportHeight="24.0"
android:viewportWidth="24.0">
<path
android:fillColor="@color/colorPrimaryDark"
android:pathData="M15.5,5.5c1.1,0 2,-0.9 2,-2s-0.9,-2 -2,-2 -2,0.9
-2,2 0.9,2 2,2zM5,12c-2.8,0 -5,2.2 -5,5s2.2,5 5,5 5,-2.2 5,-5 -2.2,
-5 -5,-5zM5,20.5c-1.9,0 -3.5,-1.6 -3.5,-3.5s1.6,-3.5 3.5,-3.5 3.5,
1.6 3.5,3.5 -1.6,3.5 -3.5,3.5zM10.8,10.5l2.4,-2.4 0.8,0.8c1.3,1.3
3,2.1 5.1,2.1L19.1,9c-1.5,0 -2.7,-0.6 -3.6,-1.5l-1.9,-1.9c-0.5,-0.4
-1,-0.6 -1.6,-0.6s-1.1,0.2 -1.4,0.6L7.8,8.4c-0.4,0.4 -0.6,0.9 -0.6,
1.4 0,0.6 0.2,1.1 0.6,1.4L11,14v5h2v-6.2l-2.2,-2.3zM19,12c-2.8,0 -5,
2.2 -5,5s2.2,5 5,5 5,-2.2 5,-5 -2.2,-5 -5,-5zM19,20.5c-1.9,0 -3.5,-1.6
-3.5,-3.5s1.6,-3.5 3.5,-3.5 3.5,1.6 3.5,3.5 -1.6,3.5 -3.5,3.5z" />
</vector>
18 changes: 18 additions & 0 deletions instrumentation/src/main/res/drawable/vector_drawable_light.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?xml version="1.0" encoding="utf-8"?>
<vector xmlns:android="http://schemas.android.com/apk/res/android"
android:width="64dp"
android:height="64dp"
android:viewportHeight="24.0"
android:viewportWidth="24.0">
<path
android:fillColor="@color/colorPrimary"
android:pathData="M15.5,5.5c1.1,0 2,-0.9 2,-2s-0.9,-2 -2,-2 -2,0.9
-2,2 0.9,2 2,2zM5,12c-2.8,0 -5,2.2 -5,5s2.2,5 5,5 5,-2.2 5,-5 -2.2,
-5 -5,-5zM5,20.5c-1.9,0 -3.5,-1.6 -3.5,-3.5s1.6,-3.5 3.5,-3.5 3.5,
1.6 3.5,3.5 -1.6,3.5 -3.5,3.5zM10.8,10.5l2.4,-2.4 0.8,0.8c1.3,1.3
3,2.1 5.1,2.1L19.1,9c-1.5,0 -2.7,-0.6 -3.6,-1.5l-1.9,-1.9c-0.5,-0.4
-1,-0.6 -1.6,-0.6s-1.1,0.2 -1.4,0.6L7.8,8.4c-0.4,0.4 -0.6,0.9 -0.6,
1.4 0,0.6 0.2,1.1 0.6,1.4L11,14v5h2v-6.2l-2.2,-2.3zM19,12c-2.8,0 -5,
2.2 -5,5s2.2,5 5,5 5,-2.2 5,-5 -2.2,-5 -5,-5zM19,20.5c-1.9,0 -3.5,-1.6
-3.5,-3.5s1.6,-3.5 3.5,-3.5 3.5,1.6 3.5,3.5 -1.6,3.5 -3.5,3.5z" />
</vector>
6 changes: 6 additions & 0 deletions instrumentation/src/main/res/values/colors.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<?xml version="1.0" encoding="utf-8"?>

<resources>
<color name="colorPrimary">#f9b840</color>
<color name="colorPrimaryDark">#ffffff</color>
</resources>
8 changes: 8 additions & 0 deletions instrumentation/src/main/res/values/styles.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?xml version="1.0" encoding="utf-8"?>
<resources>
<style name="AppTheme" parent="Theme.AppCompat.DayNight.DarkActionBar">
<!-- Customize your theme here. -->
<item name="colorPrimary">@color/colorPrimary</item>
<item name="colorPrimaryDark">@color/colorPrimaryDark</item>
</style>
</resources>
15 changes: 10 additions & 5 deletions library/src/main/java/com/bumptech/glide/RegistryFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,11 +279,15 @@ Uri.class, Bitmap.class, new ResourceBitmapDecoder(resourceDrawableDecoder, bitm
DirectResourceLoader.inputStreamFactory(context);
ModelLoaderFactory<Integer, AssetFileDescriptor> assetFileDescriptorFactory =
DirectResourceLoader.assetFileDescriptorFactory(context);
ModelLoaderFactory<Integer, Drawable> drawableFactory =
DirectResourceLoader.drawableFactory(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)
.append(int.class, Drawable.class, drawableFactory)
.append(Integer.class, Drawable.class, drawableFactory)
Comment on lines +282 to +290
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Validate the integration of the new drawableFactory with the existing registry setup. Ensure that it does not introduce conflicts with other registered loaders, especially in scenarios involving resource and drawable loading.
Code Suggestion:

+ // Ensure compatibility and no conflicts with existing loaders

.append(Uri.class, InputStream.class, ResourceUriLoader.newStreamFactory(context))
Comment on lines 279 to 291
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scalability Issue: The addition of a new drawable factory directly in the RegistryFactory without considering a pluggable or extensible approach for handling various resource types might limit the scalability of the system. As new resource types or special handling cases are introduced, this class may become bloated and harder to maintain, violating the open/closed principle.
Fix: Consider implementing a more flexible system for registering and handling different types of resources. This could involve creating a registry or manager class that allows for the dynamic addition of resource handlers or factories. This way, as new resource types are needed, they can be added without modifying the core classes, thus improving the scalability and maintainability of the system.
Code Suggestion:

+      ModelLoaderFactory<Integer, Drawable> drawableFactory =
+          DirectResourceLoader.drawableFactory(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)
+          .append(int.class, Drawable.class, drawableFactory)
+          .append(Integer.class, Drawable.class, drawableFactory)
           .append(Uri.class, InputStream.class, ResourceUriLoader.newStreamFactory(context))
           .append(
               Uri.class,

.append(
Uri.class,
Expand All @@ -292,25 +296,26 @@ Uri.class, Bitmap.class, new ResourceBitmapDecoder(resourceDrawableDecoder, bitm
} 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(int.class, ParcelFileDescriptor.class, resourceLoaderFileDescriptorFactory)
.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);
Integer.class, AssetFileDescriptor.class, resourceLoaderAssetFileDescriptorFactory);
}

// Handles resources from other applications or converting Drawable resource types to Bitmaps.
ResourceLoader.UriFactory resourceLoaderUriFactory = new ResourceLoader.UriFactory(resources);
registry
.append(Integer.class, Uri.class, resourceLoaderUriFactory)
.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
65 changes: 59 additions & 6 deletions library/src/main/java/com/bumptech/glide/load/model/DirectResourceLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import android.content.res.AssetFileDescriptor;
import android.content.res.Resources;
import android.content.res.Resources.Theme;
import android.graphics.drawable.Drawable;
import android.os.Build;
import android.os.Build.VERSION_CODES;
import androidx.annotation.NonNull;
Expand All @@ -12,9 +13,9 @@
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.DrawableDecoderCompat;
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;

Expand All @@ -26,8 +27,7 @@
* @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> {
public final class DirectResourceLoader<DataT> implements ModelLoader<Integer, DataT> {

private final Context context;
private final ResourceOpener<DataT> resourceOpener;
Expand All @@ -41,6 +41,10 @@ public static ModelLoaderFactory<Integer, AssetFileDescriptor> assetFileDescript
return new AssetFileDescriptorFactory(context);
}

public static ModelLoaderFactory<Integer, Drawable> drawableFactory(Context context) {
return new DrawableFactory(context);
}

DirectResourceLoader(Context context, ResourceOpener<DataT> resourceOpener) {
this.context = context.getApplicationContext();
this.resourceOpener = resourceOpener;
Expand Down Expand Up @@ -71,6 +75,8 @@ public boolean handles(@NonNull Integer integer) {
private interface ResourceOpener<DataT> {
DataT open(Resources resources, int resourceId);

void close(DataT data) throws IOException;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security Issue: The method 'void close(DataT data) throws IOException' in 'DirectResourceLoader.java' lacks input validation, which could lead to resource leaks or denial of service if the method is called with invalid or unexpected data. This is particularly relevant because the method is designed to close resources, and improper handling could leave resources open longer than intended or cause exceptions that disrupt normal operation.
Fix: Implement input validation within the 'close' method to ensure that the provided data is not null and meets any expected format or criteria before attempting to close it. This could involve checking for null values and potentially validating the state of the resource to ensure it is in a closable state.
Code Suggestion:

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

  private final Resources resources;
  private final ResourceOpener<DataT> resourceOpener;

  @Override
  public void cleanup() {
    DataT local = data;
    if (local != null) {
      try {
        if(local != null) {
          // Input validation to ensure data is not null and meets expected format
          resourceOpener.close(local);
        }
      } catch (IOException e) {
        // Ignored.
      }
    }
  }
}


Class<DataT> getDataClass();
}

Expand All @@ -89,6 +95,11 @@ public AssetFileDescriptor open(Resources resources, int resourceId) {
return resources.openRawResourceFd(resourceId);
}

@Override
public void close(AssetFileDescriptor data) throws IOException {
data.close();
}

@Override
public Class<AssetFileDescriptor> getDataClass() {
return AssetFileDescriptor.class;
Expand Down Expand Up @@ -125,6 +136,11 @@ public InputStream open(Resources resources, int resourceId) {
return resources.openRawResource(resourceId);
}

@Override
public void close(InputStream data) throws IOException {
data.close();
}

@Override
public Class<InputStream> getDataClass() {
return InputStream.class;
Expand All @@ -134,8 +150,45 @@ public Class<InputStream> getDataClass() {
public void teardown() {}
}

private static final class ResourceDataFetcher<DataT extends Closeable>
implements DataFetcher<DataT> {
/**
* Handles vectors, shapes and other resources that cannot be opened with
* Resources.openRawResource. Overlaps in functionality with {@link ResourceDrawableDecoder} and
* {@link com.bumptech.glide.load.resource.bitmap.ResourceBitmapDecoder} but it's more efficient
* for simple resource loads within a single application.
*/
private static final class DrawableFactory
implements ModelLoaderFactory<Integer, Drawable>, ResourceOpener<Drawable> {

private final Context context;

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

@Override
public Drawable open(Resources resources, int resourceId) {
return DrawableDecoderCompat.getDrawable(context, resourceId, resources.newTheme());
}

@Override
public void close(Drawable data) throws IOException {}

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

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

@Override
public void teardown() {}
Comment on lines +159 to +188
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Review the necessity of the empty close method in DrawableFactory. If Drawable resources do not require specific cleanup, document the rationale clearly to avoid confusion.
Code Suggestion:

+ // No specific cleanup required for Drawable resources
+ // This method is intentionally left blank

Comment on lines 150 to +188
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Consider enhancing the DrawableFactory with additional logging or error handling, especially in the open method, to handle potential issues when loading drawables.
Code Suggestion:

+ try {
+   return DrawableDecoderCompat.getDrawable(context, resourceId, resources.newTheme());
+ } catch (Exception e) {
+   Log.e(TAG, "Failed to load drawable", e);
+   return null;
+ }

}

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

private final Resources resources;
private final ResourceOpener<DataT> resourceOpener;
Expand Down Expand Up @@ -164,7 +217,7 @@ public void cleanup() {
DataT local = data;
if (local != null) {
try {
local.close();
resourceOpener.close(local);
} catch (IOException e) {
// Ignored.
}
Expand Down