From dfed2cb9c763a50e156936761df0f4bc8001d229 Mon Sep 17 00:00:00 2001 From: kevingitonga Date: Tue, 19 Jul 2022 16:26:51 +0300 Subject: [PATCH] Refactor PromotedStoryListView.kt BindableAdapterTest.kt classes to provide Adapter through injection for isuue #2658. --- .../promotedlist/PromotedStoryListView.kt | 34 +++-- .../app/recyclerview/BindableAdapter.kt | 38 ++++- .../BindableAdapterTestFragmentPresenter.kt | 9 +- .../app/recyclerview/BindableAdapterTest.kt | 144 +++++++++--------- 4 files changed, 133 insertions(+), 92 deletions(-) diff --git a/app/src/main/java/org/oppia/android/app/home/promotedlist/PromotedStoryListView.kt b/app/src/main/java/org/oppia/android/app/home/promotedlist/PromotedStoryListView.kt index 05469ded4bb..b01e0c6c7c7 100644 --- a/app/src/main/java/org/oppia/android/app/home/promotedlist/PromotedStoryListView.kt +++ b/app/src/main/java/org/oppia/android/app/home/promotedlist/PromotedStoryListView.kt @@ -41,18 +41,30 @@ class PromotedStoryListView @JvmOverloads constructor( lateinit var promotedDataList: List override fun onAttachedToWindow() { - super.onAttachedToWindow() - val viewComponentFactory = - FragmentManager.findFragment(this) as ViewComponentFactory - val viewComponent = viewComponentFactory.createViewComponent(this) as ViewComponentImpl - viewComponent.inject(this) + try { + super.onAttachedToWindow() + val viewComponentFactory = + FragmentManager.findFragment(this) as ViewComponentFactory + val viewComponent = viewComponentFactory.createViewComponent(this) as ViewComponentImpl + viewComponent.inject(this) - // The StartSnapHelper is used to snap between items rather than smooth scrolling, so that - // the item is completely visible in [HomeFragment] as soon as learner lifts the finger - // after scrolling. - val snapHelper = StartSnapHelper() - onFlingListener = null - snapHelper.attachToRecyclerView(this) + // The StartSnapHelper is used to snap between items rather than smooth scrolling, so that + // the item is completely visible in [HomeFragment] as soon as learner lifts the finger + // after scrolling. + val snapHelper = StartSnapHelper() + onFlingListener = null + snapHelper.attachToRecyclerView(this) + + checkIfComponentsInitialized() + } catch (e: IllegalStateException) { + if (::oppiaLogger.isInitialized) { + oppiaLogger.e( + "PromotedStoryListView", + "Throws exception on attach to window", + e + ) + } + } } private fun checkIfComponentsInitialized() { diff --git a/app/src/main/java/org/oppia/android/app/recyclerview/BindableAdapter.kt b/app/src/main/java/org/oppia/android/app/recyclerview/BindableAdapter.kt index cb013f8d859..fa1a036e9ee 100644 --- a/app/src/main/java/org/oppia/android/app/recyclerview/BindableAdapter.kt +++ b/app/src/main/java/org/oppia/android/app/recyclerview/BindableAdapter.kt @@ -5,7 +5,9 @@ import android.view.View import android.view.ViewGroup import androidx.databinding.ViewDataBinding import androidx.fragment.app.Fragment +import androidx.lifecycle.LifecycleOwner import androidx.recyclerview.widget.RecyclerView +import java.lang.ref.WeakReference import javax.inject.Inject import kotlin.reflect.KClass @@ -93,6 +95,34 @@ class BindableAdapter internal constructor( internal abstract fun bind(data: T) } + /** + * The base builder for [BindableAdapter]. This class should not be used directly--use either + * [SingleTypeBuilder] or [MultiTypeBuilder] instead. + */ + abstract class BaseBuilder(fragment: Fragment) { + /** + * A [WeakReference] to a [LifecycleOwner] for databinding inflation. + * Note that this needs to be a weak reference so that long-held references to the adapter do + * not potentially leak lifecycle owners (such as fragments and activities). + */ + private var lifecycleOwnerRef: WeakReference = WeakReference(fragment) + + /** + * Returns the [LifecycleOwner] bound to this adapter, or null if there isn't one. This method + * will throw if there was a lifecycle owner bound but is now expired. + */ + protected fun getLifecycleOwner(): LifecycleOwner? { + // Crash if the lifecycle owner has been cleaned up since it's not valid to use the adapter + // with an old lifecycle owner, and silently ignoring this may result in part of the layout + // not responding to events. + return lifecycleOwnerRef?.let { ref -> + checkNotNull(ref.get()) { + "Attempted to inflate data binding with expired lifecycle owner" + } + } + } + } + /** * Constructs a new [BindableAdapter] that for a single view type. * @@ -101,7 +131,7 @@ class BindableAdapter internal constructor( class SingleTypeBuilder( private val dataClassType: KClass, private val fragment: Fragment - ) { + ) : BaseBuilder(fragment) { private lateinit var viewHolderFactory: ViewHolderFactory /** @@ -173,10 +203,10 @@ class BindableAdapter internal constructor( /* attachToRoot= */ false ) - binding.lifecycleOwner = fragment.viewLifecycleOwner object : BindableViewHolder(binding.root) { override fun bind(data: T) { setViewModel(binding, data) + binding.lifecycleOwner = getLifecycleOwner() } } } @@ -216,7 +246,7 @@ class BindableAdapter internal constructor( private val dataClassType: KClass, private val computeViewType: ComputeViewType, private val fragment: Fragment - ) { + ) : BaseBuilder(fragment) { private var viewHolderFactoryMap: MutableMap> = HashMap() /** @@ -301,10 +331,10 @@ class BindableAdapter internal constructor( /* attachToRoot= */ false ) - binding.lifecycleOwner = fragment.viewLifecycleOwner object : BindableViewHolder(binding.root) { override fun bind(data: T) { setViewModel(binding, transformViewModel(data)) + binding.lifecycleOwner = getLifecycleOwner() } } } diff --git a/app/src/main/java/org/oppia/android/app/testing/BindableAdapterTestFragmentPresenter.kt b/app/src/main/java/org/oppia/android/app/testing/BindableAdapterTestFragmentPresenter.kt index a6b255058db..25961d8ba67 100644 --- a/app/src/main/java/org/oppia/android/app/testing/BindableAdapterTestFragmentPresenter.kt +++ b/app/src/main/java/org/oppia/android/app/testing/BindableAdapterTestFragmentPresenter.kt @@ -12,6 +12,8 @@ import javax.inject.Inject /** The test-only fragment presenter corresponding to [BindableAdapterTestFragment]. */ class BindableAdapterTestFragmentPresenter @Inject constructor( private val fragment: Fragment, + private val singleTypeBuilder: BindableAdapter.SingleTypeBuilder.Factory, + private val multiTypeBuilder: BindableAdapter.MultiTypeBuilder.Factory, private val testBindableAdapterFactory: BindableAdapterFactory, @VisibleForTesting val viewModel: BindableAdapterTestViewModel ) { @@ -22,7 +24,7 @@ class BindableAdapterTestFragmentPresenter @Inject constructor( /* attachToRoot= */ false ) binding.testRecyclerView.apply { - adapter = testBindableAdapterFactory.create(fragment) + adapter = testBindableAdapterFactory.create(singleTypeBuilder, multiTypeBuilder) } binding.let { it.viewModel = viewModel @@ -33,6 +35,9 @@ class BindableAdapterTestFragmentPresenter @Inject constructor( /** Factory for creating new [BindableAdapter]s for the current fragment. */ interface BindableAdapterFactory { - fun create(fragment: Fragment): BindableAdapter + fun create( + singleTypeBuilder: BindableAdapter.SingleTypeBuilder.Factory, + multiTypeBuilder: BindableAdapter.MultiTypeBuilder.Factory + ): BindableAdapter } } diff --git a/app/src/sharedTest/java/org/oppia/android/app/recyclerview/BindableAdapterTest.kt b/app/src/sharedTest/java/org/oppia/android/app/recyclerview/BindableAdapterTest.kt index ef436215158..7c0ca780cf0 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/recyclerview/BindableAdapterTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/recyclerview/BindableAdapterTest.kt @@ -6,7 +6,6 @@ import android.view.LayoutInflater import android.view.ViewGroup import android.widget.TextView import androidx.appcompat.app.AppCompatActivity -import androidx.fragment.app.Fragment import androidx.lifecycle.MutableLiveData import androidx.recyclerview.widget.RecyclerView import androidx.test.core.app.ActivityScenario.launch @@ -163,10 +162,9 @@ class BindableAdapterTest { @Test fun testSingleTypeAdapter_withOneViewType_noData_bindsNoViews() { - val testFragment = Fragment() // Set up the adapter to be used for this test. TestModule.testAdapterFactory = - { createSingleViewTypeNoDataBindingBindableAdapter(testFragment) } + { singleFactory, _ -> createSingleViewTypeNoDataBindingBindableAdapter(singleFactory) } launch(BindableAdapterTestActivity::class.java).use { scenario -> scenario.onActivity { activity -> @@ -180,10 +178,10 @@ class BindableAdapterTest { @Test fun testSingleTypeAdapter_withOneViewType_setItem_automaticallyBindsView() { - val testFragment = Fragment() // Set up the adapter to be used for this test. - TestModule.testAdapterFactory = - { createSingleViewTypeNoDataBindingBindableAdapter(testFragment) } + TestModule.testAdapterFactory = { singleTypeFactory, _ -> + createSingleViewTypeNoDataBindingBindableAdapter(singleTypeFactory) + } launch(BindableAdapterTestActivity::class.java).use { scenario -> scenario.onActivity { activity -> @@ -205,10 +203,10 @@ class BindableAdapterTest { @Test fun testSingleTypeAdapter_withOneViewType_nullData_bindsNoViews() { - val testFragment = Fragment() // Set up the adapter to be used for this test. - TestModule.testAdapterFactory = - { createSingleViewTypeNoDataBindingBindableAdapter(testFragment) } + TestModule.testAdapterFactory = { singleTypeFactory, _ -> + createSingleViewTypeNoDataBindingBindableAdapter(singleTypeFactory) + } launch(BindableAdapterTestActivity::class.java).use { scenario -> scenario.onActivity { activity -> @@ -221,10 +219,10 @@ class BindableAdapterTest { @Test fun testSingleTypeAdapter_withOneViewType_setMultipleItems_automaticallyBinds() { - val testFragment = Fragment() // Set up the adapter to be used for this test. - TestModule.testAdapterFactory = - { createSingleViewTypeNoDataBindingBindableAdapter(testFragment) } + TestModule.testAdapterFactory = { singleTypeFactory, _ -> + createSingleViewTypeNoDataBindingBindableAdapter(singleTypeFactory) + } launch(BindableAdapterTestActivity::class.java).use { scenario -> scenario.onActivity { activity -> @@ -249,10 +247,9 @@ class BindableAdapterTest { @Test fun testMultiTypeAdapter_withTwoViewTypes_setItems_autoBindsCorrectItemsPerTypes() { - val testFragment = Fragment() // Set up the adapter to be used for this test. TestModule.testAdapterFactory = - { createMultiViewTypeNoDataBindingBindableAdapter(testFragment) } + { _, multiTypeFactory -> createMultiViewTypeNoDataBindingBindableAdapter(multiTypeFactory) } launch(BindableAdapterTestActivity::class.java).use { scenario -> scenario.onActivity { activity -> @@ -287,10 +284,10 @@ class BindableAdapterTest { @Test fun testSingleTypeAdapter_withOneDataBoundViewType_setItem_automaticallyBindsView() { - val testFragment = Fragment() // Set up the adapter to be used for this test. - TestModule.testAdapterFactory = - { createSingleViewTypeWithDataBindingBindableAdapter(testFragment) } + TestModule.testAdapterFactory = { singleTypeFactory, _ -> + createSingleViewTypeWithDataBindingBindableAdapter(singleTypeFactory) + } launch(BindableAdapterTestActivity::class.java).use { scenario -> scenario.onActivity { activity -> @@ -312,13 +309,11 @@ class BindableAdapterTest { @Test fun testSingleTypeAdapter_withTwoDataBoundViewTypes_setItems_autoBindsCorrectItemsPerTypes() { - val testFragment = Fragment() // Set up the adapter to be used for this test. - TestModule.testAdapterFactory = { - createSingleViewTypeWithDataBindingBindableAdapter( - testFragment - ) - } + TestModule.testAdapterFactory = + { singleTypeFactory, _ -> + createSingleViewTypeWithDataBindingBindableAdapter(singleTypeFactory) + } launch(BindableAdapterTestActivity::class.java).use { scenario -> scenario.onActivity { activity -> @@ -344,10 +339,9 @@ class BindableAdapterTest { @Test fun testMultiTypeAdapter_partiallyDataBoundViewTypes_setItems_autoBindsCorrectItemsPerTypes() { - val testFragment = Fragment() // Set up the adapter to be used for this test. TestModule.testAdapterFactory = - { createMultiViewTypeWithDataBindingBindableAdapter(testFragment) } + { _, multiTypeFactory -> createMultiViewTypeWithDataBindingBindableAdapter(multiTypeFactory) } launch(BindableAdapterTestActivity::class.java).use { scenario -> scenario.onActivity { activity -> @@ -382,38 +376,31 @@ class BindableAdapterTest { @Test fun testSingleTypeAdapter_setLifecycleOwnerTwice_throwsException() { - val testFragment = Fragment() - val exception = assertThrows(IllegalStateException::class) { - SingleTypeBuilder - .Factory(testFragment).create() - .setLifecycleOwner(testFragment) - .setLifecycleOwner(testFragment) - .build() + TestModule.testAdapterFactory = + { singleTypeAdapter, _ -> createSingleAdapterWithoutView(singleTypeAdapter) } } + assertThat(exception).hasMessageThat().contains("lifecycle owner has already been bound") } @Test fun testMultiTypeAdapter_setLifecycleOwnerTwice_throwsException() { - val testFragment = Fragment() val exception = assertThrows(IllegalStateException::class) { - MultiTypeBuilder - .Factory(testFragment).create(ViewModelType.Companion::deriveTypeFrom) - .setLifecycleOwner(testFragment) - .setLifecycleOwner(testFragment) - .build() + TestModule.testAdapterFactory = + { _, multiTypeFactory -> createMultiViewTypeNoDataBindingBindableAdapter(multiTypeFactory) } } + assertThat(exception).hasMessageThat().contains("lifecycle owner has already been bound") } @Test fun testSingleTypeAdapter_withLiveData_noLifecycleOwner_doesNotRebindLiveDataValues() { - val testFragment = Fragment() // Set up the adapter to be used for this test. - TestModule.testAdapterFactory = - { createSingleViewTypeWithDataBindingAndLiveDataAdapter(testFragment) } + TestModule.testAdapterFactory = { singleTypeFactory, _ -> + createSingleViewTypeWithDataBindingAndLiveDataAdapter(singleTypeFactory) + } launch(BindableAdapterTestActivity::class.java).use { scenario -> val itemLiveData = MutableLiveData("initial") @@ -438,8 +425,8 @@ class BindableAdapterTest { @Test fun testSingleTypeAdapter_withLiveData_withLifecycleOwner_rebindsLiveDataValues() { // Set up the adapter to be used for this test. - TestModule.testAdapterFactory = { fragment -> - createSingleViewTypeWithDataBindingAndLiveDataAdapter(lifecycleOwner = fragment) + TestModule.testAdapterFactory = { singleTypeFactory, _ -> + createSingleViewTypeWithDataBindingAndLiveDataAdapter(singleTypeFactory) } launch(BindableAdapterTestActivity::class.java).use { scenario -> @@ -465,10 +452,9 @@ class BindableAdapterTest { @Test fun testMultiTypeAdapter_withLiveData_noLifecycleOwner_doesNotRebindLiveDataValues() { - val testFragment = Fragment() // Set up the adapter to be used for this test. TestModule.testAdapterFactory = - { createMultiViewTypeWithDataBindingBindableAdapter(testFragment) } + { _, multiTypeFactory -> createMultiViewTypeWithDataBindingBindableAdapter(multiTypeFactory) } launch(BindableAdapterTestActivity::class.java).use { scenario -> val itemLiveData = MutableLiveData("initial") @@ -493,9 +479,8 @@ class BindableAdapterTest { @Test fun testMultiTypeAdapter_withLiveData_withLifecycleOwner_rebindsLiveDataValues() { // Set up the adapter to be used for this test. - TestModule.testAdapterFactory = { fragment -> - createMultiViewTypeWithDataBindingBindableAdapter(lifecycleOwner = fragment) - } + TestModule.testAdapterFactory = + { _, multiTypeFactory -> createMultiViewTypeWithDataBindingBindableAdapter(multiTypeFactory) } launch(BindableAdapterTestActivity::class.java).use { scenario -> val itemLiveData = MutableLiveData("initial") @@ -522,21 +507,22 @@ class BindableAdapterTest { ApplicationProvider.getApplicationContext().inject(this) } - private fun createSingleViewTypeNoDataBindingBindableAdapter(testFragment: Fragment): - BindableAdapter { - return SingleTypeBuilder - .Factory(testFragment).create() - .registerViewBinder( - inflateView = this::inflateTextViewForStringWithoutDataBinding, - bindView = this::bindTextViewForStringWithoutDataBinding - ) - .build() - } + private fun createSingleViewTypeNoDataBindingBindableAdapter( + singleTypeBuilder: SingleTypeBuilder.Factory + ): BindableAdapter { + return singleTypeBuilder.create() + .registerViewBinder( + inflateView = this::inflateTextViewForStringWithoutDataBinding, + bindView = this::bindTextViewForStringWithoutDataBinding + ) + .build() + } - private fun createSingleViewTypeWithDataBindingBindableAdapter(testFragment: Fragment): + private fun createSingleViewTypeWithDataBindingBindableAdapter( + singleTypeBuilder: SingleTypeBuilder.Factory + ): BindableAdapter { - return SingleTypeBuilder - .Factory(testFragment).create() + return singleTypeBuilder.create() .registerViewDataBinderWithSameModelType( inflateDataBinding = TestTextViewForStringWithDataBindingBinding::inflate, setViewModel = TestTextViewForStringWithDataBindingBinding::setViewModel @@ -545,11 +531,9 @@ class BindableAdapterTest { } private fun createSingleViewTypeWithDataBindingAndLiveDataAdapter( - lifecycleOwner: Fragment + singleTypeBuilder: SingleTypeBuilder.Factory ): BindableAdapter { - return SingleTypeBuilder - .Factory(lifecycleOwner).create() - .setLifecycleOwner(lifecycleOwner) + return singleTypeBuilder.create() .registerViewDataBinderWithSameModelType( inflateDataBinding = TestTextViewForLiveDataWithDataBindingBinding::inflate, setViewModel = TestTextViewForLiveDataWithDataBindingBinding::setViewModel @@ -557,10 +541,16 @@ class BindableAdapterTest { .build() } - private fun createMultiViewTypeNoDataBindingBindableAdapter(testFragment: Fragment): + private fun createSingleAdapterWithoutView(singleTypeBuilder: SingleTypeBuilder.Factory): + BindableAdapter { + return singleTypeBuilder.create().build() + } + + private fun createMultiViewTypeNoDataBindingBindableAdapter( + multiTypeBuilder: MultiTypeBuilder.Factory + ): BindableAdapter { - return MultiTypeBuilder - .Factory(testFragment).create(ViewModelType.Companion::deriveTypeFrom) + return multiTypeBuilder.create(ViewModelType.Companion::deriveTypeFrom) .registerViewBinder( viewType = ViewModelType.STRING, inflateView = this::inflateTextViewForStringWithoutDataBinding, @@ -575,11 +565,9 @@ class BindableAdapterTest { } private fun createMultiViewTypeWithDataBindingBindableAdapter( - lifecycleOwner: Fragment + multiTypeBuilder: MultiTypeBuilder.Factory ): BindableAdapter { - return MultiTypeBuilder - .Factory(lifecycleOwner).create(ViewModelType.Companion::deriveTypeFrom) - .setLifecycleOwner(lifecycleOwner) + return multiTypeBuilder.create(ViewModelType.Companion::deriveTypeFrom) .registerViewDataBinderWithSameModelType( viewType = ViewModelType.STRING, inflateDataBinding = TestTextViewForStringWithDataBindingBinding::inflate, @@ -670,7 +658,10 @@ class BindableAdapterTest { class TestModule { companion object { // TODO(#1720): Move this to a test-level binding to avoid the need for static state. - var testAdapterFactory: ((Fragment) -> BindableAdapter)? = null + var testAdapterFactory: ( + (SingleTypeBuilder.Factory, MultiTypeBuilder.Factory) -> + BindableAdapter + )? = null } @Provides @@ -679,8 +670,11 @@ class BindableAdapterTest { "The test adapter factory hasn't been initialized in the test" } return object : BindableAdapterTestFragmentPresenter.BindableAdapterFactory { - override fun create(fragment: Fragment): BindableAdapter { - return createFunction(fragment) + override fun create( + singleTypeBuilder: SingleTypeBuilder.Factory, + multiTypeBuilder: MultiTypeBuilder.Factory + ): BindableAdapter { + return createFunction(singleTypeBuilder, multiTypeBuilder) } } }