From 4f979ea4c652b1cfb0a2acee5fb33650bfae6ca7 Mon Sep 17 00:00:00 2001 From: Dimitry Ivanov Date: Fri, 28 Jun 2019 21:31:28 +0300 Subject: [PATCH] Remove weak-reference from async-drawable-loader --- .../image/AsyncDrawableLoaderImpl.java | 191 ++---------------- 1 file changed, 20 insertions(+), 171 deletions(-) diff --git a/markwon-core/src/main/java/ru/noties/markwon/image/AsyncDrawableLoaderImpl.java b/markwon-core/src/main/java/ru/noties/markwon/image/AsyncDrawableLoaderImpl.java index 711508c2..f8d91931 100644 --- a/markwon-core/src/main/java/ru/noties/markwon/image/AsyncDrawableLoaderImpl.java +++ b/markwon-core/src/main/java/ru/noties/markwon/image/AsyncDrawableLoaderImpl.java @@ -4,14 +4,13 @@ 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 java.io.IOException; import java.io.InputStream; -import java.lang.ref.WeakReference; import java.util.HashMap; -import java.util.Iterator; import java.util.Map; import java.util.concurrent.ExecutorService; import java.util.concurrent.Future; @@ -24,12 +23,12 @@ class AsyncDrawableLoaderImpl extends AsyncDrawableLoader { private final MediaDecoder defaultMediaDecoder; private final DrawableProvider placeholderDrawableProvider; private final DrawableProvider errorDrawableProvider; + private final Handler handler = new Handler(Looper.getMainLooper()); - private final Handler mainThread; - // @since 3.1.0-SNAPSHOT use a hash-map with a weak AsyncDrawable as key for multiple requests - // for the same destination - private final Map, Future> requests = new HashMap<>(2); + // @since 3.1.0-SNAPSHOT use a hash-map with an AsyncDrawable (allows multiple drawables + // referencing the same source) + private final Map> requests = new HashMap<>(2); AsyncDrawableLoaderImpl(@NonNull Builder builder) { this.executorService = builder.executorService; @@ -38,143 +37,27 @@ class AsyncDrawableLoaderImpl extends AsyncDrawableLoader { this.defaultMediaDecoder = builder.defaultMediaDecoder; this.placeholderDrawableProvider = builder.placeholderDrawableProvider; this.errorDrawableProvider = builder.errorDrawableProvider; - this.mainThread = new Handler(Looper.getMainLooper()); } @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(); - } - } + handler.removeCallbacksAndMessages(drawable); } - 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(); - } - } - } - -// @Override -// public void load(@NonNull String destination, @NonNull AsyncDrawable drawable) { -// -// // todo: we cannot reliably identify request by the destination, as if -// // markdown input has multiple images with the same destination as source -// // we will be tracking only one of them (the one appears the last). We should -// // move to AsyncDrawable based identification. This method also _maybe_ -// // should include the ImageSize (comment @since 3.1.0-SNAPSHOT) -// -// requests.put(destination, execute(destination, drawable)); -// } -// -// @Override -// public void cancel(@NonNull String destination) { -// -// // todo: as we are moving away from a single request for a destination, -// // we should re-evaluate this cancellation logic, as if there are multiple images -// // in markdown input all of them will be cancelled (won't delivered), even if -// // only a single drawable is detached. Cancellation must also take -// // the AsyncDrawable argument (comment @since 3.1.0-SNAPSHOT) -// -// // -// final Future request = requests.remove(destination); -// if (request != null) { -// request.cancel(true); -// } -// } - @Nullable @Override public Drawable placeholder() { @@ -183,15 +66,10 @@ class AsyncDrawableLoaderImpl extends AsyncDrawableLoader { : null; } - private Future execute(@NonNull final String destination, @NonNull final WeakReference reference) { + @NonNull + private Future execute(@NonNull final AsyncDrawable drawable) { - // todo: error handing (simply applying errorDrawable is not a good solution - // as reason for an error is unclear (no scheme handler, no input data, error decoding, etc) - - // todo: more efficient ImageMediaDecoder... BitmapFactory.decodeStream is a bit not optimal - // for big images for sure. We _could_ introduce internal Drawable that will check for - // image bounds (but we will need to cache inputStream in order to inspect and optimize - // input image...) + final String destination = drawable.getDestination(); return executorService.submit(new Runnable() { @Override @@ -245,46 +123,17 @@ class AsyncDrawableLoaderImpl extends AsyncDrawableLoader { final Drawable out = result; - mainThread.post(new Runnable() { + handler.postAtTime(new Runnable() { @Override public void run() { - - if (out != null) { - - // this doesn't work with markdown input with multiple images with the - // same source (comment @since 3.1.0-SNAPSHOT) -// final boolean canDeliver = requests.remove(destination) != null; -// if (canDeliver) { -// final AsyncDrawable asyncDrawable = reference.get(); -// if (asyncDrawable != null && asyncDrawable.isAttached()) { -// asyncDrawable.setResult(out); -// } -// } - - // todo: 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); + if (requests.remove(drawable) != null) { + if (out != null && drawable.isAttached()) { + drawable.setResult(out); } } - - requests.remove(reference); - cleanUp(); } - }); + }, drawable, 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(); - } }