feat(list): Add Scroll Padding to Lists

[CameronBarnes]

This PR introduces scroll padding, which allows the api user to request that a certain number of ListItems be kept visible above and below the currently selected item while scrolling.

It does this by adjusting the 'index_to_display' value in the List::get_item_bounds function. Reducing the provided padding value if the padding would push the selected item out of the renderable area or if there's not enough room for the requested amount of padding, and correctly handling ListItems of inconsistent sizes.

I have stored the 'padding' value in the ListState object. Using serde(default) on the parameter to avoid creating a breaking change in relation to user deserialization of existing data.
I believe the ListState object is the correct place to put this, as if we're not rendering a stateful list then we dont need this value.

'Padding' almost certainly isnt the correct name for this value, I think 'scroll_padding' is a better option and communicates the behavior to users better. But I'm open to suggestions. I havent yet changed the code to reflect the clearer 'scroll_padding' name.

Using this new feature would be the same as existing code for manually setting the offset, so any of the following:

*state.padding_mut() = 1;
state = state.with_padding(1);
state = ListState::default().with_padding(1);

By default the 'padding' value is 0 and would have no impact on existing users without any action on their end.

I've written unit tests that cover the original unmodified behavior, the new intended behavior, and several troublesome edge cases that came up while I was developing this but was able to resolve.
Most of the code in this PR is either new test cases, or specifically for handling those edge cases.

Demo of the new behavior(visible on the left side of the gif):

Comparison of modified and unmodified behavior is shown in the issue this PR was created for.
This PR resolves #955

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.8%. Comparing base (943c043) to head (e5dc465).

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #958     +/-   ##
=======================================
+ Coverage   91.7%   91.8%   +0.1%     
=======================================
  Files         61      61             
  Lines      15742   15897    +155     
=======================================
+ Hits       14440   14606    +166     
+ Misses      1302    1291     -11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Valentin271]

Hey thanks for this this is a great addition.

I need more time to review the render because of its complexity but I think it could/should be delegated to a specific sub-method.

Also I think the padding field should be in List and not ListState. This is directly related to the render rather than state.

[CameronBarnes]

I need more time to review the render because of its complexity but I think it could/should be delegated to a specific sub-method.

I'm not sure I understand what you mean by "delegated to a specific sub-method." Currently the logic exists entirely in the get_items_bounds function, as it needs access to all the same data and would nessecarily need to be updated if that function was updated, like in the case of adding list item padding where there's empty spaces between list items.

Also I think the padding field should be in List and not ListState. This is directly related to the render rather than state.

My thought process for having it stored in ListState rather than the List was that the value wasnt needed unless an item was selected. But I suppose selection highlighting works similarly.
I'd be happy to make that change if you'd like.

Additionally, I'd like some input on 'scroll_padding' as a prefered alternate name. I havent yet changed it in code as I wanted to get input from others so that it only had to be changed once.

Thanks for your feedback.

[Valentin271]

I'm not sure I understand what you mean by "delegated to a specific sub-method." Currently the logic exists entirely in the get_items_bounds function, as it needs access to all the same data and would nessecarily need to be updated if that function was updated, like in the case of adding list item padding where there's empty spaces between list items.

I didn't pay enough attention to the modification and didn't see it was updating outside variables. I'm just trying to reduce the length and complexity of the method which is now 128 lines long.

My thought process for having it stored in ListState rather than the List was that the value wasnt needed unless an item was selected. But I suppose selection highlighting works similarly.

That's a good point. But as highlighting works like that and others widget too, I prefer having it it List. It's more a visual parameter.

I'd like some input on 'scroll_padding'

Right I forgot about that, thanks for the reminder. I do think it is a better name for this feature.

[joshka]

I'm just trying to reduce the length and complexity of the method which is now 128 lines long.

+1 to this. Making long methods more complex is definitely something to avoid.
It may be necessary to apply some simplifying refactoring either before this change or possibly after.

[CameronBarnes]

Great news, it looks like two of the three checks for dealing with scroll_padding edge cases were unnessecary as the third check covers the cases of the previous two. This has been a consistent pattern in dealing with some of the tricky edge cases for this feature.
This has notably reduced the amount of code and complexity added.

Additionally, the added code no longer has any side effects. So it could be refactored out to a separate method, though personally I think doing so might make the code harder to understand for someone viewing this later, rather than easier. But that's just my opinion. I'll move it to a separate function if requested.

[Valentin271]

This looks good to me. No need for a separate function now that it's been simplified.

[joshka]

This looks good to me. No need for a separate function now that it's been simplified.

I prefer smaller methods generally as they tend to be easier to understand and modify when maintaining things later, so I'd like to see this new functionality extracted from the function if it doesn't lead to a silly parameter list. My rationale is that when methods have large blocks of code in them, those blocks have access and can modify any mutable variable of the function. By extracting this logic, it's easier to reason about the functionality, as the input is the parameters and the output is the result.

[CameronBarnes]

I've moved the code to its own function. Though in this case I think this just hides implementation details rather than actually making it easier to understand the bounds checking code.

[joshka]

Sounds good. I'll slap a few opinionated cleanups in another PR