From: Mark Jamsek Subject: Re: tog: fix log view 'd' keymap when less than nlines/2 from the last commit To: Mikhail Cc: gameoftrees@openbsd.org Date: Fri, 12 Aug 2022 02:30:26 +1000 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 GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68