From 8f354dc527b46a69a47713e6e625cee2750eb6a0 Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Tue, 12 May 2020 12:06:27 -0700 Subject: [PATCH] Check if Activitys are FragmentActivities in RequestManagerRetriever This fixes an issue where we can end up adding both a support and a non-support Fragment to a FragmentActivity if the FragmentActivity is passed to Glide as an Activity or Context instead of a FragmentContext at some point. --- instrumentation/build.gradle | 2 +- .../bumptech/glide/RequestManagerTest.java | 68 ++++++++++++++++++- instrumentation/src/main/AndroidManifest.xml | 4 +- ...lideWithAsDifferentSupertypesActivity.java | 19 ++++++ .../manager/RequestManagerRetriever.java | 2 + 5 files changed, 92 insertions(+), 3 deletions(-) create mode 100644 instrumentation/src/main/java/com/bumptech/glide/test/GlideWithAsDifferentSupertypesActivity.java diff --git a/instrumentation/build.gradle b/instrumentation/build.gradle index 0cebef0125..054997376e 100644 --- a/instrumentation/build.gradle +++ b/instrumentation/build.gradle @@ -29,7 +29,7 @@ android { defaultConfig { applicationId 'com.bumptech.glide.instrumentation' - minSdkVersion MIN_SDK_VERSION as int + minSdkVersion 16 as int targetSdkVersion TARGET_SDK_VERSION as int versionCode 1 versionName '1.0' diff --git a/instrumentation/src/androidTest/java/com/bumptech/glide/RequestManagerTest.java b/instrumentation/src/androidTest/java/com/bumptech/glide/RequestManagerTest.java index 26b9e3be9e..854a1e8807 100644 --- a/instrumentation/src/androidTest/java/com/bumptech/glide/RequestManagerTest.java +++ b/instrumentation/src/androidTest/java/com/bumptech/glide/RequestManagerTest.java @@ -1,19 +1,32 @@ package com.bumptech.glide; +import static com.google.common.truth.Truth.assertThat; + import android.content.Context; import android.graphics.drawable.Drawable; +import android.os.Build; +import android.os.Bundle; import android.widget.ImageView; import androidx.annotation.NonNull; +import androidx.lifecycle.Lifecycle.State; +import androidx.test.core.app.ActivityScenario; +import androidx.test.core.app.ActivityScenario.ActivityAction; import androidx.test.core.app.ApplicationProvider; import androidx.test.ext.junit.runners.AndroidJUnit4; import com.bumptech.glide.manager.Lifecycle; import com.bumptech.glide.manager.LifecycleListener; +import com.bumptech.glide.manager.RequestManagerFragment; import com.bumptech.glide.manager.RequestManagerTreeNode; +import com.bumptech.glide.manager.SupportRequestManagerFragment; import com.bumptech.glide.request.target.Target; import com.bumptech.glide.test.ConcurrencyHelper; +import com.bumptech.glide.test.GlideWithAsDifferentSupertypesActivity; import com.bumptech.glide.test.ResourceIds; import com.bumptech.glide.test.ResourceIds.raw; import com.bumptech.glide.test.TearDownGlide; +import com.google.common.collect.Iterables; +import java.util.ArrayList; +import java.util.List; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -77,7 +90,7 @@ public void run() { /** Tests b/69361054. */ @Test - public void clear_withNonOwningRequestManager_onBackgroundTHread_doesNotThrow() { + public void clear_withNonOwningRequestManager_onBackgroundThread_doesNotThrow() { concurrency.runOnMainThread( new Runnable() { @Override @@ -96,4 +109,57 @@ public void run() { } }); } + + @Test + public void with_asDifferentSuperTypes_doesNotAddMultipleFragments() { + ActivityScenario scenario = + ActivityScenario.launch(GlideWithAsDifferentSupertypesActivity.class); + scenario.moveToState(State.RESUMED); + scenario.onActivity( + new ActivityAction() { + @Override + public void perform(GlideWithAsDifferentSupertypesActivity activity) { + Iterable glideSupportFragments = + Iterables.filter( + activity.getSupportFragmentManager().getFragments(), + SupportRequestManagerFragment.class); + Iterable normalFragments = + Iterables.filter( + getAllFragments(activity.getFragmentManager()), RequestManagerFragment.class); + assertThat(normalFragments).hasSize(0); + assertThat(glideSupportFragments).hasSize(1); + } + }); + } + + private List getAllFragments(android.app.FragmentManager fragmentManager) { + return Build.VERSION.SDK_INT >= Build.VERSION_CODES.O + ? fragmentManager.getFragments() + : getAllFragmentsPreO(fragmentManager); + } + + // Hacks based on the implementation of FragmentManagerImpl in the non-support libraries that + // allow us to iterate over and retrieve all active Fragments in a FragmentManager. + private static final String FRAGMENT_INDEX_KEY = "key"; + + private List getAllFragmentsPreO( + android.app.FragmentManager fragmentManager) { + Bundle tempBundle = new Bundle(); + int index = 0; + List result = new ArrayList<>(); + while (true) { + tempBundle.putInt(FRAGMENT_INDEX_KEY, index++); + android.app.Fragment fragment = null; + try { + fragment = fragmentManager.getFragment(tempBundle, FRAGMENT_INDEX_KEY); + } catch (Exception e) { + // This generates log spam from FragmentManager anyway. + } + if (fragment == null) { + break; + } + result.add(fragment); + } + return result; + } } diff --git a/instrumentation/src/main/AndroidManifest.xml b/instrumentation/src/main/AndroidManifest.xml index fa6e2915fe..179a68494d 100644 --- a/instrumentation/src/main/AndroidManifest.xml +++ b/instrumentation/src/main/AndroidManifest.xml @@ -2,5 +2,7 @@ package="com.bumptech.glide.instrumentation"> - + + + diff --git a/instrumentation/src/main/java/com/bumptech/glide/test/GlideWithAsDifferentSupertypesActivity.java b/instrumentation/src/main/java/com/bumptech/glide/test/GlideWithAsDifferentSupertypesActivity.java new file mode 100644 index 0000000000..1cc56e2b28 --- /dev/null +++ b/instrumentation/src/main/java/com/bumptech/glide/test/GlideWithAsDifferentSupertypesActivity.java @@ -0,0 +1,19 @@ +package com.bumptech.glide.test; + +import android.app.Activity; +import android.content.Context; +import android.os.Bundle; +import androidx.annotation.Nullable; +import androidx.fragment.app.FragmentActivity; +import com.bumptech.glide.Glide; + +public class GlideWithAsDifferentSupertypesActivity extends FragmentActivity { + + @Override + protected void onCreate(@Nullable Bundle savedInstanceState) { + super.onCreate(savedInstanceState); + Glide.with(this); + Glide.with((Context) this); + Glide.with((Activity) this); + } +} diff --git a/library/src/main/java/com/bumptech/glide/manager/RequestManagerRetriever.java b/library/src/main/java/com/bumptech/glide/manager/RequestManagerRetriever.java index b8ec99baa9..5bc885a245 100644 --- a/library/src/main/java/com/bumptech/glide/manager/RequestManagerRetriever.java +++ b/library/src/main/java/com/bumptech/glide/manager/RequestManagerRetriever.java @@ -149,6 +149,8 @@ public RequestManager get(@NonNull Fragment fragment) { public RequestManager get(@NonNull Activity activity) { if (Util.isOnBackgroundThread()) { return get(activity.getApplicationContext()); + } else if (activity instanceof FragmentActivity) { + return get((FragmentActivity) activity); } else { assertNotDestroyed(activity); android.app.FragmentManager fm = activity.getFragmentManager();