The approach in this PR is different than in #2496 . Not sure if this is ok or should the method move_top of VerticalScroll be modified. I'm new in rust dev, new in this project, would appreciate any help and feedback :)

Sorry it took so long for this feedback! I just had a look at your PR, and one thing I noticed was that the logic related to changing scroll_top is now in two places. First, there’s DetailsComponent::move_scroll_top and then there’s VerticalScroll::move_top. Did you consider adding the new logic, the one related to PageUp and PageDown, to VerticalScroll::move_top? To me, that would seem like the most logical place for it to live. Or did you try and encounter any difficulties?

Hi @cruessler ! Thanks for the feedback. I modified VerticalScroll::move_top as you suggested. Because PageUp and PageDown need a page height, which is not necessary for other ScrollTypes, I hope the approach with Option is okay.

@xlai89 Thanks for the update! There’s another suggestion I’d like to make. VerticalScroll::update already gets visual_height as a parameter. It also stores top and max_top on self. What do you think about following that pattern and storing visual_height/page_height as well? That way, you could remove the parameter page_height that you had to add to move_top and the component would stay consistent.

@cruessler I stored visual_height into VerticalScroll, which made VerticalScroll::move_top much neater. But I did not modify other methods e.g. VerticalScroll::update, which still takes in a visual_height parameter, because that could cause many changes in other components and honestly, I am a bit confused with the many ways of scrolling (move_top, update,update_no_selection; VerticalScroll vs Selection). But please let me know if any further modification is needed.

We’re almost there, I think! There’s a few minor suggestions I’d still like to make, though. Can you move this line self.visual_height.set(visual_height) into VerticalScroll::update and remove VerticalScroll::set_visual_height and VerticalScroll::get_visual_height? Apart from that, you’re right that this section of the code is probably not the easiest to follow. :-)

Oh sure, makes total sense :)

Perfect! I’ve now started testing your changes on my machine, and I observed that I can’t seem to fully scroll to the top when using PageUp. Instead, there’s always one line missing that I can only scroll by using Up. Scrolling to the last line of the message using PageDown works, though.

You're right. I should have noticed that myself, sorry. After I understand saturated_sub correctly, it should be fixed now :)