Make ListPreloader more tolerant of null values from its interfaces.

Progress towards #2379.
bumptech · Sep 14, 2017 · c3479c4 · c3479c4
1 parent 4239a45
commit c3479c4
Show file tree

Hide file tree

Showing 6 changed files with 80 additions and 19 deletions.
diff --git a/library/src/main/java/com/bumptech/glide/ListPreloader.java b/library/src/main/java/com/bumptech/glide/ListPreloader.java
@@ -1,5 +1,6 @@
 package com.bumptech.glide;
 
+import android.support.annotation.NonNull;
 import android.support.annotation.Nullable;
 import android.widget.AbsListView;
 import com.bumptech.glide.request.target.BaseTarget;
@@ -24,7 +25,6 @@
  * @param <T> The type of the model being displayed in the list.
  */
 public class ListPreloader<T> implements AbsListView.OnScrollListener {
-
   private final int maxPreload;
   private final PreloadTargetQueue preloadTargetQueue;
   private final RequestManager requestManager;
@@ -47,24 +47,44 @@ public class ListPreloader<T> implements AbsListView.OnScrollListener {
   public interface PreloadModelProvider<U> {
 
     /**
-     * Returns a non null list of all models that need to be loaded for the list to display adapter
-     * items in positions between {@code start} and {@code end}.
+     * Returns a {@link List} of models that need to be loaded for the list to display adapter items
+     * in positions between {@code start} and {@code end}.
+     *
+     * <p>A list of any size can be returned so there can be multiple models per adapter position.
+     *
+     * <p>Every model returned by this method is expected to produce a valid {@link RequestBuilder}
+     * in {@link #getPreloadRequestBuilder(Object)}. If that's not possible for any set of models,
+     * avoid including them in the {@link List} returned by this method.
      *
-     * <p> A list of any size can be returned so there can be multiple models per adapter position.
-     * </p>
+     * <p>Although it's acceptable for the returned {@link List} to contain {@code null} models,
+     * it's best to filter them from the list instead of adding {@code null} to avoid unnecessary
+     * logic and expanding the size of the {@link List}
      *
      * @param position The adapter position.
      */
+    @NonNull
     List<U> getPreloadItems(int position);
 
     /**
-     * Returns a non null {@link RequestBuilder} for a given item. Must exactly match the request
-     * used to load the resource in the list.
+     * Returns a {@link RequestBuilder} for a given item on which
+     * {@link RequestBuilder#load(Object)}} has been called or {@code null} if no valid load can be
+     * started.
+     *
+     * <p>For the preloader to be effective, the {@link RequestBuilder} returned here must use
+     * exactly the same size and set of options as the {@link RequestBuilder} used when the ``View``
+     * is bound. You may need to specify a size in both places to ensure that the width and height
+     * match exactly. If so, you can use
+     * {@link com.bumptech.glide.request.RequestOptions#override(int, int)} to do so.
+     *
+     * <p>The target and context will be provided by the preloader.
      *
-     * <p> The target and context will be provided by the preloader. </p>
+     * <p>If {@link RequestBuilder#load(Object)} is not called by this method, the preloader will
+     * trigger a {@link RuntimeException}. If you don't want to load a particular item or position,
+     * filter it from the list returned by {@link #getPreloadItems(int)}.
      *
      * @param item The model to load.
      */
+    @Nullable
     RequestBuilder getPreloadRequestBuilder(U item);
   }
 
@@ -80,8 +100,9 @@ public interface PreloadSizeProvider<T> {
      * Returns the size of the view in the list where the resources will be displayed in pixels in
      * the format [x, y], or {@code null} if no size is currently available.
      *
-     * <p> Note - The dimensions returned here must precisely match those of the view in the list.
-     * </p>
+     * <p>Note - The dimensions returned here must precisely match those of the view in the list.
+     *
+     * <p>If this method returns {@code null}, then no request will be started for the given item.
      *
      * @param item A model
      */
@@ -175,13 +196,22 @@ private void preloadAdapterPosition(List<T> items, int position, boolean isIncre
   }
 
   @SuppressWarnings("unchecked")
-  private void preloadItem(T item, int position, int i) {
-    final int[] dimensions = this.preloadDimensionProvider.getPreloadSize(item, position, i);
-    if (dimensions != null) {
-      RequestBuilder<Object> preloadRequestBuilder =
-          this.preloadModelProvider.getPreloadRequestBuilder(item);
-      preloadRequestBuilder.into(preloadTargetQueue.next(dimensions[0], dimensions[1]));
+  private void preloadItem(@Nullable T item, int position, int perItemPosition) {
+    if (item == null) {
+      return;
+    }
+    int[] dimensions =
+        preloadDimensionProvider.getPreloadSize(item, position, perItemPosition);
+    if (dimensions == null) {
+      return;
     }
+    RequestBuilder<Object> preloadRequestBuilder =
+        preloadModelProvider.getPreloadRequestBuilder(item);
+    if (preloadRequestBuilder == null) {
+      return;
+    }
+
+    preloadRequestBuilder.into(preloadTargetQueue.next(dimensions[0], dimensions[1]));
   }
 
   private void cancelAll() {
@@ -193,7 +223,7 @@ private void cancelAll() {
   private static final class PreloadTargetQueue {
     private final Queue<PreloadTarget> queue;
 
-    public PreloadTargetQueue(int size) {
+    PreloadTargetQueue(int size) {
       queue = Util.createQueue(size);
 
       for (int i = 0; i < size; i++) {
@@ -210,7 +240,7 @@ public PreloadTarget next(int width, int height) {
     }
   }
 
-  private static class PreloadTarget extends BaseTarget<Object> {
+  private static final class PreloadTarget extends BaseTarget<Object> {
     @Synthetic int photoHeight;
     @Synthetic int photoWidth;
 

diff --git a/library/src/test/java/com/bumptech/glide/ListPreloaderTest.java b/library/src/test/java/com/bumptech/glide/ListPreloaderTest.java
@@ -8,6 +8,7 @@
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 
+import android.support.annotation.NonNull;
 import com.bumptech.glide.request.target.SizeReadyCallback;
 import com.bumptech.glide.request.target.Target;
 import java.util.ArrayList;
@@ -45,6 +46,7 @@ public void testGetItemsIsCalledIncreasing() {
     final AtomicInteger calledCount = new AtomicInteger();
 
     ListPreloaderAdapter preloaderAdapter = new ListPreloaderAdapter() {
+      @NonNull
       @Override
       public List<Object> getPreloadItems(int position) {
         called.set(true);
@@ -75,11 +77,13 @@ public int[] getPreloadSize(Object item, int adapterPosition, int itemPosition)
         return new int[] { 10, 10 };
       }
 
+      @NonNull
       @Override
       public List<Object> getPreloadItems(int position) {
         return objects.subList(position - 11, position + 1 - 11);
       }
 
+      @NonNull
       @Override
       @SuppressWarnings("unchecked")
       public RequestBuilder<Object> getPreloadRequestBuilder(Object item) {
@@ -98,6 +102,7 @@ public void testGetItemsIsCalledDecreasing() {
     final AtomicBoolean called = new AtomicBoolean(false);
     final AtomicInteger calledCount = new AtomicInteger();
     ListPreloaderAdapter preloaderAdapter = new ListPreloaderAdapter() {
+      @NonNull
       @Override
       public List<Object> getPreloadItems(int position) {
         // Ignore the preload caused from us starting at the end
@@ -133,6 +138,7 @@ public int[] getPreloadSize(Object item, int adapterPosition, int itemPosition)
         return new int[] { 10, 10 };
       }
 
+      @NonNull
       @Override
       public List<Object> getPreloadItems(int position) {
         if (position == 40) {
@@ -141,6 +147,7 @@ public List<Object> getPreloadItems(int position) {
         return objects.subList(position, position + 1);
       }
 
+      @NonNull
       @Override
       @SuppressWarnings("unchecked")
       public RequestBuilder<Object> getPreloadRequestBuilder(Object item) {
@@ -160,6 +167,7 @@ public void testGetItemsIsNeverCalledWithEndGreaterThanTotalItems() {
     final AtomicBoolean called = new AtomicBoolean(false);
     final AtomicInteger calledCount = new AtomicInteger();
     ListPreloaderAdapter preloaderAdapter = new ListPreloaderAdapter() {
+      @NonNull
       @Override
       public List<Object> getPreloadItems(int position) {
         called.set(true);
@@ -179,6 +187,7 @@ public void testGetItemsIsNeverCalledWithStartLessThanZero() {
     final AtomicBoolean called = new AtomicBoolean(false);
     final AtomicInteger calledCount = new AtomicInteger();
     ListPreloaderAdapter preloaderAdapter = new ListPreloaderAdapter() {
+      @NonNull
       @Override
       public List<Object> getPreloadItems(int position) {
         if (position >= 17) {
@@ -202,6 +211,7 @@ public List<Object> getPreloadItems(int position) {
   public void testDontPreloadItemsRepeatedlyWhileIncreasing() {
     final AtomicInteger called = new AtomicInteger();
     ListPreloaderAdapter preloaderAdapter = new ListPreloaderAdapter() {
+      @NonNull
       @Override
       public List<Object> getPreloadItems(int position) {
         final int current = called.getAndIncrement();
@@ -222,6 +232,7 @@ public List<Object> getPreloadItems(int position) {
   public void testDontPreloadItemsRepeatedlyWhileDecreasing() {
     final AtomicInteger called = new AtomicInteger();
     ListPreloaderAdapter preloaderAdapter = new ListPreloaderAdapter() {
+      @NonNull
       @Override
       public List<Object> getPreloadItems(int position) {
         if (position >= 20) {
@@ -249,6 +260,7 @@ public void testMultipleItemsForPositionIncreasing() throws NoSuchFieldException
     ListPreloaderAdapter preloaderAdapter = new ListPreloaderAdapter() {
       private int expectedPosition = (1 + 10) * 2;
 
+      @NonNull
       @Override
       public List<Object> getPreloadItems(int position) {
         return objects;
@@ -262,6 +274,7 @@ public int[] getPreloadSize(Object item, int adapterPosition, int itemPosition)
         return itemPosition == 0 ? new int[] { 10, 11 } : new int[] { 20, 21 };
       }
 
+      @NonNull
       @Override
       public RequestBuilder<Object> getPreloadRequestBuilder(Object item) {
         return request;
@@ -285,6 +298,7 @@ public void testMultipleItemsForPositionDecreasing() throws NoSuchFieldException
     ListPreloaderAdapter preloaderAdapter = new ListPreloaderAdapter() {
       private int expectedPosition = objects.size() * 2 - 1;
 
+      @NonNull
       @Override
       public List<Object> getPreloadItems(int position) {
         return objects;
@@ -298,6 +312,7 @@ public int[] getPreloadSize(Object item, int adapterPosition, int itemPosition)
         return itemPosition == 0 ? new int[] { 10, 11 } : new int[] { 20, 21 };
       }
 
+      @NonNull
       @Override
       public RequestBuilder<Object> getPreloadRequestBuilder(Object item) {
         return request;
@@ -334,11 +349,13 @@ public void testItemsArePreloadedWithGlide() {
     objects.add(new Object());
     final HashSet<Object> loadedObjects = new HashSet<>();
     ListPreloaderAdapter preloaderAdapter = new ListPreloaderAdapter() {
+      @NonNull
       @Override
       public List<Object> getPreloadItems(int position) {
         return objects.subList(position - 11, position - 10);
       }
 
+      @NonNull
       @Override
       public RequestBuilder<Object> getPreloadRequestBuilder(Object item) {
         loadedObjects.add(item);
@@ -358,13 +375,15 @@ private static class ListPreloaderAdapter implements ListPreloader.PreloadModelP
     public ListPreloaderAdapter() {
     }
 
+    @NonNull
     @Override
     public List<Object> getPreloadItems(int position) {
       ArrayList<Object> result = new ArrayList<>(1);
       Collections.fill(result, new Object());
       return result;
     }
 
+    @NonNull
     @Override
     @SuppressWarnings("unchecked")
     public RequestBuilder<Object> getPreloadRequestBuilder(Object item) {

diff --git a/samples/flickr/src/main/java/com/bumptech/glide/samples/flickr/FlickrPhotoGrid.java b/samples/flickr/src/main/java/com/bumptech/glide/samples/flickr/FlickrPhotoGrid.java
@@ -4,6 +4,7 @@
 import android.graphics.Rect;
 import android.graphics.drawable.Drawable;
 import android.os.Bundle;
+import android.support.annotation.NonNull;
 import android.support.v4.app.Fragment;
 import android.support.v7.widget.GridLayoutManager;
 import android.support.v7.widget.RecyclerView;
@@ -191,14 +192,16 @@ public int getItemCount() {
       return photos.size();
     }
 
+    @NonNull
     @Override
     public List<Photo> getPreloadItems(int position) {
       return photos.subList(position, position + 1);
     }
 
+    @NonNull
     @Override
     public RequestBuilder<Drawable> getPreloadRequestBuilder(Photo item) {
-      return preloadRequest.load(item);
+      return fullRequest.clone().thumbnail(thumbnailRequest.clone().load(item)).load(item);
     }
   }
 

diff --git a/samples/flickr/src/main/java/com/bumptech/glide/samples/flickr/FlickrPhotoList.java b/samples/flickr/src/main/java/com/bumptech/glide/samples/flickr/FlickrPhotoList.java
@@ -7,6 +7,7 @@
 import android.graphics.drawable.ColorDrawable;
 import android.graphics.drawable.Drawable;
 import android.os.Bundle;
+import android.support.annotation.NonNull;
 import android.support.v4.app.Fragment;
 import android.support.v7.widget.LinearLayoutManager;
 import android.support.v7.widget.RecyclerView;
@@ -165,11 +166,13 @@ public int getItemCount() {
       return photos.size();
     }
 
+    @NonNull
     @Override
     public List<Photo> getPreloadItems(int position) {
       return photos.subList(position, position + 1);
     }
 
+    @NonNull
     @Override
     public RequestBuilder<Drawable> getPreloadRequestBuilder(Photo item) {
       return fullRequest.thumbnail(thumbRequest.load(item)).load(item);

diff --git a/samples/gallery/src/main/java/com/bumptech/glide/samples/gallery/RecyclerAdapter.java b/samples/gallery/src/main/java/com/bumptech/glide/samples/gallery/RecyclerAdapter.java
@@ -3,6 +3,7 @@
 import android.content.Context;
 import android.graphics.Point;
 import android.graphics.drawable.Drawable;
+import android.support.annotation.NonNull;
 import android.support.v7.widget.RecyclerView;
 import android.view.Display;
 import android.view.LayoutInflater;
@@ -91,11 +92,13 @@ public int getItemViewType(int position) {
     return 0;
   }
 
+  @NonNull
   @Override
   public List<MediaStoreData> getPreloadItems(int position) {
     return Collections.singletonList(data.get(position));
   }
 
+  @NonNull
   @Override
   public RequestBuilder<Drawable> getPreloadRequestBuilder(MediaStoreData item) {
     MediaStoreSignature signature =

diff --git a/samples/giphy/src/main/java/com/bumptech/glide/samples/giphy/MainActivity.java b/samples/giphy/src/main/java/com/bumptech/glide/samples/giphy/MainActivity.java
@@ -7,6 +7,7 @@
 import android.content.Intent;
 import android.graphics.drawable.Drawable;
 import android.os.Bundle;
+import android.support.annotation.NonNull;
 import android.support.v7.widget.LinearLayoutManager;
 import android.support.v7.widget.RecyclerView;
 import android.view.View;
@@ -137,11 +138,13 @@ public int getItemCount() {
       return results.length;
     }
 
+    @NonNull
     @Override
     public List<Api.GifResult> getPreloadItems(int position) {
       return Collections.singletonList(results[position]);
     }
 
+    @NonNull
     @Override
     public RequestBuilder<Drawable> getPreloadRequestBuilder(Api.GifResult item) {
       return requestBuilder.load(item);