feat: Query if ScrollViewState is_at_bottom()

[Teufelchen1]

Hey 🐅,

this aims to fix #72

It is close to what was discussed but I made the additional decision to respect the page size.
I believe this is the right way to do it as not respecting the page size feels very odd to the user, example:

let state = ...;
// our content is 5 rows
assert_eq!(state.size.unwrap().height, 5);
// our page size is 3
assert_eq!(state.page_size.unwrap().height, 3);

// Scrolling down 5 times
state.scroll_down();
state.scroll_down();
state.scroll_down();
state.scroll_down();
state.scroll_down();

// Render it
scroll_view.render(...);

// Are we at the bottom?
assert!(state.is_at_bottom()); // FAILS!
// This is because rendering resets our offset to cater for the page size
assert_eq!(state.offset.unwrap().y, 2);

See scroll_view.rs:210-232 for why that is.

Since that felt wrong to me, I made the judgment call for is_at_bottom() to consider the page size. So technically it is now a are_we_scrolled_down_enough_so_it_behaves_and_looks_like_we_are_at_the_bottom() 🥸

I added a unit test.

Feedback? :)

[Teufelchen1]

Not strictly necessary as that is already covered by other tests but I liked the visual story telling of scrolling down.

[joshka]

I tend to prefer not putting these sort of pre-condition assertions that are covered elsewhere in tests. My rationale for this preference is that if some other change fails to render the starting condition, I'd prefer to see this fail at the point where it's testing the end condition, rather than in the pre-conditions. As a general rule, I've found that this makes it easier to debug failing tests while developing as well as to maintain them when things change which would affect pre-conditions but not post-conditions.

[Teufelchen1]

Yes, I am with you on that opinion. Dropped it.

[Teufelchen1]

More a kind of explanation & documentation rather than a strict unit test.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.36%. Comparing base (59a01f7) to head (cbfbe82).

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #75      +/-   ##
==========================================
+ Coverage   73.90%   74.36%   +0.46%     
==========================================
  Files          16       16              
  Lines        2667     2672       +5     
==========================================
+ Hits         1971     1987      +16     
+ Misses        696      685      -11     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[joshka]

I tend to prefer not putting these sort of pre-condition assertions that are covered elsewhere in tests. My rationale for this preference is that if some other change fails to render the starting condition, I'd prefer to see this fail at the point where it's testing the end condition, rather than in the pre-conditions. As a general rule, I've found that this makes it easier to debug failing tests while developing as well as to maintain them when things change which would affect pre-conditions but not post-conditions.

[joshka]

I'm not sure why this clone is needed here

[Teufelchen1]

It is actually reassuring to see that you also don't get it immediately (makes me feel less like an idiot). I was very confused when Rust hit me with this (when you omit that clone()):

error[E0382]: use of moved value: `scroll_view`
   --> tui-scrollview/src/scroll_view.rs:482:9
    |
454 |     fn move_to_bottom(scroll_view: ScrollView) {
    |                       ----------- move occurs because `scroll_view` has type `scroll_view::ScrollView`, which does not implement the `Copy` trait
...
461 |         scroll_view.render(buf.area, &mut buf, &mut state);
    |                     -------------------------------------- `scroll_view` moved due to this method call
...
482 |         scroll_view.render(buf.area, &mut buf, &mut state);
    |         ^^^^^^^^^^^ value used here after move
    |
note: `ratatui::prelude::StatefulWidget::render` takes ownership of the receiver `self`, which moves `scroll_view`
   --> /home/teufelchen/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/ratatui-0.29.0/src/widgets.rs:244:15
    |
244 |     fn render(self, area: Rect, buf: &mut Buffer, state: &mut Self::State);
    |               ^^^^
help: you can `clone` the value and consume it, but this might not be your desired behavior
    |
461 |         scroll_view.clone().render(buf.area, &mut buf, &mut state);
    |                    ++++++++

For more information about this error, try `rustc --explain E0382`.

I have to admit I am unable to explain it really. I figured it has something to do with #[fixture] or #[rstest] which I don't know about. Hence I just went with the compiler hint / the clone.

[joshka]

nit: Wrap at 100 chars. First line should be a summary sentence followed by a newline then any detailed info.

Here, I think it's worth adding some info on the caveats such as if the size / page size is not yet computed (by rendering), then it won't be taken into account. Likely this info should be also added to scroll_to_bottom docs too.

[Teufelchen1]

I can't wrap at 100 chars as the line is not longer than 100 chars. Or my editor lies to me 🫨

I added some info on the behavior of both functions. While doing so, I figured it would be best to just define a known behavior in that case. Changing the default for the buffer from u16:Max to 0 ensures that the function always returns true in that case. Also spotted a potential panic/undefined behavior, if the scroll position is at u16:Max and pagesize is >= 1, the computation would overflow the underlying u16. Saturated add seems fine to me, objections?

[joshka]

By this I meant "[Use the entire line and without any new lines until you] wrap at 100 chars". Sorry for the ambiguity there.

As it stands wrapping with a single line would not cause a new paragraph, so the second line is a continuation of the first. It's wrapped too soon.

Saturating is fine with me. Anyone that hits the problem can work out a better answer once they hit that. I wouldn't generally suggest trying to render that larger a buffer only to throw most of it away, so I'd be inclined to point out that 64K lines are enough for any... ;)

[Teufelchen1]

As it stands wrapping with a single line would not cause a new paragraph, so the second line is a continuation of the first. It's wrapped too soon.

Is it correct now? 2fe46b9

[Teufelchen1]

@joshka any actions required from my side? If not, just slam an emoji on my message and I will know you are just busy.