On 22-08-11 07:02PM, Mikhail wrote:
> On Wed, Aug 10, 2022 at 12:49:27AM +1000, Mark Jamsek wrote:
> > We incorrectly reassign when we should be incrementing the selected
> > index. As a result, if the currently selected commit is less than
> > nlines/2 from the last commit and we use 'd' to scroll down half
> > a page, we jump up!
> > 
> > repro:
> >   $ tog  # 80x24 in got.git
> >   G
> >   5k
> >   d
> >   *selection moves up 6 lines instead of moving to the last commit!*
> > 
> > Relatedly, I've simplified the page down logic a bit so that if the last
> > commit is already on screen when in a hsplit, we move the selection
> > cursor down rather than scroll. This required accounting for the border
> > when drawing commits in a horizontal split, which was previously missed,
> > to ensure last_displayed_entry->idx is correct.
> > 
> > ok?
> 
> I can't ok, but when I was reviewing the code under question I noticed
> this thing in log_move_cursor_down() function:
> 
> 3186         } else if (s->thread_args.load_all) {
> 3187                 if (s->last_displayed_entry->idx == s->commits.ncommits - 1)
> 3188                         s->selected += MIN(s->last_displayed_entry->idx -
> 3189                             s->selected_entry->idx, page + 1);
> 3190                 else
> 3191                         err = log_scroll_down(view, MIN(page,
> 3192                             s->commits.ncommits - s->selected_entry->idx - 1));
> 3193                 s->selected = MIN(view->nlines - 2, s->commits.ncommits - 1);
> 
> On line 3188 we set s->selected, but on 3193 we overwrite it, maybe this
> can be simplified too?

3193 should arguably be in the else scope as it's redundant for the if
path--it's only needed in the else case. It's unrelated to this diff
though so should be made in a separate commit. If you want to submit
that patch that would be good. Otherwise I should have some time
tomorrow to hop in the tog tree. Thanks, Mikhail!

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