Start animatable resources after setting the resource in ImageViewTarget

After 90b3b9f we have a case where a
GifDrawable may load a frame synchronously when it’s started. If the
GifDrawable was started before it was attached to a View (and gets a 
non-null Callback) and the GifDrawable had a frame pending, then the
GifDrawable would stop itself in the start() call, causing the 
drawable not to animate. We can avoid this scenario by changing the 
order in which we start animated drawables and set them on views so that
we only start the drawable after its set on the View.

Fixes #2541

instrumentation/src/androidTest/java/com/bumptech/glide/load/resource/gif/GifDrawableTest.java

@@ -0,0 +1,157 @@
+package com.bumptech.glide.load.resource.gif;
+
+import static android.support.test.InstrumentationRegistry.getTargetContext;
+import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.fail;
+import static org.junit.Assume.assumeTrue;
+
+import android.content.Context;
+import android.graphics.drawable.Drawable;
+import android.os.Build;
+import android.os.Handler;
+import android.os.Looper;
+import android.support.annotation.Nullable;
+import android.support.test.runner.AndroidJUnit4;
+import android.view.View;
+import android.view.WindowManager;
+import android.view.WindowManager.LayoutParams;
+import android.widget.ImageView;
+import com.bumptech.glide.load.DataSource;
+import com.bumptech.glide.load.engine.GlideException;
+import com.bumptech.glide.load.resource.gif.GifDrawable.GifState;
+import com.bumptech.glide.load.resource.gif.GifFrameLoader.OnEveryFrameListener;
+import com.bumptech.glide.request.RequestListener;
+import com.bumptech.glide.request.target.Target;
+import com.bumptech.glide.test.GlideApp;
+import com.bumptech.glide.test.ResourceIds;
+import com.bumptech.glide.test.TearDownGlide;
+import com.bumptech.glide.util.Preconditions;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TestName;
+import org.junit.runner.RunWith;
+
+@RunWith(AndroidJUnit4.class)
+public class GifDrawableTest {
+  @Rule public TestName testName = new TestName();
+  @Rule public TearDownGlide tearDownGlide = new TearDownGlide();
+  private Context context;
+  private Handler mainHandler;
+
+  @Before
+  public void setUp() {
+    // Required for us to add a View to a Window.
+    assumeTrue(Build.VERSION.SDK_INT < Build.VERSION_CODES.M);
+
+    context = getTargetContext();
+    mainHandler = new Handler(Looper.getMainLooper());
+  }
+
+  @Test
+  public void loadGif_intoImageView_afterStop_restartsGif()
+      throws ExecutionException, InterruptedException {
+    // Mimic the state the Drawable can get into if it was loaded into a View previously and stopped
+    // so that it ended up with a pending frame that finished after the stop call.
+    final GifDrawable gifDrawable = GlideApp.with(context)
+        .asGif()
+        .load(ResourceIds.raw.dl_world_anim)
+        .submit(Target.SIZE_ORIGINAL, Target.SIZE_ORIGINAL)
+        .get();
+    final CountDownLatch waitForGifFrame = new CountDownLatch(1);
+    // Starting/Stopping loads in GIFs must happen on the main thread.
+    mainHandler.post(
+        new Runnable() {
+          @Override
+          public void run() {
+            // Make sure a frame is loaded while the drawable is stopped.
+            GifState gifState =
+                (GifState) Preconditions.checkNotNull(gifDrawable.getConstantState());
+            gifState.frameLoader.setOnEveryFrameReadyListener(new OnEveryFrameListener() {
+              @Override
+              public void onFrameReady() {
+                waitForGifFrame.countDown();
+              }
+            });
+            gifDrawable.start();
+            gifDrawable.stop();
+          }
+        });
+    waitOrThrow(waitForGifFrame);
+
+    // Load the Drawable with the pending frame into a new View and make sure it ends up in the
+    // running state.
+    final ImageView imageView = new ImageView(context);
+    final WaitForLoad<Drawable> waitForLoad = new WaitForLoad<>();
+    // Starting loads into Views must happen on the main thread.
+    mainHandler
+        .post(new Runnable() {
+          @Override
+          public void run() {
+            addViewToWindow(imageView);
+            GlideApp.with(context)
+                .load(gifDrawable)
+                .listener(waitForLoad)
+                .override(Target.SIZE_ORIGINAL)
+                .into(imageView);
+          }
+        });
+    waitForLoad.await();
+
+    GifDrawable drawableFromView = (GifDrawable) imageView.getDrawable();
+    assertThat(drawableFromView.isRunning()).isTrue();
+  }
+
+  // LayoutParams.TYPE_SYSTEM_ALERT.
+  @SuppressWarnings("deprecation")
+  private void addViewToWindow(View view) {
+    final WindowManager.LayoutParams layoutParams = new WindowManager.LayoutParams();
+    layoutParams.height = WindowManager.LayoutParams.MATCH_PARENT;
+    layoutParams.width = WindowManager.LayoutParams.MATCH_PARENT;
+    layoutParams.type = LayoutParams.TYPE_SYSTEM_ALERT;
+    final WindowManager windowManager =
+        (WindowManager) context.getSystemService(Context.WINDOW_SERVICE);
+    windowManager.addView(view, layoutParams);
+  }
+
+  private static void waitOrThrow(CountDownLatch latch) {
+    try {
+      if (!latch.await(5, TimeUnit.SECONDS)) {
+        fail("Failed to reach expected condition in the alloted time.");
+      }
+    } catch (InterruptedException e) {
+      throw new RuntimeException(e);
+    }
+  }
+
+  private final class WaitForLoad<T> implements RequestListener<T> {
+    private final CountDownLatch countDownLatch = new CountDownLatch(1);
+
+    void await() {
+      waitOrThrow(countDownLatch);
+    }
+
+    @Override
+    public boolean onLoadFailed(@Nullable GlideException e, Object model,
+        Target<T> target,
+        boolean isFirstResource) {
+      throw new RuntimeException(e);
+    }
+
+    @Override
+    public boolean onResourceReady(T resource, Object model, Target<T> target,
+        DataSource dataSource,
+        boolean isFirstResource) {
+      mainHandler.post(new Runnable() {
+        @Override
+        public void run() {
+          countDownLatch.countDown();
+        }
+      });
+      return false;
+    }
+  }
+}

instrumentation/src/androidTest/java/com/bumptech/glide/test/ResourceIds.java

@@ -13,6 +13,10 @@ private ResourceIds() {
     // Utility class.
   }
 
+  public interface raw {
+    int dl_world_anim = getResourceId("raw", "dl_world_anim");
+  }
+
   public interface drawable {
     int bitmap_alias = getResourceId("drawable", "bitmap_alias");
     int googlelogo_color_120x44dp= getResourceId("drawable", "googlelogo_color_120x44dp");

instrumentation/src/main/AndroidManifest.xml

@@ -1,5 +1,6 @@
 <manifest xmlns:android="http://schemas.android.com/apk/res/android"
   package="com.bumptech.glide.instrumentation">
   <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" />
+  <uses-permission android:name="android.permission.SYSTEM_ALERT_WINDOW" />
   <application />
 </manifest>

library/src/main/java/com/bumptech/glide/load/resource/gif/GifFrameLoader.java

@@ -8,6 +8,8 @@
 import android.os.Looper;
 import android.os.Message;
 import android.os.SystemClock;
+import android.support.annotation.Nullable;
+import android.support.annotation.VisibleForTesting;
 import com.bumptech.glide.Glide;
 import com.bumptech.glide.RequestBuilder;
 import com.bumptech.glide.RequestManager;
@@ -44,6 +46,8 @@ class GifFrameLoader {
   private Bitmap firstFrame;
   private Transformation<Bitmap> transformation;
   private DelayTarget pendingTarget;
+  @Nullable
+  private GifFrameLoader.OnEveryFrameListener onEveryFrameListener;
 
   public interface FrameCallback {
     void onFrameReady();
@@ -237,8 +241,16 @@ void setNextStartFromFirstFrame() {
     }
   }
 
-  // Visible for testing.
+  @VisibleForTesting
+  void setOnEveryFrameReadyListener(@Nullable OnEveryFrameListener onEveryFrameListener) {
+    this.onEveryFrameListener = onEveryFrameListener;
+  }
+
+  @VisibleForTesting
   void onFrameReady(DelayTarget delayTarget) {
+    if (onEveryFrameListener != null) {
+      onEveryFrameListener.onFrameReady();
+    }
     isLoadPending = false;
     if (isCleared) {
       handler.obtainMessage(FrameLoaderCallback.MSG_CLEAR, delayTarget).sendToTarget();
@@ -333,4 +345,9 @@ private static Key getFrameSignature() {
     // See #1510.
     return new ObjectKey(Math.random());
   }
+
+  @VisibleForTesting
+  interface OnEveryFrameListener {
+    void onFrameReady();
+  }
 }

library/src/main/java/com/bumptech/glide/request/target/ImageViewTarget.java

@@ -111,8 +111,10 @@ public void onStop() {
   }
 
   private void setResourceInternal(@Nullable Z resource) {
-    maybeUpdateAnimatable(resource);
+    // Order matters here. Set the resource first to make sure that the Drawable has a valid and
+    // non-null Callback before starting it.
     setResource(resource);
+    maybeUpdateAnimatable(resource);
   }
 
   private void maybeUpdateAnimatable(@Nullable Z resource) {

library/src/test/java/com/bumptech/glide/request/target/ImageViewTargetTest.java

@@ -4,18 +4,21 @@
 import static org.junit.Assert.assertNull;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.eq;
+import static org.mockito.Mockito.inOrder;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 import android.graphics.Color;
+import android.graphics.drawable.Animatable;
 import android.graphics.drawable.ColorDrawable;
 import android.graphics.drawable.Drawable;
 import android.widget.ImageView;
 import com.bumptech.glide.request.transition.Transition;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.InOrder;
 import org.robolectric.RobolectricTestRunner;
 import org.robolectric.RuntimeEnvironment;
 import org.robolectric.annotation.Config;
@@ -102,7 +105,23 @@ public void testProvidesCurrentPlaceholderToAnimationIfPresent() {
     verify(animation).transition(eq(placeholder), eq(target));
   }
 
-  private static class TestTarget extends ImageViewTarget<Drawable> {
+  @Test
+  public void onResourceReady_withAnimatableResource_startsAnimatableAfterSetResource() {
+    AnimatedDrawable drawable = mock(AnimatedDrawable.class);
+    ImageView view = mock(ImageView.class);
+    target = new TestTarget(view);
+    target.onResourceReady(drawable, /*transition=*/ null);
+
+    InOrder order = inOrder(view, drawable);
+    order.verify(view).setImageDrawable(drawable);
+    order.verify(drawable).start();
+  }
+
+  private abstract static class AnimatedDrawable extends Drawable implements Animatable {
+    // Intentionally empty.
+  }
+
+  private static final class TestTarget extends ImageViewTarget<Drawable> {
     public Drawable resource;
 
     public TestTarget(ImageView view) {
@@ -112,6 +131,7 @@ public TestTarget(ImageView view) {
     @Override
     protected void setResource(Drawable resource) {
       this.resource = resource;
+      view.setImageDrawable(resource);
     }
   }
 }

samples/gallery/src/main/java/com/bumptech/glide/samples/gallery/RecyclerAdapter.java

@@ -2,7 +2,6 @@
 
 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;
@@ -15,6 +14,7 @@
 import com.bumptech.glide.ListPreloader;
 import com.bumptech.glide.RequestBuilder;
 import com.bumptech.glide.load.Key;
+import com.bumptech.glide.load.resource.gif.GifDrawable;
 import com.bumptech.glide.signature.MediaStoreSignature;
 import java.util.Collections;
 import java.util.List;
@@ -28,13 +28,13 @@ class RecyclerAdapter extends RecyclerView.Adapter<RecyclerAdapter.ListViewHolde
 
   private final List<MediaStoreData> data;
   private final int screenWidth;
-  private final GlideRequest<Drawable> requestBuilder;
+  private final GlideRequest<GifDrawable> requestBuilder;
 
   private int[] actualDimensions;
 
   RecyclerAdapter(Context context, List<MediaStoreData> data, GlideRequests glideRequests) {
     this.data = data;
-    requestBuilder = glideRequests.asDrawable().fitCenter();
+    requestBuilder = glideRequests.asGif().fitCenter();
 
     setHasStableIds(true);
 
@@ -100,7 +100,7 @@ public List<MediaStoreData> getPreloadItems(int position) {
 
   @NonNull
   @Override
-  public RequestBuilder<Drawable> getPreloadRequestBuilder(MediaStoreData item) {
+  public RequestBuilder<GifDrawable> getPreloadRequestBuilder(MediaStoreData item) {
     MediaStoreSignature signature =
         new MediaStoreSignature(item.mimeType, item.dateModified, item.orientation);
     return requestBuilder