turing-tech / MaterialScrollBar

An Android library that brings the Material Design 5.1 sidebar to pre-5.1 devices.
Apache License 2.0
781 stars 126 forks source link

GridLayoutManager + Headers => jumping when scrolling fast scroller #37

Closed MFlisar closed 7 years ago

MFlisar commented 8 years ago

I wrap my items in a Wrapper class which ads header items to the underlying data. Headers will have a span size equal to the columns in the GridLayoutManager and headers have one. Setup looks like following:

((GridLayoutManager) rvContent2.getLayoutManager()).setSpanSizeLookup(new GridLayoutManager.SpanSizeLookup() {
            @Override
            public int getSpanSize(int position) {
                if (rvContent2 == null) // may happen while activity is destroyed!
                    return 1;
                return adapter.isHeader(position) ? ((GridLayoutManager) rvContent2.getLayoutManager()).getSpanCount() : 1;
            }
        });

Seems like scrolling logic does not work with such a setup due to different item sizes and spans?

turing-tech commented 8 years ago

No, for right now it depends on each item being consistently sized. I can look into changing this but that is the current setup

turing-tech commented 8 years ago

After looking into it this seems to be very difficult to achieve. Each row would need to be measured which would either effectively defeat the purpose of a recyclerView (by keeping an instance of every view alive at the same time) or require some really innovative solutions. If you know of a lib that can do it point it out and I'll look into it but as far as I can tell it would be very difficult to do.

MFlisar commented 8 years ago

I don't know a solution. I had adjusted an old version of your lib (v6 or v5) to have a custom offset calculator, but this is not compatible anymore and I need your improvements...

Idea1

I thought some calculator interface would work, something with that I could calc the offset manually. I thought about something like following:

interface IOffsetCalculator
{
    int getItemIndexForScroll(float scrollBarPosition);
    float getScrollPositionForItemIndex(int index);
}

If the user knows how big a header is and how big items are, he can calculate above values manually (without laying out any view)...

I'm not sure if the interface is still working with your current code though and it's no solution for completely free layout (unknown size of headers and items).

Idea2

What about having a solution that is based on view types + consistent sizes for every single view type? This may be possible to implement... Then you at least need to layout every view type once, but this would not defeat the purpose of the RecyclerView....

CillianMyles commented 8 years ago

I realise that this will be very difficult to do for dynamic views. As you eluded too - you make take away a lot of the advantage we gain by using RecyclerView at all.

However, I tend to agree with @MFlisar's first idea. This could mean that you could have a smoother experience for lists which have different view types (lots of lists - that's another big advantage of Recycler) which are static (sizes don't vary for each view type specifically).

Would this be possible to implement?

turing-tech commented 8 years ago

@MFlisar Are you saying if I can pass a progress float on scrolls that you can return the item index and vice-versa? If so I could definitely pass those values to custom dev scrolling logic if that's all you need.

MFlisar commented 8 years ago

@krimin-killr21 for my use case, this would be enough...

I have a simple adapter that even could be abstracted and would fulfill a common use case: fixed size items in a grid with header items of a another fixed size (span size for headers is number of cols, span size for items is 1). I think I could even provide a default calcutor for this use case. It would basically just do following:

Still, it's an important precondition that all items of one kind have the same size. And of course, the intermediate calculated data must be invalidated when items are inserted or deleted, but for my use case I would let my adapter handle that and just recacluate the intermediate data when I call one of the notifyData... function of the adapter.

turing-tech commented 8 years ago

I'll give you an option for custom scroll calculation and then if you can get a good solution worked out tell me and I can put it in.

turing-tech commented 8 years ago

@MFlisar Try out RC1-9.2.0 and see if it works for you.

MFlisar commented 8 years ago

@krimin-killr21

I can see the tag RC1-9.2.0 but this does not seem to use the new ICustomScroller... I can also see a new branch CustomScroll, this seems to use the interface... But I can't find the definition of ICustomScroller in the CustomScroll branch...

2 other things I've seen in this branch is following:

  1. I did not check where you use the function ScrollingUtilities.getRowCount(), but this function should be somehow dependent of an ICustomScroller as well... A header will result in a line break (defined with a span size), so the default row calculation will be invalid for this setup...
  2. I think it would be useful to add a boolean function to enable/disable the custom scrolling to the interface. In my case I will use the adapter with lists and grids...
turing-tech commented 8 years ago

@MFlisar I'm not grasping what it is you're missing in the first part of your comment.

As to turning it off, I can add that but turning it on only applies to that instance of the bar. Also, the get row count is not part of the custom calculation.

MFlisar commented 8 years ago

I downloaded the RC-9.2.0 tag. And I can't find where you added the custom scroll... I checked your repo and saw you as well have a CustomScroll branch. There I see the handling of the custom scroll in code but I can't find a place where `ICustomScroller`` interface is defined, when adding this to my project a get compile time error confirming this...

Following is missing from CustomScroll branch:

package com.turingtechnologies.materialscrollbar;

public interface ICustomScroller
{
    float getScrollPositionForIndex(int index);
    int getItemIndexForScroll(float touchFraction);
}

I added it to the code and will try my code with this branch...

turing-tech commented 8 years ago

Okay, I'll check on it when I get home in a few hours. Thanks.

MFlisar commented 8 years ago

Ok, I think so far this looks good.

Still two small things:

  1. I think, the ICustomScroller should not need to calculate a y position directly but to rather return a float with following condition: 0 <= scrollPosition <= 1. The actual real position that corresponds to that relative position should be calculated from the scrollbar itself...
  2. Currently I have to call MaterialScrollBar.addIndicator(...) to add my custom scroll logic, I think this should not be necessary

Will you adjust that? I gladly would check out the adopted project and add my code and make a pull request then... From my point I have everything already, at least for a simple header with children adapter... Still two small problems in there, but all in all already working...

Thanks for this great library and for willing to extend it

turing-tech commented 8 years ago

I'm not sure I understand the first recommendation, but I'll definitely implement the second. What did you mean? Like the relative position related to the bar instead of the absolute against the screen?

MFlisar commented 8 years ago

I mean the interface should return the progress of the bar in percentages and the rest should be done by the library... with the percentage value returned by the interface the library should calculate the scroll bar position. Currently you expect the interface to return the absolute y position of the scrollbar... I'm talking about the getScrollPositionForIndex function...

turing-tech commented 8 years ago

How does this work for you?

Also I checked and I don't think you need to call addIndicator to use custom scroll logic.

turing-tech commented 8 years ago

PS, I've changed the naming scheme to EXPERIMENTAL-[release]

MFlisar commented 8 years ago

I just made a pull request...

Usage (and example)

Example adapter:

public class ExampleAdapter extends Adapter<VH> implement FastScrollerUtil.IHeaderAdapter, ICustomScroller
{
    @Override
    public int getItemCount()
    {
        // this is a default recycler view adapter function, but it is also part if the IHeaderAdapter interface, but it must be implemented for every adapter anyway!
    }

    // -----------------------
    // Header and CustomScrolling
    // -----------------------

    // this manager MUST be invalidated
    // - whenever the count of items change
    // - items are moved from one header to another
    // - span count of the LayoutManager is changed

    private FastScrollerUtil.HeaderScrollManager mHeaderScrollManager;

    @Override
    public boolean isHeader(int index)
    {
        // return whether this item is a header or not
    }

    @Override
    public void initScrollManager(int span)
    {
        mHeaderScrollManager = new FastScrollerUtil.HeaderScrollManager(span);
        mHeaderScrollManager.calcData(this);
    }

    @Override
    public int getDepthForItem(int index)
    {
        return mHeaderScrollManager.getDepthForItem(index);
    }

    @Override
    public int getTotalDepth()
    {
        return mHeaderScrollManager.getTotalDepth();
    }

    @Override
    public int getItemIndexForScroll(float scrollBarPos)
    {
        return mHeaderScrollManager.getItemIndexForScroll(scrollBarPos);
    }

    @Override
    public int getHeaderHeight()
    {
        // here you calculate the height of a header row in pixels and return it
        return 50;
    }

    @Override
    public int getRowHeight()
    {
        // here you calculate the height of a default row in pixels and return it
        return 200;
    }
}

Example initialisation

ExampleAdapter adapter = new ExampleAdapter();
recyclerView.setAdapter(adapter);
scrollBar.useCustomScrolling();
adapter.setItems(items);
// this will take care to init everything. Span size lookup and all the necessary helper setup
FastScrollerUtil.initHeaderScroller(mMediaHelper.rvContent2);

Observations/Problems/Suggestions

turing-tech commented 8 years ago

Okay, I definantly like most of the changes. I'll need to test them out in a bit but if all goes well I'll approve it. Thanks for the hard work :)

MFlisar commented 8 years ago

Just added the last small update...

Thanks for your work as well

turing-tech commented 8 years ago

Test EXP-4 and let me know if everything works for you. I'll leave it alone for a few days and wait for any bugs to arise and then I'll merge into the master and write up a guide.

MFlisar commented 8 years ago

Something with your tags is confusing for me.

You made a EXP-4 tag but when I look into the code the last commit is 2 days ago and there's nothing in it about custom scrolling.

I can see your pull request #40 and this seems to be thing I should test, isn't it? So I will test the master of the CustomScroll branch.

turing-tech commented 8 years ago

Sorry, I just published from the wrong branch by accident. I redid and it should be correct now. The code on jCenter was correct in any case.

MFlisar commented 8 years ago

One thing that's missing for me now is following:

FastScrollerUtil.getSpanSize() should be public... it's helpful to init a new HeaderScrollManager... could you change that with your next update? Thanks

turing-tech commented 8 years ago

Sure thing. I'll be making a set of minor adjustments in the next version, which will hopefully come out sometime before Monday.