Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a generic data-binding-enabled RecyclerView adapter #172

Merged
merged 7 commits into from
Oct 8, 2019

Conversation

BenHenning
Copy link
Member

This introduces a data-binding-enabled RecyclerView adapter that can be used to both data-bind a list to a RecyclerView, but also continue data-binding for RecyclerView-owned Views. It also simplifies RecyclerView construction immensely. See #106 for a working example. Snippets from that CL include:

Data-binding a list of items to the RecyclerView adapter

  <androidx.recyclerview.widget.RecyclerView
    android:id="@+id/recyclerView"
    android:layout_width="match_parent"
    android:layout_height="match_parent"
    app:layoutManager="androidx.recyclerview.widget.LinearLayoutManager"
    app:data="@{viewModel.topicListSummaryLiveData}"/>

Data-binding each topic list summary to a constituent view contained in RecyclerView

<?xml version="1.0" encoding="utf-8"?>
<layout
  xmlns:android="http://schemas.android.com/apk/res/android"
  xmlns:app="http://schemas.android.com/apk/res-auto">

  <data>
    <variable
      name="topicSummary"
      type="org.oppia.app.model.TopicSummary"/>
  </data>
...
    <TextView
      android:layout_width="wrap_content"
      android:layout_height="wrap_content"
      android:text="@{@plurals/lesson_count(topicSummary.canonicalStoryCount)}"
      android:textColor="@android:color/white"
      android:fontFamily="sans-serif-light"
      android:textStyle="italic"
      android:textSize="14sp"
      android:layout_margin="8dp"
      app:layout_constraintStart_toStartOf="parent"
      app:layout_constraintBottom_toBottomOf="@+id/bottom_topic_bar"/>
  </androidx.constraintlayout.widget.ConstraintLayout>
</layout>

Setting up the adapter for the RecyclerView

  private fun createRecyclerViewAdapter(): BindableAdapter<TopicSummary> {
    return BindableAdapter.Builder
      .newBuilder<TopicSummary>()
      .registerViewDataBinder(
        inflateDataBinding = TopicSummaryViewBinding::inflate,
        setViewModel = TopicSummaryViewBinding::setTopicSummary)
      .build()
  }

Creating the root RecyclerView

    val binding = TopicListFragmentBinding.inflate(inflater, container, /* attachToRoot= */ false)
    binding.recyclerView.apply {
      adapter = createRecyclerViewAdapter()
      // https://stackoverflow.com/a/50075019/3689782
      layoutManager = GridLayoutManager(context, /* spanCount= */ 2)
    }
    binding.let {
      it.viewModel = getTopicListViewModel()
      it.lifecycleOwner = fragment
    }
    return binding.root

There are some limitations right now (such as assuming only one view model and nothing else needs to be bound to child views), however since functions are passed in these can be worked around. The builder interface for the adapter also isn't great, and runtime type checking is needed due to an issue with the data-binding library not supporting binding to reified inline Kotlin functions (for the custom binding adapter). That being said, this approach is really powerful given that it enables reactive data-binding fully through RecyclerView, and it significantly simplifies the creation process for a RecyclerView adapter.

I haven't tested the multiple view types case yet, and there aren't yet any tests for this adapter. I'm looking for early feedback before I work on stabilizing this.

@BenHenning
Copy link
Member Author

@veena14cs answering your question from #106, setting up multiple views should look something like this:

  private enum class ItemTypes(internal val numericType: Int) {
    FIRST_ITEM_TYPE(0),
    SECOND_ITEM_TYPE(1)
  }

  private fun createRecyclerViewAdapter(): BindableAdapter<TopicSummary> {
    return BindableAdapter.Builder
      .newBuilder<TopicSummary>()
      .registerViewTypeComputer { item ->
        return when(item) {
          is FirstItemType -> FIRST_ITEM_TYPE.numericType
          is SecondItemType -> SECOND_ITEM_TYPE.numericType
          else throw Exception("Unexpected item: $item")
        }
      }.registerViewDataBinder(
        viewType = FIRST_ITEM_TYPE.numericType,
        inflateDataBinding = FirstViewBinding::inflate,
        setViewModel = FirstViewBinding::setFirstViewModel)
      .registerViewDataBinder(
        viewType = SECOND_ITEM_TYPE.numericType,
        inflateDataBinding = SecondViewBinding::inflate,
        setViewModel = SecondViewBinding::setSecondViewModel)
      .build()
  }

Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rt4914 rt4914 removed their assignment Sep 20, 2019
Copy link
Contributor

@vinitamurthi vinitamurthi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General lint question to @BenHenning and others. Are we going to follow Github's line length or keep it to android studio?
I personally would prefer keeping it same as Github because reviewing gets a little weird when comments/code wrap to the next line in the review view

import androidx.recyclerview.widget.RecyclerView
import kotlin.reflect.KClass

/** A function that returns the type of view that should can bind the specified data object. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove 'should' in the comment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Good catch.

@vinitamurthi vinitamurthi removed their assignment Sep 20, 2019
Copy link
Contributor

@jamesxu0 jamesxu0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Conflicts:
	app/build.gradle
	app/src/main/AndroidManifest.xml
	app/src/main/java/org/oppia/app/activity/ActivityComponent.kt
	app/src/main/java/org/oppia/app/fragment/FragmentComponent.kt
	app/src/main/java/org/oppia/app/player/audio/AudioFragment.kt
@BenHenning
Copy link
Member Author

@jamesxu0 this has now been updated to develop for you to merge downstream for your pending PR. I still need to finish it before both PRs can be submitted, though.

rt4914 added a commit that referenced this pull request Oct 4, 2019
* All xml files for topic-train except layout folder

* Skill item related files

* Nit change to Javadoc

* Added TODO #172

* Nit changes

* Nit changes
@BenHenning
Copy link
Member Author

I found my issue with the tests. It turns out that not setting a content view and simply adding a fragment to an activity without a container results in its views never being attached to the activity (presumably because there's no root view). This led to both Robolectric & Espresso failing (the Espresso tests also had an unintentional deadlock). Both are fixed and now writing new tests is unblocked.

@BenHenning
Copy link
Member Author

Thanks all for the review.

@vinitamurthi regarding your earlier line length comment: GitHub doesn't enforce a line length. We should follow the Google style guide 100 characters, but at the moment I'm arbitrarily following Android Studio's 120 characters. I'm hesitant to enforce anything below 120 until we have a decent reformatter to use to fix existing code.

@BenHenning
Copy link
Member Author

NB: Please do take a look at tests @vinitamurthi / @rt4914 / @jamesxu0 however I'm submitting this as-is to unblock downstream work.

@BenHenning BenHenning assigned BenHenning and unassigned veena14cs Oct 8, 2019
@BenHenning BenHenning merged commit 219d9a4 into develop Oct 8, 2019
@BenHenning BenHenning deleted the introduce-data-binding-recycler-view-adapter branch June 10, 2020 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants