Skip to content

Commit

Permalink
Use Lifecycle instead of Fragments by default
Browse files Browse the repository at this point in the history 
PiperOrigin-RevId: 494046303
  • Loading branch information
@sjudd @glide-copybara-robot
sjudd authored and glide-copybara-robot committed Dec 9, 2022
1 parent e6f5eec commit 18bba92
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 195 deletions.
19 changes: 2 additions & 17 deletions instrumentation/src/androidTest/java/com/bumptech/glide/RequestManagerLifecycleTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -45,19 +41,8 @@ public class RequestManagerLifecycleTest {

private ActivityScenario<DefaultFragmentActivity> scenario;

@Parameter public boolean useLifecycleInsteadOfInjectingFragments;

@Parameters(name = "useLifecycleInsteadOfInjectingFragments = {0}")
public static ImmutableList<Boolean> parameters() {
return ImmutableList.of(true, false);
}

@Before
public void setUp() {
Glide.init(
ApplicationProvider.getApplicationContext(),
new GlideBuilder()
.useLifecycleInsteadOfInjectingFragments(useLifecycleInsteadOfInjectingFragments));
scenario = scenarioRule.getScenario();
}

Expand Down
124 changes: 0 additions & 124 deletions instrumentation/src/androidTest/java/com/bumptech/glide/RequestManagerTest.java
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -113,111 +96,4 @@ public void run() {
}
});
}

@Test
public void with_beforeActivitySuperOnCreate_onlyAddsOneRequestManagerFragment() {
ActivityScenario<GlideWithBeforeSuperOnCreateActivity> scenario =
ActivityScenario.launch(GlideWithBeforeSuperOnCreateActivity.class);
scenario.moveToState(State.RESUMED);
scenario.onActivity(
new ActivityAction<GlideWithBeforeSuperOnCreateActivity>() {
@Override
public void perform(GlideWithBeforeSuperOnCreateActivity activity) {
List<Fragment> fragments = activity.getSupportFragmentManager().getFragments();
List<Fragment> 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<GlideWithBeforeSuperOnCreateActivity>() {
@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<GlideWithBeforeSuperOnCreateActivity>() {
@Override
public void perform(GlideWithBeforeSuperOnCreateActivity activity) {
List<Fragment> fragments = activity.getSupportFragmentManager().getFragments();
List<Fragment> 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<GlideWithAsDifferentSupertypesActivity> scenario =
ActivityScenario.launch(GlideWithAsDifferentSupertypesActivity.class);
scenario.moveToState(State.RESUMED);
scenario.onActivity(
new ActivityAction<GlideWithAsDifferentSupertypesActivity>() {
@Override
public void perform(GlideWithAsDifferentSupertypesActivity activity) {
Iterable<SupportRequestManagerFragment> glideSupportFragments =
Iterables.filter(
activity.getSupportFragmentManager().getFragments(),
SupportRequestManagerFragment.class);
Iterable<RequestManagerFragment> normalFragments =
Iterables.filter(
getAllFragments(activity.getFragmentManager()), RequestManagerFragment.class);
assertThat(normalFragments).hasSize(0);
assertThat(glideSupportFragments).hasSize(1);
}
});
}

private List<android.app.Fragment> 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<android.app.Fragment> getAllFragmentsPreO(
android.app.FragmentManager fragmentManager) {
Bundle tempBundle = new Bundle();
int index = 0;
List<android.app.Fragment> 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;
}
}
62 changes: 10 additions & 52 deletions library/src/main/java/com/bumptech/glide/manager/RequestManagerRetriever.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<View, Fragment> tempViewToSupportFragment = new ArrayMap<>();
Expand All @@ -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);
Expand Down Expand Up @@ -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
Expand All @@ -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());
}

/**
Expand Down Expand Up @@ -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(
Expand Down
3 changes: 1 addition & 2 deletions library/test/src/test/java/com/bumptech/glide/manager/RequestManagerRetrieverTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 18bba92

Please sign in to comment.