Improving the reusability and maintainability in ScrollbarExt.kt file and Fix the scroll bar thumb jumping a little bit due to lack of smothness on interpolatedIndex

[rodrigodiasferreira]

What I have done and why

Improved the reusability and maintainability of the file ScrollbarExt.kt file by implementing a generic: <LazyState : ScrollableState, LazyStateItem> LazyState.genericScrollbarState, and reducing the repetitive logic and code.

Related issue:

• [FR]: In the file ScrollbarExt.kt there is room for reducing the logic and code repeating in order to improve the reusability and maintainability #1549
• It also fixes the issue: [Bug]: Scrollbar is hardly stable #1526 (the part related to the interpolated index).

[yongsuk44]

Sometimes, I believe that dividing similar logic into multiple extension functions is done to reduce the learning curve of the logic.

When I look at the refactored code, it seems that jumping from the extension function to genericScrollbarState and then jumping back to the extension function to check arguments could occur. This might lower the readability for developers who are learning the code, and I am concerned that it could increase the learning curve. 🥲

[rodrigodiasferreira]

Hi @yongsuk44, thanks for your comment making this discussion richer.
I disagree. When I was studying the Now In Android project, I had to compare the 3 functions in order to check what they had in common, and then I figure it out, it is pretty much the same logic, but with the differences that each type brings: LazyListState, LazyGridState, LazyStaggeredGridState. This refactoring unify the logic into a unique function, and makes very clear that they perform the same logic, which makes easier for study in my opinion, because you don't need to read the 3 different functions to realize that they are the same logic.

Moreover, if there will be some need to fix something in that logic in the future, it won't be necessary to change on the 3 different functions, only in one place, which improves also the maintainability. Besides one could even forget to change on these other places, which would cause different behavior for the scroll bar for the different type.

In summay, in my opinion, this refactoring is a good improvement for Now In Android code base in aspects of study, reusability and maintainability.

[anhtuannd]

Hi,
I think LazyStaggeredGridState's ScrollBar needs some improvement on how we calculate thumb size and position, as seen in #1526

So merging them into once might not be the best approach.

[rodrigodiasferreira]

Hi @anhtuannd , thanks for raising this concern.
As I commented on the issue: #1526, I believe I've fixed that issue on the PR: #1553, and the solution was done in a common place, so the unifying done on this PR, won't affect the fix of that issue. Please check when you have some time, and let me know, if you agree...

[tunjid]

Hi hi! Original author of the Scrollbar here. It used to have a generic abstraction, but we split up out for readability.

You can see the original PR where the generic code was split up favoring readability over copy pasting here: #947 (comment)

[rodrigodiasferreira]

Hi TJ / @tunjid , how are you?
I watched several videos that you recorded on the Android Developers site.
It's an honor to meet you!

I understand what you are saying, but on my opinion I still think that the generic abstraction eliminates some duplicated logic and improves the maintainability. And also I prefer to read the more concise code, which I know for sure that the parts that use the same generic abstraction, they are the same, no need to compare the code and checking that they do the same thing.

But since you guys made this decision consciously, I will open a new PR with the fix on the commit: 31b77d5, and close this one.

Thanks a lot for the feedback. :-)

[rodrigodiasferreira]

Hi @tunjid,

I've uploaded the commit: 31b77d5 into the PR: #1553. So the fix related to issue: #1526 is concentrated on the same PR.
I will close this one.

Once again, thanks for the feedback.