From 8f536f2b7012c3c4d7bf80fec0de62893d53edbc Mon Sep 17 00:00:00 2001 From: Ian Lake Date: Wed, 8 Jan 2020 16:23:45 -0800 Subject: [PATCH] Avoid references to the Fragment's Views after onDestroyView() After onDestroyView() runs, Fragment's Views are eligible for garbage collection. Ensure that we don't hold onto any references after onDestroyView() to allow for the Views to be garbage collected as soon as possible. For the GithubBrowserSample, this required changing the AutoClearedValue class to clear the backing field when the Fragment's view is destroyed, rather than wait for the Fragment itself to be destroyed. Test: ./run_all_tests.sh --- .../persistence/ui/ProductFragment.java | 7 +++++ .../persistence/ui/ProductListFragment.java | 7 +++++ .../github/util/AutoClearedValueTest.kt | 29 ++++++++++++++++--- .../github/testing/SingleFragmentActivity.kt | 7 ++++- .../example/github/util/AutoClearedValue.kt | 23 +++++++++------ .../listscreen/Leaderboard.kt | 7 ++--- .../android/navigationsample/Leaderboard.kt | 8 ++--- 7 files changed, 63 insertions(+), 25 deletions(-) diff --git a/BasicSample/app/src/main/java/com/example/android/persistence/ui/ProductFragment.java b/BasicSample/app/src/main/java/com/example/android/persistence/ui/ProductFragment.java index c5aa10e3..f84a0af8 100644 --- a/BasicSample/app/src/main/java/com/example/android/persistence/ui/ProductFragment.java +++ b/BasicSample/app/src/main/java/com/example/android/persistence/ui/ProductFragment.java @@ -87,6 +87,13 @@ private void subscribeToModel(final ProductViewModel model) { }); } + @Override + public void onDestroyView() { + mBinding = null; + mCommentAdapter = null; + super.onDestroyView(); + } + /** Creates product fragment for specific product ID */ public static ProductFragment forProduct(int productId) { ProductFragment fragment = new ProductFragment(); diff --git a/BasicSample/app/src/main/java/com/example/android/persistence/ui/ProductListFragment.java b/BasicSample/app/src/main/java/com/example/android/persistence/ui/ProductListFragment.java index b0d14a25..80a97c86 100644 --- a/BasicSample/app/src/main/java/com/example/android/persistence/ui/ProductListFragment.java +++ b/BasicSample/app/src/main/java/com/example/android/persistence/ui/ProductListFragment.java @@ -90,6 +90,13 @@ private void subscribeUi(LiveData> liveData) { }); } + @Override + public void onDestroyView() { + mBinding = null; + mProductAdapter = null; + super.onDestroyView(); + } + private final ProductClickCallback mProductClickCallback = product -> { if (getLifecycle().getCurrentState().isAtLeast(Lifecycle.State.STARTED)) { ((MainActivity) requireActivity()).show(product); diff --git a/GithubBrowserSample/app/src/androidTest/java/com/android/example/github/util/AutoClearedValueTest.kt b/GithubBrowserSample/app/src/androidTest/java/com/android/example/github/util/AutoClearedValueTest.kt index 610ac372..1b57469d 100644 --- a/GithubBrowserSample/app/src/androidTest/java/com/android/example/github/util/AutoClearedValueTest.kt +++ b/GithubBrowserSample/app/src/androidTest/java/com/android/example/github/util/AutoClearedValueTest.kt @@ -15,6 +15,10 @@ */ package com.android.example.github.util +import android.os.Bundle +import android.view.LayoutInflater +import android.view.View +import android.view.ViewGroup import androidx.fragment.app.DialogFragment import androidx.fragment.app.Fragment import androidx.test.platform.app.InstrumentationRegistry @@ -47,7 +51,6 @@ class AutoClearedValueTest { @Test fun clearOnReplace() { - testFragment.testValue = "foo" activityRule.activity.replaceFragment(TestFragment()) InstrumentationRegistry.getInstrumentation().waitForIdleSync() try { @@ -57,9 +60,22 @@ class AutoClearedValueTest { } } + @Test + fun clearOnReplaceBackStack() { + activityRule.activity.replaceFragment(TestFragment(), true) + InstrumentationRegistry.getInstrumentation().waitForIdleSync() + try { + testFragment.testValue + Assert.fail("should throw if accessed when not set") + } catch (ex: IllegalStateException) { + } + activityRule.activity.supportFragmentManager.popBackStack() + InstrumentationRegistry.getInstrumentation().waitForIdleSync() + assertThat(testFragment.testValue, `is`("foo")) + } + @Test fun dontClearForChildFragment() { - testFragment.testValue = "foo" testFragment.childFragmentManager.beginTransaction() .add(Fragment(), "foo").commit() InstrumentationRegistry.getInstrumentation().waitForIdleSync() @@ -68,9 +84,8 @@ class AutoClearedValueTest { @Test fun dontClearForDialog() { - testFragment.testValue = "foo" val dialogFragment = DialogFragment() - dialogFragment.show(testFragment.fragmentManager!!, "dialog") + dialogFragment.show(testFragment.parentFragmentManager, "dialog") dialogFragment.dismiss() InstrumentationRegistry.getInstrumentation().waitForIdleSync() assertThat(testFragment.testValue, `is`("foo")) @@ -78,5 +93,11 @@ class AutoClearedValueTest { class TestFragment : Fragment() { var testValue by autoCleared() + + override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?, + savedInstanceState: Bundle?): View? { + testValue = "foo" + return View(context) + } } } \ No newline at end of file diff --git a/GithubBrowserSample/app/src/debug/java/com/android/example/github/testing/SingleFragmentActivity.kt b/GithubBrowserSample/app/src/debug/java/com/android/example/github/testing/SingleFragmentActivity.kt index 3b3b3628..6d4cf768 100644 --- a/GithubBrowserSample/app/src/debug/java/com/android/example/github/testing/SingleFragmentActivity.kt +++ b/GithubBrowserSample/app/src/debug/java/com/android/example/github/testing/SingleFragmentActivity.kt @@ -46,9 +46,14 @@ class SingleFragmentActivity : AppCompatActivity() { .commit() } - fun replaceFragment(fragment: Fragment) { + fun replaceFragment(fragment: Fragment, addToBackStack: Boolean = false) { supportFragmentManager.beginTransaction() .replace(R.id.container, fragment) + .apply { + if (addToBackStack) { + addToBackStack(null) + } + } .commit() } } diff --git a/GithubBrowserSample/app/src/main/java/com/android/example/github/util/AutoClearedValue.kt b/GithubBrowserSample/app/src/main/java/com/android/example/github/util/AutoClearedValue.kt index bcbf5e4f..eed7535f 100644 --- a/GithubBrowserSample/app/src/main/java/com/android/example/github/util/AutoClearedValue.kt +++ b/GithubBrowserSample/app/src/main/java/com/android/example/github/util/AutoClearedValue.kt @@ -16,26 +16,31 @@ package com.android.example.github.util -import androidx.lifecycle.Lifecycle -import androidx.lifecycle.LifecycleObserver -import androidx.lifecycle.OnLifecycleEvent import androidx.fragment.app.Fragment +import androidx.lifecycle.DefaultLifecycleObserver +import androidx.lifecycle.LifecycleOwner +import androidx.lifecycle.observe import kotlin.properties.ReadWriteProperty import kotlin.reflect.KProperty /** - * A lazy property that gets cleaned up when the fragment is destroyed. + * A lazy property that gets cleaned up when the fragment's view is destroyed. * - * Accessing this variable in a destroyed fragment will throw NPE. + * Accessing this variable while the fragment's view is destroyed will throw NPE. */ class AutoClearedValue(val fragment: Fragment) : ReadWriteProperty { private var _value: T? = null init { - fragment.lifecycle.addObserver(object : LifecycleObserver { - @OnLifecycleEvent(Lifecycle.Event.ON_DESTROY) - fun onDestroy() { - _value = null + fragment.lifecycle.addObserver(object: DefaultLifecycleObserver { + override fun onCreate(owner: LifecycleOwner) { + fragment.viewLifecycleOwnerLiveData.observe(fragment) { viewLifecycleOwner -> + viewLifecycleOwner?.lifecycle?.addObserver(object: DefaultLifecycleObserver { + override fun onDestroy(owner: LifecycleOwner) { + _value = null + } + }) + } } }) } diff --git a/NavigationAdvancedSample/app/src/main/java/com/example/android/navigationadvancedsample/listscreen/Leaderboard.kt b/NavigationAdvancedSample/app/src/main/java/com/example/android/navigationadvancedsample/listscreen/Leaderboard.kt index 4c6beead..8f2b919a 100644 --- a/NavigationAdvancedSample/app/src/main/java/com/example/android/navigationadvancedsample/listscreen/Leaderboard.kt +++ b/NavigationAdvancedSample/app/src/main/java/com/example/android/navigationadvancedsample/listscreen/Leaderboard.kt @@ -33,17 +33,14 @@ import com.example.android.navigationadvancedsample.R */ class Leaderboard : Fragment() { - private lateinit var recyclerView: RecyclerView - private lateinit var viewAdapter: RecyclerView.Adapter<*> - override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?, savedInstanceState: Bundle?): View? { // Inflate the layout for this fragment val view = inflater.inflate(R.layout.fragment_leaderboard, container, false) - viewAdapter = MyAdapter(Array(10) { "Person ${it + 1}" }) + val viewAdapter = MyAdapter(Array(10) { "Person ${it + 1}" }) - recyclerView = view.findViewById(R.id.leaderboard_list).apply { + view.findViewById(R.id.leaderboard_list).run { // use this setting to improve performance if you know that changes // in content do not change the layout size of the RecyclerView setHasFixedSize(true) diff --git a/NavigationBasicSample/app/src/main/java/com/example/android/navigationsample/Leaderboard.kt b/NavigationBasicSample/app/src/main/java/com/example/android/navigationsample/Leaderboard.kt index 1e8580dc..e8e4bd12 100644 --- a/NavigationBasicSample/app/src/main/java/com/example/android/navigationsample/Leaderboard.kt +++ b/NavigationBasicSample/app/src/main/java/com/example/android/navigationsample/Leaderboard.kt @@ -32,24 +32,20 @@ import androidx.navigation.Navigation */ class Leaderboard : Fragment() { - private lateinit var recyclerView: RecyclerView - private lateinit var viewAdapter: RecyclerView.Adapter<*> - override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?, savedInstanceState: Bundle?): View? { // Inflate the layout for this fragment val view = inflater.inflate(R.layout.fragment_leaderboard, container, false) - viewAdapter = MyAdapter(arrayOf("Flo", "Ly", "Jo")) + val viewAdapter = MyAdapter(arrayOf("Flo", "Ly", "Jo")) - recyclerView = view.findViewById(R.id.leaderboard_list).apply { + view.findViewById(R.id.leaderboard_list).run { // use this setting to improve performance if you know that changes // in content do not change the layout size of the RecyclerView setHasFixedSize(true) // specify an viewAdapter (see also next example) adapter = viewAdapter - } return view }