fix(list): Modify List and List example to support saving offsets.

[bblsh]

The current List example will unselect and reset the position of a list.

This PR will save the last selected item, and updates List to honor its offset, preventing the list from resetting when the user unselect()s a StatefulList.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2819eea) 92.4% compared to head (99ab834) 92.5%.

Additional details and impacted files

@@          Coverage Diff          @@
##            main    #667   +/-   ##
=====================================
  Coverage   92.4%   92.5%           
=====================================
  Files         61      61           
  Lines      16262   16290   +28     
=====================================
+ Hits       15042   15070   +28     
  Misses      1220    1220           

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

[joshka]

I personally think this PR is probably the right thing to do (i.e. don't reset offset when unselecting). It was mentioned recently in another PR / issue, but I don't know the history of why this current behavior is like it is. Would you mind digging into the tui-rs history to find some details on the existing behavior if possible? The questions I'd generally ask about odd behavior like this are (a non-exhaustive list):

1. Was it always like this?
2. When and why did it change?
3. Was there any discussion about it?
4. Are there any bug reports about the behavior?
5. What does it break when we change it (is this a load bearing bug)?

Assuming that we go ahead with this change, this PR needs a few extra things:

• BREAKING CHANGES info
• Docs for the new functionality
• Tests for the change

[orhun]

I like this change and I think it is something nice to have in the long run.

Seconding Josh here and I would like to add git blame would probably help figuring out the historical changes that led to the current behavior.

[bblsh]

I've looked through the discussions and issues in tui-rs and found nothing. I've also browsed the git history, which indicates this behavior was added when ListItems were introduced. The commit message for that change was logged as add new text primitives.

That said I'm still unsure if this was intentional behavior after reading through these bits.

However, this change to list.rsis not breaking and does not change existing behavior unless the user makes changes to their own StatefulList as shown in the PR. Take a look at the existing ListState's select()method:

pub fn select(&mut self, index: Option<usize>) {
    self.selected = index;
    if index.is_none() {
        self.offset = 0;
    }
}

Currently, the offset will always be 0 when unselect()ing.
Now let's look at the unselect() example method historically provided for a StatefulList:

fn unselect(&mut self) {
    self.state.select(None);
}

Finally, let's look at the pre-PR line in list.rs:191:

let selected = selected.unwrap_or(0).min(self.items.len() - 1);

With the pre-PR setup, whenever the user unselect()s a list, changing the offset will not work correctly since selected will be None. With this change, the user can still unselect() and keep behavior that currently exists, but also have the ability to retain position by resetting the offset when unselect()ing as shown below (in the PR):

    fn unselect(&mut self) {
        let offset = self.state.offset();
        self.last_selected = self.state.selected();
        self.state.select(None);
        *self.state.offset_mut() = offset;
    }

Because of this I do not believe this change is breaking or changes pre-existing behavior.

[joshka]

Thanks for doing the history dive on. Your findings make me much less concerned that it was intentional and more like something that just seemed right at the time.

I have to admit that I don't know this code deeply (you're more of an expert on it right now than anyon), but if you can change a line in a private method without breaking any unit tests, I think there's a problem somewhere - possibly in the existing unit test coverage of that line. Incidentally I can change it to 1 or 2 and all the tests still pass (but not 3).

Can you add a test that shows how this change impacts things (rendering / state)? It might be helpful to the next person to look at that code to also to add a comment that explains why that method exists. If you can discern the intent, writing it down helps validate that the change is correct.

[bblsh]

Two things:

• The List's StatefulWidget::render() method now has plenty of comments to explain what is happening.
• A unit test was added to display this functionality in a couple ways.

The gist of why that test was previously failing is because of the way rendering happens for the List.
The new unit test added here in the PR shows why your mentioned test was failing, and what should have been expected.

[bblsh]

Thank you very much for the detailed explanations and suggestions.

In this push I've removed some of the comments and made some changes to variable names as suggested.
Specifically, renaming offset, start and end has made the code much more understandable without overly verbose explanations. A function doc comment was also added for get_item_bounds().

I will address potential optimizations/readability with iterators and ranges in a future PR if that is okay with everyone.

[orhun]

Sounds good.

There seems to be a conflict with main, can you fix it?

[bblsh]

Done.

[joshka]

Can you write a small message that explains the change for the Changelog? (i.e. a one liner for the title of the change and a more full message for the body). If you're familiar with rebasing and squashing your changes, feel free to force push a single commit with the updated message, otherwise we can apply one for you using the github squash and merge. (Note - you may have to pull that commit above into your local branch)

[Valentin271]

Appart from the test, would be happy to merge that

[Valentin271]

Looks good. Do you want to move your initial test to an integration test?

[Valentin271]

Hey @bblsh, do you still want to work on this. We couldn't merge it at the time because of unsigned commit and kind of forgot to tell you 😓.

If you still want to work on this, it will need to be rebased on main.

If you could sign your commits that's be great (we need all commits to be signed so the easiest is to squash them).
Otherwise if you don't want to setup commit signing, ask us to merge this manually.

Don't hesitate to ask for help if needed!

EDIT: You can find help on signing in CONTRIBUTING.md or on Github doc

[github-actions]

Thank you for opening this pull request!

We require commits to be signed and it looks like this PR contains unsigned commits.

Get help in the CONTRIBUTING.md
or on Github doc.

[github-actions]

Thank you for opening this pull request!

We require commits to be signed and it looks like this PR contains unsigned commits.

Get help in the CONTRIBUTING.md
or on Github doc.

[bblsh]

Rebased and signed! Thanks for the reminder.

[Valentin271]

Thanks!