Omar Polo <op@omarpolo.com> wrote:
> On 2024/07/03 00:14:04 +1000, Mark Jamsek <mark@jamsek.com> wrote:
> > This is a fix of another diff view J keymap case that presents when
> > selecting the last visible commit in the horizontally split log view and
> > then toggling fullscreen. The view fails to load the next commit when
> > entering the J keymap due to too broad a predicate; the line selection
> > clamp should only be applied when scrolling from a focused, horizontally
> > split log view and not when scrolling from the diff view with J.
> > 
> > This can be seen by running the added regress without the fix applied,
> > although it might be best observed with a manual test by running tog in
> > an 80x24 xterm and entering:
> > 
> > 	S	# toggle horizontal split
> > 	4j	# jump to 5th commit
> > 	return	# open diff view in hsplit
> > 	F	# toggle fullscreen diff view
> > 	J	# diff view won't load the next commit
> > 
> > There's also an off-by-one in the calculation to determine how many
> > commits we need to populate the log, which is revealed if that part of
> > this diff (i.e., first hunk) is reverted. By entering 20J instead of J
> > in the above manual test case to continue the scroll beyond the last
> > loaded (i.e., 24th) commit, the 25th commit will not be loaded--it gets
> > stuck on the 24th. There's a test added to cover this too.
> > 
> > ok?
> 
> I can reproduce and confirm that the diff fixes it.  Have to admit that
> the off-by-one is a bit over my head, I'm not sure exactly why it works,
> the rest of the diff looks fine to me.
> 
> So, fwiw, ok op@ :)

Thanks, op :)

I committed it with your caveat.

This part of the cursor logic when dealing with a hidden hsplit log view
is a little counterintuitive imo. I had to reason about it for a bit
because I hadn't worked on this part of the code for a while. But in
this case, because we get here from a horizontal split and then toggle
fullscreen in the diff view and scroll the log from there, the last
displayed entry's index is one less than the selected entry's index, so
ncommits_needed (last_displayed_entry->idx + 1 + maxscroll) is N-1,
which is s->commits->ncommits. Therefore, we don't trigger the log
thread to queue more commits.

My first fix changed the predicate in log_move_cursor_down() that guards
decrementing eos to account for the horizontal border like so:

diff /home/mark/src/got
commit - c26e21eef791002a56bc9c626f5a94cba4165d09
path + /home/mark/src/got
blob - 38cb4a714d73a34d03df0760e883cf0196000dbc
file + tog/tog.c
--- tog/tog.c
+++ tog/tog.c
@@ -3975,7 +3975,7 @@ log_move_cursor_down(struct tog_view *view, int page)
 	    s->selected_entry->idx >= s->commits->ncommits - 1)
 		return NULL;
 
-	if (view_is_hsplit_top(view))
+	if (view->mode == TOG_VIEW_SPLIT_HRZN && !view_is_fullscreen(view))
 		--eos;  /* border consumes the last line */
 
 	if (!page) {

so the last displayed entry index and the selected entry index are
isomorphic when scrolling from the last commit on screen even when
scrolling a hsplit log view from a fullscreen diff view because we treat
the log view as though it's still in a split state^. But I wasn't certain
if it had further unwanted implications. Whereas I'm confident that
asking the log for more commits won't break anything else :)

The only other change the committed fix has is that when scrolling from
the last commit, the index now shows N/N+1 instead of N/N because we now
always have at least one commit prefetched. But IIRC tog used to show
N/N+1 and at some point this changed?

In any case, if you're more comfortable with the above fix (I think it's
easier to reason about), I'd be happy to test it more thorougly to make
sure it's not breaking anything and if all good, backout the +2 hunk and
replace it with this.

I'm going to grow tog regress this week, too, to try and improve our
confidence when making changes in these tricky bits.

^It's like Schrodinger's view: is the view split or not when covered by
its fullscreen child view? It's both untill we look at it (i.e., whether
the user makes it visible with tab, q, F, or S)!


-- 
Mark Jamsek <https://bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68