From 6968f6a284ae5f08984bde6e265e8a5d36653507 Mon Sep 17 00:00:00 2001 From: Greg Pomerantz Date: Thu, 20 Aug 2026 19:31:51 -0400 Subject: [PATCH] Restore scroll by logical line, not pixel offset (wrap-aware) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On device: scroll far down a wrapped file, relaunch, and the app lands further DOWN than where the user left off — the deeper the scroll, the further off. Root cause: the persisted Scroll is a pixel offset in VISUAL-line space. Restoring maps it through the WrapIndex (scrollDecompose -> LineForVisual), but on relaunch every count is the estimate (1) until the line is shaped, and shaping covers the visible window only — the lines ABOVE the restored viewport are never shaped. With all-ones counts LineForVisual maps the offset 1:1, landing a logical line deeper by every wrapped continuation above the viewport, and the state is stable (the under-counted lines never re-enter the window), so it never self-corrects. The snapshot now persists wrap-independent coordinates: the logical line at the viewport top (derived with the same mapping the layout uses, against the current index, so it is exactly the shown line) plus the sub-line remainder. The restore re-derives the offset as line*lh + sub, which maps to the saved line under any wrap state (all-ones or populated). The raw Dp offset is kept for pre-line-coordinate session files (loadSession defaults the missing key to -1; BeginRestore rejects the ambiguous zero value: a genuine line-0 snapshot always has Scroll < lh). TestRestore_ScrollSurvivesWrapState reproduces it: 150 lines recorded as wrapped x3, viewport at logical line 200 (visual 500); a relaunch with a fresh WrapIndex must land the window on line 200. Fails pre-fix (window at line 254, i.e. deeper) and passes with the fix. Docs: spec §2.4 (line-based scroll persist + current save policy), architecture §6.7 (why the offset is unrestorable by re-mapping). --- cmd/pad/main.go | 7 ++ doc/architecture.md | 12 +++ doc/spec.md | 18 +++- internal/editor/session.go | 56 ++++++++++++ internal/test/e2e/restore_test.go | 144 ++++++++++++++++++++++++++++++ 5 files changed, 233 insertions(+), 4 deletions(-) diff --git a/cmd/pad/main.go b/cmd/pad/main.go index fced39d..0ae6c24 100644 --- a/cmd/pad/main.go +++ b/cmd/pad/main.go @@ -423,6 +423,7 @@ func loadSession(path string) (editor.SessionState, bool) { return editor.SessionState{}, false } var s editor.SessionState + s.ScrollLine = -1 // pre-line session files lack the key; 0 would restore to the top if err := json.Unmarshal(b, &s); err != nil { return editor.SessionState{}, false } @@ -439,6 +440,12 @@ func loadSession(path string) (editor.SessionState, bool) { if s.FindCurByte < -1 { s.FindCurByte = -1 } + if s.ScrollLine < -1 { + s.ScrollLine = -1 + } + if s.ScrollSub < 0 { + s.ScrollSub = 0 + } if s.File == "" { return editor.SessionState{}, false } diff --git a/doc/architecture.md b/doc/architecture.md index b3344a9..6f2ea7b 100644 --- a/doc/architecture.md +++ b/doc/architecture.md @@ -292,6 +292,18 @@ Only the visible byte range is shaped and drawn each frame: - Max scroll is `TotalVisuals()·lh − regionH + lh/2` with the font-scale-effective line height; it grows incrementally as shaped counts arrive (pre-shaping it equals the no-wrap estimate). + - **Relaunch snapshot (spec §2.4):** the counts are process-local (rebuilt + from the all-ones estimate on relaunch; only the visible window is ever + shaped), so a persisted pixel scroll offset cannot be restored by + re-mapping — with every count above the restored viewport at the + estimate, `LineForVisual` maps the offset 1:1 to a logical line deeper + by all the wrapped continuations above it, with no self-correction + (those lines never enter the window). The snapshot therefore persists + the wrap-independent coordinates: the logical line at the viewport top + (derived with the same scrollDecompose + LineForVisual mapping the + layout uses, against the CURRENT index) plus the sub-line remainder; + the restore re-derives the offset as `line·lh + sub`. The raw offset is + kept as the fallback for pre-line-coordinate session files. - **Font-scale axis.** The shaper draws baselines in sp, so on Android the rendered line pitch in density-dp is `EditorLineHeight()*fontScale` (`fontScale = Metric.PxPerSp/PxPerDp`, the user font-size setting). diff --git a/doc/spec.md b/doc/spec.md index fb4b493..99c1aab 100644 --- a/doc/spec.md +++ b/doc/spec.md @@ -114,18 +114,28 @@ elsewhere. ### 2.4 State restoration on relaunch - On launch, Pad re-opens the file from the last session and lands straight - on the editor page, restoring the cursor, scroll offset, live selection, + on the editor page, restoring the cursor, scroll position, live selection, and the find bar (query, open/closed, current match). A relaunch therefore never requires re-browsing to the last file. +- The scroll position is persisted as the LOGICAL line at the viewport top + plus the sub-line remainder (the raw pixel offset is kept too, for + pre-line-coordinate session files). The pixel offset alone is not + restorable: it lives in visual-line space, and the wrap counts that map + it to a line are rebuilt from estimates on relaunch — lines above the + restored viewport are never shaped, so the offset would land a line + deeper by every wrapped continuation above it. - The snapshot is a tiny JSON file (a few hundred bytes) written by the cmd layer: `$HOME/.pad/session.json` off-Android, and `/storage/emulated/0/Pad/session.json` on Android (the dir that already carries the browser's `.pad` index caches). Search results are NOT stored: they are regenerated by re-scanning the restored query, and the current match is re-selected by byte offset. -- The snapshot is written rate-limited (≤ 1/s, only on change) while the app - runs and unconditionally at shutdown, so a kill shortly after a change - loses at most ~1 s of state. +- The snapshot is written rate-limited (250 ms, only on change) while the + app runs; a file change or a selection appearing/vanishing saves + immediately; on Android the activity's onStop (recents-wipe, app switch) + flushes it one last time; and it is written unconditionally at shutdown. + A kill shortly after a change therefore loses at most a fraction of a + second of state. - If the restored file no longer exists (deleted/moved, e.g. by Syncthing), the app falls back to the browser page; positions that exceed a shrunk file are clamped to its new end. diff --git a/internal/editor/session.go b/internal/editor/session.go index 41ce1eb..3ab0bf2 100644 --- a/internal/editor/session.go +++ b/internal/editor/session.go @@ -40,10 +40,23 @@ const sessionSaveInterval = 250 * time.Millisecond // SessionState is the relaunch snapshot (see the file doc above). All // positions are absolute file byte offsets; Scroll is in Dp. It is a plain // comparable value (used with == for change detection). +// +// Scroll is backed by ScrollLine: the pixel offset lives in VISUAL-line +// space (a wrapped line occupies several visual lines, and the scroll-to- +// line mapping runs through the WrapIndex), but on relaunch the counts of +// every line above the restored viewport are still the estimate (1) until +// the line is shaped, and lines above the visible window are never shaped. +// Mapping the saved offset through that fresh index would land a logical +// line DEEPER by all the wrapped continuations above the viewport, with no +// self-correction. The logical line at the viewport top and the sub-line +// remainder are wrap-independent, so they are the restorable unit; Scroll +// is kept as the pre-line-coordinate value (legacy session files). type SessionState struct { File string // last opened file ("" = nothing to restore) Cursor int // cursor byte offset Scroll float64 // editor scroll offset, Dp + ScrollLine int // logical line at the viewport top (-1 = unknown) + ScrollSub float64 // Scroll's sub-line remainder, Dp (0 <= r < lineHeight) SelStart int // selection start byte (-1 = no selection) SelEnd int // selection end byte, exclusive FindQuery string // find bar query ("" = none) @@ -77,10 +90,30 @@ func (l *Logic) SnapshotSession() SessionState { if f.Cur >= 0 && f.Cur < len(f.Matches) { curByte = f.Matches[f.Cur][0] } + // Persist the scroll in wrap-independent coordinates (see the + // SessionState doc). The derivation mirrors visibleByteRangePrecise + // (scrollDecompose, then LineForVisual against the CURRENT index), so + // the line is exactly the one the layout is showing at this offset. + scrollLine := -1 + var scrollSub float64 + if cb := e.ChunkedBuffer; cb != nil && !e.TooLarge { + lh := EffectiveLineHeight() + if gl := e.GlyphLayout; gl.LineHeight > 0 { + lh = gl.LineHeight + } + v0, r0 := scrollDecompose(s.ScrollOffset, lh) + scrollLine = int(v0) + if w := cb.WrapIndex; w != nil { + scrollLine = w.LineForVisual(int32(v0)) + } + scrollSub = r0 + } return SessionState{ File: e.Filename, Cursor: cur, Scroll: float64(s.ScrollOffset), + ScrollLine: scrollLine, + ScrollSub: scrollSub, SelStart: e.SelectionStart, SelEnd: e.SelectionEnd, FindQuery: f.Query, @@ -171,6 +204,14 @@ func (l *Logic) BeginRestore(s SessionState) { if s.File == "" { return } + // The zero-value ScrollLine (0) is ambiguous: a genuine top-of-file + // snapshot always has Scroll < lineHeight (no line above line 0 + // contributes visual lines), so line 0 with a deep offset is an unset + // field (an in-process caller or test built the snapshot without it) — + // fall back to the pixel offset rather than snapping to the top. + if s.ScrollLine == 0 && s.Scroll >= 2*float64(EffectiveLineHeight()) { + s.ScrollLine = -1 + } l.session = s e := &l.state.Editor e.Filename = s.File @@ -232,6 +273,21 @@ func (l *Logic) maybeApplyRestoreScroll() bool { if l.state.Page() != EditorPage || !l.restoreContentLanded { return false } + // Re-derive the offset from the persisted logical line (see the + // SessionState doc): the snapshot's pixel offset mapped to its position + // only while the saving session's WrapIndex was valid. On relaunch the + // counts above the restored viewport are all the estimate until shaped, + // and those lines never are (shaping covers the visible window only), + // so the raw offset would land a line deeper by every wrapped + // continuation above the viewport. The line-based offset lands on the + // saved line regardless of wrap state. + if s := l.session; s.ScrollLine >= 0 { + lh := EffectiveLineHeight() + if gl := l.state.Editor.GlyphLayout; gl.LineHeight > 0 { + lh = gl.LineHeight + } + l.restoreScroll = ui.Dp(float64(s.ScrollLine)*float64(lh) + s.ScrollSub) + } if l.restoreScroll > l.state.MaxScroll { l.restoreScroll = l.state.MaxScroll } diff --git a/internal/test/e2e/restore_test.go b/internal/test/e2e/restore_test.go index a345999..ff51ac1 100644 --- a/internal/test/e2e/restore_test.go +++ b/internal/test/e2e/restore_test.go @@ -472,6 +472,150 @@ func TestRestore_ShutdownSavesFinalSnapshot(t *testing.T) { } } +// TestRestore_ScrollSurvivesWrapState reproduces the on-device report +// "relaunch lands further DOWN than where I left off": the snapshot's pixel +// scroll lives in visual-line space (wrapped lines occupy several visual +// lines, and the scroll-to-line mapping runs through the WrapIndex), but on +// relaunch the counts of every line above the restored viewport are the +// estimate (1) until shaped, and lines above the visible window are never +// shaped. Mapping the saved offset through that fresh index lands a logical +// line deeper by all the wrapped continuations above it. The snapshot +// therefore persists the logical line at the viewport top (and the sub-line +// remainder), and the restore derives the offset from the line. +func TestRestore_ScrollSurvivesWrapState(t *testing.T) { + const lines = 300 + var b strings.Builder + for i := 0; i < lines; i++ { + fmt.Fprintf(&b, "line %03d content\n", i) + } + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "wrap.txt"), []byte(b.String()), 0644); err != nil { + t.Fatal(err) + } + + // First session: open the file, record the first 150 lines as wrapped + // into 3 visual lines each (as a shaping pass would), and scroll so the + // viewport top sits on logical line 200. In visual space that is line + // 150*3 + 50 = 500, so the pixel offset is 500 line-heights. + saves := make(chan editor.SessionState, 16) + h1 := e2e.NewHarness(e2e.WithFileSystem(real.NewRealFileSystem(dir), "/")) + h1.Logic().SetSessionSaver(func(s editor.SessionState) { saves <- s }) + h1.Run() + h1.SendConfig(780, 1688) + h1.SendScale(2.0) + defer h1.Cleanup() + + if err := h1.WithState(func(st *editor.State) { editor.OpenFile("/wrap.txt") }); err != nil { + t.Fatalf("OpenFile: %v", err) + } + for i := 0; i < 100; i++ { + v, err := h1.Inspect(func(st *editor.State) any { + cb := st.Editor.ChunkedBuffer + return cb != nil && cb.FileLen() > 0 && cb.LineIndex != nil + }) + if err == nil && v.(bool) { + break + } + time.Sleep(20 * time.Millisecond) + } + // Set the wrapped counts and the deep scroll on the owner; the line + // height comes from the owner too (EffectiveLineHeight reads state). + lhAny, err := h1.Inspect(func(st *editor.State) any { + w := st.Editor.ChunkedBuffer.WrapIndex + for i := 0; i < 150; i++ { + w.Set(i, 3) + } + st.ScrollOffset = ui.Dp(500 * float64(editor.EffectiveLineHeight())) + return float64(editor.EffectiveLineHeight()) + }) + if err != nil { + t.Fatalf("Inspect: %v", err) + } + lh := lhAny.(float64) + // Wait past the rate limit and force a frame: the saver must capture + // the deep scroll together with its logical line (200). Match on the + // line, not the exact Dp: ui.Dp is float32-based, so ui.Dp(500*lh) and + // 500*float64(lh) round differently. + time.Sleep(300 * time.Millisecond) + h1.SendConfig(780, 1688) + snap := editor.SessionState{} + deadline := time.After(5 * time.Second) + for { + select { + case s := <-saves: + if s.File == "/wrap.txt" && s.ScrollLine == 200 && s.Scroll > 0 { + snap = s + goto got + } + case <-deadline: + t.Fatal("timed out: saver did not capture the deep-scroll snapshot") + } + } +got: + if snap.Scroll < 499*lh || snap.Scroll > 501*lh { + t.Fatalf("snapshot Scroll = %v, want ~500*lh (%v): the pixel offset must stay the visual-space value", + snap.Scroll, 500*lh) + } + if snap.ScrollLine != 200 { + t.Fatalf("snapshot ScrollLine = %d, want 200 (the logical line at the viewport top under wrap)", snap.ScrollLine) + } + if snap.ScrollSub >= 0.01 { // 0 apart from ui.Dp float32 rounding + t.Errorf("snapshot ScrollSub = %v, want ~0", snap.ScrollSub) + } + + // Second session (the relaunch): the WrapIndex is fresh, every count the + // estimate (1). The restore must land on logical line 200, not on visual + // line 500 mapped 1:1 (which would be line 500, far deeper). + h2 := e2e.NewHarness(e2e.WithFileSystem(real.NewRealFileSystem(dir), "/")) + h2.Logic().BeginRestore(snap) + h2.Run() + h2.SendConfig(780, 1688) + h2.SendScale(2.0) + defer h2.Cleanup() + + // The restore must land the scroll within one line of 200*lh (the ui.Dp + // float32 rounding keeps an exact match out of reach); the exact + // assertion is the window start line below. + for i := 0; i < 100; i++ { + v, err := h2.Inspect(func(st *editor.State) any { + cb := st.Editor.ChunkedBuffer + if cb == nil || cb.FileLen() == 0 || cb.LineIndex == nil { + return false + } + s := float64(st.ScrollOffset) + return s > 199*lh && s < 201*lh + }) + if err == nil && v.(bool) { + break + } + time.Sleep(20 * time.Millisecond) + } + // One more frame so the layout pass records the window start line for + // the landed scroll. + h2.SendConfig(780, 1688) + time.Sleep(100 * time.Millisecond) + v, err := h2.Inspect(func(st *editor.State) any { + return struct { + Scroll ui.Dp + WinLine int + }{st.ScrollOffset, st.WindowStartLine} + }) + if err != nil { + t.Fatalf("Inspect: %v", err) + } + got := v.(struct { + Scroll ui.Dp + WinLine int + }) + if s := float64(got.Scroll); s < 199*lh || s > 201*lh { + t.Errorf("restored scroll = %v, want ~200*lh (%v): the saved logical line's offset", + got.Scroll, 200*lh) + } + if got.WinLine != 200 { + t.Errorf("window starts at line %d, want 200 (the saved line); a deeper line is the pre-fix visual-space mapping", got.WinLine) + } +} + // TestRestore_UrgentSaveOnSelection verifies the kill-race fix: a selection // appearing (the user just highlighted text) saves immediately, even inside // the rate-limit window that the file-open save opened. Without the urgent