Fix vector/shape drawables not loading with DirectResourceLoader. #4999
Conversation
copybara-service
bot
force-pushed
the
test_500212780
branch
2 times, most recently
from
2f1b8d9
to
250ac36
Compare
PiperOrigin-RevId: 500212780
copybara-service
bot
force-pushed
the
test_500212780
branch
from
250ac36
to
dcd1c08
Compare
Code Review Agent Run Status
Code Review Overview
>>See detailed code suggestions<< High-level FeedbackGeneral feedback for improvement includes ensuring consistent code documentation across new methods and tests, verifying the backward compatibility and potential impact on existing functionalities, especially with the changes in resource handling, and considering the implications of the changes in AndroidManifest and styles on the overall application behavior in different Android versions. |
| @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); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| 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; | ||
| } |
There was a problem hiding this comment.
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() {
| @@ -5,7 +5,7 @@ | |||
| android:viewportHeight="24.0" | |||
| android:viewportWidth="24.0"> | |||
| <path | |||
| android:fillColor="#f9b840" | |||
| android:fillColor="?attr/colorControlNormal" | |||
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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
| 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() {} |
There was a problem hiding this comment.
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
| 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() {} |
There was a problem hiding this comment.
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;
+ }
| @@ -5,7 +5,7 @@ | |||
| android:viewportHeight="24.0" | |||
| android:viewportWidth="24.0"> | |||
| <path | |||
| android:fillColor="#f9b840" | |||
| android:fillColor="?attr/colorControlNormal" | |||
There was a problem hiding this comment.
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"
| @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); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
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();
| @@ -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; | |||
There was a problem hiding this comment.
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.
}
}
}
}
| 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) | ||
| .append(Uri.class, InputStream.class, ResourceUriLoader.newStreamFactory(context)) |
There was a problem hiding this comment.
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,
| 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; | ||
| } |
There was a problem hiding this comment.
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;
}
Fix vector/shape drawables not loading with DirectResourceLoader.