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

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

Closed
jt-gilkeson opened this issue Sep 18, 2018 · 9 comments
Assignees
Labels
Milestone

Comments

@jt-gilkeson
Copy link
Contributor

jt-gilkeson commented Sep 18, 2018

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
Copy link
Contributor Author

jt-gilkeson commented Sep 18, 2018

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
Copy link
Contributor Author

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)

jt-gilkeson added a commit to jt-gilkeson/RecyclerViewSnap that referenced this issue Sep 27, 2018
Fallback to findFirstVisibleItemPosition if findFirstCompletelyVisibleItemPosition is reporting NO_POSITION to handle recyclerviews that have enough padding to be able to snap all items  (rubensousa#49)
@rubensousa
Copy link
Owner

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!

@rubensousa rubensousa added the bug label Sep 30, 2018
@jt-gilkeson
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Owner

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

@rubensousa rubensousa added this to the 2.0 milestone Nov 4, 2018
@rubensousa rubensousa self-assigned this Nov 4, 2018
@llleodeleon
Copy link

@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
Copy link
Owner

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
Copy link
Owner

Released in 2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants