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

Fixed to to reset carousel position when CarouselItem is displayed again #309

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

NUmeroAndDev
Copy link

Overview

  • Fixed to reset item position in the carousel when CarouselItem is displayed again.

Screenshot

Before After
ezgif-6-2c4eef954faf ezgif-6-7e0cfba6e2c0

Copy link
Collaborator

@ValCanBuild ValCanBuild left a comment

Choose a reason for hiding this comment

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

I don't think its appropriate to do this in this way. I've played around with the problem and the most reliable way to restore the scroll position is to store it in a variable at the unbind call and then restore it in bind:

class CarouselItem(private val carouselDecoration: RecyclerView.ItemDecoration,
                   private val carouselAdapter: GroupAdapter<com.xwray.groupie.GroupieViewHolder>) : Item() {
    .....
    private var visibleItemPos = 0
    .....
    override fun bind(viewHolder: GroupieViewHolder, position: Int) {
        viewHolder.recyclerView.apply {
            adapter = carouselAdapter
            ........
            this.scrollToPosition(visibleItemPos)
        }
    }

    override fun unbind(viewHolder: GroupieViewHolder) {
        super.unbind(viewHolder)
        visibleItemPos = (viewHolder.recyclerView.layoutManager as? LinearLayoutManager)?.findFirstVisibleItemPosition() ?: 0
    }
}

If you agree with this feel free to apply the change to both classes and I can approve

return super.createViewHolder(itemView).apply {
recyclerView.apply {
layoutManager = LinearLayoutManager(context, LinearLayoutManager.HORIZONTAL, false)
adapter = carouselAdapter
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will cause an issue if you have more than one CarouselItem in the adapter and you rebind the item. It means both will share the adapter of the first one instead of the 2nd one having a new adapter set because createViewHolder will only have been called once.

@Skyyo
Copy link

Skyyo commented Sep 12, 2020

I don't think it's appropriate to do this in this way. I've played around with the problem and the most reliable way to restore the scroll position is to store it in a variable at the unbind call and then restore it in bind:

class CarouselItem(private val carouselDecoration: RecyclerView.ItemDecoration,
                   private val carouselAdapter: GroupAdapter<com.xwray.groupie.GroupieViewHolder>) : Item() {
    .....
    private var visibleItemPos = 0
    .....
    override fun bind(viewHolder: GroupieViewHolder, position: Int) {
        viewHolder.recyclerView.apply {
            adapter = carouselAdapter
            ........
            this.scrollToPosition(visibleItemPos)
        }
    }

    override fun unbind(viewHolder: GroupieViewHolder) {
        super.unbind(viewHolder)
        visibleItemPos = (viewHolder.recyclerView.layoutManager as? LinearLayoutManager)?.findFirstVisibleItemPosition() ?: 0
    }
}

If you agree with this feel free to apply the change to both classes and I can approve

Hey @ValCanBuild. Position restoration isn't perfect due to missing offset calculations. Is there any reason why the following approach shouldn't be used? Seems to result in a perfect restoration without any noticeable performance issues

class CarouselItem(private val carouselDecoration: RecyclerView.ItemDecoration,
                   private val carouselAdapter: GroupAdapter<com.xwray.groupie.GroupieViewHolder>) : Item() {
    .....
   private var recyclerViewState : Parcelable? = null
    .....
    override fun bind(viewHolder: GroupieViewHolder, position: Int) {
        viewHolder.recyclerView.apply {
            adapter = carouselAdapter
            ........
            (layoutManager as LinearLayoutManager).onRestoreInstanceState(recyclerViewState)
        }
    }

    override fun unbind(viewHolder: GroupieViewHolder) {
        super.unbind(viewHolder)
        recyclerViewState = (viewHolder.recyclerView.layoutManager as LinearLayoutManager).onSaveInstanceState()
    }
}

@Zhuinden
Copy link
Collaborator

The code in the discussion here does not seem to match the code on the branch, that's unfortunate because I think the code discussed here is technically correct. 😅

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.

4 participants