onBindViewHolder binding view inconsistently

Question

I have a ViewHolder that is meant to appear differently depending on whether it is on the left or right side of a two-column RecyclerView with a GridLayoutManager. Note the connector lines on either side of the view:

<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
              xmlns:tools="http://schemas.android.com/tools"
              android:layout_width="wrap_content"
              android:layout_marginTop="12px"
              android:layout_marginBottom="12px"
              android:layout_gravity="center"
              android:gravity="center_vertical"
              android:layout_height="wrap_content"
              android:id="@+id/citation_select_holder">
    <ImageView
            android:src="@drawable/connector_line"
            android:layout_width="wrap_content"
            android:layout_height="wrap_content"
            android:layout_weight="1"
            android:id="@+id/citation_select_connector_right"/>


    <RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"
                    xmlns:tools="http://schemas.android.com/tools"
                    android:layout_width="348px"
                    android:layout_height="104px"
                    android:layout_weight="1"
                    android:background="@drawable/button_background_white"
                    android:id="@+id/citation_select_citation_holder">

        <LinearLayout android:layout_width="wrap_content"
                      android:layout_height="wrap_content"
                      android:orientation="vertical"
                      android:layout_centerVertical="true"
                      android:layout_alignParentLeft="true"
                      android:layout_marginLeft="28px"

        >
            <TextView
                    tools:text="123456"
                    android:textAppearance="@style/citation_select_item_number"
                    android:layout_width="wrap_content"
                    android:layout_height="wrap_content"
                    android:id="@+id/citation_select_citation_number_text"/>

            <TextView
                    tools:text="Pay by: Nov 18th, 2019"
                    android:textAppearance="@style/citation_select_item_due_date"
                    android:layout_width="wrap_content"
                    android:layout_height="wrap_content"
                    android:id="@+id/citation_select_due_date_text"/>
            <TextView
                    tools:text="a category label"
                    android:textAppearance="@style/citation_select_item_category"
                    android:layout_width="wrap_content"
                    android:layout_height="wrap_content"
                    android:id="@+id/citation_select_category_text"/>

        </LinearLayout>

        <TextView
                tools:text="$10.00"
                android:textAppearance="@style/citation_select_item_cost"
                android:layout_width="wrap_content"
                android:layout_height="wrap_content"
                android:layout_alignParentRight="true"
                android:layout_centerVertical="true"
                android:id="@+id/citation_select_cost_text" android:layout_marginRight="28px"/>

    </RelativeLayout>

    <ImageView
            android:src="@drawable/connector_line"
            android:layout_width="wrap_content"
            android:layout_height="wrap_content"
            android:layout_weight="1"
            android:id="@+id/citation_select_connector_left"/>


</LinearLayout>

The connector line far from the side that the view appears on is meant to disappear when onBindViewHolder is called, and the margins updated accordingly.

if (position % 2 == 0) {

    holder.itemView.findViewById<ImageView>(R.id.citation_select_connector_left).visibility = View.GONE
    val marginLayoutParams1 = holder.citationHolder.layoutParams as GridLayoutManager.LayoutParams
    marginLayoutParams1.setMargins(0, 12, 12, 12)
    holder.itemView.findViewById<LinearLayout>(R.id.citation_select_holder).layoutParams =
        marginLayoutParams1

} else {

    holder.itemView.findViewById<ImageView>(R.id.citation_select_connector_right).visibility = View.GONE
    val marginLayoutParams2 = holder.citationHolder.layoutParams as GridLayoutManager.LayoutParams
    marginLayoutParams2.setMargins(12, 12, 0, 12)
    holder.itemView.findViewById<LinearLayout>(R.id.citation_select_holder).layoutParams =
        marginLayoutParams2
}

Scrolling is done exclusively via on-screen buttons in increments of six. The first two pages load normally:

But the pattern begins to break down at citation #14. Keep in mind that the citation numbers correspond are the view's position within the RecyclerView:

What is happening to change the behavior?

Answer 1

I think I know what can help you fix this. I presume that it is reusing the old view, as a RecyclerView should and nowhere in your code is there a line to set the visibility of the connector lines back to visible.

You should add to both your GONE visibilities also the code to set the other to visible:

if (position % 2 == 0) {

holder.itemView.findViewById<ImageView>(R.id.citation_select_connector_right).visibility = View.VISIBLE
holder.itemView.findViewById<ImageView>(R.id.citation_select_connector_left).visibility = View.GONE
val marginLayoutParams1 = holder.citationHolder.layoutParams as GridLayoutManager.LayoutParams
marginLayoutParams1.setMargins(0, 12, 12, 12)
holder.itemView.findViewById<LinearLayout>(R.id.citation_select_holder).layoutParams =
    marginLayoutParams1

} else {

holder.itemView.findViewById<ImageView>(R.id.citation_select_connector_left).visibility = View.VISIBLE
holder.itemView.findViewById<ImageView>(R.id.citation_select_connector_right).visibility = View.GONE
val marginLayoutParams2 = holder.citationHolder.layoutParams as GridLayoutManager.LayoutParams
marginLayoutParams2.setMargins(12, 12, 0, 12)
holder.itemView.findViewById<LinearLayout>(R.id.citation_select_holder).layoutParams =
    marginLayoutParams2
}

Additional explanation

The RecyclerView will reuse some old view, right? Well since both lines are VISIBLE at the start, you assume that's their default state. But when you set the lines to GONE you never put them back to visible, and thus if RecyclerView reuses that view, it'll not add the margin there and it will just be missing the connector line. You always want to have EVERY line of code in onBindViewHolder to have a matching line that reverts it.

Answer 2

Vucko's answer is good, and the overall point (always update every component of your viewholder) is something you should absolutely do.

I wanted to add, however, that it appears as though you are not following the ViewHolder pattern correctly: your onBindViewHolder() method should never call findViewById(). Instead, your ViewHolder class should find each view once and then save references to them.

class MyViewHolder(itemView: View) : RecyclerView.ViewHolder(itemView) {

    val connectorRight: ImageView = itemView.findViewById(R.id.citation_select_connector_right)
    val connectorLeft: ImageView = itemView.findViewById(R.id.citation_select_connector_left)
    // ...
}

And then you can use these fields directly inside onBindViewHolder():

if (position % 2 == 0) {
    holder.connectorRight.visibility = View.VISIBLE
    holder.connectorLeft.visibility = View.GONE
    // ...
} else {
    holder.connectorLeft.visibility = View.VISIBLE
    holder.connectorRight.visibility = View.GONE
    // ...
}