Keep cursor at top/bottom when navigating by page

[RaphaeleL]

When pressing '.' (next page) or ',' (previous page), the selection now stays at the bottom or top of the viewport respectively, instead of being centered which caused items to scroll off. (If the selection is not already on the last/first line of the view, '.'/',' moves it there without scrolling.)

This implements a special case for page navigation as suggested by the maintainer in issue #5017, keeping the cursor position consistent with user expectations for page-based navigation.

Fixes #5017

[stefanhaller]

Thanks for the PR, this looks mostly good.

There's one small thing that feels like a regression to me: when you have non-model items at the top of a view, they are no longer scrolled into view when pressing ,. One example is the --- Pending rebase todos --- line at the top of the commits window when rebasing. To reproduce, select the head commit, then press i to enter an interactive rebase, then press . to scroll down one page, and then , to scroll back up. Now the top-most "pick" todo is at the top of the window, and the "Pending rebase todos" line is not visible. Before this branch it was, and when pressing < instead of , it gets scrolled into view too.

I don't have an immediate great idea what to do about it, and I don't have time to wrap my head around what's conceptually wrong here, but if you can think of ways to improve this, that would be great.

[stefanhaller]

I don't think this is needed. The method is only called from keybinding handlers, so the view must be focused already, otherwise the binding wouldn't have been dispatched to it.

I realize that this is copied from handleLineChangeAux, but it doesn't seem to be needed there, either (for the same reason), so it might be nice to add a cleanup commit to remove it there, too.

[stefanhaller]

Removed in 2576258.

[stefanhaller]

Ok, this is trickier than I thought. Your additional commit improves it a bit, but I felt this is too complicated, and there must be an easier way to express what we want. I pushed a commit (e2fd771) that simplifies this by decoupling the selection logic from how much we scroll the view. In other words, if we press , and we move the selection by delta, but we end up moving it by one less than that because the item we would land on is a non-model item, then we still scroll the view by delta, which makes the non-model item visible.

In addition, this commit fixes jumping over-non-model items. One example is the ? menu, where there's a "--- Global ---" item in the middle of the view (and a blank line before it); if you start at the top and press ., we don't want to move the selection by delta, because that would take us two items down too far.

However, with that commit I still wasn't happy because when you have a non-model item at the top (e.g. the "--- Pending rebase todos ---" line in a rebase), then if you press . it scrolls the non-model item out of view, and if you press , again it doesn't make it visible. (You can, however, press , a second time to make it visible, but that still feels awkward).

I intuitively felt that in that situation I want to move the cursor to the bottom of the view without scrolling at all, and I wondered what's the logic to achieve that. I decided that in general, if the selection isn't already at the bottom of the view when you press . we simply move it there without scrolling (and likewise for ,). I pushed another commit (8b550c8) that does this. I like this because with this logic either the selection moves but the page does not, or the page moves but the selection does not. A possible downside could be that when the selection is in the middle of the view it means that it takes one more keypress before things start to scroll.

Sorry for the wall of text, all of this is hard to explain, but with this latest version the behavior feels good to me. Let me know what you think.

[RaphaeleL]

Thanks for taking the time to refine this! Your changes make a lot of sense. The behavior feels good to me! Thanks for improving this. Let me know if you need me to test anything specific or if there's anything else I should do.

[stefanhaller]

@RaphaeleL Thanks for reviewing my changes. I squashed the commits and am going to merge once CI is green.