turing-tech / MaterialScrollBar

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

Scrollbar doesn't move between positions when using custom scroll #71

Closed Mina-H-Samy closed 7 years ago

Mina-H-Samy commented 7 years ago

So I have different row heights in my recyclerview so I implemented ICustomScroller in my adapter for scrolling to work - which it does.

However it is not very smooth because the scroll bar doesn't move between two positions if that makes sense. As in: if I have position 1 at the very top of the screen and position 2 in the middle of the screen, dragging the scrollbar from top to middle doesn't show the scrollbar being dragged, however when I reach the middle of the screen the scrollbar suddenly jumps from the top of the screen to the middle. Am I missing something?

turing-tech commented 7 years ago

Do you mind posting the code of your adapter so I can see what you wrote?

Mina-H-Samy commented 7 years ago

so my recyclerview is just textviews (and most of them are longer than the full height of the screen). I have a List that contains the height of each of the textviews. My adapter looks like this:

private final List<Integer> sectionHeights;

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

@Override
public int getItemIndexForScroll(float progress) {
    float depth = 0;
    float totalDepth = getTotalDepth();
    for (int i = 0; i < sectionHeights.size(); i++) {
        depth += sectionHeights.get(i);
        if (progress <= depth / totalDepth) {
            return i;
        }
    }
    return sectionHeights.size() - 1;
}

@Override
public int getTotalDepth() {
    return getDepth(sectionHeights.size());
}

private int getDepth(int endIndex) {
    int depth = 0;
    for (int i = 0; i < endIndex; i++) {
        depth += sectionHeights.get(i);
    }
    return depth;
}
turing-tech commented 7 years ago

I've just verified myself and this should be scrolling smoothly. The code responsible for doing this is here, taken from ScrollingUtils:

        View child = materialScrollBar.recyclerView.getChildAt(0);

        if(child == null){
            scrollPosState.rowTopOffset = 0;
        } else {
            scrollPosState.rowTopOffset = materialScrollBar.recyclerView.getLayoutManager().getDecoratedTop(child);
        }

When I added ICustomScroller to my test adapter just now this worked properly. If it's not working for you still here's what I'd say could be causing it:

  1. The sectionHeights list must be in DP, not pixels, or else the scrolling would jerk.

  2. materialScrollBar.recyclerView.getChildAt(0); must not be null.

  3. materialScrollBar.recyclerView.getLayoutManager().getDecoratedTop(child); must change as you scroll down.

Please verify that all of these conditions are true and let me know.

Mina-H-Samy commented 7 years ago

Thanks for investigating!

So number 3 is false for me. materialScrollBar.recyclerView.getLayoutManager().getDecoratedTop(child); always returns 0.

turing-tech commented 7 years ago

Ok, let me check on that on my end.

turing-tech commented 7 years ago

So I've verified and materialScrollBar.recyclerView.getLayoutManager().getDecoratedTop(child); does change for me.

What layoutManager are you using for your recyclerView? Is it a custom one or is it included in the OS? Any other ideas about why that might be returning zero?

As you scroll down the number should become negative because the top of the first visible view will be above the screen.

Mina-H-Samy commented 7 years ago

just using LinearLayoutManager from the support library:
android.support.v7.widget.LinearLayoutManager

I set it like this:

recyclerView.setLayoutManager(new LinearLayoutManager(getContext()));

No idea why it always returns 0...

turing-tech commented 7 years ago

So that method returns:

child.getTop() - getTopDecorationHeight(child);

So I'm guessing that child.getTop() is always equal to 0? If you could find some way of verifying that. I really can't imagine what's causing this to happen unless you're employing some odd view topology. Are the views in the list direct children of the recyclerView? It must be because they're obtained by calling:

View child = materialScrollBar.recyclerView.getChildAt(0);

This implies that the first visible view's top isn't moving, which doesn't make any sense.

Mina-H-Samy commented 7 years ago

hmm very strange... This is my first time using RecyclerView so I might be missing something. I'll do some more investigation and let you know.

turing-tech commented 7 years ago

Ok, good luck. If you need to send me any code to look over and see if I can help just email turing.technologies@gmail.com and I'll be glad to help!

MFlisar commented 7 years ago

I recently encountered problems in a BottomSheetDialogFragment with RecyclerView and realised, that the LinearLayoutManager disables any recycling in this case and just lays out all items (like the old ListView did).

You may check if this has some influences and causes this behaviour? In this case RecyclerView may always return the first item as first child and the first items decoration height would stay the same...

I've not tested it, it's just a guess...

Mina-H-Samy commented 7 years ago

I now realise we've been misunderstanding each other. I'm sorry I should have been more clear. So my problem is actually only when fast scrolling so when I press on the scroll bar and drag it. That's when it's not smooth for me.

However when I'm scrolling by swiping up and down, everything works great (and btw .getDecoratedTop(child) does decrease as you said).

Mina-H-Samy commented 7 years ago

Sorry to be a pain but here's my current status: I'm trying to actually implement my own indicator, so what I've done is removed the .setIndicator() call and my adapter does not implement ICustomScroller or ICustomAdapter now.

So currently, this works great except one tiny problem: scrollbar jumps up and down when scrolling because of the different heights of my rows.

I think what I really want is to only implement ICustomScroller#getTotalDepth without implementing the other two methods in the ICustomScroller.

I thought of changing the ICustomScroller so that its methods return Integer's which can be null (and opening a PR). Do you have a better solution? I tried poking around the lib for classes/methods I can override to achieve what I want but it doesn't seem to be possible.

Mina-H-Samy commented 7 years ago

mm actually... now that I think of it, it would probably be better to have something like DragScrollBar.setTotalDepth() ?

turing-tech commented 7 years ago

You'd need all three methods of ICustomScroller in order to get a smooth scroll. I don't see how making your own indicator has to do with removing ICustomScroller. You can use ICustomScroller without an indicator.

Mina-H-Samy commented 7 years ago

ok please bear with me: I'm trying to implement my own indicator and remove ICustomScroller to overcome 2 issues.

So I am using a DragScrollBar, my 2 issues only happen when I am dragging the scroll thumb (not scrolling by swiping up and down the screen, which works fine). The 2 issues are:

1) The issue in OP (so when I drag the scroll thumb, it doesn't move between two positions but only jumps to the position once my finger has reached that position).

2) Again when dragging, the item in the RecyclerView that should be at the top of the screen is wrong sometimes (it's off by quite a bit). So say I'm at position 5 with the dragging, that means 2 things: a) the text for position 5 is shown in the indicator. b) the start of child 5 in the RecyclerView is at the top of the screen. (a) works fine but (b) does not when the child in the RecyclerView has height that is less than the screen height.

Now, with 1 and 2, I realise they are very specific to my case and I don't want to burden you with fixing them for one person. Also, it would be much easier for me to implement my own indicator rather than you fixing this in such a way that works for everyone (that's why I didn't create an issue for 2).

If I implement ICustomScroller, it solves the "scroll bar jumping up and down problem" but it causes 1 & 2 to happen.

So essentially what I'm trying to do is: fix the "scroll bar jumping up and down problem" without implementing ICustomScroller.

turing-tech commented 7 years ago

Hold up, hold up. If I'm understanding point 1 correctly, then the OP is about the scroll thumb dragging not causing the recyclerView to scroll smoothly? The way it was phrased it sounded like the thumb wouldn't scroll smoothly when the recyclerView was being dragged. If you could clarify which one it is. If that's the case then I think I could figure out how to fix that. As far as issue 2(b) goes, it is important that I fix it for everyone because it seems like that would happen for every custom scroller. Is the view displayed too far down or too high up as compared to what it should be? If you could give me more details on that I'd be much obliged.

Mina-H-Samy commented 7 years ago

ok I'll code up an example that shows both problems when I get home (much easier than explaining...)

turing-tech commented 7 years ago

Sounds good!

Mina-H-Samy commented 7 years ago

ok I tried pushing a branch to the repo here but I got 403 so I created a gist with a .patch file (just download the patch file and apply it in android studio to your master branch): https://gist.github.com/MinaHany/ddf4d83e5ffecb30134d19e0373723cd

btw, the depths for each section is a bit off, couldn't be bothered to set it correctly and I believe my issues are unrelated to this.

So I'll explain the two issues in context of that gist:

1) If you move to say section 7 and then drag the scroll thumb between to section 8 (the next section), you'll find that the scroll thumb itself is locked in the "7" position, until your finger reaches the "8" position. When you reach "8" it jumps straight away from "7" to "8" in the scroll track. It feels unnatural because between 7 & 8 is quite a bit of space and I would expect the scroll thumb to move in the track with my finger. So for example if my finger is halfway between 7 & 8 on the track, the thumb position should be halfway as well, but that's not the case it remains locked in position, until I reach 8

2) If you drag the scroll thumb from section 2 to section 3 say, you'll find that the title of section 3 is NOT at the top of the recyclerView. You can see this problem in the first 5 sections and not the last 5 (this I believe is because the first 5 sections do not take up the whole height of the screen). Notice also that this only happens when scrolling from top to bottom and not the other way around! So if you scroll from section 3 to section 2, the title of section 2 is correctly displayed at the top of the screen.

Again, please don't feel compelled to fix these issues, I realise my use-case is a bit rare and I do have other alternatives to achieving what I want.

turing-tech commented 7 years ago

I've just downloaded the patch. I'm checking it out now. I'm seeing both issues 1 and 2 so I'll see what I can do to resolve them.

turing-tech commented 7 years ago

Issue 2 is resolved. Still working on issue 1.

turing-tech commented 7 years ago

Fixed issue 1. I'm going to go ahead with the refactor and then I'll push the solution.

Mina-H-Samy commented 7 years ago

dude you're a LEGEND! thank you very much, everything works great now :)

turing-tech commented 7 years ago

I can't thank you enough for the support you've shown. It's 01:32 in the morning here and I just woke up and checked my phone briefly. When I saw what you did I had to come and thank you. I really do appreciate it. I really can't express how encouraging it is to receive this kind of support. Thank you again, and if you ever need any kind of help in the future, please let me know! You're awesome.

Mina-H-Samy commented 7 years ago

You absolutely deserve it! It is not very often that I find someone committed, fully motivated and also knows what they're doing. Seriously the few problems I were having you solved in a day or so... Kudos to you my friend, wishing you all the best!