Skip to content

Commit

Permalink
Add LifecycleOwner support to BindableAdapter.
Browse files Browse the repository at this point in the history
This is in response to an issue found in #2392. See this commit's PR
description for more details.
  • Loading branch information
BenHenning committed Jan 22, 2021
1 parent 1144075 commit b4dc253
Show file tree
Hide file tree
Showing 14 changed files with 554 additions and 209 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ import org.oppia.android.app.settings.profile.ProfileResetPinActivity
import org.oppia.android.app.splash.SplashActivity
import org.oppia.android.app.story.StoryActivity
import org.oppia.android.app.testing.AudioFragmentTestActivity
import org.oppia.android.app.testing.BindableAdapterTestActivity
import org.oppia.android.app.testing.ConceptCardFragmentTestActivity
import org.oppia.android.app.testing.DragDropTestActivity
import org.oppia.android.app.testing.ExplorationInjectionActivity
Expand Down Expand Up @@ -78,7 +77,6 @@ interface ActivityComponent {
fun inject(appLanguageActivity: AppLanguageActivity)
fun inject(appVersionActivity: AppVersionActivity)
fun inject(audioFragmentTestActivity: AudioFragmentTestActivity)
fun inject(bindableAdapterTestActivity: BindableAdapterTestActivity)
fun inject(completedStoryListActivity: CompletedStoryListActivity)
fun inject(conceptCardFragmentTestActivity: ConceptCardFragmentTestActivity)
fun inject(audioLanguageActivity: AudioLanguageActivity)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import org.oppia.android.app.settings.profile.ProfileListFragment
import org.oppia.android.app.shim.IntentFactoryShimModule
import org.oppia.android.app.shim.ViewBindingShimModule
import org.oppia.android.app.story.StoryFragment
import org.oppia.android.app.testing.BindableAdapterTestFragment
import org.oppia.android.app.testing.ImageRegionSelectionTestFragment
import org.oppia.android.app.topic.TopicFragment
import org.oppia.android.app.topic.conceptcard.ConceptCardFragment
Expand Down Expand Up @@ -78,7 +77,6 @@ interface FragmentComponent {
fun inject(appVersionFragment: AppVersionFragment)
fun inject(audioFragment: AudioFragment)
fun inject(autoAppDeprecationNoticeDialogFragment: AutomaticAppDeprecationNoticeDialogFragment)
fun inject(bindableAdapterTestFragment: BindableAdapterTestFragment)
fun inject(completedStoryListFragment: CompletedStoryListFragment)
fun inject(conceptCardFragment: ConceptCardFragment)
fun inject(audioLanguageFragment: AudioLanguageFragment)
Expand Down
154 changes: 105 additions & 49 deletions app/src/main/java/org/oppia/android/app/recyclerview/BindableAdapter.kt
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@ import android.view.LayoutInflater
import android.view.View
import android.view.ViewGroup
import androidx.databinding.ViewDataBinding
import androidx.lifecycle.LifecycleOwner
import androidx.recyclerview.widget.RecyclerView
import org.oppia.android.app.recyclerview.BindableAdapter.MultiTypeBuilder.Companion.newBuilder
import org.oppia.android.app.recyclerview.BindableAdapter.SingleTypeBuilder.Companion.newBuilder
import java.lang.ref.WeakReference
import kotlin.reflect.KClass

/** A function that returns the integer-based type of view that can bind the specified data object. */
/** A function that returns the integer-based type of view that can bind the specified object. */
private typealias ComputeIntViewType<T> = (T) -> Int

/** A function that returns the enum-based type of view that can bind the specified data object. */
Expand Down Expand Up @@ -93,23 +95,75 @@ class BindableAdapter<T : Any> 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<BuilderType : Any> {
/**
* A [WeakReference] to a [LifecycleOwner] for databinding inflation. See [setLifecycleOwner].
* 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<LifecycleOwner>? = null

/**
* Sets the [LifecycleOwner] corresponding to this adapter. This will automatically be used as
* the lifecycle owner for all databinding classes created during view inflation. Note that the
* adapter holds a weak reference to the owner to ensure long-lived references to the adapter
* class itself does not result in leaks, however it's up to the caller's responsibility to make
* sure that the adapter itself is not actually used after the lifecycle owner has expired
* (otherwise the app may crash).
*
* @return this
*/
fun setLifecycleOwner(lifecycleOwner: LifecycleOwner): BuilderType {
check(lifecycleOwnerRef == null) {
"A lifecycle owner has already been bound to this adapter."
}
lifecycleOwnerRef = WeakReference(lifecycleOwner)

// This cast is not, strictly speaking, safe, but child classes are expected to inherit from
// the builder & pass their own type in.
@Suppress("UNCHECKED_CAST") return this as BuilderType
}

/**
* 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.
*
* Instances of [MultiTypeBuilder] should be instantiated using [newBuilder].
* Instances of [SingleTypeBuilder] should be instantiated using [newBuilder].
*/
class SingleTypeBuilder<T : Any>(private val dataClassType: KClass<T>) {
class SingleTypeBuilder<T : Any>(
private val dataClassType: KClass<T>
) : BaseBuilder<SingleTypeBuilder<T>>() {
private lateinit var viewHolderFactory: ViewHolderFactory<T>

/**
* Registers a [View] inflater and bind function for views in the recycler view.
*
* The inflateView and bindView functions passed in here must not hold any references to UI objects except those
* that own the RecyclerView.
* The inflateView and bindView functions passed in here must not hold any references to UI
* objects except those that own the RecyclerView.
*
* @param inflateView function that takes a parent [ViewGroup] and returns a newly inflated [View] of type [V]
* @param bindView function that takes a [RecyclerView]-owned [View] of type [V] and binds a data element typed [T]
* to it
* @param inflateView function that takes a parent [ViewGroup] and returns a newly inflated
* [View] of type [V]
* @param bindView function that takes a [RecyclerView]-owned [View] of type [V] and binds a
* data element typed [T] to it
* @return this
*/
fun <V : View> registerViewBinder(
Expand Down Expand Up @@ -139,29 +193,25 @@ class BindableAdapter<T : Any> internal constructor(
): SingleTypeBuilder<T> {
return registerViewDataBinder(
inflateDataBinding = inflateDataBinding,
setViewModel = setViewModel,
transformViewModel = { it }
setViewModel = setViewModel
)
}

/**
* Behaves in the same way as [registerViewBinder] except the inflate and bind methods correspond to a [View]
* data-binding typed [DB].
* Behaves in the same way as [registerViewBinder] except the inflate and bind methods
* correspond to a [View] data-binding typed [DB].
*
* @param inflateDataBinding a function that inflates the root view of a data-bound layout (e.g.
* MyDataBinding::inflate). This may also be a function that initializes the data-binding with additional
* properties as necessary.
* MyDataBinding::inflate). This may also be a function that initializes the data-binding
* with additional properties as necessary.
* @param setViewModel a function that initializes the view model in the data-bound view (e.g.
* MyDataBinding::setSpecialViewModel). This may also be a function that initializes the view model & other
* view-accessible properties as necessary.
* @param transformViewModel a function that converts the input model to a model of another type (such as for cases
* when subclassing is used to represent more complex lists of data).
* MyDataBinding::setSpecialViewModel). This may also be a function that initializes the
* view model & other view-accessible properties as necessary.
* @return this
*/
private fun <DB : ViewDataBinding, T2 : T> registerViewDataBinder(
private fun <DB : ViewDataBinding> registerViewDataBinder(
inflateDataBinding: (LayoutInflater, ViewGroup, Boolean) -> DB,
setViewModel: (DB, T2) -> Unit,
transformViewModel: (T) -> T2
setViewModel: (DB, T) -> Unit
): SingleTypeBuilder<T> {
check(!::viewHolderFactory.isInitialized) { "A view binder is already initialized" }
viewHolderFactory = { viewGroup ->
Expand All @@ -172,9 +222,10 @@ class BindableAdapter<T : Any> internal constructor(
viewGroup,
/* attachToRoot= */ false
)
binding.lifecycleOwner = getLifecycleOwner()
object : BindableViewHolder<T>(binding.root) {
override fun bind(data: T) {
setViewModel(binding, transformViewModel(data))
setViewModel(binding, data)
}
}
}
Expand All @@ -200,29 +251,31 @@ class BindableAdapter<T : Any> internal constructor(
}

/**
* Constructs a new [BindableAdapter] that supports multiple view types. Each type returned by the computer should
* have an associated view binder.
* Constructs a new [BindableAdapter] that supports multiple view types. Each type returned by the
* computer should have an associated view binder.
*
* Instances of [Builder] should be instantiated using [newBuilder].
* Instances of [MultiTypeBuilder] should be instantiated using [newBuilder].
*/
class MultiTypeBuilder<T : Any, E : Enum<E>>(
private val dataClassType: KClass<T>,
private val computeViewType: ComputeViewType<T, E>
) {
) : BaseBuilder<MultiTypeBuilder<T, E>>() {
private var viewHolderFactoryMap: MutableMap<E, ViewHolderFactory<T>> = HashMap()

/**
* Registers a [View] inflater and bind function for views of the specified view type (with default value
* [DEFAULT_VIEW_TYPE] for single-view [RecyclerView]s). Note that the viewType specified here must correspond to a
* view type registered in [registerViewTypeComputer] if non-default, or if any view type computer has been
* registered.
* Registers a [View] inflater and bind function for views of the specified view type (with
* default value [DEFAULT_VIEW_TYPE] for single-view [RecyclerView]s). Note that the viewType
* specified here must be properly returned in the [ComputeViewType] function passed into
* [newBuilder].
*
* The inflateView and bindView functions passed in here must not hold any references to UI objects except those
* that own the RecyclerView.
* The inflateView and bindView functions passed in here must not hold any references to UI
* objects except those that own the RecyclerView.
*
* @param inflateView function that takes a parent [ViewGroup] and returns a newly inflated [View] of type [V]
* @param bindView function that takes a [RecyclerView]-owned [View] of type [V] and binds a data element typed [T]
* to it
* @param viewType the type of the view being bound
* @param inflateView function that takes a parent [ViewGroup] and returns a newly inflated
* [View] of type [V]
* @param bindView function that takes a [RecyclerView]-owned [View] of type [V] and binds a
* data element typed [T] to it
* @return this
*/
fun <V : View> registerViewBinder(
Expand All @@ -232,10 +285,11 @@ class BindableAdapter<T : Any> internal constructor(
): MultiTypeBuilder<T, E> {
checkViewTypeIsUnique(viewType)
val viewHolderFactory: ViewHolderFactory<T> = { viewGroup ->
// This is lifecycle-safe since it will become dereferenced when the factory method returns.
// The version referenced in the anonymous BindableViewHolder object should be copied into a
// class field that binds that reference's lifetime to the view holder's lifetime. This
// approach avoids needing to perform an unsafe cast later when binding the view.
// Note on lifecycle safety: this is lifecycle-safe since it will become dereferenced when
// the factory method returns. The version referenced in the anonymous BindableViewHolder
// object should be copied into a class field that binds that reference's lifetime to the
// view holder's lifetime. This approach avoids needing to perform an unsafe cast later when
// binding the view.
val inflatedView = inflateView(viewGroup)
object : BindableViewHolder<T>(inflatedView) {
override fun bind(data: T) {
Expand All @@ -260,17 +314,18 @@ class BindableAdapter<T : Any> internal constructor(
}

/**
* Behaves in the same way as [registerViewBinder] except the inflate and bind methods correspond to a [View]
* data-binding typed [DB].
* Behaves in the same way as [registerViewBinder] except the inflate and bind methods
* correspond to a [View] data-binding typed [DB].
*
* @param viewType the type of the view being bound
* @param inflateDataBinding a function that inflates the root view of a data-bound layout (e.g.
* MyDataBinding::inflate). This may also be a function that initializes the data-binding with additional
* properties as necessary.
* MyDataBinding::inflate). This may also be a function that initializes the data-binding
* with additional properties as necessary.
* @param setViewModel a function that initializes the view model in the data-bound view (e.g.
* MyDataBinding::setSpecialViewModel). This may also be a function that initializes the view model & other
* view-accessible properties as necessary.
* @param transformViewModel a function that converts the input model to a model of another type (such as for cases
* when subclassing is used to represent more complex lists of data).
* MyDataBinding::setSpecialViewModel). This may also be a function that initializes the
* view model & other view-accessible properties as necessary.
* @param transformViewModel a function that converts the input model to a model of another type
* (such as for cases when subclassing is used to represent more complex lists of data).
* @return this
*/
fun <DB : ViewDataBinding, T2 : T> registerViewDataBinder(
Expand All @@ -288,6 +343,7 @@ class BindableAdapter<T : Any> internal constructor(
viewGroup,
/* attachToRoot= */ false
)
binding.lifecycleOwner = getLifecycleOwner()
object : BindableViewHolder<T>(binding.root) {
override fun bind(data: T) {
setViewModel(binding, transformViewModel(data))
Expand Down Expand Up @@ -317,8 +373,8 @@ class BindableAdapter<T : Any> internal constructor(

companion object {
/**
* Returns a new [MultiTypeBuilder] with the specified function that returns the enum type of view a specific data
* item corresponds to.
* Returns a new [MultiTypeBuilder] with the specified function that returns the enum type of
* view a specific data item corresponds to.
*/
inline fun <reified T : Any, reified E : Enum<E>> newBuilder(
noinline computeViewType: ComputeViewType<T, E>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import org.oppia.android.app.activity.InjectableAppCompatActivity
class BindableAdapterTestActivity : InjectableAppCompatActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
activityComponent.inject(this)
(activityComponent as TestInjector).inject(this)
setContentView(R.layout.test_activity)
supportFragmentManager.beginTransaction()
.add(
Expand All @@ -19,4 +19,9 @@ class BindableAdapterTestActivity : InjectableAppCompatActivity() {
)
.commitNow()
}

/** Test-only injector for the activity that needs to be set up in the test. */
interface TestInjector {
fun inject(bindableAdapterTestActivity: BindableAdapterTestActivity)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package org.oppia.android.app.testing

import androidx.lifecycle.LiveData

/** Test data model for bindable adapter tests. */
sealed class BindableAdapterTestDataModel {
val boundStringValue get() = (this as StringModel).stringValue
val boundIntValue get() = (this as IntModel).intValue
val boundLiveDataValue get() = (this as LiveDataModel).liveData

data class StringModel(val stringValue: String) : BindableAdapterTestDataModel()

data class IntModel(val intValue: Int) : BindableAdapterTestDataModel()

data class LiveDataModel(val liveData: LiveData<String>) : BindableAdapterTestDataModel()
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class BindableAdapterTestFragment : InjectableFragment() {

override fun onAttach(context: Context) {
super.onAttach(context)
fragmentComponent.inject(this)
(fragmentComponent as TestInjector).inject(this)
}

override fun onCreateView(
Expand All @@ -28,4 +28,9 @@ class BindableAdapterTestFragment : InjectableFragment() {
): View? {
return bindableAdapterTestFragmentPresenter.handleCreateView(inflater, container)
}

/** Test-only injector for the fragment that needs to be set up in the test. */
interface TestInjector {
fun inject(bindableAdapterTestFragment: BindableAdapterTestFragment)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,39 +5,24 @@ import android.view.View
import android.view.ViewGroup
import androidx.annotation.VisibleForTesting
import androidx.fragment.app.Fragment
import org.oppia.android.app.model.TestModel
import org.oppia.android.app.recyclerview.BindableAdapter
import org.oppia.android.app.viewmodel.ViewModelProvider
import org.oppia.android.databinding.TestFragmentBinding
import javax.inject.Inject

/** The test-only fragment presenter corresponding to [BindableAdapterTestFragment]. */
class BindableAdapterTestFragmentPresenter @Inject constructor(
private val fragment: Fragment,
private val viewModelProvider: ViewModelProvider<BindableAdapterTestViewModel>
private val testBindableAdapterFactory: BindableAdapterFactory,
@VisibleForTesting val viewModel: BindableAdapterTestViewModel
) {
@VisibleForTesting
val viewModel: BindableAdapterTestViewModel by lazy {
getBindableAdapterTestViewModel()
}

companion object {
// TODO(#59): Move away from this fragile static testing state by leveraging a test-only DI graph that can be
// configured within tests to provide the bindable adapter to be used by this presenter.
var testBindableAdapter: BindableAdapter<TestModel>? = null
}

fun handleCreateView(inflater: LayoutInflater, container: ViewGroup?): View? {
val binding = TestFragmentBinding.inflate(
inflater,
container,
/* attachToRoot= */ false
)
checkNotNull(testBindableAdapter) {
"Expected a bindable adapter to be provided in a test module"
}
binding.testRecyclerView.apply {
adapter = testBindableAdapter
adapter = testBindableAdapterFactory.create(fragment)
}
binding.let {
it.viewModel = viewModel
Expand All @@ -46,7 +31,8 @@ class BindableAdapterTestFragmentPresenter @Inject constructor(
return binding.root
}

private fun getBindableAdapterTestViewModel(): BindableAdapterTestViewModel {
return viewModelProvider.getForFragment(fragment, BindableAdapterTestViewModel::class.java)
/** Factory for creating new [BindableAdapter]s for the current fragment. */
interface BindableAdapterFactory {
fun create(fragment: Fragment): BindableAdapter<BindableAdapterTestDataModel>
}
}
Loading

0 comments on commit b4dc253

Please sign in to comment.