Skip to content

feat: Query if ScrollViewState is_at_bottom() #75

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Teufelchen1 wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat: Query if ScrollViewState is_at_bottom() #75

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7b0e319
feat: Query if ScrollViewState is_at_bottom()
Teufelchen1 May 19, 2025
7a62a60
review: remove accidential added change to for loop
Teufelchen1 May 20, 2025
805d875
review: Enhance doc strings
Teufelchen1 May 20, 2025
cbfbe82
review: Remove precondition test from unit test
Teufelchen1 May 20, 2025
2fe46b9
review: Adjust line wrapping
Teufelchen1 May 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 61 additions & 3 deletions tui-scrollview/src/scroll_view.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,6 @@ impl ScrollView {
///
/// This should not be confused with the `render` method, which renders the visible area of the
/// ScrollView into the main buffer.

pub fn render_stateful_widget<W: StatefulWidget>(
&mut self,
widget: W,
Expand Down Expand Up @@ -451,6 +450,65 @@ mod tests {
)
}

#[rstest]
fn move_to_bottom(scroll_view: ScrollView) {
let mut buf = Buffer::empty(Rect::new(0, 0, 6, 6));
let mut state = ScrollViewState::default();
scroll_view.clone().render(buf.area, &mut buf, &mut state);

// The vertical view size is five which means the page size is five.
// We have not scrolled yet, view is at the top and not the at the bottom.
// => We see the top five rows
assert_eq!(state.offset.y, 0);
assert!(!state.is_at_bottom());
assert_eq!(
buf,
Buffer::with_lines(vec![
"ABCDE▲",
"KLMNO█",
"UVWXY█",
"EFGHI║",
"OPQRS▼",
"◄██═► ",
])
);
Copy link
Author

@Teufelchen1 Teufelchen1 May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@joshka joshka May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@Teufelchen1 Teufelchen1 May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Since the content height is ten,
assert_eq!(state.size.unwrap().height, 10);
// if we scroll down one page (five rows),
state.scroll_down();
state.scroll_down();
state.scroll_down();
state.scroll_down();
state.scroll_down();

// we reach the bottom,
assert!(state.is_at_bottom());
assert_eq!(state.offset.y, 5);

// and we see the last five rows of the content.
scroll_view.render(buf.area, &mut buf, &mut state);
assert_eq!(
buf,
Buffer::with_lines(vec![
"YZABC▲",
"IJKLM║",
"STUVW█",
"CDEFG█",
"MNOPQ▼",
"◄██═► ",
])
);

// We could also jump directly to the bottom
state.scroll_to_bottom();
assert!(state.is_at_bottom());

// which sets the offset to the last row of content,
// ensuring to be at the bottom regardless of the page size.
assert_eq!(state.offset.y, state.size.unwrap().height - 1);
Copy link
Author

@Teufelchen1 Teufelchen1 May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

#[rstest]
fn hides_both_scrollbars(scroll_view: ScrollView) {
let mut buf = Buffer::empty(Rect::new(0, 0, 10, 10));
Expand Down Expand Up @@ -787,10 +845,10 @@ mod tests {
let mut buf = Buffer::empty(Rect::new(0, 0, 7, 5));
let mut state = ScrollViewState::default();
let mut list_state = ListState::default();
let items: Vec<String> = (1..=10).map(|i| format!("Item {}", i)).collect();
let items: Vec<String> = (1..10).map(|i| format!("Item {}", i)).collect();
let list = List::new(items);
scroll_view.render_stateful_widget(list, scroll_view.area(), &mut list_state);
scroll_view.render(buf.area, &mut buf, &mut state);
scroll_view.clone().render(buf.area, &mut buf, &mut state);
Comment on lines -793 to +843
Copy link
Member

@joshka joshka May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why this clone is needed here

Copy link
Author

@Teufelchen1 Teufelchen1 May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

assert_eq!(
buf,
Buffer::with_lines(vec![
Expand Down
10 changes: 10 additions & 0 deletions tui-scrollview/src/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,14 @@ impl ScrollViewState {
.map_or(u16::MAX, |size| size.height.saturating_sub(1));
self.offset.y = bottom;
}

/// True if the scroll view state is at the bottom of the buffer
/// Takes the pagesize into account
Copy link
Member

@joshka joshka May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@Teufelchen1 Teufelchen1 May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@joshka joshka May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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... ;)

Copy link
Author

@Teufelchen1 Teufelchen1 May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pub fn is_at_bottom(&self) -> bool {
let bottom = self
.size
.map_or(u16::MAX, |size| size.height.saturating_sub(1));
let page_size = self.page_size.map_or(0, |size| size.height);
self.offset.y + page_size >= bottom
}
}
Loading