Add ScrollbarState.is_at_start(), ScrollbarState.is_at_end() methods

[DeflateAwning]

Problem

I want to have logic that's something like:

let was_at_bottom = sb_state.is_at_bottom()

// ... a bunch of code that changes the amount of content in the paragraph controlled by the scrollbar

if was_at_bottom {
    sb_state.last()
}

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:

pub fn is_at_bottom(&self) -> bool {
      self.position == self.content_length.saturating_sub(1)
  }

Alternatives

• Expose the private ScrollbarState struct attribute ScrollbarState.content_length
• Also, maybe expose the private struct attribute ScrollbarState.position

Additional context

[joshka]

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?

[DeflateAwning]

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.

[EdJoPaTo]

My approach of this: I have a Table with the entries and the last one should be seen. When the user selects something the TableState has something selected. When nothing is selected its None. That way I know when something is selected by the user or not. This should also work with other widgets like the List.

I use that to provide either the TableState of the user selection or create one which fakes this selection at the end.
I also adapt the highlight color to hide this fake selection.

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.

[joshka]

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.

This seems like a specific example of a problem where the suggested fix (adding the is_at_beginning/end methods) generalizes to solving a few similar related problems well. E.g. off the top of my head each of these problems would be helped by adding this:

• scrolling when adding lines at the bottom
• animating scrolling until we hit the last item
• triggering fetching new items in an infinite scroll

That is to say, I'm convinced that this is necessary in addition to the other scroll work.

[DeflateAwning]

Help me out; what exactly does the position represent? The comment says that it's the "current position", but what does that mean?

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 is_at_top() only needs to consider the position, but I think is_at_bottom() may need to account for all three struct member variables (content_length, position, and viewport_content_length).

Relevant Snippet

pub 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,
}

[DeflateAwning]

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?

[joshka]

Is it the index of the start of the visible region

yes

would it be appropriate to add `is_at_start/is_at_end tests to many of the existing test cases

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.

[EdJoPaTo]

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.

This seems like a specific example of a problem where the suggested fix (adding the is_at_beginning/end methods) generalizes to solving a few similar related problems well. E.g. off the top of my head each of these problems would be helped by adding this:

• scrolling when adding lines at the bottom
• animating scrolling until we hit the last item
• triggering fetching new items in an infinite scroll

That is to say, I'm convinced that this is necessary in addition to the other scroll work.

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.

[DeflateAwning]

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 Scrollbar and the ScrollbarState isn't quite right.

[joshka]

Something definitely feels a little off with how the scrollbar works though; I can get behind that. I feel like the line between the Scrollbar and the ScrollbarState isn't quite right.

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

[joshka]

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:

• when rendering the values to the screen using whatever widget (I'm assuming this is custom), track the indexes of the rendered values in your own state variable implementing StatefulWidget/Ref (or implement Widget/Ref on &mut YourWidget).
• Pull the information about the visible area from your widget state rather than the scrollbar state.
• Treat the scrollbar as write only rather than read/write.

I might be missing something obvious in that idea, feel free to help me understand it better if I am.