rubensousa / GravitySnapHelper

A SnapHelper that snaps a RecyclerView to an edge.
Apache License 2.0
5k stars 614 forks source link

SnapListener broken if enough padding is added to end of items to snap to last item #49

Closed jt-gilkeson closed 5 years ago

jt-gilkeson commented 6 years ago

Use Case: I want a Gravity.Start list where I can have the last item snapped to the first position (all the way at the start). The reason for this is that I want to highlight / show something related to the item currently snapped and I want to be able to do this for all items.

To accomplish this, you can add enough padding to the end of the recyclerview that you can scroll the last item all the way to the start of the view. The RecyclerViewSnap functions perfectly - it correctly can snap to all the items including the Last item - no problem. However, the SnapListener does not fire ever in this scenario. No matter where you are in the list (first item, middle of the list, last item), the code in GravityDelegate for onScrollStateChanged where it calls getSnappedPosition(recyclerView) returns RecyclerView.NO_POSITION even though the snap is working completely correctly and the item is completely visible.

Example: Make an item that is 150dp wide, set up your recyclerview like follows:

<android.support.v7.widget.RecyclerView
    android:id="@+id/myRecyclerview"
    android:layout_width="match_parent"
    android:layout_height="wrap_content"
    android:paddingStart="12dp"
    android:paddingEnd="300dp"
    android:clipChildren="false"
    android:clipToPadding="false"/>

Snapping works correctly for all items, SnapListener never fires for any item.

Change the paddingEnd to be 200, you'll be able to snap to the 2nd to last item (further than the default with no padding - which is the 3rd to last item), and the SnapListener works correctly, however you can't snap to the last item.

In summary with enough padding added, the code to execute the snap appears to work 100% correctly, but the code to report the item snapped to seems to break.

jt-gilkeson commented 6 years ago

It looks like this bug is actually in the Support Lib's findFirstCompletelyVisibleItemPosition (https://issuetracker.google.com/issues/37019979) - which your listener is relying on - they don't seem to handle the padding correctly when it comes to visibility. Is it possible to use the logic for the snap to also report which item has been snapped instead of using the findFirstCompletelyVisibleItemPosition which doesn't work correctly for clipTopPadding = false?

jt-gilkeson commented 5 years ago

It looks like findFirstVisibleItemPosition() works when do you a Gravity.Start with padding at the end.

Perhaps an easy fix for this issue would be to add fallback logic to getSnappedPosition() that calls findFirstVisibleItemPosition() if findFirstCompletelyVisibleItemPosition() returns -1? (and vice versa for end/bottom case)

rubensousa commented 5 years ago

Sorry for the late reply, but I've been busy and can't work on this right now. I saw that you created a PR and I appreciate that. Give me some time to reproduce the behavior you described and then test the fix you proposed. Thanks!

jt-gilkeson commented 5 years ago

I have a sample project that shows the issue with findFirstCompletelyVisibleItemPosition always returning -1 for a recyclerview with end padding (no snap in this example project - but it should show you the issue) https://github.com/jt-gilkeson/RecyclerViewPaddingDemo

rubensousa commented 5 years ago

I've merged your fix but noticed that snap is still broken when there's padding on either start or end.

findFirstVisibleItemPosition() doesn't work reliably when there's padding and it breaks the methods that find the target view.

rubensousa commented 5 years ago

I've managed to fix this tonight. Please check the develop branch.

llleodeleon commented 5 years ago

@rubensousa just to inform. I have this exact case where I want to snap to start but I add padding so te last item also snaps, but the listener is not called. Is there any approximate when this is going to be released?

rubensousa commented 5 years ago

Hopefully this weekend if I have time. But please check if the code from the develop branch fixes the issue for you. You can copy the 2 classes.

rubensousa commented 5 years ago

Released in 2.0