From f3476ca5cc117b065e0e069a0629fb2ffc5cf696 Mon Sep 17 00:00:00 2001 From: Dimitry Ivanov Date: Tue, 4 Jun 2019 15:41:05 +0300 Subject: [PATCH] Use Call.Factory instead of OkHttpClient in images module --- .../image/AsyncDrawableLoaderBuilder.java | 7 +- .../image/AsyncDrawableLoaderImpl.java | 156 ++++-------------- .../ru/noties/markwon/image/ImagesPlugin.java | 4 +- .../network/OkHttpNetworkSchemeHandler.java | 23 ++- .../image/AsyncDrawableLoaderImplTest.java | 99 +++++++++++ 5 files changed, 153 insertions(+), 136 deletions(-) create mode 100644 markwon-image/src/test/java/ru/noties/markwon/image/AsyncDrawableLoaderImplTest.java diff --git a/markwon-image/src/main/java/ru/noties/markwon/image/AsyncDrawableLoaderBuilder.java b/markwon-image/src/main/java/ru/noties/markwon/image/AsyncDrawableLoaderBuilder.java index 57347b70..a57ca99b 100644 --- a/markwon-image/src/main/java/ru/noties/markwon/image/AsyncDrawableLoaderBuilder.java +++ b/markwon-image/src/main/java/ru/noties/markwon/image/AsyncDrawableLoaderBuilder.java @@ -29,6 +29,8 @@ class AsyncDrawableLoaderBuilder { // we should not use file-scheme as it's a bit complicated to assume file usage (lack of permissions) addSchemeHandler(DataUriSchemeHandler.create()); addSchemeHandler(NetworkSchemeHandler.create()); + + defaultMediaDecoder = DefaultImageMediaDecoder.create(); } void executorService(@NonNull ExecutorService executorService) { @@ -78,11 +80,6 @@ class AsyncDrawableLoaderBuilder { isBuilt = true; - // @since 4.0.0-SNAPSHOT - if (defaultMediaDecoder == null) { - defaultMediaDecoder = DefaultImageMediaDecoder.create(); - } - if (executorService == null) { executorService = Executors.newCachedThreadPool(); } diff --git a/markwon-image/src/main/java/ru/noties/markwon/image/AsyncDrawableLoaderImpl.java b/markwon-image/src/main/java/ru/noties/markwon/image/AsyncDrawableLoaderImpl.java index a0226c48..01b12282 100644 --- a/markwon-image/src/main/java/ru/noties/markwon/image/AsyncDrawableLoaderImpl.java +++ b/markwon-image/src/main/java/ru/noties/markwon/image/AsyncDrawableLoaderImpl.java @@ -4,8 +4,10 @@ import android.graphics.drawable.Drawable; import android.net.Uri; import android.os.Handler; import android.os.Looper; +import android.os.SystemClock; import android.support.annotation.NonNull; import android.support.annotation.Nullable; +import android.support.annotation.VisibleForTesting; import android.util.Log; import java.lang.ref.WeakReference; @@ -24,126 +26,45 @@ class AsyncDrawableLoaderImpl extends AsyncDrawableLoader { private final ImagesPlugin.PlaceholderProvider placeholderProvider; private final ImagesPlugin.ErrorHandler errorHandler; - private final Handler mainThread; + private final Handler handler; - // @since 3.1.0-SNAPSHOT use a hash-map with a weak AsyncDrawable as key for multiple requests + // @since 4.0.0-SNAPSHOT use a hash-map with a AsyncDrawable as key for multiple requests // for the same destination - private final Map, Future> requests = new HashMap<>(2); + private final Map> requests = new HashMap<>(2); AsyncDrawableLoaderImpl(@NonNull AsyncDrawableLoaderBuilder builder) { + this(builder, new Handler(Looper.getMainLooper())); + } + + // @since 4.0.0-SNAPSHOT + @VisibleForTesting + AsyncDrawableLoaderImpl(@NonNull AsyncDrawableLoaderBuilder builder, @NonNull Handler handler) { this.executorService = builder.executorService; this.schemeHandlers = builder.schemeHandlers; this.mediaDecoders = builder.mediaDecoders; this.defaultMediaDecoder = builder.defaultMediaDecoder; this.placeholderProvider = builder.placeholderProvider; this.errorHandler = builder.errorHandler; - this.mainThread = new Handler(Looper.getMainLooper()); + this.handler = handler; } @Override public void load(@NonNull final AsyncDrawable drawable) { - - // primitive synchronization via main-thread - if (!isMainThread()) { - mainThread.post(new Runnable() { - @Override - public void run() { - load(drawable); - } - }); - return; + final Future future = requests.get(drawable); + if (future == null) { + requests.put(drawable, execute(drawable)); } - - // okay, if by some chance requested drawable already has a future associated -> no-op - // as AsyncDrawable cannot change `destination` (immutable field) - // @since 3.1.0-SNAPSHOT - if (hasTaskAssociated(drawable)) { - return; - } - - final WeakReference reference = new WeakReference<>(drawable); - requests.put(reference, execute(drawable.getDestination(), reference)); } @Override public void cancel(@NonNull final AsyncDrawable drawable) { - if (!isMainThread()) { - mainThread.post(new Runnable() { - @Override - public void run() { - cancel(drawable); - } - }); - return; + final Future future = requests.remove(drawable); + if (future != null) { + future.cancel(true); } - final Iterator, Future>> iterator = - requests.entrySet().iterator(); - - AsyncDrawable key; - Map.Entry, Future> entry; - - while (iterator.hasNext()) { - - entry = iterator.next(); - key = entry.getKey().get(); - - // if key is null or it contains requested AsyncDrawable -> cancel - if (shouldCleanUp(key) || key == drawable) { - entry.getValue().cancel(true); - iterator.remove(); - } - } - } - - private boolean hasTaskAssociated(@NonNull AsyncDrawable drawable) { - - final Iterator, Future>> iterator = - requests.entrySet().iterator(); - - boolean result = false; - - AsyncDrawable key; - Map.Entry, Future> entry; - - while (iterator.hasNext()) { - - entry = iterator.next(); - key = entry.getKey().get(); - - // clean-up - if (shouldCleanUp(key)) { - entry.getValue().cancel(true); - iterator.remove(); - } else if (key == drawable) { - result = true; - // do not break, let iteration continue to possibly clean-up the rest references - } - } - - return result; - } - - private void cleanUp() { - - final Iterator, Future>> iterator = - requests.entrySet().iterator(); - - AsyncDrawable key; - Map.Entry, Future> entry; - - while (iterator.hasNext()) { - - entry = iterator.next(); - key = entry.getKey().get(); - - // clean-up of already referenced or detached drawables - if (shouldCleanUp(key)) { - entry.getValue().cancel(true); - iterator.remove(); - } - } + handler.removeCallbacksAndMessages(drawable); } @Nullable @@ -155,7 +76,7 @@ class AsyncDrawableLoaderImpl extends AsyncDrawableLoader { } @NonNull - private Future execute(@NonNull final String destination, @NonNull final WeakReference reference) { + private Future execute(@NonNull final AsyncDrawable asyncDrawable) { // todo: more efficient DefaultImageMediaDecoder... BitmapFactory.decodeStream is a bit not optimal // for big images for sure. We _could_ introduce internal Drawable that will check for @@ -166,6 +87,8 @@ class AsyncDrawableLoaderImpl extends AsyncDrawableLoader { @Override public void run() { + final String destination = asyncDrawable.getDestination(); + final Uri uri = Uri.parse(destination); Drawable drawable = null; @@ -214,35 +137,22 @@ class AsyncDrawableLoaderImpl extends AsyncDrawableLoader { final Drawable out = drawable; - mainThread.post(new Runnable() { + handler.postAtTime(new Runnable() { @Override public void run() { - - if (out != null) { - // AsyncDrawable cannot change destination, so if it's - // attached and not garbage-collected, we can deliver the result. - // Note that there is no cache, so attach/detach of drawables - // will always request a new entry.. (comment @since 3.1.0-SNAPSHOT) - final AsyncDrawable asyncDrawable = reference.get(); - if (asyncDrawable != null && asyncDrawable.isAttached()) { - asyncDrawable.setResult(out); - } + // validate that + // * request was not cancelled + // * out-result is present + // * async-drawable is attached + final Future future = requests.remove(asyncDrawable); + if (future != null + && out != null + && asyncDrawable.isAttached()) { + asyncDrawable.setResult(out); } - - requests.remove(reference); - cleanUp(); } - }); + }, asyncDrawable, SystemClock.uptimeMillis()); } }); } - - private static boolean shouldCleanUp(@Nullable AsyncDrawable drawable) { - return drawable == null || !drawable.isAttached(); - } - - @SuppressWarnings("BooleanMethodIsAlwaysInverted") - private static boolean isMainThread() { - return Looper.myLooper() == Looper.getMainLooper(); - } } diff --git a/markwon-image/src/main/java/ru/noties/markwon/image/ImagesPlugin.java b/markwon-image/src/main/java/ru/noties/markwon/image/ImagesPlugin.java index e5cac9b3..ee5d3cc9 100644 --- a/markwon-image/src/main/java/ru/noties/markwon/image/ImagesPlugin.java +++ b/markwon-image/src/main/java/ru/noties/markwon/image/ImagesPlugin.java @@ -107,13 +107,13 @@ public class ImagesPlugin extends AbstractMarkwonPlugin { /** * Please note that if not specified a {@link DefaultImageMediaDecoder} will be used. So - * if you need to disable default-image-media-decoder specify here own no-op implementation. + * if you need to disable default-image-media-decoder specify here own no-op implementation or null. * * @see DefaultImageMediaDecoder * @since 4.0.0-SNAPSHOT */ @NonNull - public ImagesPlugin defaultMediaDecoder(@NonNull MediaDecoder mediaDecoder) { + public ImagesPlugin defaultMediaDecoder(@Nullable MediaDecoder mediaDecoder) { checkBuilderState(); builder.defaultMediaDecoder(mediaDecoder); return this; diff --git a/markwon-image/src/main/java/ru/noties/markwon/image/network/OkHttpNetworkSchemeHandler.java b/markwon-image/src/main/java/ru/noties/markwon/image/network/OkHttpNetworkSchemeHandler.java index 9feddd0f..b470841d 100644 --- a/markwon-image/src/main/java/ru/noties/markwon/image/network/OkHttpNetworkSchemeHandler.java +++ b/markwon-image/src/main/java/ru/noties/markwon/image/network/OkHttpNetworkSchemeHandler.java @@ -7,6 +7,7 @@ import java.io.InputStream; import java.util.Arrays; import java.util.Collection; +import okhttp3.Call; import okhttp3.OkHttpClient; import okhttp3.Request; import okhttp3.Response; @@ -24,21 +25,31 @@ public class OkHttpNetworkSchemeHandler extends SchemeHandler { */ @NonNull public static OkHttpNetworkSchemeHandler create() { - return new OkHttpNetworkSchemeHandler(new OkHttpClient()); + return create(new OkHttpClient()); } @NonNull public static OkHttpNetworkSchemeHandler create(@NonNull OkHttpClient client) { - return new OkHttpNetworkSchemeHandler(client); + // explicit cast, otherwise a recursive call + return create((Call.Factory) client); + } + + /** + * @since 4.0.0-SNAPSHOT + */ + @NonNull + public static OkHttpNetworkSchemeHandler create(@NonNull Call.Factory factory) { + return new OkHttpNetworkSchemeHandler(factory); } private static final String HEADER_CONTENT_TYPE = "Content-Type"; - private final OkHttpClient client; + // @since 4.0.0-SNAPSHOT, previously just OkHttpClient + private final Call.Factory factory; @SuppressWarnings("WeakerAccess") - OkHttpNetworkSchemeHandler(@NonNull OkHttpClient client) { - this.client = client; + OkHttpNetworkSchemeHandler(@NonNull Call.Factory factory) { + this.factory = factory; } @NonNull @@ -52,7 +63,7 @@ public class OkHttpNetworkSchemeHandler extends SchemeHandler { final Response response; try { - response = client.newCall(request).execute(); + response = factory.newCall(request).execute(); } catch (Throwable t) { throw new IllegalStateException("Exception obtaining network resource: " + raw, t); } diff --git a/markwon-image/src/test/java/ru/noties/markwon/image/AsyncDrawableLoaderImplTest.java b/markwon-image/src/test/java/ru/noties/markwon/image/AsyncDrawableLoaderImplTest.java new file mode 100644 index 00000000..80e2bf53 --- /dev/null +++ b/markwon-image/src/test/java/ru/noties/markwon/image/AsyncDrawableLoaderImplTest.java @@ -0,0 +1,99 @@ +package ru.noties.markwon.image; + +import android.os.Handler; +import android.support.annotation.NonNull; +import android.support.annotation.Nullable; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.annotation.Config; + +import java.util.concurrent.ExecutorService; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +@RunWith(RobolectricTestRunner.class) +@Config(manifest = Config.NONE) +public class AsyncDrawableLoaderImplTest { + + private BuilderImpl builder; + private AsyncDrawableLoaderImpl impl; + + @Before + public void before() { + builder = new BuilderImpl(); + } + + @Test + public void placeholder() { + final ImagesPlugin.PlaceholderProvider placeholderProvider = + mock(ImagesPlugin.PlaceholderProvider.class); + impl = builder.placeholderProvider(placeholderProvider) + .build(); + impl.placeholder(mock(AsyncDrawable.class)); + verify(placeholderProvider, times(1)) + .providePlaceholder(any(AsyncDrawable.class)); + } + + private static class BuilderImpl { + + AsyncDrawableLoaderBuilder builder; + Handler handler = mock(Handler.class); + + public BuilderImpl executorService(@NonNull ExecutorService executorService) { + builder.executorService(executorService); + return this; + } + + public BuilderImpl addSchemeHandler(@NonNull SchemeHandler schemeHandler) { + builder.addSchemeHandler(schemeHandler); + return this; + } + + public BuilderImpl addMediaDecoder(@NonNull MediaDecoder mediaDecoder) { + builder.addMediaDecoder(mediaDecoder); + return this; + } + + public BuilderImpl defaultMediaDecoder(@Nullable MediaDecoder mediaDecoder) { + builder.defaultMediaDecoder(mediaDecoder); + return this; + } + + public BuilderImpl removeSchemeHandler(@NonNull String scheme) { + builder.removeSchemeHandler(scheme); + return this; + } + + public BuilderImpl removeMediaDecoder(@NonNull String contentType) { + builder.removeMediaDecoder(contentType); + return this; + } + + public BuilderImpl placeholderProvider(@NonNull ImagesPlugin.PlaceholderProvider placeholderDrawableProvider) { + builder.placeholderProvider(placeholderDrawableProvider); + return this; + } + + public BuilderImpl errorHandler(@NonNull ImagesPlugin.ErrorHandler errorHandler) { + builder.errorHandler(errorHandler); + return this; + } + + @NonNull + public BuilderImpl handler(Handler handler) { + this.handler = handler; + return this; + } + + @NonNull + AsyncDrawableLoaderImpl build() { + return new AsyncDrawableLoaderImpl(builder, handler); + } + } +} \ No newline at end of file