On Tue, Nov 24, 2020 at 11:13:34PM +0100, Christian Weisgerber wrote:
> In tog's tree view, the page up/down handling is different from the
> log and blame views as well as buggy.
> 
> There are two aspects to this:
> (1) how many lines to move up or down
> (2) which entry to select afterwards
> 
> Each of those views has a fixed header and a certain number of
> visible lines below.
> 
> (1)
> log and blame always move the full number of visible lines up and
> down (assuming there are sufficient entries).
> 
> For page down, tree moves too many lines.  It uses the full window
> height.  It needs to subtract the height of the fixed header.
> 
> For page up, tree moves one line too little.
> 
> (2)
> If the nth visible entry is selected, log and blame will also select
> the nth visible entry after scrolling up/down (assuming there are
> sufficient entries).
> 
> For page down, tree behaves like log and blame.
> 
> For page up, tree always moves the selection to the first visible
> entry.
> 
> 
> There may be an additional logic bug in the tree view when going a
> page up from the bottom; or maybe it's just the combination of the
> two above, I'm not sure.
> 
> The ref view scroll logic was copied from the tree view, so it
> shares these problems, along with the problem I already reported
> that feels like an off-by-one somewhere.

Thanks for the detailed report!

This should fix page-up/page-down behaviour in 'tog tree'.
I will look into fixing the ref view next.

diff c42c9805fb36a73a8de6f81318a07617264ca7e0 /home/stsp/src/got
blob - 271c8a99135a8ba952cac81f6a4ab6c07a67e10c
file + tog/tog.c
--- tog/tog.c
+++ tog/tog.c
@@ -5020,7 +5020,7 @@ tree_scroll_up(struct tog_view *view,
 		*first_displayed_entry = NULL;
 }
 
-static int
+static void
 tree_scroll_down(struct got_tree_entry **first_displayed_entry, int maxscroll,
 	struct got_tree_entry *last_displayed_entry,
 	struct got_tree_object *tree)
@@ -5041,7 +5041,6 @@ tree_scroll_down(struct got_tree_entry **first_display
 			next = got_tree_entry_get_next(tree, next);
 		}
 	}
-	return n;
 }
 
 static const struct got_error *
@@ -5366,7 +5365,7 @@ input_tree_view(struct tog_view **new_view, struct tog
 	const struct got_error *err = NULL;
 	struct tog_tree_view_state *s = &view->state.tree;
 	struct tog_view *log_view;
-	int begin_x = 0, nscrolled;
+	int begin_x = 0;
 
 	switch (ch) {
 	case 'i':
@@ -5407,13 +5406,17 @@ input_tree_view(struct tog_view **new_view, struct tog
 		break;
 	case KEY_PPAGE:
 	case CTRL('b'):
-		tree_scroll_up(view, &s->first_displayed_entry,
-		    MAX(0, view->nlines - 4 - s->selected), s->tree,
-		    s->tree == s->root);
-		s->selected = 0;
 		if (got_object_tree_get_first_entry(s->tree) ==
-		    s->first_displayed_entry && s->tree != s->root)
-			s->first_displayed_entry = NULL;
+		    s->first_displayed_entry ||
+		    s->first_displayed_entry == NULL) {
+			if (s->tree == s->root ||
+			    s->first_displayed_entry == NULL)
+				s->selected = 0;
+			else if (s->tree != s->root)
+				s->first_displayed_entry = NULL;
+		}
+		tree_scroll_up(view, &s->first_displayed_entry,
+		    MAX(0, view->nlines - 3), s->tree, s->tree == s->root);
 		break;
 	case 'j':
 	case KEY_DOWN:
@@ -5437,18 +5440,8 @@ input_tree_view(struct tog_view **new_view, struct tog
 				s->selected = s->ndisplayed - 1;
 			break;
 		}
-		nscrolled = tree_scroll_down(&s->first_displayed_entry,
-		    view->nlines, s->last_displayed_entry, s->tree);
-		if (nscrolled < view->nlines) {
-			int ndisplayed = 0;
-			struct got_tree_entry *te;
-			te = s->first_displayed_entry;
-			do {
-				ndisplayed++;
-				te = got_tree_entry_get_next(s->tree, te);
-			} while (te);
-			s->selected = ndisplayed - 1;
-		}
+		tree_scroll_down(&s->first_displayed_entry, view->nlines - 3,
+		    s->last_displayed_entry, s->tree);
 		break;
 	case KEY_ENTER:
 	case '\r':