-
Notifications
You must be signed in to change notification settings - Fork 310
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
Add ScrollbarState.is_at_start()
, ScrollbarState.is_at_end()
methods
#1018
Comments
A question regarding alternative approaches. Reading between the lines, you want the scrollbar not to scroll beyond the visible items (which it doesn't currently do). Would it be better to implement the ideas in #966 / #965 for this, or is this need mostly orthogonal to that request? Put another way, does it make sense to do both this and the other PRs? |
So overscrolling is part of the issues, and I think that that issue should be top priority to get fixed within this library (and not left to devs using this lib). The other (perhaps main) use case is that I'm building a serial terminal. New data comes in at the bottom and gets added as lines. When the user is scrolled to the bottom, it should automatically keep scrolling them down as new rows get added. However, if they begin scrolling up, I want to ensure that their scroll position is maintained so as not to make them lose their position in the incoming data. To do this, I need to know if their scroll position is at the bottom. |
My approach of this: I have a I use that to provide either the In the future this should be possible by the State::offset rather than the selection. That's part of #174. I don't need any additional scrollbar logic for this. I calculate it from the widget / state I'm using and this reflects exactly what I would like to have. |
This seems like a specific example of a problem where the suggested fix (adding the
That is to say, I'm convinced that this is necessary in addition to the other scroll work. |
ScrollbarState.is_at_bottom()
, ScrollbarState.is_at_top()
methodsScrollbarState.is_at_start()
, ScrollbarState.is_at_end()
methods
Help me out; what exactly does the Is it the index of the start of the visible region, within the viewport? I think that makes the most sense, but I can't totally tell. I only ask, because I think that perhaps Relevant Snippetpub struct ScrollbarState {
/// The total length of the scrollable content.
content_length: usize,
/// The current position within the scrollable content.
position: usize,
/// The length of content in current viewport.
///
/// FIXME: this should be `Option<usize>`, but it will break serialization to change it.
viewport_content_length: usize,
} |
Also, would it be appropriate to add `is_at_start/is_at_end tests to many of the existing test cases (i.e., taking advantage of the states already constructed throughout)? Or should they go in entirely new test cases? |
yes
No. Most of the tests are about rendering and how it affects the display. For these new methods on State, it's important only to test that the calculation logic is correct. Unit tests should only be made more complex when they are testing things which change due to the tested code, and they should generally only check immediate first order effects (e.g. position) rather than second order effects like this. |
This feels backwards to me. We have data. We create a widget to display this data. We create a scrollbar to visually represent the WidgetState. Then we ask the visual representation to decide how to manipulate the State. Then we create another visual representation of the State to present the changes to the user. Maybe we need a (Vertical/Horizontal)Scrollable trait for State implementations. These could have such query methods. But querying the visual representation feels wrong and a mix of responsibilities that shouldn’t be mixed. Which also will end up running code that isn’t needed for decision making introducing performance impacts by design. Yes I thing this is a topic that’s relevant and that should be generalised over multiple widgets. But I don’t think the scrollbar widget is the right place for this. |
The reason "querying the widget" is required is because it knows how big it is, and it knows how much is visible to the user. We can't tell it how big it is. Something definitely feels a little off with how the scrollbar works though; I can get behind that. I feel like the line between the |
Yeah. It was added sort of as a partial solution to scrolling rather than implementing everything that's necessary to support scrolling properly in #174 |
To add a little more on this, I proposed that we use ScrollbarState instead of just passing the values directly as properties to the Scrollbar. On reflection, I think this was a mistake. I think the scrollbar should have been just given the exact values to render, with no reason to need to update state etc. I think @EdJoPaTo has a good point above that what's actually visible is a higher level concept than the scrollbar and should be handled outside of it for now to avoid putting too much non-reversible baggage on something which is not the right place. For this reason, I'd like to hold off on implementing this at least for now. Does that sound reasonable. As a workaround, I think I'd implement your requirement as follows:
I might be missing something obvious in that idea, feel free to help me understand it better if I am. |
Problem
I want to have logic that's something like:
The idea is that I want the scrollbar section to always be at its bottom, but to also be able to be scrolled up without immediately jumping back to the bottom.
Solution
I think it might be as simple as adding a new function in
src/widgets/scrollbar.rs
with the following content:Alternatives
ScrollbarState
struct attributeScrollbarState.content_length
ScrollbarState.position
Additional context
The text was updated successfully, but these errors were encountered: