timusus / RecyclerView-FastScroll

A simple FastScroller for Android's RecyclerView
Other
1.39k stars 183 forks source link

Allow varying item view heights #51

Closed marverenic closed 7 years ago

marverenic commented 7 years ago

Adds a fix for #42.

Any adapters that have multiple item view types with different heights can implement the MeasurableAdapter interface. FastScrollRecyclerView can then rely on this calculation to figure out the height of the RecyclerView's contents instead of assuming that all rows have equal heights. The previous behavior is still used if this interface is not implemented.

Because the result of MeasurableAdapter.getHeightOfFirstViewsPx(...) is used directly by FastScrollRecyclerView, implementors need to take into account everything that affects the height of the contents of the RecyclerView (including layout managers and item decorations).

timusus commented 7 years ago

Thanks for this. This is very close to what I had in mind as a solution to this problem, though I've never gotten around to attempting it.

I have a couple of concerns.. These are probably potential improvements more than anything.

1.) The method name getHeightOfFirstViewsPx for the MeasurableAdapter is a bit confusing. It took me a while to understand what exactly the 'first views' was. Maybe something like scrollDistanceToPosition() might be better?

2.) It's quite onerous for the adapter to have to calculate the height for every view attached up to a certain position. I wonder if the adapter can instead return a value for a method such as heightForViewType(int viewType). So we perform the logic ourselves to determine the scroll height for a certain position, based on the number of views and their types in the adapter, and cross referencing them with heightForViewType(). Does that make sense?

marverenic commented 7 years ago

scrollDistanceToPosition() is a way better name. I was also thinking of renaming synchronizeScrollBarThumbOffsetToViewScrollWithHeight, but I wanted it to be consistent with synchronizeScrollBarThumbOffsetToViewScroll and wasn't sure how you felt about renaming that since it would technically be an API-breaking change.

I hadn't thought of calculating the heights that way, but that's a really good idea. Are these the method signatures you were thinking of having in the interface?

int[] getViewTypes();
int getViewTypeHeight(int viewType);
int getViewTypeCountBeforeIndex(int index, int ViewType);
timusus commented 7 years ago

Happy for that to be renamed. It's protected anyway, so I'm not sure it is 'API breaking'.. It's definitely not significant.

As for the method signatures, ideally this would be kept to a minimum. Perhaps getViewTypeHeight(int position) is all that's required - the view types themselves can be retrieved from the adapter via getViewType(int position). getViewTypeCountBeforeIndex is also something I see being calculated by this library, rather than the implementer of MeasurableAdapter.

If you'd like to chat about this, feel free to hit me up on slack: http://shuttle-slack-inviter.herokuapp.com/

Or hangouts (t.malseed at gmail.com)

timusus commented 7 years ago

@marverenic would you be able to update the Readme to include some usage instructions, and explain what the caveats are when using this feature? (I know it's limited to certain LayoutManagers, etc.)

marverenic commented 7 years ago

Just added documentation to the readme. I'm working on adding support for GridLayoutManager since I've finally started getting more free time towards the end of the semester, but I'll make another PR for it when it's finished.