search

zhanghai / AndroidFastScroll

Fast scroll for Android RecyclerView and more
https://play.google.com/store/apps/details?id=me.zhanghai.android.fastscroll.sample
Apache License 2.0
698 stars 64 forks source link

Bug: position of the thumb can jump when scrolling #34

Closed AndroidDeveloperLB closed 3 years ago

AndroidDeveloperLB commented 3 years ago

I tried to use this library on my app, and while on some places it seems to work fine, on others it has at least one of these issues:

  1. Scrolling can cause the thumb to jump too much away from current location, sometimes in reverse.
  2. Even dragging the thumb can move it away from the touch location. Here I will present both using a POC, even though I have no idea if they are exactly the same as on my app.

Expected Behavior

That scrolling will move the thumb in the same direction of scrolling, without big jumps. And, that moving the thumb by dragging it will follow your touch.

Actual Behavior

The opposite.

Steps to Reproduce the Problem

Import the attached sample, and try both: scroll up and down slowly. And then also drag the thumb up and down slowly.

At first I thought it's because of some clipping attributes or padding, but then I noticed it can occur in other cases too. So no idea what really causes it.

Specifications

My Application.zip

XJu1vOdw4H.zip

zhanghai commented 3 years ago

The default ViewHelper doesn't work with span size out of the box. Please provide your own ViewHelper implementation as mentioned in README.

AndroidDeveloperLB commented 3 years ago

Why is it required? How come for fastScrollEnabled it's not required? It puts the thumb fine without jumping. The dragging however doesn't work well. I can't provide a height for each item. I use wrap_content. For this I will have to measure each item myself, no? I've also noticed there is no way to tell the library to avoid being shown when there are too few items (so there is no need for fast scroll as there is barely scrolling available).

See attached:

My Application.zip

Wrh0QSMS96.zip

zhanghai commented 3 years ago

It's up to you to provide an impl that works best for your case. I only use it for fixed-height items because of the complexity you mentioned.

I don't have a plan to allow hiding the scrollbar if there's not too much too scroll because I don't know how to define this"not too much", and I don't think builtin fastscroll allows it either.

AndroidDeveloperLB commented 3 years ago

About the heights, ok. I hope this is the only reason I get these issues. I also hope I will succeed with this.

The builtin fastscroll doesn't show up if it doesn't have much scrolling. Is it possible for me to make it this way though? I also don't see much point in showing the fast scroller, if there isn't much to scroll... Let's say that in some place I want to show it only if I scrolling is of at least 2 "pages". Is it possible to toggle it on/off in this case, or I have to think of something else?

AndroidDeveloperLB commented 3 years ago

@zhanghai I tried to see what ViewHelper needs, but it doesn't seem to match what you wrote, that it needs the size of the items. In fact it has many function calls that it needs and I don't see how I can give them.

Looking at the source code, it seems RecyclerViewHelper is a proper implementation of it, but it uses getItemHeight in multiple places, and uses an existing View to measure it. What I need is probably Views that don't exist yet, or exist outside of what's shown to the user.

This seems very hard to implement. If every View can have a different height, it means I would have to measure them all and give those functions the calculation based on it (sum). Can you please present a sample of implementing this properly? For example, a RecyclerView that has all of its Views with different height? Including both GridLayoutManager and LinearLayoutManager ?

AndroidDeveloperLB commented 3 years ago

I've prepared a POC based on 2 solutions I've found: https://stackoverflow.com/q/47846873/878126 https://stackoverflow.com/a/58652264/878126

This solves the original FastScroller issue while also handling insets (though maybe it got fixed over time). FastScrollerEx.zip

Sadly, it has an issue in case I want it to handle both RecyclerView padding AND transparent navigation bar.

Please consider a more flexible approach, one that won't need "ViewHelper" to be implemented, or at least won't have serious issues if I don't use it.

RikkaW commented 3 years ago

@AndroidDeveloperLB You have too many unreasonable requests.

To get the fast scroller to work with items with non-fixed height, you have to calculate the height for each item. If you do this, you lose the meaning of using RecyclerView, since all views are created. So the only way is to create your own algorithm that fits your situation. Create a perfect algorithm is impossible.

I have an implementation that uses the normal scroll bar's value (https://github.com/RikkaApps/libraries/blob/master/recyclerview-utils/src/main/java/rikka/recyclerview/RecyclerView.kt#L200-L206). It better but also has a problem when the height difference is too large.

About the "transparent navigation bar", the default behavior, "inside padding", does not have any problem. I don't know how you handle insets, but I have multiple projects that use this, no problem at all.

Anyway, it's better to have an option to control "inside" or "outside" padding.

AndroidDeveloperLB commented 3 years ago

@RikkaW What's unreasonable here? The RecyclerView supports non-fixed size of items (it has "setHasFixedSize" function in it). And it's not losing its meaning when doing so. The purpose of RecyclerView is to recycle the Views, so that it won't have to re-inflate them.

What if I could give an estimation instead of real size of each of the items? Say, for first item, it's exactly ABC height, and for the rest it's around XYZ height ? If I have plenty of items, it doesn't make sense that I will have to measure them all.

What's the part of " have an implementation that uses the normal scroll bar's value"? Where is it written?

RikkaW commented 3 years ago

@AndroidDeveloperLB

The cost of calculating the height for each view is too expensive, for example, TextViews. Or sometimes it is impossible, such as ImageViews that load images from the Internet, do you want to preload all images just for the fast scroller?

What's the part of " have an implementation that uses the normal scroll bar's value"? Where is it written?

I have an implementation that uses the normal scroll bar's value (https://github.com/RikkaApps/libraries/blob/master/recyclerview-utils/src/main/java/rikka/recyclerview/RecyclerView.kt#L200-L206).

What if I could give an estimation instead of real size of each of the items? Say, for first item, it's exactly ABC height, and for the rest it's around XYZ height ?

DO IT YOURSELF, WHO KNOWS WHAT HEIGHT OF YOUR VIEW ARE.

Stop it, pls

zhanghai commented 3 years ago

If you have read what you linked to, you should be able to find out that setHasFixedSize() is actually referring to the height of RecyclerView, instead of height of each individual item, so it is completely unrelated to this discussion.

I don't think we have any bug here, and this library never claimed it can handle variable item height out-of-the-box - in fact it is clearly written in README this isn't supported directly and you need to provide how to calculate the progress/height based on your specific use case. I have stated this for a number of times now and I don't think there's any more explanation to be made now.

Regarding another issue about minimum scrollable height before showing the scroll bar - I think either way works, e.g. always showing the bar is consistent and always clearly hints the user that there's some more content to reveal. And it's hard to determine the minimum scrollable height - one scaled touch slop? But that's for tap handling. In the end, I still prefer the simplicity of always showing the scroll bar if scrollable.

AndroidDeveloperLB commented 3 years ago

@RikkaW "The cost of calculating the height for each view is too expensive, for example, TextViews. Or sometimes it is impossible, such as ImageViews that load images from the Internet, do you want to preload all images just for the fast scroller? " That's the reason of what I said, that it's not a good thing to measure them all, and I'd prefer to give an estimation when it's not possible.

"DO IT YOURSELF, WHO KNOWS WHAT HEIGHT OF YOUR VIEW ARE.Stop it, pls" Stop what? Do what? I don't understand.

@zhanghai OK. You are right about the function. Sorry about this, but using RecyclerView doesn't mean all of its items are of the same size.

I know it's a problem. I just show that if there are many items, of different sizes, it's illogical to use it this way, and it can cause issues on this library, whether I do it as the repository says or not. What should I do, then? What would happen, if I give an estimation of average height for items that are of non-fixed-height, instead? Is this something that could fit for this library?

zhanghai commented 3 years ago

That's the reason of what I said, that it's not a good thing to measure them all, and I'd prefer to give an estimation when it's not possible.

Yes, so please provide the estimation yourself, there's no universal way so it can't be in this library.

Stop what? Do what? I don't understand.

I think they meant please stop asking me to add direct support for variable item height in this library - it's simply not possible to have a universal and good solution.

What should I do, then?

Read the README carefully and implement a ViewHelper.

What would happen, if I give an estimation of average height for items that are of non-fixed-height, instead?

If the height doesn't add up, your scroll thumb may jump abruptly during scroll.

Is this something that could fit for this library?

No.

AndroidDeveloperLB commented 3 years ago

@zhanghai

Yes, so please provide the estimation yourself, there's no universal way so it can't be in this library.

This will work well with the library? It won't cause issues as I showed?

I think they meant please stop asking me to add direct support for variable item height in this library - it's simply not possible to have a universal and good solution.

But the library has this support. It needs ViewHelper implementation to tell the height of each item. So, let's say I have the first item of some type, and the rest of a different type that have variable heights. Would it be ok if I measure the first 2 items? The second item would be an estimation of the rest.

Read the README carefully and implement a ViewHelper.

The thing is that it wants a height for each of the items. But when there are plenty of them, this is not practical. That's why I ask if it's ok to provide an estimation instead, hoping this won't cause an issue like what I've shown.

If the height doesn't add up, your scroll thumb may jump abruptly during scroll.

This is why I want to prevent it from happening. If I can't practically measure the height of all items, what could I do? A RecyclerView supports having many items in it.

zhanghai commented 3 years ago

This is why I want to prevent it from happening. If I can't practically measure the height of all items, what could I do?

As stated before, if you can't provide the height of each item, it's logically impossible to implement smooth fast scrolling, which is what this library does.

You may still have luck with fast scroll implementations that make the items jump (thus the scroll isn't smooth) instead of the scroll thumb, but that's not what this library is tring to offer.

AndroidDeveloperLB commented 3 years ago

@zhanghai So there is no way to have a nice fast-scroller in case the (many) items are of different sizes... :(

zhanghai commented 3 years ago

No if you can't know the item height without measuring.

AndroidDeveloperLB commented 3 years ago

@zhanghai So there is no solution for this.

zhanghai commented 3 years ago

I've said no a number of times now. No, it's logically impossible, you can't know where the thumb should be without knowing the heights. There is no magic, the answer is just no.

AndroidDeveloperLB commented 3 years ago

@zhanghai This wasn't a question. The built in fast-scroller though doesn't have this issue. Sure it changes its size, but at least it doesn't have weird jumping.

zhanghai commented 3 years ago

The built in fast-scroller though doesn't have this issue. > Sure it changes its size, but at least it doesn't have weird jumping.

Changing thumb size and inconsistent speed of thumb movement is the compromise. It's a different design and exactly the reason I created this library instead of building upon the existing one. You can use it if that's okay to you.

AndroidDeveloperLB commented 3 years ago

@zhanghai Right. Sadly it had an issue of becoming very small when there are many items. That's why I came here. I made some solution combining some code from some places to fix this, but I don't think it's best. Anyway thank you.