fix: make ctrl+d/u scroll half-page instead of full page#25
fix: make ctrl+d/u scroll half-page instead of full page#25umputun merged 7 commits intoumputun:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates key navigation so Ctrl+d / Ctrl+u perform half-page movement while keeping PgDn / PgUp as full-page paging, aligning behavior with common terminal navigators (vim/less/tmux).
Changes:
- Split
Ctrl+d/uhandling fromPgDn/PgUpin tree, TOC, and diff navigation. - Added half-page diff cursor movement helpers and expanded unit tests for new behavior.
- Updated user-facing docs to describe half-page scrolling for
Ctrl+d/u.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
ui/model.go |
Separates Ctrl+d/u from PgDn/PgUp across panes; routes to half-page movement. |
ui/diffview.go |
Introduces half-page diff cursor movement functions. |
ui/model_test.go |
Updates existing paging tests and adds new tests for half-page movement. |
README.md |
Documents Ctrl+d/u as half-page scroll. |
.claude-plugin/skills/revdiff/references/usage.md |
Mirrors README keybinding doc update. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
umputun
left a comment
There was a problem hiding this comment.
nice and clean change, thx. one real issue though.
moveDiffCursorHalfPageDown/Up use syncViewportToCursor() which only adjusts viewport when cursor leaves the visible area. with a typical viewport of ~36 lines, first ctrl+d moves cursor to line 18 but viewport stays at offset 0 since line 18 is still visible. in vim/less ctrl+d scrolls both viewport and cursor by half a page simultaneously.
the fix should be straightforward - use explicit YOffset adjustment like the existing full-page functions do, but with halfPage offset instead of viewport.Height. smth like:
// half-page down
m.viewport.SetYOffset(min(m.viewport.YOffset+halfPage, maxOffset))// half-page up
m.viewport.SetYOffset(max(0, m.viewport.YOffset-halfPage))also worth adding viewport.YOffset assertions to the half-page tests - currently they only check cursor position, which is why the viewport issue wasn't caught.
btw, Copilot flagged the same thing in inline comments 1, 2 and 5 - those are valid. the other two (about max(1, ...) in tests) are noise since test viewport is always large enough.
ctrl+d/u now scroll half a page in diff, tree, and TOC panes, matching vim convention. PgDn/PgUp remain full-page scroll.
- Update README.md and usage.md: ctrl+d/u described as "Half-page scroll" instead of "Page scroll" - Add changelog entry for half-page scroll fix - Add missing ctrl+u test case in TestModel_TreeCtrlDUMovesHalfPage
Replace syncViewportToCursor() with explicit YOffset adjustment so viewport scrolls by half page simultaneously with cursor, matching vim/less ctrl+d/u behavior. Add viewport.YOffset assertions to half-page tests.
00b02a9 to
c72270b
Compare
|
Rebased on latest master. Fixed the viewport scrolling issue — good catch. syncViewportToCursor() only adjusts when cursor leaves the visible area, so the first ctrl+d would move the cursor but the viewport stayed put. Now both moveDiffCursorHalfPageDown/Up use explicit SetYOffset adjustment (like the full-page functions do), so cursor and viewport scroll simultaneously by half a page. Added viewport.YOffset assertions to both ctrl+d and ctrl+u tests to prevent regressions. |
umputun
left a comment
There was a problem hiding this comment.
lgtm, viewport scrolling fix looks correct. thx
Summary
Splits
ctrl+d/ctrl+ufromPgDn/PgUpso they scroll half a page, matching vim/less/tmux convention.Closes #24
What's changed
ctrl+d/ctrl+unow scrolls half the viewport height in all three panes (diff, tree, TOC)PgDn/PgUpunchanged — still scrolls full pageTest plan