Fix AsyncDrawable display when it has placeholder with empty bounds

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Changelog
 
+# 4.2.2-SNAPSHOT
+* Fixed `AsyncDrawable` display when it has placeholder with empty bounds ([#189])
+
+[#189]: https://github.com/noties/Markwon/issues/189
+
 # 4.2.1
 * Fix SpannableBuilder `subSequence` method
 * Introduce Nougat check in `BulletListItemSpan` to position bullet (for bullets to be

gradle.properties

@@ -8,7 +8,7 @@ android.enableJetifier=true
 android.enableBuildCache=true
 android.buildCacheDir=build/pre-dex-cache
 
-VERSION_NAME=4.2.1
+VERSION_NAME=4.2.2-SNAPSHOT
 
 GROUP=io.noties.markwon
 POM_DESCRIPTION=Markwon markdown for Android

markwon-core/src/main/java/io/noties/markwon/image/AsyncDrawable.java

@@ -164,20 +164,44 @@ public class AsyncDrawable extends Drawable {
     @SuppressWarnings("WeakerAccess")
     protected void setPlaceholderResult(@NonNull Drawable placeholder) {
         // okay, if placeholder has bounds -> use it, otherwise use original imageSize
+        // it's important to NOT pass to imageSizeResolver when placeholder has bounds
+        // this is done, so actual result and placeholder can have _different_
+        // bounds. Assume image is loaded with HTML and has ImageSize width=100%,
+        // so, even if placeholder has exact bounds, it will still be scaled up.
+
+        // this condition should not be true for placeholder (at least for now)
+        // (right now this method is always called from constructor)
+        if (result != null) {
+            // but it is, unregister current result
+            result.setCallback(null);
+        }
 
         final Rect rect = placeholder.getBounds();
 
         if (rect.isEmpty()) {
-            // if bounds are empty -> just use placeholder as a regular result
+            // check for intrinsic bounds
-            DrawableUtils.applyIntrinsicBounds(placeholder);
+            final Rect intrinsic = DrawableUtils.intrinsicBounds(placeholder);
+            if (intrinsic.isEmpty()) {
+                // @since 4.2.2.-SNAPSHOT
+                // if intrinsic bounds are empty, use _any_ non-empty bounds,
+                // they must be non-empty so when result is obtained - proper invalidation will occur
+                // (0, 0, 1, 0) is still considered empty
+                placeholder.setBounds(0, 0, 1, 1);
+            } else {
+                // use them
+                placeholder.setBounds(intrinsic);
+            }
+
+            // it is very important (if we have a placeholder) to set own bounds to it (and they must not be empty
+            // otherwise result won't be rendered)
+            // @since 4.2.2-SNAPSHOT
+            setBounds(placeholder.getBounds());
             setResult(placeholder);
+
         } else {
 
-            // this condition should not be true for placeholder (at least for now)
+            // this method is not the same as above, as we do not want to trigger image-size-resolver
-            if (result != null) {
+            // in case when placeholder has exact bounds
-                // but it is, unregister current result
-                result.setCallback(null);
-            }
 
             // placeholder has bounds specified -> use them until we have real result
             this.result = placeholder;

markwon-core/src/test/java/io/noties/markwon/image/AsyncDrawableTest.java

@@ -4,6 +4,7 @@ import android.graphics.Canvas;
 import android.graphics.ColorFilter;
 import android.graphics.Rect;
 import android.graphics.drawable.Drawable;
+
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 
@@ -18,7 +19,12 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 @RunWith(RobolectricTestRunner.class)
 @Config(manifest = Config.NONE)
@@ -78,6 +84,123 @@ public class AsyncDrawableTest {
         assertNotNull(result2.getCallback());
     }
 
+    @Test
+    public void placeholder_no_bounds_no_intrinsic_bounds() {
+        // when there is a placeholder and its
+        // * bounds are empty
+        // * intrinsic bounds are empty
+        // AsyncDrawable.this must have any non-empty bounds (otherwise result won't be rendered,
+        //  due to missing invalidation call)
+
+        final Drawable placeholder = new AbstractDrawable() {
+            @Override
+            public int getIntrinsicWidth() {
+                return 0;
+            }
+
+            @Override
+            public int getIntrinsicHeight() {
+                return 0;
+            }
+        };
+
+        assertTrue(placeholder.getBounds().isEmpty());
+
+        final AsyncDrawableLoader loader = mock(AsyncDrawableLoader.class);
+        when(loader.placeholder(any(AsyncDrawable.class))).thenReturn(placeholder);
+
+        final AsyncDrawable drawable = new AsyncDrawable(
+                "",
+                loader,
+                mock(ImageSizeResolver.class),
+                null
+        );
+
+        final Rect bounds = drawable.getBounds();
+        assertFalse(bounds.toShortString(), bounds.isEmpty());
+        assertEquals(bounds.toShortString(), bounds, placeholder.getBounds());
+    }
+
+    @Test
+    public void placeholder_no_bounds_has_intrinsic() {
+        // placeholder has no bounds, but instead has intrinsic bounds
+
+        final Drawable placeholder = new AbstractDrawable() {
+            @Override
+            public int getIntrinsicWidth() {
+                return 42;
+            }
+
+            @Override
+            public int getIntrinsicHeight() {
+                return 24;
+            }
+        };
+
+        assertTrue(placeholder.getBounds().isEmpty());
+
+        final AsyncDrawableLoader loader = mock(AsyncDrawableLoader.class);
+        when(loader.placeholder(any(AsyncDrawable.class))).thenReturn(placeholder);
+
+        final AsyncDrawable drawable = new AsyncDrawable(
+                "",
+                loader,
+                mock(ImageSizeResolver.class),
+                null
+        );
+
+        final Rect bounds = drawable.getBounds();
+        assertFalse(bounds.isEmpty());
+        assertEquals(0, bounds.left);
+        assertEquals(42, bounds.right);
+        assertEquals(0, bounds.top);
+        assertEquals(24, bounds.bottom);
+
+        assertEquals(bounds, placeholder.getBounds());
+    }
+
+    @Test
+    public void placeholder_has_bounds() {
+
+        final Rect rect = new Rect(0, 0, 12, 99);
+        final Drawable placeholder = mock(Drawable.class);
+        when(placeholder.getBounds()).thenReturn(rect);
+
+        assertFalse(rect.isEmpty());
+
+        final AsyncDrawableLoader loader = mock(AsyncDrawableLoader.class);
+        when(loader.placeholder(any(AsyncDrawable.class))).thenReturn(placeholder);
+
+        final AsyncDrawable drawable = new AsyncDrawable(
+                "",
+                loader,
+                mock(ImageSizeResolver.class),
+                null
+        );
+
+        final Rect bounds = drawable.getBounds();
+        assertEquals(rect, bounds);
+
+        verify(placeholder, times(1)).getBounds();
+        verify(placeholder, never()).getIntrinsicWidth();
+        verify(placeholder, never()).getIntrinsicHeight();
+        verify(placeholder, never()).setBounds(any(Rect.class));
+    }
+
+    @Test
+    public void no_placeholder_empty_bounds() {
+        // when AsyncDrawable has no placeholder, then its bounds must be empty at the start
+
+        final AsyncDrawable drawable = new AsyncDrawable(
+                "",
+                mock(AsyncDrawableLoader.class),
+                mock(ImageSizeResolver.class),
+                null
+        );
+
+        assertTrue(drawable.getBounds().isEmpty());
+    }
+
     private static class AbstractDrawable extends Drawable {
 
         @Override

sample/src/main/java/io/noties/markwon/sample/recycler/RecyclerActivity.java

@@ -2,6 +2,8 @@ package io.noties.markwon.sample.recycler;
 
 import android.app.Activity;
 import android.content.Context;
+import android.graphics.drawable.ColorDrawable;
+import android.graphics.drawable.Drawable;
 import android.net.Uri;
 import android.os.Bundle;
 import android.text.TextUtils;
@@ -27,7 +29,12 @@ import io.noties.markwon.MarkwonConfiguration;
 import io.noties.markwon.MarkwonVisitor;
 import io.noties.markwon.core.CorePlugin;
 import io.noties.markwon.html.HtmlPlugin;
+import io.noties.markwon.image.AsyncDrawable;
+import io.noties.markwon.image.ImagesPlugin;
+import io.noties.markwon.image.file.FileSchemeHandler;
+import io.noties.markwon.image.network.OkHttpNetworkSchemeHandler;
 import io.noties.markwon.image.picasso.PicassoImagesPlugin;
+import io.noties.markwon.image.svg.SvgMediaDecoder;
 import io.noties.markwon.recycler.MarkwonAdapter;
 import io.noties.markwon.recycler.SimpleEntry;
 import io.noties.markwon.recycler.table.TableEntry;
@@ -74,13 +81,14 @@ public class RecyclerActivity extends Activity {
     private static Markwon markwon(@NonNull Context context) {
         return Markwon.builder(context)
                 .usePlugin(CorePlugin.create())
-//                .usePlugin(ImagesPlugin.create(plugin -> {
+                .usePlugin(ImagesPlugin.create(plugin -> {
-//                    plugin
+                    plugin
-//                            .addSchemeHandler(FileSchemeHandler.createWithAssets(context))
+                            .addSchemeHandler(FileSchemeHandler.createWithAssets(context))
-//                            .addSchemeHandler(OkHttpNetworkSchemeHandler.create())
+                            .addSchemeHandler(OkHttpNetworkSchemeHandler.create())
-//                            .addMediaDecoder(SvgMediaDecoder.create());
+                            .addMediaDecoder(SvgMediaDecoder.create())
-//                }))
+                            .placeholderProvider(drawable -> new ColorDrawable(0xFFff0000));
-                .usePlugin(PicassoImagesPlugin.create(context))
+                }))
+//                .usePlugin(PicassoImagesPlugin.create(context))
 //                .usePlugin(GlideImagesPlugin.create(context))
 //                .usePlugin(CoilImagesPlugin.create(context))
                 // important to use TableEntryPlugin instead of TablePlugin