Android - RecyclerView display wrong items after adding new ones-CodePudding

I am working on a WebSocket-based chat application and I am having troubles to add new incoming messages to the RV (RecyclerView).

I have the following adapter's overriten functions

    override fun onCreateViewHolder(
        parent: ViewGroup,
        viewType: Int
    ): UserChatMessagesAdapter.Holder {
        val binding = AdapterUserChatMessagesRvBinding.inflate(
            LayoutInflater.from(parent.context),
            parent,
            false
        )
        return Holder(binding)
    }

    override fun onBindViewHolder(holder: Holder, position: Int) {
        holder.bindData(messages[position])
    }

    override fun getItemId(position: Int): Long {
        return position.toLong()
    }

    override fun getItemViewType(position: Int): Int {
        return position
    }

    override fun getItemCount() = messages.size

The data comes from API calls and are posted by a ViewModel through LiveData.

For testing purposes, on the first connection, I am returning only 10 messages to be displayed. Then, when scrolling, I return the following 10 and so on.

I am not having issues to populate already existing message from the past when scrolling.

Problem adding new messages

The problem is when User A sends messages to User B (or viceversa), the messages are not being displayed correctly.

For example, supposing I have already displayed the messages: M1, M2, ..., M10.

Then User A sends M11. But instead of displaying that new M11, it is displaying another one, such as, M3 (or any other, I haven't detected a pattern).

The adapter's method to display a new message:

fun newMessage(newMessage: ChatMessageView) {
   val index = LAST_MESSAGE_INDEX
   messages.add(index, newMessage)
   notifyItemInserted(index)
}

When debugging I didn't find anything wrong. The newMessage value is the expected one and the messages list is updated correctly. I doesn't seem a data issue.

The messages are cached in memory, so when re-entering to the chat, they are not requested again to the API. Having said that, when I re-enter to the chat, the messages are displayed correctly.

I have also tried:

Using notifyDataSetChanged.
Replacing the entire messages list
Smooth scroller (I've read somewhere it may be a problem related to the fast user scrolling)
Overriding the methods getItemViewType, getItemId.
Setting adapter.setHasStableIds(true)

If it helps, this is the Holder definition:

inner class Holder(private val binding: AdapterUserChatMessagesRvBinding) :
    RecyclerView.ViewHolder(binding.root) {

    fun bindData(message: ChatMessageView) {
        if (message.isSentByMe) {
            binding.userChatMessagesAdapterSentMessage.configure(
                SentMessageComponent.Configuration(
                    message.content,
                    message.sentMoment.time
                )
            )
            binding.userChatMessagesAdapterSentMessage.visible()
        } else {
            binding.userChatMessagesAdapterReceivedMessage.configure(
                ReceivedMessageComponent.Configuration(
                    message.content,
                    message.sentMoment.time
                )
            )
            binding.userChatMessagesAdapterReceivedMessage.visible()
        }

            configureDateSeparator(message.sentMoment.date)
    }

And this my custom RecyclerView.OnScrollListener definition:

internal abstract class PaginationScrollListener(
    private val layoutManager: LinearLayoutManager
) : RecyclerView.OnScrollListener() {

    abstract fun isLastPage(): Boolean

    abstract fun isLoading(): Boolean

    override fun onScrolled(recyclerView: RecyclerView, dx: Int, dy: Int) {
        super.onScrolled(recyclerView, dx, dy)

        if (!isLoading() && !isLastPage()) {
            val visibleItemCount = layoutManager.childCount
            val totalItemCount = layoutManager.itemCount
            val firstVisibleItemPosition = layoutManager.findFirstVisibleItemPosition()

            if (visibleItemCount   firstVisibleItemPosition >= totalItemCount
                && firstVisibleItemPosition >= 0) {
                loadMoreItems()
            }
        }
    }

    abstract fun loadMoreItems()
}

--------- EDIT - 1 ---------

I add the corresponding XML:

<?xml version="1.0" encoding="utf-8"?>
<androidx.constraintlayout.widget.ConstraintLayout xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:app="http://schemas.android.com/apk/res-auto"
    android:id="@ id/adapter_chat_room_main_container"
    android:layout_width="match_parent"
    android:layout_height="wrap_content"
    android:paddingVertical="10dp">

    <...components.ChatDateSeparatorComponent
        android:id="@ id/user_chat_messages_adapter_date_separator"
        android:layout_width="wrap_content"
        android:layout_height="wrap_content"
        android:visibility="gone"
        app:layout_constraintTop_toTopOf="parent"
        app:layout_constraintStart_toStartOf="parent"
        app:layout_constraintEnd_toEndOf="parent" />

    <...components.ReceivedMessageComponent
        android:id="@ id/user_chat_messages_adapter_received_message"
        android:layout_width="match_parent"
        android:layout_height="wrap_content"
        android:visibility="gone"
        app:layout_constraintStart_toStartOf="parent"
        app:layout_constraintTop_toBottomOf="@id/user_chat_messages_adapter_date_separator" />

    <...components.SentMessageComponent
        android:id="@ id/user_chat_messages_adapter_sent_message"
        android:layout_width="match_parent"
        android:layout_height="wrap_content"
        android:visibility="gone"
        app:layout_constraintEnd_toEndOf="parent"
        app:layout_constraintTop_toBottomOf="@id/user_chat_messages_adapter_date_separator" />

</androidx.constraintlayout.widget.ConstraintLayout>

CodePudding user response：

Your bindData function makes a component visible depending on the message type, but it doesn't make the other component invisible - so if that ViewHolder is used to display an item where isSentByMe is true, and then you scroll down and it gets reused for an item where isSentByMe is false, userChatMessagesAdapterSentMessage and userChatMessagesAdapterReceivedMessage will both be visible.

I'm guessing those are two different Views in your layout - you haven't shown your XML, but is it possible the ViewHolder's height is fixed (not wrap_content) so the lower of those two views is outside the visible area? Or maybe one overlays the other, so you can only see the top one?

edit: yeah see, you're overlaying them on each other:

<...components.ChatDateSeparatorComponent
    android:id="@ id/user_chat_messages_adapter_date_separator"
    app:layout_constraintTop_toTopOf="parent"/>

<...components.ReceivedMessageComponent
    android:id="@ id/user_chat_messages_adapter_received_message"
    android:layout_width="match_parent"
    ...
    app:layout_constraintStart_toStartOf="parent"
    app:layout_constraintTop_toBottomOf="@id/user_chat_messages_adapter_date_separator" />

<...components.SentMessageComponent
    android:id="@ id/user_chat_messages_adapter_sent_message"
    android:layout_width="match_parent"
    ...
    app:layout_constraintEnd_toEndOf="parent"
    app:layout_constraintTop_toBottomOf="@id/user_chat_messages_adapter_date_separator" />

Both have their tops constrained to the bottom of the separator view that's pinned to the top - so vertically they're in the exact same place. One's constrained to the left and the other to the right, but their widths are both match_parent so they fill the whole width anyway.

If both of those MessageComponent views are visible, only the SentMessage one will be seen since it's specified last (drawn on top of the other because it has a higher Z-order). If the ReceivedMessage one has significantly more content making it taller (because both their heights are wrap_content) then it'll peek out below the other.

Like I said, you're only setting them to visible in the code you posted. You never set the other one to invisible/gone to make sure it's hidden. You're not toggling between showing one or the other, you show one and then it sticks as visible even if that ViewHolder needs to show the other type of message. So according to your XML, if a sent message is displayed in that ViewHolder, you won't be able to see any received messages that it ends up displaying as you scroll, and you'll just see the old sent content.

That's why you're probably seeing M3 instead of M11 - because a VH that was previously displaying M3 is being reused, M11 is displayed, but you can't see it behind the old M3 content. And if you "re-enter the chat" and everything's basically reset, it's possible that you'll see M11 in a VH that hasn't been used to display a sent message yet, so it looks ok (but stuff will probably break if you scroll up and back down again).

In case you don't know, the point of a RecyclerView is to reuse (recycle) a handful of ViewHolders, using them to display the data for a specific item. In onBindViewHolder you're given one of those ViewHolders, which may have already been used to display something else, and you set it up to display your current item.

That bit in bold is important, because it means the VH will have old state, including in your case a View that's been set to visible. So you have to explicitly set the state for everything so there's no old state left over - assume it's dirty and needs setting up completely. That means when you set one of your Views to VISIBLE, you have to set the other to INVISIBLE or GONE. Otherwise if it was visible before, now you have two visible things, and that's a problem!

So you have to do this:

if (message.isSentByMe) {
    binding.userChatMessagesAdapterSentMessage.configure(
        SentMessageComponent.Configuration(
            message.content,
            message.sentMoment.time
        )
    )
    binding.userChatMessagesAdapterSentMessage.visible()
    // explicitly hide the other one - I'm assuming you'll write another function
    binding.userChatMessagesAdapterReceivedMessage.invisible()
} else {
    binding.userChatMessagesAdapterReceivedMessage.configure(
        ReceivedMessageComponent.Configuration(
            message.content,
            message.sentMoment.time
        )
    )
    binding.userChatMessagesAdapterReceivedMessage.visible()
    // same as the other branch - you're showing this, make sure the other is hidden
    binding.userChatMessagesAdapterSentMessage.invisible()
}