search

rubensousa / DpadRecyclerView

A RecyclerView built for Android TV with Compose in mind and as a replacement for Leanback's BaseGridView.
https://rubensousa.github.io/DpadRecyclerView/
Apache License 2.0
136 stars 17 forks source link

Bug: DpadSpanSizeLookup's getSpanSize function is being called infinitely while fast scrolling #218

Closed tyrel-carlson closed 5 months ago

tyrel-carlson commented 5 months ago

Hi there,

I'm using DpadRecyclerView to show a grid of items in 4 columns, and I have header views to show the items in sections. The important part is that section items are not full, there are some empty cells in each section's last row.

The issue is when I scroll up from an item that doesn't belong to the first column, it stops scrolling and after a while, and crashes with an ANR. I found that there is a problem with the getSpanSize function. Here is part of my logs:

11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 57
11:43:47.514 DebugTag       D  getSpanSize: 57
11:43:47.514 DebugTag       D  getSpanSize: 60
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 57
11:43:47.514 DebugTag       D  getSpanSize: 57
11:43:47.514 DebugTag       D  getSpanSize: 60
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 57
11:43:47.514 DebugTag       D  getSpanSize: 57
11:43:47.514 DebugTag       D  getSpanSize: 60
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 57
11:43:47.514 DebugTag       D  getSpanSize: 57
11:43:47.514 DebugTag       D  getSpanSize: 60
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 57
11:43:47.514 DebugTag       D  getSpanSize: 57
11:43:47.514 DebugTag       D  getSpanSize: 60
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 57
11:43:47.514 DebugTag       D  getSpanSize: 57
11:43:47.514 DebugTag       D  getSpanSize: 60
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 57
11:43:47.514 DebugTag       D  getSpanSize: 57
11:43:47.514 DebugTag       D  getSpanSize: 60
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 57
11:43:47.514 DebugTag       D  getSpanSize: 57
11:43:47.514 DebugTag       D  getSpanSize: 60
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 57
11:43:47.514 DebugTag       D  getSpanSize: 57
11:43:47.514 DebugTag       D  getSpanSize: 60
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 59
11:43:47.514 DebugTag       D  getSpanSize: 58
11:43:47.514 DebugTag       D  getSpanSize: 58

and here is my code:

 private fun setupRecyclerView(recycler: DpadRecyclerView) {
        recycler.adapter = adapter
        recycler.setSpanCount(SPAN_COUNT)

        recycler.setSpanSizeLookup(object : DpadSpanSizeLookup() {
            override fun getSpanSize(position: Int): Int {
                return if (position in 0 until adapter.itemCount && adapter.isSectionHeader(position)) SPAN_COUNT else 1
            }
        })

        recycler.attachListController(viewModel.listController)

        recycler.setSmoothScrollSpeedFactor(2f)
        recycler.setSmoothScrollMaxPendingMoves(1)
    }

I would appreciate it if you could help me fix this. Is there anything I can do to have it before the next library release? Thanks in advance.

tyrel-carlson commented 5 months ago

FYI, I have fixed it using recycler.setSmoothScrollMaxPendingMoves(0)

rubensousa commented 5 months ago

Hi @tyrel-carlson. Which version of the library are you using? Please give me the entire configuration of your adapter and number of rows/columns that have a section header. This is a continuation of this issue, right? https://github.com/rubensousa/DpadRecyclerView/issues/210

rubensousa commented 5 months ago

Fix should be here: https://github.com/rubensousa/DpadRecyclerView/pull/219

tyrel-carlson commented 5 months ago

Hi @tyrel-carlson. Which version of the library are you using? Please give me the entire configuration of your adapter and number of rows/columns that have a section header. This is a continuation of this issue, right? #210

I'm using the latest version 1.3.0-alpha03. I suggest including a new fragment using SpanSizeLookup in the sample project. That would be a good practice to have it to detect any issues.

rubensousa commented 5 months ago

@tyrel-carlson that's exactly what I did in https://github.com/rubensousa/DpadRecyclerView/pull/219 I plan to release this soon

rubensousa commented 5 months ago

Could you do me a favor and check https://github.com/rubensousa/DpadRecyclerView/pull/219/files#diff-50cbcf55268ef507b233dc6a02a723b8ada87fc2f470ff8e834534ee090be7f8 and the branch span_size_focus? You can checkout the project on your machine and help me with testing that it's working as you expected

tyrel-carlson commented 5 months ago

Sure, I will check it

tyrel-carlson commented 5 months ago

I have tested the branch. Grids > Span Headers works fine on my emulator.

rubensousa commented 5 months ago

@tyrel-carlson thanks! I will add some UI tests and then ship the release

rubensousa commented 5 months ago

Released in 1.3.0-alpha04. Please try it out

tyrel-carlson commented 5 months ago

Did you change any dependencies in this release? After syncing I get some errors on my dispatchKeyEvent overridden functions in other files.

rubensousa commented 5 months ago

@tyrel-carlson yes, I updated RecyclerView to the latest version. They added an override to dispatchKeyEvent with nullable annotation: https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:recyclerview/recyclerview/src/main/java/androidx/recyclerview/widget/RecyclerView.java;l=1977

rubensousa commented 5 months ago

@tyrel-carlson was everything fine with the newest version?

tyrel-carlson commented 5 months ago

I'm going to update to the newest version soon. I will update you once I've tested the changes.

tyrel-carlson commented 5 months ago

I've tested new version, I can confirm that reported bugs are fixed, but there are two more issues there:

I have fixed both of these issue, but you may want to fix them as well.

rubensousa commented 5 months ago

when we have one item in a row and the right cells of the item is empty, right arrow will move the focus to second item on the next row. for example lets say we have this view:

What's your expected state in that scenario? Would you prefer to not focus position 4? The default focus search from the system kicks-in when there's no focusable view at the focus direction, in that case the system finds position 4 because it's at the right side of position 1.

the other issue is when I go up from first row, it won't focus the parent which is the top menu, I'm not sure if this behavior should be the default one.

Are you using these?

image

Please try setFocusOutAllowed(throughFront = true, throughBack = false) and let me know if it works

tyrel-carlson commented 5 months ago

I have used setFocusOutAllowed(throughFront = true, throughBack = false) and that works fine. thanks

for the other issue, I expect the focus to remain unchanged, since there is no cell at the right of the cell. here is the code I'm using to prevent this behavior:

View.FOCUS_RIGHT -> {
    // Prevent unusual focus movement if the right cell is empty
    val nextPosition = currentPosition + 1
    if (getSpanIndex(nextPosition, spanCount) <= currentSpanIndex) currentPosition else nextPosition
}

and also I see if we move right from other columns (second or third column for example) focus remains unchanged. So there should be same behavior for all movements to the right.

rubensousa commented 5 months ago

@tyrel-carlson

Thanks for the feedback. I will make focusOutFront and focusOutBack enabled by default in the next release.

Regarding the grid focus, I think that makes sense, I will open a new issue and solve that as well

rubensousa commented 5 months ago

@tyrel-carlson both fixes are now available in 1.3.0-beta01