fix(ui): keep cursor on same screen row when paging#125
Conversation
PgDown placed the cursor at the viewport top and PgUp at the bottom, so PgDown then PgUp did not return to the prior position. Share the existing half-page helper logic so full-page and half-page both scroll cursor and viewport by the same amount, preserving the cursor's relative on-screen row. Related to #124
Deploying revdiff with
|
| Latest commit: |
f2f43b4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9ef30b59.revdiff.pages.dev |
| Branch Preview URL: | https://fix-124-page-cursor-relative.revdiff.pages.dev |
There was a problem hiding this comment.
Pull request overview
This PR fixes diff-pane paging behavior so PgDown/PgUp keep the cursor on the same on-screen row (symmetric with Ctrl-D/Ctrl-U), addressing usability issue #124.
Changes:
- Refactors paging to use shared helpers (
moveDiffCursorDownBy/moveDiffCursorUpBy) for full-page and half-page cursor movement. - Updates PgDown/PgUp behavior to scroll cursor + viewport together rather than re-anchoring the cursor at the viewport edge.
- Adds a regression test covering PgDown→PgUp symmetry and the reported multi-page scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| app/ui/diffnav.go | Refactors paging logic into shared helpers and applies them to PgUp/PgDown and Ctrl-U/Ctrl-D. |
| app/ui/diffnav_test.go | Adds regression test ensuring PgDown/PgUp preserve relative cursor position and are symmetric. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // moveDiffCursorDownBy advances the cursor down by approximately n visual rows | ||
| // and scrolls the viewport by the same amount so the cursor's on-screen row stays stable. | ||
| // accounts for divider lines, wrap continuations, and annotation rows that occupy rendered space. | ||
| func (m *Model) moveDiffCursorDownBy(n int) { | ||
| startY := m.cursorViewportY() | ||
| for { | ||
| prev := m.nav.diffCursor | ||
| m.moveDiffCursorDown() | ||
| if m.nav.diffCursor == prev { | ||
| break | ||
| } | ||
| if m.cursorViewportY()-startY >= halfPage { | ||
| if m.cursorViewportY()-startY >= n { | ||
| break | ||
| } | ||
| } | ||
| maxOffset := max(0, m.layout.viewport.TotalLineCount()-m.layout.viewport.Height) | ||
| m.layout.viewport.SetYOffset(min(m.layout.viewport.YOffset+halfPage, maxOffset)) | ||
| m.layout.viewport.SetYOffset(min(m.layout.viewport.YOffset+n, maxOffset)) | ||
| m.layout.viewport.SetContent(m.renderDiff()) |
There was a problem hiding this comment.
moveDiffCursorDownBy/moveDiffCursorUpBy scroll the viewport by n, but the cursor can move by a different number of visual rows (e.g. in wrap mode or with multi-row annotations) because cursorViewportY() advances by hunkLineHeight() per step. When deltaY != n, the cursor’s on-screen row will drift, contradicting the function docstring/PR intent (keep relative cursor position stable) and can break PgDown/PgUp symmetry.
Consider computing the actual visual delta after the loop (delta := m.cursorViewportY() - startY / startY - m.cursorViewportY()) and adjusting YOffset by that delta (with clamping), rather than always using n.
| // moveDiffCursorPageDown moves the diff cursor down by one visual page. | ||
| // accounts for divider lines and annotation rows that occupy rendered space. | ||
| // scrolls the viewport so cursor appears near the top of the new page. | ||
| // keeps the cursor's relative screen position stable by scrolling both | ||
| // cursor and viewport by the same amount. | ||
| func (m *Model) moveDiffCursorPageDown() { | ||
| startY := m.cursorViewportY() | ||
| for { | ||
| prev := m.nav.diffCursor | ||
| m.moveDiffCursorDown() | ||
| if m.nav.diffCursor == prev { | ||
| break | ||
| } | ||
| if m.cursorViewportY()-startY >= m.layout.viewport.Height { | ||
| break | ||
| } | ||
| } | ||
| // place cursor at the top of the viewport for a true page-scroll feel | ||
| m.layout.viewport.SetYOffset(m.cursorViewportY()) | ||
| m.layout.viewport.SetContent(m.renderDiff()) | ||
| m.moveDiffCursorDownBy(m.layout.viewport.Height) | ||
| } | ||
|
|
||
| // moveDiffCursorPageUp moves the diff cursor up by one visual page. | ||
| // accounts for divider lines and annotation rows that occupy rendered space. | ||
| // scrolls the viewport so cursor appears near the bottom of the new page. | ||
| // keeps the cursor's relative screen position stable by scrolling both | ||
| // cursor and viewport by the same amount. | ||
| func (m *Model) moveDiffCursorPageUp() { | ||
| startY := m.cursorViewportY() | ||
| for { | ||
| prev := m.nav.diffCursor | ||
| m.moveDiffCursorUp() | ||
| if m.nav.diffCursor == prev { | ||
| break | ||
| } | ||
| if startY-m.cursorViewportY() >= m.layout.viewport.Height { | ||
| break | ||
| } | ||
| } | ||
| // place cursor at the bottom of the viewport for a true page-scroll feel | ||
| m.layout.viewport.SetYOffset(max(0, m.cursorViewportY()-m.layout.viewport.Height+1)) | ||
| m.layout.viewport.SetContent(m.renderDiff()) | ||
| m.moveDiffCursorUpBy(m.layout.viewport.Height) |
There was a problem hiding this comment.
moveDiffCursorPageDown/Up pass m.layout.viewport.Height directly into the helper. If the viewport height is 0 (e.g. before the first WindowSizeMsg/layout init), paging degenerates into effectively a 1-line move (loop exits immediately because >= n is true for n==0) and the viewport won’t scroll. Consider clamping to at least 1 (similar to half-page: max(1, ...)) or early-return when Height <= 0.
| // pgdown/pgup must keep the cursor's on-screen row stable — same relative position | ||
| // before and after. symmetric to ctrl+d/ctrl+u. regression test for issue #124. | ||
| func TestModel_PgDownPgUpPreservesRelativeCursorPosition(t *testing.T) { | ||
| lines := make([]diff.DiffLine, 200) | ||
| for i := range lines { | ||
| lines[i] = diff.DiffLine{NewNum: i + 1, Content: "line", ChangeType: diff.ChangeContext} | ||
| } |
There was a problem hiding this comment.
This regression test only exercises the 1-visual-row-per-line case. With wrap mode enabled (or multi-row annotations), the cursor can move by more than pageHeight visual rows in a single step, and the current paging implementation can drift the cursor’s on-screen row. Adding a subtest that enables model.modes.wrap = true with at least one long line (or a multi-line annotation) would catch regressions for the original “keep cursor row stable” requirement.
Extend the page/half-page helpers to treat an annotation-row flip (diffCursor unchanged but cursorOnAnnotation toggled) as real progress, and scroll the viewport by the cursor's actual visual delta instead of the requested row count. Without this, PgDown from a line with an annotation exited the helper loop immediately on the cursorOnAnnotation flip, then scrolled the viewport by a full page anyway, pushing the cursor above the viewport. Rename the helper parameter from n to rows to match the godoc. Extend TestModel_PgDownPgUpPreservesRelativeCursorPosition with a mid-screen subcase (so the relative-row assertion is not degenerate at screen row 0) and split into subtests with a fresh model per case. Add TestModel_PgDownKeepsCursorVisibleOnAnnotatedLine to lock in the visibility invariant. Related to #124
Adds a wrap-mode subtest to TestModel_PgDownPgUpPreservesRelativeCursorPosition with a mix of short and long lines. Verified wrap is active (long lines occupy 4 visual rows). Locks in the invariant that paging preserves the cursor's on-screen row when cursorViewportY math spans multi-row wrapped lines. Addresses Copilot review feedback on PR #125. Related to #124
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| m.moveDiffCursorUpBy(max(1, m.layout.viewport.Height/2)) | ||
| } | ||
|
|
||
| // moveDiffCursorDownBy advances the cursor down by up to rows visual rows |
There was a problem hiding this comment.
Doc comment has a duplicated/awkward phrase: "advances the cursor down by up to rows visual rows". Consider rewording to something like "...by up to rows visual rows" → "...by up to visual rows" (or "...by up to rows visual lines") to avoid the repetition and clarify what the parameter represents.
| // moveDiffCursorDownBy advances the cursor down by up to rows visual rows | |
| // moveDiffCursorDownBy advances the cursor down by up to the specified number of visual rows |
| // scrolls viewport by half page explicitly, matching vim/less ctrl+u behavior. | ||
| func (m *Model) moveDiffCursorHalfPageUp() { | ||
| halfPage := max(1, m.layout.viewport.Height/2) | ||
| // moveDiffCursorUpBy moves the cursor up by up to rows visual rows |
There was a problem hiding this comment.
Doc comment has the same duplicated/awkward phrasing as the down variant: "moves the cursor up by up to rows visual rows". Please reword for clarity (e.g., "...by up to visual rows").
| // moveDiffCursorUpBy moves the cursor up by up to rows visual rows | |
| // moveDiffCursorUpBy moves the cursor up by up to `rows` visual rows |
Fixes the usability bug reported in #124. Full-page navigation in the diff pane previously placed the cursor at the viewport top after PgDown and at the bottom after PgUp, so PgDown then PgUp did not return to the previous position.
The half-page actions (Ctrl-D / Ctrl-U) already scroll cursor and viewport by the same amount, keeping the cursor on the same screen row. This PR pulls that logic into a shared helper and uses it for full-page too, so PgDown/PgUp now preserve relative cursor position and are symmetric with Ctrl-D/Ctrl-U.
Change:
moveDiffCursorPageDown/moveDiffCursorPageUpdelegate to newmoveDiffCursorDownBy(n)/moveDiffCursorUpBy(n)helpers, which match the existing half-page behavior. No new keybindings or config surface.Regression test
TestModel_PgDownPgUpPreservesRelativeCursorPositioncovers both scenarios from the issue report.Related to #124