From 18bba927a5e5fe7d07ada9667e9e503b9f0596a2 Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Thu, 8 Dec 2022 18:25:33 -0800 Subject: [PATCH] Use Lifecycle instead of Fragments by default PiperOrigin-RevId: 494046303 --- .../glide/RequestManagerLifecycleTest.java | 19 +-- .../bumptech/glide/RequestManagerTest.java | 124 ------------------ .../manager/RequestManagerRetriever.java | 62 ++------- .../manager/RequestManagerRetrieverTest.java | 3 +- 4 files changed, 13 insertions(+), 195 deletions(-) diff --git a/instrumentation/src/androidTest/java/com/bumptech/glide/RequestManagerLifecycleTest.java b/instrumentation/src/androidTest/java/com/bumptech/glide/RequestManagerLifecycleTest.java index f54eb54a67..1fd227fb86 100644 --- a/instrumentation/src/androidTest/java/com/bumptech/glide/RequestManagerLifecycleTest.java +++ b/instrumentation/src/androidTest/java/com/bumptech/glide/RequestManagerLifecycleTest.java @@ -16,23 +16,19 @@ 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.rules.ActivityScenarioRule; +import androidx.test.ext.junit.runners.AndroidJUnit4; import com.bumptech.glide.instrumentation.R; import com.bumptech.glide.test.DefaultFragmentActivity; import com.bumptech.glide.testutil.TearDownGlide; -import com.google.common.collect.ImmutableList; import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; -import org.junit.runners.Parameterized; -import org.junit.runners.Parameterized.Parameter; -import org.junit.runners.Parameterized.Parameters; // This test avoids using FragmentScenario because it doesn't seem to let us to get into the common // created but not yet started state, only either before onCreateView or after onResume. -@RunWith(Parameterized.class) +@RunWith(AndroidJUnit4.class) public class RequestManagerLifecycleTest { private static final String FRAGMENT_TAG = "fragment"; private static final String FRAGMENT_SIBLING_TAG = "fragment_sibling"; @@ -45,19 +41,8 @@ public class RequestManagerLifecycleTest { private ActivityScenario scenario; - @Parameter public boolean useLifecycleInsteadOfInjectingFragments; - - @Parameters(name = "useLifecycleInsteadOfInjectingFragments = {0}") - public static ImmutableList parameters() { - return ImmutableList.of(true, false); - } - @Before public void setUp() { - Glide.init( - ApplicationProvider.getApplicationContext(), - new GlideBuilder() - .useLifecycleInsteadOfInjectingFragments(useLifecycleInsteadOfInjectingFragments)); scenario = scenarioRule.getScenario(); } diff --git a/instrumentation/src/androidTest/java/com/bumptech/glide/RequestManagerTest.java b/instrumentation/src/androidTest/java/com/bumptech/glide/RequestManagerTest.java index 92c0aa17d2..d7139f54f9 100644 --- a/instrumentation/src/androidTest/java/com/bumptech/glide/RequestManagerTest.java +++ b/instrumentation/src/androidTest/java/com/bumptech/glide/RequestManagerTest.java @@ -1,36 +1,19 @@ 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.os.Handler; -import android.os.Looper; import android.widget.ImageView; import androidx.annotation.NonNull; -import androidx.fragment.app.Fragment; -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.GlideWithAsDifferentSupertypesActivity; -import com.bumptech.glide.test.GlideWithBeforeSuperOnCreateActivity; import com.bumptech.glide.test.ResourceIds; import com.bumptech.glide.test.ResourceIds.raw; import com.bumptech.glide.testutil.ConcurrencyHelper; import com.bumptech.glide.testutil.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; @@ -113,111 +96,4 @@ public void run() { } }); } - - @Test - public void with_beforeActivitySuperOnCreate_onlyAddsOneRequestManagerFragment() { - ActivityScenario scenario = - ActivityScenario.launch(GlideWithBeforeSuperOnCreateActivity.class); - scenario.moveToState(State.RESUMED); - scenario.onActivity( - new ActivityAction() { - @Override - public void perform(GlideWithBeforeSuperOnCreateActivity activity) { - List fragments = activity.getSupportFragmentManager().getFragments(); - List glideFragments = new ArrayList<>(); - for (Fragment fragment : fragments) { - if (fragment instanceof SupportRequestManagerFragment) { - glideFragments.add(fragment); - } - } - // Ideally this would be exactly 1, but it's a bit tricky to implement. For now we're - // content making sure that we're not adding multiple fragments. - assertThat(glideFragments.size()).isAtMost(1); - } - }); - scenario.onActivity( - new ActivityAction() { - @Override - public void perform(final GlideWithBeforeSuperOnCreateActivity activity) { - new Handler(Looper.getMainLooper()) - .post( - new Runnable() { - @Override - public void run() { - Glide.with(activity); - } - }); - } - }); - scenario.onActivity( - new ActivityAction() { - @Override - public void perform(GlideWithBeforeSuperOnCreateActivity activity) { - List fragments = activity.getSupportFragmentManager().getFragments(); - List glideFragments = new ArrayList<>(); - for (Fragment fragment : fragments) { - if (fragment instanceof SupportRequestManagerFragment) { - glideFragments.add(fragment); - } - } - // Now that we've called Glide.with() after commitAllowingStateLoss will actually add - // the - // fragment, ie after onCreate, we can expect exactly one Fragment instance. - assertThat(glideFragments.size()).isEqualTo(1); - } - }); - } - - @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/library/src/main/java/com/bumptech/glide/manager/RequestManagerRetriever.java b/library/src/main/java/com/bumptech/glide/manager/RequestManagerRetriever.java index 3eb85cdb47..d47965d43a 100644 --- a/library/src/main/java/com/bumptech/glide/manager/RequestManagerRetriever.java +++ b/library/src/main/java/com/bumptech/glide/manager/RequestManagerRetriever.java @@ -23,7 +23,6 @@ import androidx.fragment.app.FragmentManager; import androidx.fragment.app.FragmentTransaction; import com.bumptech.glide.Glide; -import com.bumptech.glide.GlideBuilder; import com.bumptech.glide.GlideBuilder.WaitForFramesAfterTrimMemory; import com.bumptech.glide.GlideExperiments; import com.bumptech.glide.RequestManager; @@ -71,7 +70,6 @@ public class RequestManagerRetriever implements Handler.Callback { private final Handler handler; private final RequestManagerFactory factory; - private final GlideExperiments experiments; // Objects used to find Fragments and Activities containing views. private final ArrayMap tempViewToSupportFragment = new ArrayMap<>(); @@ -86,7 +84,6 @@ public class RequestManagerRetriever implements Handler.Callback { public RequestManagerRetriever( @Nullable RequestManagerFactory factory, GlideExperiments experiments) { this.factory = factory != null ? factory : DEFAULT_FACTORY; - this.experiments = experiments; handler = new Handler(Looper.getMainLooper(), this /* Callback */); lifecycleRequestManagerRetriever = new LifecycleRequestManagerRetriever(this.factory); frameWaiter = buildFrameWaiter(experiments); @@ -156,24 +153,14 @@ public RequestManager get(@NonNull FragmentActivity activity) { } assertNotDestroyed(activity); frameWaiter.registerSelf(activity); - FragmentManager fm = activity.getSupportFragmentManager(); boolean isActivityVisible = isActivityVisible(activity); - if (useLifecycleInsteadOfInjectingFragments()) { - Context context = activity.getApplicationContext(); - Glide glide = Glide.get(context); - return lifecycleRequestManagerRetriever.getOrCreate( - context, - glide, - activity.getLifecycle(), - activity.getSupportFragmentManager(), - isActivityVisible); - } else { - return supportFragmentGet(activity, fm, /* parentHint= */ null, isActivityVisible); - } - } - - private boolean useLifecycleInsteadOfInjectingFragments() { - return experiments.isEnabled(GlideBuilder.UseLifecycleInsteadOfInjectingFragments.class); + Glide glide = Glide.get(activity.getApplicationContext()); + return lifecycleRequestManagerRetriever.getOrCreate( + activity, + glide, + activity.getLifecycle(), + activity.getSupportFragmentManager(), + isActivityVisible); } @NonNull @@ -193,13 +180,9 @@ public RequestManager get(@NonNull Fragment fragment) { } FragmentManager fm = fragment.getChildFragmentManager(); Context context = fragment.getContext(); - if (useLifecycleInsteadOfInjectingFragments()) { - Glide glide = Glide.get(context.getApplicationContext()); - return lifecycleRequestManagerRetriever.getOrCreate( - context, glide, fragment.getLifecycle(), fm, fragment.isVisible()); - } else { - return supportFragmentGet(context, fm, fragment, fragment.isVisible()); - } + Glide glide = Glide.get(context.getApplicationContext()); + return lifecycleRequestManagerRetriever.getOrCreate( + context, glide, fragment.getLifecycle(), fm, fragment.isVisible()); } /** @@ -504,31 +487,6 @@ private SupportRequestManagerFragment getSupportRequestManagerFragment( return current; } - @NonNull - private RequestManager supportFragmentGet( - @NonNull Context context, - @NonNull FragmentManager fm, - @Nullable Fragment parentHint, - boolean isParentVisible) { - SupportRequestManagerFragment current = getSupportRequestManagerFragment(fm, parentHint); - RequestManager requestManager = current.getRequestManager(); - if (requestManager == null) { - // TODO(b/27524013): Factor out this Glide.get() call. - Glide glide = Glide.get(context); - requestManager = - factory.build( - glide, current.getGlideLifecycle(), current.getRequestManagerTreeNode(), context); - // This is a bit of hack, we're going to start the RequestManager, but not the - // corresponding Lifecycle. It's safe to start the RequestManager, but starting the - // Lifecycle might trigger memory leaks. See b/154405040 - if (isParentVisible) { - requestManager.onStart(); - } - current.setRequestManager(requestManager); - } - return requestManager; - } - // We care about the instance specifically. @SuppressWarnings({"ReferenceEquality", "PMD.CompareObjectsWithEquals"}) private boolean verifyOurFragmentWasAddedOrCantBeAdded( diff --git a/library/test/src/test/java/com/bumptech/glide/manager/RequestManagerRetrieverTest.java b/library/test/src/test/java/com/bumptech/glide/manager/RequestManagerRetrieverTest.java index c8a4cfa1f1..3c75f01de6 100644 --- a/library/test/src/test/java/com/bumptech/glide/manager/RequestManagerRetrieverTest.java +++ b/library/test/src/test/java/com/bumptech/glide/manager/RequestManagerRetrieverTest.java @@ -62,8 +62,7 @@ public void setUp() { retriever = new RequestManagerRetriever(/* factory= */ null, mock(GlideExperiments.class)); - harnesses = - new RetrieverHarness[] {new DefaultRetrieverHarness(), new SupportRetrieverHarness()}; + harnesses = new RetrieverHarness[] {new DefaultRetrieverHarness()}; initialSdkVersion = Build.VERSION.SDK_INT; Util.setSdkVersionInt(18);