fix: content scrolling so it stops at bottom of viewable area instead… #29
Conversation
There was a problem hiding this comment.
Can you please add unit tests (similar to the ones below) and doc comments that talk about how this works (and which will show up in the docs.rs page).
If you write the tests first, this should be able to confirm that the simplification is the same as what you had (I'm not 100% sure it is).
src/scroll_view.rs
Outdated
| let max_y_offset = self.size.height - area.height; | ||
| let next_y_offset = y.min(self.buf.area.height.saturating_sub(1)); | ||
| let y_offset = if next_y_offset > max_y_offset { | ||
| max_y_offset | ||
| } else { | ||
| next_y_offset | ||
| }; | ||
|
|
||
| x = x.min(self.buf.area.width.saturating_sub(1)); | ||
| y = y.min(self.buf.area.height.saturating_sub(1)); | ||
| y = y.min(y_offset); |
There was a problem hiding this comment.
I suspect this can be simplified to:
let min_height = area.height.min(1); // show at least one row
let max_offset = self.size.height.saturating_sub(min_height);
y = y.min(max_offset);
|
#30 changes the implementation a bunch, so I'm going to redo this and credit you for the fix. |
|
Ok sounds good! I was trying to add a test to try the simplification but I couldn’t figure out how to get the scroll thumb to size correctly, it kept drawing two cells tall instead of one. |
|
I merged the latest changes to my fork and I added a I also added a fixture that prints the row numbers to make it easier to verify that the scroll position is at the right location. I simplified the logic around the y position that I updated and I cleaned up some messages that Clippy was complaining about. Let me know what you think. |
|
Hey there, it might be some time before I respond to this, but I will get back to it as soon as I can. |
|
Any updates? |
There was a problem hiding this comment.
Hey, I just took a look - thanks for reminding me to come back to this.
I merged main into your branch and added some more tests. This now uses the following more convenient test data (which makes it easier to see when there's problems with both rows and columns):
tui-scrollview/src/scroll_view.rs
Lines 229 to 238 in 533150d
It works fine when you don't have a horizontal scrollbar:
tui-scrollview/src/scroll_view.rs
Lines 438 to 442 in 533150d
But fails when you do have a horizontal scrollbar (the last row is never shown):
tui-scrollview/src/scroll_view.rs
Lines 395 to 400 in 533150d
This also should handle scrolling to the right as well.
tui-scrollview/src/scroll_view.rs
Lines 341 to 346 in 533150d
Note the failing tests likely have the wrong scrollbar positions, but the visible data should likely be correct. Hopefully the tests should guide you towards the right solution here.
| x = x.min(self.buf.area.width.saturating_sub(1)); | ||
| y = y.min(self.buf.area.height.saturating_sub(1)); | ||
| y = y.min(min(next_y_offset, max_y_offset)); |
There was a problem hiding this comment.
This is a bit convoluted y.min(min(a calculation with min from above)). I'd suggest finding a way to make this simple. Don't intersperse x and y here, but instead do x and then y, and perhaps find a way to either simplify the logic into a single expression, or a set of chained assignments (y = ...; y = ...; y = ...;)
| // ensure that we don't scroll past the end of the buffer in either direction | ||
| // also, ensure that the scrolling stops with the end of the content at the | ||
| // bottom of the visible area | ||
| let max_y_offset = self.buf.area.height.saturating_sub(area.height); |
There was a problem hiding this comment.
I changed this to a saturating sub as one of the tests was panicking due to underflow here.
… of at the top.
Before fix the content would scroll on the last page to the top of the viewable area, leaving a large blank space on the last page. This fixes the scrolling so the bottom of the last page stops at the bottom of the viewable area. Adjustment is made to the scrollbar thumb so it stops at the bottom of the scroll track.