feat(scrollbar): overscroll and hide_when_not_scrollable

[EdJoPaTo]

Personally I don't like the scrollbar to reflect the overscroll. This adds a setting to turn this off. (personal opinion: overscroll shouldn't be the default)

Also, there is often content without the need for scrolling (Like a List with fewer items than its height allows). I also added a setting to disable rendering in such a case.

There are no test cases for this yet as I would prefer to discuss this first before taking even more effort into this.

Workaround until there is a proper solution (like this PR attempts): state.saturating_sub(viewport_content_length) both hides the scrollbar without scroll and hides the overscroll.

This PR will have merge conflicts with #963, and I will fix them after it's merged. Done

According to the code comment this change will definitely be a breaking change:

https://github.com/ratatui-org/ratatui/blob/d0067c8815d5244d319934d58a9366c8ad36b3e5/src/widgets/scrollbar.rs#L131

[codecov]

Codecov Report

Attention: Patch coverage is 55.26316% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 91.6%. Comparing base (c4ce7e8) to head (be41770).

❗ Current head be41770 differs from pull request most recent head 6326d6e. Consider uploading reports for the commit 6326d6e to get more accurate results

Files	Patch %	Lines
src/widgets/scrollbar.rs	55.2%	17 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #965     +/-   ##
=======================================
- Coverage   92.0%   91.6%   -0.4%     
=======================================
  Files         61      61             
  Lines      15933   15764    -169     
=======================================
- Hits       14660   14445    -215     
- Misses      1273    1319     +46     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[EdJoPaTo]

@joshka interested in providing thoughts before adding tests?

[DeflateAwning]

I support the development and encorporation of this feature!

[DeflateAwning]

What's the status of this feature? Is there any way I can help out. Would love to get it merged

[EdJoPaTo]

There are no test cases for this yet as I would prefer to discuss this first before taking even more effort into this.

Basically… is this the way we want to go forward?

[joshka]

Apologies for not getting to this one sooner for review.

Yes this looks good to progress on. Do you think it would be worth splitting the hide feature and overscroll functionality into separate PRs - the hide functionality seems pretty simple and easy to merge without much discussion, but the overscroll seems more complex and has a few things I'd like to consider.

For both, I'd suggest introducing enums that clarify the behavior rather than bools.

For overscroll, I think it's worth considering the behavior of preventing overscrolling, and not just the display. It might be nice to pull these out into a set of "when blah, then blah" statements that more meaningfully describes the behavior These would end up in the docs for the scrollbar and in the tests that help describe how it works.

[DeflateAwning]

Hmm this doesn't seem to work for me when I try to use this in the project I'm working on (in the test-overscroll-prevention branch's latest commit). It still allows me to scroll past the limits vertically.

I think this is tied to the lack of "communication" between the Scrollbar (and its viewport size) to the ScrollbarState.

[EdJoPaTo]

Hmm this doesn't seem to work for me […]. It still allows me to scroll past the limits vertically.

The Scrollbar does not change the scrolling behaviour in any way. It only represents the scroll visually by displaying a scrollbar.

The overscroll behaviour is currently represented on the scrollbar which is what this PR allows to configure. It simply allows the scrollbar to show the end is reached. Whatever the widget itself does.

[DeflateAwning]

Hmm I'm not sure I follow entirely, but that might be okay.

Can you give an example of a before-and-after of what this PR actually does?

[joshka]

The overscroll behaviour is currently represented on the scrollbar which is what this PR allows to configure. It simply allows the scrollbar to show the end is reached. Whatever the widget itself does.

Oh yeah, your right. My brain is still thinking that the scroll position is actually controlling the scroll of the data of whatever element is involved and not just the display of the bar.

[EdJoPaTo]

Hmm I'm not sure I follow entirely

Every widget by itself defines how scrolling works and widgets::Scrollbar defines how the scrollbar looks.

This in itself describes the scroll mess-up of ratatui currently well: There is no common ground. Widgets define the scrolling behavior differently (mqttui mainly uses 3 different widgets and has workarounds to get them behave somewhat similarly) and the widgets::Scrollbar tries to visualize what different widgets define differently.

Do you think it would be worth splitting the hide feature and overscroll functionality into separate PRs - the hide functionality seems pretty simple and easy to merge without much discussion, but the overscroll seems more complex and has a few things I'd like to consider.

Hiding the scrollbar when there is nothing to scroll is very pointless with the current overscroll behavior as its basic idea is to be always able to scroll when there is content.

https://github.com/ratatui-org/ratatui/blob/6326d6e516499a5df60be25c2a54c31aca67e296/src/widgets/scrollbar.rs#L649-L652

The only option not to scroll with the current overscroll behavior is when the content is 0 or 1 lines high. Then there is no last line which could be scrolled to. Yes, we could split this but the hide method on its own is pointless then.

For both, I'd suggest introducing enums that clarify the behavior rather than bools.

I like thinking about it, but I'll postpone this because of my other points.

For overscroll, I think it's worth considering the behavior of preventing overscrolling, and not just the display.

Here we are at widgets behaviour / scrollbar visualization point again (my first point). Widgets define how scrolling works and the scrollbar only tries to visualize it. The current widgets::Scrollbar should not manipulate other widgets. That would be horrible design.

Personal opinion: We need to have some shared idea of scrollable widgets (#174). Then we can ensure the widgets have a shared scrolling behavior. Only then we can visualize the scrolling behavior of the individual widgets consistently.

Therefore, I think this PR is currently pointless until #174 advances. I would close it.

[DeflateAwning]

After revisiting this with a better understanding of what this change does (and doesn't do), I'd love to see this merged!

@EdJoPaTo, would you like help to implement the minor changes suggested? Or are you still not feeling great about it?

[EdJoPaTo]

It would improve the visual situation a little, but it would not solve this mix up and confusion around it (probably even increase it). There is a workaround (described in my first post) which I use for my applications.

I think my suggested ScrollBehavior of #174 could actually be a good step forward for this. Guess it should be tested with a single widget. Once that's done it could be used in more places.

[DeflateAwning]

I think that improving the visual situation is an important first step.

I came up with what I assume is a similar work-around: not changing the scroll value when it reaches the limit (based on the viewport size, content length, etc.)