Permalink
Browse files

fixing issue 54, refactoring -

issue caused by storing the bitmaps in the cache with inconsistent bitmap height/width due to scaling (or not scaling), fixed by removing the height/width when adding the url as the key
refactoring loader task to take a imagewrapper instead of an imageview as it was needlessly recreate the imagewrapper from the imageview but...
the imagewrapper causes issues when used as a weakreference, so for now it's a final field but I need to make sure it isn't leaking
  • Loading branch information...
1 parent 4920d7f commit 8d0ca0bc6556124e2ab04382ac3553a13eee441d @ouchadam ouchadam committed Jan 17, 2013
View
13 core/src/main/java/com/novoda/imageloader/core/cache/LruBitmapCache.java
@@ -18,6 +18,7 @@
import android.app.ActivityManager;
import android.content.Context;
import android.graphics.Bitmap;
+
import com.novoda.imageloader.core.cache.util.LruCache;
/**
@@ -80,20 +81,12 @@ protected int sizeOf(String key, Bitmap bitmap) {
@Override
public Bitmap get(String url, int width, int height) {
- int i = width;
- if(width < height) {
- i = height;
- }
- return cache.get(url + i);
+ return cache.get(url);
}
@Override
public void put(String url, Bitmap bmp) {
- int i = bmp.getWidth();
- if(bmp.getWidth() < bmp.getHeight()) {
- i = bmp.getHeight();
- }
- cache.put(url + i, bmp);
+ cache.put(url, bmp);
}
@Override
View
165 core/src/main/java/com/novoda/imageloader/core/loader/ConcurrentLoader.java
@@ -29,73 +29,94 @@
public class ConcurrentLoader implements Loader {
- private final LoaderSettings loaderSettings;
+ private final LoaderSettings loaderSettings;
private WeakReference<OnImageLoadedListener> onImageLoadedListener;
public ConcurrentLoader(LoaderSettings loaderSettings) {
this.loaderSettings = loaderSettings;
}
- @Override
- public void load(ImageView imageView) {
- ImageWrapper w = new ImageWrapper(imageView);
-
- if (w.getUrl() == null) {
- Log.w("ImageLoader", "You should never call load if you don't set a ImageTag on the view");
- return;
- }
-
- if (!isTaskAlreadyRunning(w)) {
- // only continue if a concurrent task has not yet been started
-
- try {
- if (isBitmapAlreadyInCache(w)) {
- return;
- }
-
- // get preview or loading image
- String thumbUrl = w.getPreviewUrl();
- if (thumbUrl != null) {
- Bitmap b = loaderSettings.getCacheManager().get(thumbUrl, w.getPreviewHeight(), w.getPreviewWidth());
- if (b != null && !b.isRecycled()) {
- w.setBitmap(b);
- } else {
- setResource(w, w.getLoadingResourceId());
- }
- } else {
- setResource(w, w.getLoadingResourceId());
- }
-
- if (w.isUseCacheOnly()) {
- return;
- }
-
- // spin off a new task for this url
- LoaderTask task = createTask(imageView);
- w.setLoaderTask(task);
- task.execute();
-
- } catch (ImageNotFoundException inf) {
- setResource(w, w.getNotFoundResourceId());
- } catch (Throwable t) {
- setResource(w, w.getNotFoundResourceId());
- }
- }
- }
-
- private boolean isBitmapAlreadyInCache(ImageWrapper w) {
- Bitmap b = loaderSettings.getCacheManager().get(w.getUrl(), w.getHeight(), w.getWidth());
- if (b != null && !b.isRecycled()) {
- w.setBitmap(b);
- return true;
+ @Override
+ public void load(ImageView imageView) {
+ if (!isValidImageView(imageView)) {
+ Log.w("ImageLoader", "You should never call load if you don't set a ImageTag on the view");
+ return;
}
- return false;
+ loadBitmap(new ImageWrapper(imageView));
+ }
+
+ private boolean isValidImageView(ImageView imageView) {
+ return imageView.getTag() != null;
+ }
+
+ private void loadBitmap(ImageWrapper w) {
+ if (!isTaskAlreadyRunning(w)) {
+ if (isBitmapAlreadyInCache(getCachedBitmap(w))) {
+ Bitmap cachedBitmap = getCachedBitmap(w);
+ w.setBitmap(cachedBitmap);
+ return;
+ }
+ setDefaultImage(w);
+ if (!w.isUseCacheOnly()) {
+ startTask(w);
+ }
+ }
+ }
+
+ private boolean isBitmapAlreadyInCache(Bitmap bitmap) {
+ return bitmap != null && !bitmap.isRecycled();
}
- private LoaderTask createTask(ImageView imageView) {
- return onImageLoadedListener == null ? new LoaderTask(imageView, loaderSettings) :
- new LoaderTask(imageView, loaderSettings, onImageLoadedListener);
+ private Bitmap getCachedBitmap(ImageWrapper w) {
+ return loaderSettings.getCacheManager().get(w.getUrl(), w.getHeight(), w.getWidth());
+ }
+
+ private void setDefaultImage(ImageWrapper w) {
+ if (hasPreviewUrl(w.getPreviewUrl())) {
+ if (isBitmapAlreadyInCache(getPreviewCachedBitmap(w))) {
+ w.setBitmap(getPreviewCachedBitmap(w));
+ } else {
+ w.setBitmap(getResourceAsBitmap(w, w.getLoadingResourceId()));
+ }
+ } else {
+ w.setBitmap(getResourceAsBitmap(w, w.getLoadingResourceId()));
+ }
+ }
+
+ private Bitmap getPreviewCachedBitmap(ImageWrapper w) {
+ return loaderSettings.getCacheManager().get(w.getPreviewUrl(), w.getPreviewHeight(), w.getPreviewWidth());
+ }
+
+ private void startTask(ImageWrapper w) {
+ try {
+ LoaderTask task = createTask(w);
+ w.setLoaderTask(task);
+ task.execute();
+ } catch (ImageNotFoundException inf) {
+ w.setBitmap(getResourceAsBitmap(w, w.getNotFoundResourceId()));
+ } catch (Throwable t) {
+ w.setBitmap(getResourceAsBitmap(w, w.getNotFoundResourceId()));
+ }
+ }
+
+ private Bitmap getResourceAsBitmap(ImageWrapper w, int resId) {
+ Bitmap b = loaderSettings.getResCacheManager().get("" + resId, w.getWidth(), w.getHeight());
+ if (b != null) {
+ return b;
+ }
+ b = loaderSettings.getBitmapUtil().decodeResourceBitmapAndScale(w, resId, loaderSettings.isAllowUpsampling());
+ loaderSettings.getResCacheManager().put(String.valueOf(resId), b);
+ return b;
+ }
+
+ private boolean hasPreviewUrl(String previewUrl) {
+ return previewUrl != null;
+ }
+
+ private LoaderTask createTask(ImageWrapper imageWrapper) {
+ return onImageLoadedListener == null ? new LoaderTask(imageWrapper, loaderSettings) :
+ new LoaderTask(imageWrapper, loaderSettings, onImageLoadedListener);
}
@Override
@@ -104,40 +125,24 @@ public void setLoadListener(WeakReference<OnImageLoadedListener> onImageLoadedLi
}
/**
- * checks whether a previous task is loading the same url
- *
- * @param imageWrapper
- * url of the image to be fetched
- * task that might already fetching an image, might be null
- * @return false if there is no other concurrent task running
- */
+ * checks whether a previous task is loading the same url
+ *
+ * @param imageWrapper url of the image to be fetched
+ * task that might already fetching an image, might be null
+ * @return false if there is no other concurrent task running
+ */
private static boolean isTaskAlreadyRunning(ImageWrapper imageWrapper) {
LoaderTask oldTask = imageWrapper.getLoaderTask();
if (oldTask == null) {
return false;
}
- String url = imageWrapper.getUrl();
- if ((!url.equals(oldTask.getUrl()))) {
+ if ((!imageWrapper.getUrl().equals(oldTask.getUrl()))) {
return true;
}
- // task != null && url == task.getUrl
- // there is already a concurrent task fetching the image
oldTask.cancel(true);
-
return false;
}
- private void setResource(ImageWrapper w, int resId) {
- Bitmap b = loaderSettings.getResCacheManager().get("" + resId, w.getWidth(), w.getHeight());
- if (b != null) {
- w.setBitmap(b);
- return;
- }
- b = loaderSettings.getBitmapUtil().decodeResourceBitmapAndScale(w, resId, loaderSettings.isAllowUpsampling());
- loaderSettings.getResCacheManager().put(String.valueOf(resId), b);
- w.setBitmap(b);
- }
-
}
View
92 core/src/main/java/com/novoda/imageloader/core/loader/util/LoaderTask.java
@@ -17,21 +17,20 @@
import android.content.Context;
import android.graphics.Bitmap;
-import android.view.animation.Animation;
-import android.widget.ImageView;
+import android.util.Log;
import com.novoda.imageloader.core.LoaderSettings;
import com.novoda.imageloader.core.OnImageLoadedListener;
import com.novoda.imageloader.core.exception.ImageNotFoundException;
-import com.novoda.imageloader.core.model.ImageTag;
import com.novoda.imageloader.core.model.ImageWrapper;
import java.io.File;
import java.lang.ref.WeakReference;
public class LoaderTask extends AsyncTask<String, Void, Bitmap> {
- private final WeakReference<ImageView> imageViewReference;
+// private final WeakReference<ImageWrapper> imageViewReference;
+ private final ImageWrapper imageWrapper;
private final LoaderSettings loaderSettings;
private final WeakReference<OnImageLoadedListener> onImageLoadedListener;
@@ -42,50 +41,49 @@
private int height;
private int notFoundResourceId;
- public LoaderTask(ImageView imageView, LoaderSettings loaderSettings) {
- this(imageView, loaderSettings, null);
+ public LoaderTask(ImageWrapper imageWrapper, LoaderSettings loaderSettings) {
+ this(imageWrapper, loaderSettings, null);
}
- public LoaderTask(ImageView imageView, LoaderSettings loaderSettings, WeakReference<OnImageLoadedListener> onImageLoadedListener) {
- this.imageViewReference = new WeakReference<ImageView>(imageView);
+ public LoaderTask(ImageWrapper imageWrapper, LoaderSettings loaderSettings, WeakReference<OnImageLoadedListener> onImageLoadedListener) {
+// this.imageViewReference = new WeakReference<ImageWrapper>(imageWrapper);
+ this.imageWrapper = imageWrapper;
this.loaderSettings = loaderSettings;
this.onImageLoadedListener = onImageLoadedListener;
}
@Override
- protected Bitmap doInBackground(String... arg0) {
- if (imageViewReference == null) {
+ protected Bitmap doInBackground(String... args) {
+// ImageWrapper imageWrapper = imageViewReference.get();
+ if (imageWrapper == null) {
+ Log.e("XXXXXX", "ImageWrapper is null");
return null;
}
- ImageView imageView = imageViewReference.get();
- if (imageView == null) {
- return null;
- }
- ImageWrapper imageWrapper = setAndValidateTagInformation(imageView);
+
+ setTagInformation(imageWrapper);
if (url == null || url.length() <= 0 || url.equals("_url_error")) {
return getNotFoundImage(imageWrapper.getContext());
}
- if (hasImageViewUrlChanged(imageView)) {
+ if (hasImageViewUrlChanged(imageWrapper)) {
+ Log.e("XXXXXX", "ImageWrapper url has changed");
return null;
}
- Bitmap b = loaderSettings.getCacheManager().get(url, width, height);
- if (b != null && !b.isRecycled()) {
- return b;
- }
+
File imageFile = getImageFile(imageWrapper);
if (!imageFile.exists()) {
if (useCacheOnly) {
return null;
}
try {
loaderSettings.getNetworkManager().retrieveImage(url, imageFile);
+ Log.e("XXXXXX", "Image fetched from network : " + imageWrapper.getUrl());
} catch (ImageNotFoundException inf) {
return getNotFoundImage(imageWrapper.getContext());
}
}
- if (hasImageViewUrlChanged(imageView)) {
+ if (hasImageViewUrlChanged(imageWrapper)) {
return null;
}
return getImageFromFile(imageFile);
@@ -101,6 +99,7 @@ private Bitmap getImageFromFile(File imageFile) {
if (b == null) {
// decoding failed
+ Log.e("XXXXXX", "Bitmap decoding failed");
return b;
}
@@ -111,14 +110,12 @@ private Bitmap getImageFromFile(File imageFile) {
return b;
}
- private ImageWrapper setAndValidateTagInformation(ImageView imageView) {
- ImageWrapper imageWrapper = new ImageWrapper(imageView);
+ private void setTagInformation(ImageWrapper imageWrapper) {
url = imageWrapper.getUrl();
width = imageWrapper.getWidth();
height = imageWrapper.getHeight();
notFoundResourceId = imageWrapper.getNotFoundResourceId();
useCacheOnly = imageWrapper.isUseCacheOnly();
- return imageWrapper;
}
private void saveScaledImage(File imageFile, Bitmap b) {
@@ -139,11 +136,11 @@ private File getImageFile(ImageWrapper imageWrapper) {
return imageFile;
}
- private boolean hasImageViewUrlChanged(ImageView imageView) {
+ private boolean hasImageViewUrlChanged(ImageWrapper imageWrapper) {
if (url == null) {
return false;
} else {
- return !url.equals(new ImageWrapper(imageView).getCurrentUrl());
+ return !url.equals(imageWrapper.getCurrentUrl());
}
}
@@ -154,46 +151,33 @@ protected void onPostExecute(Bitmap bitmap) {
}
if (isCancelled()) {
bitmap = null;
+ Log.e("XXXXXX", "onPostExecute isCancelled");
return;
}
- if (imageViewReference == null) {
- return;
- }
+// if (imageViewReference == null) {
+// Log.e("XXXXXX", "onPostExecute ImageWrapper reference is null");
+// return;
+// }
- ImageView imageView = imageViewReference.get();
- if (validateImageView(imageView)) {
- listenerCallback(imageView);
- stopExistingAnimation(imageView);
- imageView.setImageBitmap(bitmap);
- startImageViewAnimation(imageView);
- }
- }
-
- private void startImageViewAnimation(ImageView imageView) {
- Animation animation = ((ImageTag) imageView.getTag()).getAnimation();
- if (animation != null) {
- imageView.startAnimation(animation);
- }
- }
-
- private void stopExistingAnimation(ImageView imageView) {
- Animation old = imageView.getAnimation();
- if (old != null && !old.hasEnded()) {
- old.cancel();
+// ImageWrapper imageWrapper = imageViewReference.get();
+ if (validateImageView(imageWrapper)) {
+ listenerCallback(imageWrapper);
+ imageWrapper.setBitmap(bitmap);
+ } else {
+ Log.e("XXXXXX", "onPostExecute ImageWrapper invalid");
}
}
- private boolean validateImageView(ImageView imageView) {
- if (imageView == null || hasImageViewUrlChanged(imageView) ||
- ((ImageTag) imageView.getTag()).getLoaderTask() != this) {
+ private boolean validateImageView(ImageWrapper imageWrapper) {
+ if (imageWrapper == null || hasImageViewUrlChanged(imageWrapper) || imageWrapper.getLoaderTask() != this) {
return false;
}
return true;
}
- private void listenerCallback(ImageView imageView) {
+ private void listenerCallback(ImageWrapper imageWrapper) {
if (onImageLoadedListener != null && onImageLoadedListener.get() != null) {
- onImageLoadedListener.get().onImageLoaded(imageView);
+ onImageLoadedListener.get().onImageLoaded(imageWrapper.getImageView());
}
}
View
9 core/src/main/java/com/novoda/imageloader/core/model/ImageWrapper.java
@@ -40,7 +40,6 @@
private int notFoundResourceId;
private boolean isUseCacheOnly;
private boolean saveThumbnail;
-
private Animation animation;
public ImageWrapper(ImageView imageView) {
@@ -159,14 +158,6 @@ public int getPreviewHeight() {
return previewHeight;
}
- public Animation getAnimation() {
- return animation;
- }
-
- public void setAnimation(Animation animation) {
- this.animation = animation;
- }
-
public LoaderTask getLoaderTask() {
ImageTag tag = (ImageTag) imageView.getTag();
return tag.getLoaderTask();

0 comments on commit 8d0ca0b

Please sign in to comment.