-
Notifications
You must be signed in to change notification settings - Fork 258
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.
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
feat: add scrollbar widget #228
Conversation
Codecov Report
@@ Coverage Diff @@
## main #228 +/- ##
==========================================
+ Coverage 81.92% 82.38% +0.45%
==========================================
Files 34 35 +1
Lines 6627 7197 +570
==========================================
+ Hits 5429 5929 +500
- Misses 1198 1268 +70
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done a bit of experimenting with scrolling logic too, but mainly playing with the state part not the widget part. I just slapped a WIP PR for you to take a look at (#229). Perhaps there's some ideas that you could incorporate into this there?
My main concern here is over having a single position rather than a range. It's probably easy to move to a range if we use an enum to represent the position enum ScrollPosition { Ratio(f64), RatioRange(f64, f64), Range(u16, u16), etc. }
Thanks for working on this btw - looking forward to seeing it be part of ratatui. |
Thanks for the detailed feedback!!! Let me address some of the comments and get back to you when it is ready for another round for review. I'm also planning to update this scrollbar widget so that it works horizontally as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice overall
I'd still prefer to have the position part out of the scroll bar - based on the stuff in the other PR / design Issue, but this is really good. I'll take a full review sometime soon.
You can ignore this comment, thanks to @joshka this has been resolved. Original comment in the details of this message. I'm getting a failing test on CI:
How is this test passing for the |
For the build failure check the cargo toml to see how to configure examples. |
I think this is ready for a final review now! |
Quick glance looks good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of comments still
@joshka I've addressed a number of your comments and asked for some clarification on a new questions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Picking back up the conversation about the cross. The code doesn't make it clear how the cross (perhaps you mean corner?) is relevant to the the algorithm for rendering the scroll_bar.
If I'm thinking through this from basics, 200 lines to render a scrollbar seems excessive.
It's possible that there are some assumptions or non obvious edge cases that you have factored in here, but they are difficult to see. I'm not sure it would be easy to fix a problem in this code, or to work out how to add a new feature.
I think perhaps this works too much in absolute coordinates and worries about the vertical / horizontal distinction throughout rather than using a much simpler relative approach.
An algorithm that treats the rendering as rendering a line from 0 to bar length would look like:
// this is psuedo rust, please forgive any errors
let length = length_of_bar_without_arrows(); // e.g. either area.width or area.height -1 or -2
let (start_track, end_track) = relative_positions_of_track_without_arrows(); // e.g. (0, length - 1) or (1, length -2);
let (start_thumb, end_thumb) = relative_positions_of_thumb(); // e.g. (3, 6)
// this can be a method... absolute_positions()
let (left, top, right, bottom) =
(area.x, area.y, area.x + area.width - 1, area.y + area.height - 1);
let (start_x, start_y, end_x, end_y) = match position {
Position::HorizontalTop => (left, top, right, top),
Position::HorizontalBottom => (left, bottom, right, bottom),
Position::VerticalLeft => (left, top, left, bottom),
Position::VerticalRight => right, top, right, bottom),
};
index = 0;
for x in (start_x..end_x) { // either this line
for y in (start_y..end_y) { // or this line will just be a single iteration
if index < start_track { draw_start_arrow(x, y); }
else if index < start_thumb { draw_track(x, y); }
else if index < end_thumb { draw_thumb(x, y); }
else if index < end_track { draw_track(x, y); }
else { draw_end_arrow(x, y); }
index++;
}
}
If I have indeed missed some complexity that is non obvious here, I apologize.
I guess the cross is confusing naming. I made two changes based on your most recent comment:
Here's these concepts ascii visualized:
If it is still not clear, I can refactor the render function to exactly the way you wrote it, i.e. with two for loops instead of one, and with more abstractions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome stuff guys, looks great !!
Only a couple nit picks, not blocking per say. Looking forward to seeing this in the next release ! 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
To make this ready to merge, I'm going to push a rebase squash and cleanup the commit message |
Represents a scrollbar widget that renders a track, thumb and arrows either horizontally or vertically. State is kept in ScrollbarState, and passed as a parameter to the render function.
Represents a scrollbar widget that renders a track, thumb and arrows either horizontally or vertically. State is kept in ScrollbarState, and passed as a parameter to the render function.
I've added a simple scrollbar widget.
It can be used to add a scrollbar to any
Rect
.It can also be customized with different styles and symbols for, the bar, the "scrollwheel" and arrows.
This addresses #173
A larger discussion is to be had about scrollable widgets in general: #174
I'm open to feedback on improving this. I've needed a scrollbar for a couple of projects now and figured I'd clean up the code and share it as a
Widget
here.Docstrings were generated using ChatGPT.