FIAM: Fix FirebaseInAppMessagingDisplayErrorListener not being called (…

…#5719) Changed `GlideErrorListener` to a normal class and pass a new instance of the listener into each load image request instead of being injected as a defaultRequestListener. Refactored `InAppMessage` and `FirebaseInAppMessagingDisplayCallbacks` assignment code for clarity. Internal tracking: b/321732964 Issue: #5644
firebase · Feb 28, 2024 · f873ed4 · f873ed4
1 parent ba90420
commit f873ed4
Show file tree

Hide file tree

Showing 9 changed files with 48 additions and 38 deletions.
diff --git a/firebase-inappmessaging-display/CHANGELOG.md b/firebase-inappmessaging-display/CHANGELOG.md
@@ -1,5 +1,6 @@
 # Unreleased
-
+* [fixed] Fixed FirebaseInAppMessagingDisplayErrorListener not being called
+  (GitHub [#5644](//github.com/firebase/firebase-android-sdk/issues/5644))
 
 # 20.4.0
 * [changed] Added Kotlin extensions (KTX) APIs from `com.google.firebase:firebase-inappmessaging-display-ktx`

diff --git a/...c/main/java/com/google/firebase/inappmessaging/display/FirebaseInAppMessagingDisplay.java b/...c/main/java/com/google/firebase/inappmessaging/display/FirebaseInAppMessagingDisplay.java
@@ -40,6 +40,7 @@
 import com.google.firebase.inappmessaging.display.internal.FiamImageLoader;
 import com.google.firebase.inappmessaging.display.internal.FiamWindowManager;
 import com.google.firebase.inappmessaging.display.internal.FirebaseInAppMessagingDisplayImpl;
+import com.google.firebase.inappmessaging.display.internal.GlideErrorListener;
 import com.google.firebase.inappmessaging.display.internal.InAppMessageLayoutConfig;
 import com.google.firebase.inappmessaging.display.internal.Logging;
 import com.google.firebase.inappmessaging.display.internal.RenewableTimer;
@@ -144,9 +145,18 @@ public void testMessage(
       Activity activity,
       InAppMessage inAppMessage,
       FirebaseInAppMessagingDisplayCallbacks callbacks) {
+    setInAppMessageAndCallbacks(inAppMessage, callbacks);
+    showActiveFiam(activity);
+  }
+
+  private void setInAppMessageAndCallbacks(
+      InAppMessage inAppMessage, FirebaseInAppMessagingDisplayCallbacks callbacks) {
     this.inAppMessage = inAppMessage;
     this.callbacks = callbacks;
-    showActiveFiam(activity);
+  }
+
+  private void clearInAppMessageAndCallbacks() {
+    setInAppMessageAndCallbacks(null, null);
   }
 
   /**
@@ -204,8 +214,7 @@ private void bindFiamToActivity(Activity activity) {
               Logging.logd("Active FIAM exists. Skipping trigger");
               return;
             }
-            inAppMessage = iam;
-            callbacks = cb;
+            setInAppMessageAndCallbacks(iam, cb);
             showActiveFiam(activity);
           });
       // set the current activity to be the one passed in so that we know not to bind again to the
@@ -329,8 +338,7 @@ public void onClick(View v) {
                 // Ensure that we remove the displayed FIAM, and ensure that on re-load, the message
                 // isn't re-displayed
                 removeDisplayedFiam(activity);
-                inAppMessage = null;
-                callbacks = null;
+                clearInAppMessageAndCallbacks();
               }
             };
       } else {
@@ -432,8 +440,7 @@ public void onError(Exception e) {
                   .removeGlobalOnLayoutListener(layoutListener);
             }
             cancelTimers(); // Not strictly necessary.
-            inAppMessage = null;
-            callbacks = null;
+            clearInAppMessageAndCallbacks();
           }
         });
   }
@@ -492,6 +499,7 @@ private void loadNullableImage(
     if (isValidImageData(imageData)) {
       imageLoader
           .load(imageData.getImageUrl())
+          .addErrorListener(new GlideErrorListener(inAppMessage, callbacks))
           .tag(activity.getClass())
           .placeholder(R.drawable.image_placeholder)
           .into(fiam.getImageView(), callback);
@@ -506,8 +514,7 @@ private void dismissFiam(Activity activity) {
     Logging.logd("Dismissing fiam");
     notifyFiamDismiss();
     removeDisplayedFiam(activity);
-    inAppMessage = null;
-    callbacks = null;
+    clearInAppMessageAndCallbacks();
   }
 
   private void removeDisplayedFiam(Activity activity) {

diff --git a/...ay/src/main/java/com/google/firebase/inappmessaging/display/internal/FiamImageLoader.java b/...ay/src/main/java/com/google/firebase/inappmessaging/display/internal/FiamImageLoader.java
@@ -97,6 +97,11 @@ public FiamImageRequestCreator placeholder(int placeholderResId) {
       return this;
     }
 
+    public FiamImageRequestCreator addErrorListener(GlideErrorListener glideErrorListener) {
+      requestBuilder.addListener(glideErrorListener);
+      return this;
+    }
+
     public FiamImageRequestCreator tag(Class c) {
       tag = c.getSimpleName();
       checkAndTag();

diff --git a/...src/main/java/com/google/firebase/inappmessaging/display/internal/GlideErrorListener.java b/...src/main/java/com/google/firebase/inappmessaging/display/internal/GlideErrorListener.java
@@ -14,40 +14,28 @@
 
 package com.google.firebase.inappmessaging.display.internal;
 
-// Picasso 's api forces us to listen to errors only using a global listener set on the picasso
-// singleton. Since we initialize picasso from a static context and the in app message param to the
-// logError method is not available statically, we are forced to introduce a error listener with
-// mutable state so that the error from picasso can be translated to a logError on
-// fiam headless, with the in app message as a parameter
-
+import android.graphics.drawable.Drawable;
 import androidx.annotation.Nullable;
 import com.bumptech.glide.load.DataSource;
 import com.bumptech.glide.load.engine.GlideException;
 import com.bumptech.glide.request.RequestListener;
 import com.bumptech.glide.request.target.Target;
 import com.google.firebase.inappmessaging.FirebaseInAppMessagingDisplayCallbacks;
-import com.google.firebase.inappmessaging.display.internal.injection.scopes.FirebaseAppScope;
 import com.google.firebase.inappmessaging.model.InAppMessage;
-import javax.inject.Inject;
-
-/** @hide */
-@FirebaseAppScope
-public class GlideErrorListener implements RequestListener<Object> {
-  private InAppMessage inAppMessage;
-  private FirebaseInAppMessagingDisplayCallbacks displayCallbacks;
 
-  @Inject
-  GlideErrorListener() {}
+public class GlideErrorListener implements RequestListener<Drawable> {
+  private final InAppMessage inAppMessage;
+  private final FirebaseInAppMessagingDisplayCallbacks displayCallbacks;
 
-  public void setInAppMessage(
+  public GlideErrorListener(
       InAppMessage inAppMessage, FirebaseInAppMessagingDisplayCallbacks displayCallbacks) {
     this.inAppMessage = inAppMessage;
     this.displayCallbacks = displayCallbacks;
   }
 
   @Override
   public boolean onLoadFailed(
-      @Nullable GlideException e, Object model, Target<Object> target, boolean isFirstResource) {
+      @Nullable GlideException e, Object model, Target<Drawable> target, boolean isFirstResource) {
     Logging.logd("Image Downloading  Error : " + e.getMessage() + ":" + e.getCause());
 
     if (inAppMessage != null && displayCallbacks != null) {
@@ -67,9 +55,9 @@ public boolean onLoadFailed(
 
   @Override
   public boolean onResourceReady(
-      Object resource,
+      Drawable resource,
       Object model,
-      Target<Object> target,
+      Target<Drawable> target,
       DataSource dataSource,
       boolean isFirstResource) {
     Logging.logd("Image Downloading  Success : " + resource);

diff --git a/...om/google/firebase/inappmessaging/display/internal/injection/components/AppComponent.java b/...om/google/firebase/inappmessaging/display/internal/injection/components/AppComponent.java
@@ -16,7 +16,6 @@
 
 import com.google.firebase.inappmessaging.display.FirebaseInAppMessagingDisplay;
 import com.google.firebase.inappmessaging.display.internal.FiamImageLoader;
-import com.google.firebase.inappmessaging.display.internal.GlideErrorListener;
 import com.google.firebase.inappmessaging.display.internal.injection.modules.GlideModule;
 import com.google.firebase.inappmessaging.display.internal.injection.modules.HeadlessInAppMessagingModule;
 import com.google.firebase.inappmessaging.display.internal.injection.scopes.FirebaseAppScope;
@@ -31,7 +30,5 @@ public interface AppComponent {
   @FirebaseAppScope
   FirebaseInAppMessagingDisplay providesFirebaseInAppMessagingUI();
 
-  GlideErrorListener glideErrorListener();
-
   FiamImageLoader fiamImageLoader();
 }
diff --git a/...va/com/google/firebase/inappmessaging/display/internal/injection/modules/GlideModule.java b/...va/com/google/firebase/inappmessaging/display/internal/injection/modules/GlideModule.java
@@ -17,7 +17,6 @@
 import android.app.Application;
 import com.bumptech.glide.Glide;
 import com.bumptech.glide.RequestManager;
-import com.google.firebase.inappmessaging.display.internal.GlideErrorListener;
 import com.google.firebase.inappmessaging.display.internal.injection.scopes.FirebaseAppScope;
 import dagger.Module;
 import dagger.Provides;
@@ -27,8 +26,7 @@
 public class GlideModule {
   @Provides
   @FirebaseAppScope
-  RequestManager providesGlideRequestManager(
-      Application application, GlideErrorListener glideErrorListener) {
-    return Glide.with(application).addDefaultRequestListener(glideErrorListener);
+  RequestManager providesGlideRequestManager(Application application) {
+    return Glide.with(application);
   }
 }
diff --git a/...st/java/com/google/firebase/inappmessaging/display/FirebaseInAppMessagingDisplayTest.java b/...st/java/com/google/firebase/inappmessaging/display/FirebaseInAppMessagingDisplayTest.java
@@ -26,6 +26,7 @@
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.ArgumentMatchers.refEq;
 import static org.mockito.Mockito.atLeastOnce;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
@@ -61,6 +62,7 @@
 import com.google.firebase.inappmessaging.display.internal.FiamImageLoader;
 import com.google.firebase.inappmessaging.display.internal.FiamImageLoader.Callback;
 import com.google.firebase.inappmessaging.display.internal.FiamWindowManager;
+import com.google.firebase.inappmessaging.display.internal.GlideErrorListener;
 import com.google.firebase.inappmessaging.display.internal.InAppMessageLayoutConfig;
 import com.google.firebase.inappmessaging.display.internal.RenewableTimer;
 import com.google.firebase.inappmessaging.display.internal.bindingwrappers.BannerBindingWrapper;
@@ -419,6 +421,8 @@ public void streamListener_whenImageUrlExists_loadsImage() {
 
     verify(fiamImageRequestCreator).tag(TestActivity.class);
     verify(fiamImageRequestCreator).placeholder(R.drawable.image_placeholder);
+    verify(fiamImageRequestCreator)
+        .addErrorListener(refEq(new GlideErrorListener(IMAGE_MESSAGE_MODEL, callbacks)));
     verify(fiamImageRequestCreator).into(any(ImageView.class), any(Callback.class));
   }
 
@@ -428,6 +432,7 @@ public void streamListener_whenNoImageUrlExists_doesNotLoadImage() {
     listener.displayMessage(null, callbacks);
     verify(fiamImageRequestCreator, times(0)).tag(TestActivity.class);
     verify(fiamImageRequestCreator, times(0)).placeholder(R.drawable.image_placeholder);
+    verify(fiamImageRequestCreator, times(0)).addErrorListener(any(GlideErrorListener.class));
     verify(fiamImageRequestCreator, times(0)).into(any(ImageView.class), any(Callback.class));
   }
 

diff --git a/...rc/test/java/com/google/firebase/inappmessaging/display/internal/FiamImageLoaderTest.java b/...rc/test/java/com/google/firebase/inappmessaging/display/internal/FiamImageLoaderTest.java
@@ -64,6 +64,14 @@ public void placeholder_setsPlaceholderOnUnderlyingRequestCreator() {
     verify(requestBuilder).placeholder(1);
   }
 
+  @Test
+  public void addErrorListener_setsErrorListenerOnUnderlyingRequestCreator() {
+    FiamImageLoader.FiamImageRequestCreator fiamImageRequestCreator = imageLoader.load(IMAGE_URL);
+    GlideErrorListener errorListener = new GlideErrorListener(null, null);
+    fiamImageRequestCreator.addErrorListener(errorListener);
+    verify(requestBuilder).addListener(errorListener);
+  }
+
   @Test
   public void tag_tagsUnderlyingRequestCreator() {
     ImageView imageView = mock(ImageView.class);

diff --git a/firebase-inappmessaging/CHANGELOG.md b/firebase-inappmessaging/CHANGELOG.md
@@ -1,5 +1,6 @@
 # Unreleased
-
+* [fixed] Fixed FirebaseInAppMessagingDisplayErrorListener not being called
+  (GitHub [#5644](//github.com/firebase/firebase-android-sdk/issues/5644))
 
 # 20.4.0
 * [changed] Added Kotlin extensions (KTX) APIs from `com.google.firebase:firebase-inappmessaging-ktx`