feat: message tab supports pageup and pagedown (#2623)

[xlai89]

This Pull Request fixes/closes #2623 .

It changes the following:

• extend DetailsComponent with events of PageUp and PageDown

I followed the checklist:

• I added unittests
• I ran make check without errors
• I tested the overall application
• I added an appropriate item to the changelog

Many thanks to #2496 as a clear and good reference.

[xlai89]

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 :)

[cruessler]

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?

[xlai89]

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.

[cruessler]

@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.

gitui/src/components/utils/scroll_vertical.rs

Line 82 in 60912c0

visual_height: usize,

[xlai89]

@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.

[cruessler]

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. :-)

[xlai89]

Oh sure, makes total sense :)

[cruessler]

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.

[xlai89]

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

[cruessler]

Just one minor question. Thanks a lot for your patience! :-)

[cruessler]

Would this also work?

Suggested change

	if old < self.visual_height.get() {
	0
	} else {
	old.saturating_sub(self.visual_height.get())
	.saturating_add(1)
	}
	old.saturating_add(1)
	.saturating_sub(self.visual_height.get())

[xlai89]

Absolutely! Thank you very much for your patience and great hints!