Merge pull request #200 from noties/v4.2.2

V4.2.2

CHANGELOG.md

@@ -1,5 +1,14 @@
 # Changelog
 
+# 4.2.2
+* Fixed `AsyncDrawable` display when it has placeholder with empty bounds ([#189])
+* Fixed `syntax-highlight` where code input is empty string ([#192])
+* Add `appendFactory`/`prependFactory` in `MarkwonSpansFactory.Builder` for more explicit `SpanFactory` ordering ([#193])
+
+[#189]: https://github.com/noties/Markwon/issues/189
+[#192]: https://github.com/noties/Markwon/issues/192
+[#193]: https://github.com/noties/Markwon/issues/193
+
 # 4.2.1
 * Fix SpannableBuilder `subSequence` method
 * Introduce Nougat check in `BulletListItemSpan` to position bullet (for bullets to be

docs/docs/v4/core/spans-factory.md

@@ -62,7 +62,12 @@ builder.setFactory(Link.class, new SpanFactory() {
 
 ---
 
-Since <Badge text="3.0.1" /> you can _add_ multiple `SpanFactory` for a single node:
+:::warning
+Deprecated in <Badge text="4.2.2" type="error" vertical="middle" />. Use `appendFactory` or `prependFactory` for
+more explicit factories ordering. `addFactories` behaves like new `prependFactory` method.
+:::
+
+Since <Badge text="3.0.1" /><Badge text="4.2.2" type="error" /> you can _add_ multiple `SpanFactory` for a single node:
 
 ```java
 final Markwon markwon = Markwon.builder(context)
@@ -81,6 +86,22 @@ final Markwon markwon = Markwon.builder(context)
         .build();
 ```
 
+## appendFactory/prependFactory <Badge text="4.2.2" />
+
+* use `appendFactory` if you wish to add a factory **after** original (can be used to post-process original factory)
+* use `prependFactory` if you wish to add a factory **before** original (original factory will be applied after this one)
+
+```java
+@Override
+public void configureSpansFactory(@NonNull MarkwonSpansFactory.Builder builder) {
+    // `RemoveUnderlineSpan` will be added AFTER original, thus it will remove underline applied by original
+    builder.appendFactory(Link.class, (configuration, props) -> new RemoveUnderlineSpan());
+
+    // will be added BEFORE origin (origin can override this)
+    builder.prependFactory(Link.class, (configuration, props) -> new AbsoluteSizeSpan(48, true));
+}
+```
+
 ---
 
 If you wish to inspect existing factory you can use:

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
 
 GROUP=io.noties.markwon
 POM_DESCRIPTION=Markwon markdown for Android

markwon-core/src/main/java/io/noties/markwon/MarkwonSpansFactory.java

@@ -39,10 +39,34 @@ public interface MarkwonSpansFactory {
          * {@link SpanFactory} with the specified one.
          *
          * @since 3.0.1
+         * @deprecated 4.2.2 consider using {@link #appendFactory(Class, SpanFactory)} or
+         * {@link #prependFactory(Class, SpanFactory)} methods for more explicit factory ordering.
+         * `addFactory` behaved like {@link #prependFactory(Class, SpanFactory)}, so
+         * this method call can be replaced with it
          */
         @NonNull
+        @Deprecated
         <N extends Node> Builder addFactory(@NonNull Class<N> node, @NonNull SpanFactory factory);
 
+        /**
+         * Append a factory to existing one (or make the first one for specified node). Specified factory
+         * will be called <strong>after</strong> original (if present) factory. Can be used to
+         * <em>change</em> behavior or original span factory.
+         *
+         * @since 4.2.2
+         */
+        @NonNull
+        <N extends Node> Builder appendFactory(@NonNull Class<N> node, @NonNull SpanFactory factory);
+
+        /**
+         * Prepend a factory to existing one (or make the first one for specified node). Specified factory
+         * will be called <string>before</string> original (if present) factory.
+         *
+         * @since 4.2.2
+         */
+        @NonNull
+        <N extends Node> Builder prependFactory(@NonNull Class<N> node, @NonNull SpanFactory factory);
+
         /**
          * Can be useful when <em>enhancing</em> an already defined SpanFactory with another one.
          */

markwon-core/src/main/java/io/noties/markwon/MarkwonSpansFactoryImpl.java

@@ -56,7 +56,32 @@ class MarkwonSpansFactoryImpl implements MarkwonSpansFactory {
 
         @NonNull
         @Override
+        @Deprecated
         public <N extends Node> Builder addFactory(@NonNull Class<N> node, @NonNull SpanFactory factory) {
+            return prependFactory(node, factory);
+        }
+
+        @NonNull
+        @Override
+        public <N extends Node> Builder appendFactory(@NonNull Class<N> node, @NonNull SpanFactory factory) {
+            final SpanFactory existing = factories.get(node);
+            if (existing == null) {
+                factories.put(node, factory);
+            } else {
+                if (existing instanceof CompositeSpanFactory) {
+                    ((CompositeSpanFactory) existing).factories.add(0, factory);
+                } else {
+                    final CompositeSpanFactory compositeSpanFactory =
+                            new CompositeSpanFactory(factory, existing);
+                    factories.put(node, compositeSpanFactory);
+                }
+            }
+            return this;
+        }
+
+        @NonNull
+        @Override
+        public <N extends Node> Builder prependFactory(@NonNull Class<N> node, @NonNull SpanFactory factory) {
             // if there is no factory registered for this node -> just add it
             final SpanFactory existing = factories.get(node);
             if (existing == null) {

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

@@ -164,21 +164,45 @@ public class AsyncDrawable extends Drawable {
     @SuppressWarnings("WeakerAccess")
     protected void setPlaceholderResult(@NonNull Drawable placeholder) {
         // okay, if placeholder has bounds -> use it, otherwise use original imageSize
-
-        final Rect rect = placeholder.getBounds();
-
-        if (rect.isEmpty()) {
-            // if bounds are empty -> just use placeholder as a regular result
-            DrawableUtils.applyIntrinsicBounds(placeholder);
-            setResult(placeholder);
-        } else {
+        // 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()) {
+            // check for intrinsic bounds
+            final Rect intrinsic = DrawableUtils.intrinsicBounds(placeholder);
+            if (intrinsic.isEmpty()) {
+                // @since 4.2.2
+                // 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
+            setBounds(placeholder.getBounds());
+            setResult(placeholder);
+
+        } else {
+
+            // this method is not the same as above, as we do not want to trigger image-size-resolver
+            // in case when placeholder has exact bounds
+
             // placeholder has bounds specified -> use them until we have real result
             this.result = placeholder;
             this.result.setCallback(callback);

markwon-core/src/test/java/io/noties/markwon/MarkwonSpansFactoryImplTest.java

@@ -113,6 +113,7 @@ public class MarkwonSpansFactoryImplTest {
     }
 
     @Test
+    @Deprecated
     public void builder_add_factory() {
         // here is what we should validate:
         // * if we call addFactory and there is none already -> supplied factory
@@ -146,4 +147,74 @@ public class MarkwonSpansFactoryImplTest {
         assertEquals(compositeSpanFactory, builder.getFactory(node));
         assertEquals(Arrays.asList(first, second, third), compositeSpanFactory.factories);
     }
+
+    @Test
+    public void builder_prepend_factory() {
+        // here is what we should validate:
+        // * if we call prependFactory and there is none already -> supplied factory
+        // * if there is
+        // * * if not composite -> make composite
+        // * * if composite -> add to it
+
+        final MarkwonSpansFactoryImpl.BuilderImpl builder = new MarkwonSpansFactoryImpl.BuilderImpl();
+
+        final SpanFactory first = mock(SpanFactory.class);
+        final SpanFactory second = mock(SpanFactory.class);
+        final SpanFactory third = mock(SpanFactory.class);
+
+        final Class<Node> node = Node.class;
+
+        // assert none yet
+        assertNull(builder.getFactory(node));
+
+        // add first, none yet -> it should be added without modifications
+        builder.prependFactory(node, first);
+        assertEquals(first, builder.getFactory(node));
+
+        // add second -> composite factory will be created
+        builder.prependFactory(node, second);
+        final MarkwonSpansFactoryImpl.CompositeSpanFactory compositeSpanFactory =
+                (MarkwonSpansFactoryImpl.CompositeSpanFactory) builder.getFactory(node);
+        assertNotNull(compositeSpanFactory);
+        assertEquals(Arrays.asList(first, second), compositeSpanFactory.factories);
+
+        builder.prependFactory(node, third);
+        assertEquals(compositeSpanFactory, builder.getFactory(node));
+        assertEquals(Arrays.asList(first, second, third), compositeSpanFactory.factories);
+    }
+
+    @Test
+    public void builder_append_factory() {
+        // here is what we should validate:
+        // * if we call appendFactory and there is none already -> supplied factory
+        // * if there is
+        // * * if not composite -> make composite
+        // * * if composite -> add to it
+
+        final MarkwonSpansFactoryImpl.BuilderImpl builder = new MarkwonSpansFactoryImpl.BuilderImpl();
+
+        final SpanFactory first = mock(SpanFactory.class);
+        final SpanFactory second = mock(SpanFactory.class);
+        final SpanFactory third = mock(SpanFactory.class);
+
+        final Class<Node> node = Node.class;
+
+        // assert none yet
+        assertNull(builder.getFactory(node));
+
+        // add first, none yet -> it should be added without modifications
+        builder.appendFactory(node, first);
+        assertEquals(first, builder.getFactory(node));
+
+        // add second -> composite factory will be created
+        builder.appendFactory(node, second);
+        final MarkwonSpansFactoryImpl.CompositeSpanFactory compositeSpanFactory =
+                (MarkwonSpansFactoryImpl.CompositeSpanFactory) builder.getFactory(node);
+        assertNotNull(compositeSpanFactory);
+        assertEquals(Arrays.asList(second, first), compositeSpanFactory.factories);
+
+        builder.appendFactory(node, third);
+        assertEquals(compositeSpanFactory, builder.getFactory(node));
+        assertEquals(Arrays.asList(third, second, first), compositeSpanFactory.factories);
+    }
 }

markwon-core/src/test/java/io/noties/markwon/core/CorePluginTest.java

@@ -166,6 +166,18 @@ public class CorePluginTest {
                 throw new RuntimeException();
             }
 
+            @NonNull
+            @Override
+            public <N extends Node> MarkwonSpansFactory.Builder appendFactory(@NonNull Class<N> node, @NonNull SpanFactory factory) {
+                throw new RuntimeException();
+            }
+
+            @NonNull
+            @Override
+            public <N extends Node> MarkwonSpansFactory.Builder prependFactory(@NonNull Class<N> node, @NonNull SpanFactory factory) {
+                throw new RuntimeException();
+            }
+
             @Nullable
             @Override
             public <N extends Node> SpanFactory getFactory(@NonNull Class<N> node) {

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

markwon-syntax-highlight/src/main/java/io/noties/markwon/syntax/Prism4jSyntaxHighlight.java

@@ -41,6 +41,13 @@ public class Prism4jSyntaxHighlight implements SyntaxHighlight {
     @NonNull
     @Override
     public CharSequence highlight(@Nullable String info, @NonNull String code) {
+
+        // @since 4.2.2
+        // although not null, but still is empty
+        if (code.isEmpty()) {
+            return code;
+        }
+
         // if info is null, do not highlight -> LICENCE footer very commonly wrapped inside code
         // block without syntax name specified (so, do not highlight)
         return info == null

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

@@ -2,9 +2,13 @@ package io.noties.markwon.sample.recycler;
 
 import android.app.Activity;
 import android.content.Context;
+import android.graphics.drawable.ColorDrawable;
 import android.net.Uri;
 import android.os.Bundle;
+import android.text.TextPaint;
 import android.text.TextUtils;
+import android.text.style.CharacterStyle;
+import android.text.style.UpdateAppearance;
 
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
@@ -13,6 +17,7 @@ import androidx.recyclerview.widget.RecyclerView;
 
 import org.commonmark.ext.gfm.tables.TableBlock;
 import org.commonmark.node.FencedCodeBlock;
+import org.commonmark.node.Link;
 
 import java.io.BufferedReader;
 import java.io.IOException;
@@ -24,10 +29,14 @@ import io.noties.debug.Debug;
 import io.noties.markwon.AbstractMarkwonPlugin;
 import io.noties.markwon.Markwon;
 import io.noties.markwon.MarkwonConfiguration;
+import io.noties.markwon.MarkwonSpansFactory;
 import io.noties.markwon.MarkwonVisitor;
 import io.noties.markwon.core.CorePlugin;
 import io.noties.markwon.html.HtmlPlugin;
-import io.noties.markwon.image.picasso.PicassoImagesPlugin;
+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.svg.SvgMediaDecoder;
 import io.noties.markwon.recycler.MarkwonAdapter;
 import io.noties.markwon.recycler.SimpleEntry;
 import io.noties.markwon.recycler.table.TableEntry;
@@ -74,13 +83,14 @@ public class RecyclerActivity extends Activity {
     private static Markwon markwon(@NonNull Context context) {
         return Markwon.builder(context)
                 .usePlugin(CorePlugin.create())
-//                .usePlugin(ImagesPlugin.create(plugin -> {
-//                    plugin
-//                            .addSchemeHandler(FileSchemeHandler.createWithAssets(context))
-//                            .addSchemeHandler(OkHttpNetworkSchemeHandler.create())
-//                            .addMediaDecoder(SvgMediaDecoder.create());
-//                }))
-                .usePlugin(PicassoImagesPlugin.create(context))
+                .usePlugin(ImagesPlugin.create(plugin -> {
+                    plugin
+                            .addSchemeHandler(FileSchemeHandler.createWithAssets(context))
+                            .addSchemeHandler(OkHttpNetworkSchemeHandler.create())
+                            .addMediaDecoder(SvgMediaDecoder.create())
+                            .placeholderProvider(drawable -> new ColorDrawable(0xFFff0000));
+                }))
+//                .usePlugin(PicassoImagesPlugin.create(context))
 //                .usePlugin(GlideImagesPlugin.create(context))
 //                .usePlugin(CoilImagesPlugin.create(context))
                 // important to use TableEntryPlugin instead of TablePlugin
@@ -106,10 +116,24 @@ public class RecyclerActivity extends Activity {
                             visitor.builder().append(code);
                         });
                     }
+
+                    @Override
+                    public void configureSpansFactory(@NonNull MarkwonSpansFactory.Builder builder) {
+                        // `RemoveUnderlineSpan` will be added AFTER original, thus it will remove underline applied by original
+                        builder.appendFactory(Link.class, (configuration, props) -> new RemoveUnderlineSpan());
+                    }
                 })
                 .build();
     }
 
+    private static class RemoveUnderlineSpan extends CharacterStyle implements UpdateAppearance {
+
+        @Override
+        public void updateDrawState(TextPaint tp) {
+            tp.setUnderlineText(false);
+        }
+    }
+
     @NonNull
     private static String loadReadMe(@NonNull Context context) {
         InputStream stream = null;