From 12d3dac8b3ce86c3bf0945aebc2653cd0bc0d4ff Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 18 Jul 2026 17:36:59 +0200 Subject: [PATCH] Rework the plan: diff-renderer terminology, landed threading rework Two corrections from review: - "Pager" is retired in favor of "diff renderer" throughout (the spec already made this change); a new mini PR 3 renames the pagers config to diffRenderers with migration and docs, and the remaining PRs are renumbered accordingly. - The locked-decision list wrongly said concurrency stays mutex-based; the main-thread-mutation rework has landed on master and the prototype is rebased on top of it. The plan now states the landed threading contract that all PRs must honor, and PR 1 gained a re-validate-against-master preamble. Its reset-after-CopyContent commit was verified still needed (master's CopyContent still copies the source origin); the two locking fixes were reshaped onto the viewLines readers master actually has, since the prototype's HyperLinkInLine guard belongs to the dropped hyperlink backend. Co-Authored-By: Claude Fable 5 --- focused-main-view-production-plan.md | 425 ++++++++++++++++----------- 1 file changed, 260 insertions(+), 165 deletions(-) diff --git a/focused-main-view-production-plan.md b/focused-main-view-production-plan.md index 99ce4c1af..130e6c2ff 100644 --- a/focused-main-view-production-plan.md +++ b/focused-main-view-production-plan.md @@ -20,6 +20,18 @@ current: check off commits as they land, record deviations, and add findings. `amend!` for iteration, no conventional-commit prefixes, `just generate` after keybinding/test changes, docs in `docs-master/` only, translatable strings via Go templates, no PRs created by agents (the user opens them). +- **Terminology: say "diff renderer", never "pager".** "Pager" is a misnomer + that leaked out of the implementation (the `GIT_PAGER` env var); the OSC 1717 + spec now says "diff renderer" throughout, and so do we — in code identifiers, + commit messages, PR descriptions, docs, and user-facing strings. The + prototype's identifiers and test names still say "pager" + (`ProbePagerEmitsDiffMetadata`, `onPagerChanged`, + `stage_from_main_view_with_unsupported_pager`, …); rename them during + transcription. `GIT_PAGER` itself and the "stdin-pager invocation route" (as + opposed to `externalDiffCommand`) keep their technical names where the + mechanism is literally meant. PR 3 renames the user-facing config. +- **Threading:** master has the landed main-thread rework — see §2.8 for the + contract every PR must honor. The prototype is already rebased on top of it. - **Prototype references** are given as *subject line* plus short SHA. The prototype branch gets rebased; **find commits by subject, not SHA**. When a plan item says "reference: X", read that prototype commit (message + diff) @@ -33,32 +45,32 @@ current: check off commits as they land, record deviations, and add findings. - **Branch naming/stacking:** each PR gets its own branch; PR N+1 branches off PR N's branch (linear stack). Rebase the stack when a PR merges. - **Verification:** every PR runs `just build`, `just unit-test`, `just lint`, - `just e2e`. PRs touching the async render or pager paths additionally need - the interactive sign-off listed in §6 — the headless harness cannot exercise - the pty/pager path (N§13.1: cmd-path only, env allowlist blocks + `just e2e`. PRs touching the async render or diff-renderer paths additionally + need the interactive sign-off listed in §6 — the headless harness cannot + exercise the pty/renderer path (N§13.1: cmd-path only, env allowlist blocks `LAZYGIT_SLOW_RENDER`). ## 2. Locked scope decisions (do not relitigate) -Decided with the user (2026-07-17 planning session, plus earlier locked +Decided with the user (2026-07-17/18 planning sessions, plus earlier locked decisions from the notes): 1. **The staging and patch-building panels are removed.** `enter` on a file (files panel and commit-files panel) focuses the main view at that file's diff. The `Staging`/`StagingSecondary`/`CustomPatchBuilder` contexts, views, - and `patch_exploring` machinery are deleted (PR 8). The prototype kept them + and `patch_exploring` machinery are deleted (PR 9). The prototype kept them as an A/B reference (N§21.24) — production does not. 2. **`enter` / double-click in the focused main view are dropped.** With the explorers gone the dive gesture has no target. Enter is unbound there; double-click behaves like a single click (select). Esc exits; space/d/e act on the selection. 3. **Sequencing:** stacked PRs, merged in quick succession; **no release ships - with both staging UIs**. Brief coexistence on master between PR 6/7 and - PR 8 is fine. + with both staging UIs**. Brief coexistence on master between PRs 7/8 and + PR 9 is fine. 4. **The nav/selection and position-preserve features land as their own PRs - before the staging series** (PRs 4 and 5), independently releasable. + before the staging series** (PRs 5 and 6), independently releasable. 5. **Both extras are in scope** as final small PRs: alt/shift-click-to-edit - (PR 9) and open-PR-at-selected-line (PR 10). + (PR 10) and open-PR-at-selected-line (PR 11). 6. **The hyperlink backend is dropped** (N§14.5): no `lazygit-edit://`-based line identity resolution. (Master's existing click-a-delta-hyperlink feature in `pkg/gui/gui.go` is untouched — only the prototype's use of hyperlinks as @@ -66,14 +78,32 @@ decisions from the notes): 7. **Selection is always shown in diff main views** (N§21.6), anchored at the first visible change line / hunk; no on-demand toggle, no config for it. Non-diff main views (branch log, …) keep no selection. -8. **Concurrency stays mutex-based** (N§15 locked); the main-thread-mutation - rework is a separate later effort. Do not design around it now. +8. **Follow master's landed threading contract.** The main-thread-mutation + rework has landed on master (Front A #5767; "Synchronize async view + rendering" #5791; the popup, command-log, and status fronts; + `RefreshFromWorker` + captured refresh inputs; `BatchUIUpdates` replacing + BLOCK_UI), and the prototype is rebased on top of it (merge-base + `4cf12a5b7`). The contract for all new code: **model/context/selection + mutations are UI-thread-only** (asserted in `-debug` builds); **view + geometry and origin (`ox`/`oy`) are UI-thread-only**; the view's **write + buffer may be written off-thread under `writeMutex`, which is kept + permanently** (the batch-to-UI-thread attempt deadlocked and was + abandoned); task-side origin/dimension work bounces via + `ViewBufferManager.onUIThread`; worker-issued refreshes use + `RefreshFromWorker` with UI-owned inputs captured up front; incidental + display work bounces as *background* tasks so it doesn't count toward + `Busy()` and block repo switches. The prototype tree is already adapted + (atomic `loading` flag, bounced `onEndOfInput`/`onNewKey`, `OnUIThread` + hops in restore `Apply`, off-screen swap under `writeMutex`) — transcribe + it as-is; where a threading question comes up, the answer is "match the + current tree / master's contract". See memory + [[main-thread-over-mutexes-direction]] and `docs/dev/Repo_Switch_Safety.md`. 9. **Side-by-side staging includes all records on a row** — no stage-one-side gesture (N§21.3, accepted restriction). -10. **Pager capability detection is the empty-input handshake probe** - (N§21.30), not render observation. Non-conforming pagers get the raw-diff - fallback at focus time; `useExternalDiffGitConfig` is always-raw when - focused (documented limitation). +10. **Diff-renderer capability detection is the empty-input handshake probe** + (N§21.30), not render observation. Non-conforming renderers get the + raw-diff fallback at focus time; `useExternalDiffGitConfig` is always-raw + when focused (documented limitation). 11. **The escape/snapshot machinery is never built** (`FocusedMainViewSnapshot`, `EscapeFromPatchExplorer`, the N§12.2 escape routing): it existed only to return *from the explorers*, which no longer exist. @@ -86,18 +116,19 @@ The branch contains superseded/reverted work. Do not transcribe any of these | Not ported | Superseded by / reason | |---|---| | `gui.showSelectionInFocusedMainView` config; on-demand space-toggled selection | always-shown selection (N§21.6) | -| Middle-visible-line as the *selection* anchor ("Select the line in the middle…") | first-visible-change/hunk anchor. (`MiddleVisibleLineIdx` itself survives as the `-U`-preserve anchor when no selection shows — PR 5) | +| Middle-visible-line as the *selection* anchor ("Select the line in the middle…") | first-visible-change/hunk anchor. (`MiddleVisibleLineIdx` itself survives as the `-U`-preserve anchor when no selection shows — PR 6) | | `enter`/double-click dive into staging/patch-building at a line; `CommitFilesHelper.EnterCommitFile` threading | gesture dropped (§2.2) | | `FocusedMainViewSnapshot`, `EscapeFromPatchExplorer`, escape-restore-by-identity, N§12.2 routing | explorers removed (§2.11) | | Hyperlink identity backend (`GetFileAndLineForClickedDiffLine` hyperlink parsing, `HyperLinkInLine` as a *backend*) | buffer-parse + OSC metadata only (§2.6) | -| `ScrollToOriginYForNextTask` / `thenForNextTask` / `KeepOriginForNextTask` / `LinesToRead.ApplyInitialScroll` | `RenderRestore` (PR 5) — build the final mechanism directly | +| `ScrollToOriginYForNextTask` / `thenForNextTask` / `KeepOriginForNextTask` / `LinesToRead.ApplyInitialScroll` | `RenderRestore` (PR 6) — build the final mechanism directly | | "Hold the placeholder until first paint" + `freshViewLineCount` stale-tail guard (both reverted on the branch too) | off-screen render (PR 1) | -| Observe-at-focus pager detection (N§21.29) | handshake probe (N§21.30) | +| Observe-at-focus diff-renderer detection (N§21.29) | handshake probe (N§21.30) | | `HighlightInset` and `selectionBgColorEdgeWidth` experiments | `narrowSelectionHighlight` (N§21.34) | | `matchByWorktreeChange` and `AdjacentChangeLine` reveal matchers | change-line-ordinal reveal (N§21.17) | | The three separate handler channels (`onClick`/`onStage`/`onTogglePatch FocusedMainViewFn`) | build `FocusedMainViewActions` directly in its final one-interface shape (N§21.25) | | Unconditional gutter reset-on-preview-render + the `strings.Join(cmd.Args)` content-equality hack | focused-pair-only gutter model (N§21.22(3), N§21.35) | | `backUpOverHeader` file-nav landing | land on the first located row; f/h header records make headers resolvable ("Parse the f/h header records…", af98be48d) | +| "Pager" naming in identifiers, strings, docs | "diff renderer" (§1 terminology; PR 3 renames the config) | | OSC number `456`, env vars `EMIT_OSC1717_METADATA`/`OSC1717_METADATA` | OSC **1717**, env var **`OSC1717`** (final rename, 665149b11) | | The in-repo spec file | the spec lives on the `osc-1717-spec` branch / worktree (fe3c5ac21) | | Session-notes commits, `.claude/settings.json` commits, WIP commits | n/a | @@ -105,21 +136,22 @@ The branch contains superseded/reverted work. Do not transcribe any of these ## 4. The PR stack (overview) PR titles become release-notes lines — they are written for users. Order is -dependency order; 1–5 are independently releasable; 6–8 merge in quick -succession (§2.3); 9–10 any time after their dependencies. +dependency order; 1–6 are independently releasable; 7–9 merge in quick +succession (§2.3); 10–11 any time after their dependencies. | # | Title (draft) | Depends on | Nature | |---|---|---|---| | 1 | Fix flicker, scroll glitches, and crashes in async diff rendering | — | fixes, gocui/tasks | | 2 | Internal: resolve diff lines to (file, line, kind) identities | 1 | infra | -| 3 | Support pagers that emit OSC 1717 diff line metadata | 2 | infra + protocol | -| 4 | Select, navigate, edit and copy diff lines in the focused main view | 2 (3 for pagers) | feature | -| 5 | Keep your position in the diff when changing context size or switching pagers | 1, 2, 4 | feature | -| 6 | Stage, unstage and discard changes directly from the focused main view | 3, 4, 5 | feature | -| 7 | Build custom patches directly from a commit's diff view | 6 | feature | -| 8 | Replace the staging and patch-building panels with the focused main view | 6, 7 | removal + migration | -| 9 | Alt- or shift-click a diff line to open it in your editor | 2, 3 | feature | -| 10 | Open the selected diff line in the branch's GitHub PR | 4 | feature | +| 3 | Rename the "pagers" config to "diff renderers" | — | rename + migration | +| 4 | Support diff renderers that emit OSC 1717 diff line metadata | 2, 3 | infra + protocol | +| 5 | Select, navigate, edit and copy diff lines in the focused main view | 2 (4 for renderers) | feature | +| 6 | Keep your position in the diff when changing context size or switching diff renderers | 1, 2, 5 | feature | +| 7 | Stage, unstage and discard changes directly from the focused main view | 4, 5, 6 | feature | +| 8 | Build custom patches directly from a commit's diff view | 7 | feature | +| 9 | Replace the staging and patch-building panels with the focused main view | 7, 8 | removal + migration | +| 10 | Alt- or shift-click a diff line to open it in your editor | 2, 4 | feature | +| 11 | Open the selected diff line in the branch's GitHub PR | 5 | feature | --- @@ -131,6 +163,13 @@ All standalone master-worthy fixes; users benefit regardless of the rest of the series. Everything here lives in `pkg/gocui`, `pkg/tasks`, and the gui layout/render plumbing. +**Re-validate each fix against current master before transcribing.** The +landed rework (#5791) restructured exactly this area — the read loop now +bounces `onEndOfInput`/`onNewKey` to the UI thread, `readLines` is an atomic +pointer, several buffer accessors gained locks. The prototype commits are +rebased on top of all that and are the authoritative shapes, but a fix may +have been absorbed or may need reshaping; check before porting. + Commits (in order): 1. **Route all view origin writes through `SetOriginX`/`SetOriginY`** — pure @@ -138,50 +177,58 @@ Commits (in order): 2. **Add `LAZYGIT_SLOW_RENDER` debug knob** — sleeps N ms per written line so async render frames become visible; inert when unset. Needed by reviewers to see what the later commits fix. Ref: e8682b3fd. -3. **Lock the view and guard the line index when reading a hyperlink** — - fixes a real master panic (`HyperLinkInLine` reads `v.lines`/`v.viewLines` - without `writeMutex` and can index a stale tail). Use the - demonstrate-then-fix pattern if a deterministic test is feasible. - Ref: 2cc42fc81. -4. **Lock the view while reading viewLines on mouse move** — same class, - `onMouseMove`/`findHyperlinkAt` panic. Ref: a44bf5d05. -5. **Fire queued ReadToEnd callbacks when the initial read reaches EOF** — +3. **Lock the event-thread viewLines readers that still skip `writeMutex`** — + task goroutines rebuild `viewLines` under `writeMutex`; master still reads + them unlocked on the event-handling thread in the click-path hyperlink + lookup (`pkg/gocui/gui.go` ~1686, `viewLines[newY].line[newX].hyperlink`) + and in `onMouseMove`/`findHyperlinkAt` (hover) — the rework locked + `ClearViewLines`/`IsTainted`/`Buffer` but not these; audit for any others. + The prototype fixed this class in its own `HyperLinkInLine` reader (part of + the dropped hyperlink backend) and in `onMouseMove`; transcribe the lock + pattern onto the readers master actually has. Use the demonstrate-then-fix + pattern if a deterministic test is feasible. Refs: 2cc42fc81, a44bf5d05. +4. **Fire queued ReadToEnd callbacks when the initial read reaches EOF** — the read loop abandoned queued `{Total:-1}` requests when the initial request hit EOF, silently dropping their `Then`. Ref: b6f99abc6. -6. **Don't scroll a view up to fill blank space while its content is loading** - — the layout clamp used the partially-loaded height; add the synchronous - `loading` flag and skip the clamp while `IsLoading()`. Ref: 695842291. -7. **Reset other main views' scroll after copying content, not before** — +5. **Don't scroll a view up to fill blank space while its content is loading** + — the layout clamp used the partially-loaded height; add the `loading` + flag (an `atomic.Bool` in the rebased shape) and skip the clamp while + `IsLoading()`. Ref: 695842291. +6. **Reset other main views' scroll after copying content, not before** — `refreshMainViews` zeroed the source view's origin before `CopyContent` - used it, so every cross-pair placeholder jumped to the top. Ref: c35c9316c. -8. **Bundle a view's cell buffer and write state into a `viewBuffer`** — prep. + used it, so every cross-pair placeholder jumped to the top. *(Verified + against master 2026-07-18: still needed — `CopyContent` still copies the + source's `ox`/`oy` and `refreshMainViews` still resets first; master's + `1efcfcc14` only stopped sharing the buffer slices.)* Ref: c35c9316c. +7. **Bundle a view's cell buffer and write state into a `viewBuffer`** — prep. Ref: fd858cd98. -9. **Make the buffer-writing methods operate on a `viewBuffer`** — prep. +8. **Make the buffer-writing methods operate on a `viewBuffer`** — prep. Ref: 2cfc0e24d. -10. **Render async content into an off-screen buffer and swap it in** — the - core mechanism: cmd/pty tasks write to `View.offscreen`; at first-paint - (or EOF) the buffer swaps in atomically; `refreshViewLinesIfNeeded` - truncates view lines on swap (kills the stale-tail class); - `clear()`/`Reset()` abandon an in-progress off-screen render. Includes the - **scrollbar height freeze** (`FreezeScrollbarHeight` at `StartLoading`, - release at EOF and in `clear()`) — in the prototype this was an `amend!` - into the same commit precisely because the off-screen render introduces - the scrollbar regression; keep them together here too. Tests: - `TestOffscreenRender`, `TestBufferLineForViewLineStaleTail`, - `TestScrollbarHeightHeldWhileLoading`, - `TestScrollbarHeightReleasedWhenContentReplaced`. Refs: 27ce0a6bc + its - scrollbar amend, N§13.5, N§13.6. -11. **Don't run end-of-input handling for a render that was stopped** — the +9. **Render async content into an off-screen buffer and swap it in** — the + core mechanism: cmd/pty tasks write to `View.offscreen`; at first-paint + (or EOF) the buffer swaps in atomically (under `writeMutex` — buffer + writes are the writeMutex domain per §2.8); `refreshViewLinesIfNeeded` + truncates view lines on swap (kills the stale-tail class); + `clear()`/`Reset()` abandon an in-progress off-screen render. Includes the + **scrollbar height freeze** (`FreezeScrollbarHeight` at `StartLoading`, + release at EOF and in `clear()`) — in the prototype this was an `amend!` + into the same commit precisely because the off-screen render introduces + the scrollbar regression; keep them together here too. Tests: + `TestOffscreenRender`, `TestBufferLineForViewLineStaleTail`, + `TestScrollbarHeightHeldWhileLoading`, + `TestScrollbarHeightReleasedWhenContentReplaced`. Refs: 27ce0a6bc + its + scrollbar amend, N§13.5, N§13.6. +10. **Don't run end-of-input handling for a render that was stopped** — the stopped-task EOF coin-flip (`select` between stop and closed `lineChan`) let a stopped task swap in a truncated buffer. No deterministic test (the bug *is* the nondeterministic select) — justified skip, N§13.6. Ref: 8e3dc3eff. -12. **Reset the scroll to the top at first paint, not when the task starts** — +11. **Reset the scroll to the top at first paint, not when the task starts** — with the off-screen render the old content stays visible until the swap; resetting oy at task start made it jump first. Ref: 411681502. Notes: -- If commit 10's stale-tail test needs `BufferLineForViewLine`, introduce that +- If commit 9's stale-tail test needs `BufferLineForViewLine`, introduce that accessor here (PR 2 then reuses it) rather than contorting the test. - Gotcha for the future: **fast renders unmask ordering transients that slow renders hide** (N§20.5). Re-test at normal speed *and* under slow render. @@ -195,8 +242,8 @@ flicking through commits/files scrolled down; the 10 s auto-refresh with The host-side primitive: rendered row → `(path, kind, new/old line)`. No user-visible change; the PR description should say what it enables. Backends in this PR: **buffer-parse only** (raw / `--color` / structure-preserving -pager output). Precedence seam is built here; the metadata backend slots in -via PR 3. +renderer output). Precedence seam is built here; the metadata backend slots +in via PR 4. Commits: @@ -222,9 +269,9 @@ Commits: one parse per file section — **O(n), never per-line**; the single-line resolver delegates to it), `StagingHelper.resolveDiffLines` / `GetDiffLineInfo` seam with backend precedence (metadata → buffer-parse; - metadata arrives in PR 3). Port the prototype's unit tests + metadata arrives in PR 4). Port the prototype's unit tests (`diff_line_parser_test.go`, `diff_line_info_test.go`, - `diff_line_navigation_test.go` comes with PR 4). Refs: 7cf9b5037, + `diff_line_navigation_test.go` comes with PR 5). Refs: 7cf9b5037, 9c0bb5357, 556ba1213 (final O(n) shape — build it O(n) directly; N§20). 4. **Decode C-quoted paths in the buffer parser** — flagged as an unclosed prototype gap (M§8): git C-quotes unusual paths in `diff --git` headers; @@ -236,12 +283,43 @@ Gotchas: and index together (the `DiffLineContents` snapshot approach does this). - Multi-file diffs: `fileSectionBounds` handles rows outside any file section. -### PR 3 — Support pagers that emit OSC 1717 diff line metadata +### PR 3 — Rename the "pagers" config to "diff renderers" -Reads the protocol; sets the env var so conforming pagers emit. No consumer -behavior changes yet (consumers land in PRs 4–7), but the PR is the public -face of the protocol on the lazygit side — write the description for pager -authors, link the spec (osc-1717-spec branch). +A small standalone PR, before the protocol/feature PRs so their user-facing +text is consistent. "Pager" is an implementation-detail name (`GIT_PAGER`); +what these programs actually are is diff renderers, and the OSC 1717 spec +already uses that term throughout. No behavior change. + +Commits: + +1. **Rename the config with migration** — `git.pagers` → `git.diffRenderers` + (yaml key + struct/field: `Pagers []PagingConfig` → e.g. + `DiffRenderers []DiffRendererConfig`), and the keybindings + `keybinding.universal.cyclePagers`/`cyclePagersReverse` → e.g. + `cycleDiffRenderers`/`cycleDiffRenderersReverse` — both via lazygit's + config-migration mechanism (`pkg/config`; follow the existing migration + precedents) so user config files keep working/are rewritten. Reword the + affected i18n strings (`CyclePagers`(+Tooltip), status-bar text) in + english.go only. +2. **Rename internal identifiers** — mechanical sweep (`PagingConfig`, + `usingExternalDiff` neighbors, helper names) as far as reasonable; keep + `GIT_PAGER` and the pty "stdin-pager route" naming where the mechanism is + meant (§1 terminology). +3. **Docs** — rename/rewrite `docs-master/Custom_Pagers.md` (→ + `Custom_Diff_Renderers.md`; fix inbound links; leave `docs/` alone per + AGENTS.md), `just generate` for Config.md/schema and cheatsheets. + +Open question (resolve with the user at PR start): does the per-entry +`pager:` field keep its name? It specifically selects the GIT_PAGER +invocation route (vs `externalDiffCommand`), so a rename there trades +consistency against precision. + +### PR 4 — Support diff renderers that emit OSC 1717 diff line metadata + +Reads the protocol; sets the env var so conforming renderers emit. No +consumer behavior changes yet (consumers land in PRs 5–8), but the PR is the +public face of the protocol on the lazygit side — write the description for +diff-renderer authors, link the spec (osc-1717-spec branch). Commits: @@ -253,8 +331,15 @@ Commits: record** (N§21.30, `TestDiffLineMetadataHandshakeSwallowed`); multi-record rows: `View.DiffLineMetadataPayloads()` returns *all* distinct payloads per buffer line (side-by-side rows carry two — N§17.1, N§21.12). Unit tests - incl. wrapped rows (every pager-wrapped output row carries the record — + incl. wrapped rows (every renderer-wrapped output row carries the record — M§10.8). Refs: 1cc7ecbbb, e8385b3cf, 13595f0a8, 3018289e8 (gocui half). + **Note:** at planning time (2026-07-18) the prototype has *uncommitted* + gocui work in flight extending this to **zero-width record regions** + (back-to-back records with no cells between them — e.g. difftastic's + combined file+hunk banner, or a modification pair collapsed to one column; + orphaned payloads are drained into content-less carrier cells so every + record on a row stays discoverable). Check its committed state when + transcribing and include the final shape. 2. **The metadata backend, slotted ahead of buffer-parse** — resolve `a`/`d`/`c` records to `DiffLineInfo`; **accept the `f`/`h` header records** (file header: no line number; hunk header: first line of its @@ -262,17 +347,17 @@ Commits: `SamePatchLine` requires header kinds to match (a hunk header shares its number with the hunk's first content line — af98be48d's message has the full reasoning). Refs: 836f768cb, af98be48d. -3. **Advertise the protocol to pagers** — set `OSC1717=V1` in the environment - of pager/ext-diff invocations (pty task env + ext-diff cmd env). Ref: - 9975a8fac + 665149b11 (final name: `OSC1717`). +3. **Advertise the protocol to diff renderers** — set `OSC1717=V1` in the + environment of renderer/ext-diff invocations (pty task env + ext-diff cmd + env). Ref: 9975a8fac + 665149b11 (final name: `OSC1717`). Cross-repo note: the reference emitters live on `osc-1717-metadata` branches in `/Users/stk/Stk/Dev/Builds/{delta,difftastic,diff-so-fancy}`; nothing is upstreamed yet. lazygit must remain fully functional without any conforming -pager (buffer-parse + PR 6's raw fallback guarantee this). Interactive -verification of this PR needs locally built patched pagers. +renderer (buffer-parse + PR 7's raw fallback guarantee this). Interactive +verification of this PR needs locally built patched renderers. -### PR 4 — Select, navigate, edit and copy diff lines in the focused main view +### PR 5 — Select, navigate, edit and copy diff lines in the focused main view The focused main view (already reachable via `0`/click on master) gains a real selection and line-level interactions. After this PR: diff main views @@ -289,7 +374,7 @@ Commits: `GetDiffMainViewType() DiffMainViewType` (`None|Staging|PatchBuilding`) on the side-panel contexts (files=Staging; commitFiles/localCommits/ subCommits/stash=PatchBuilding; reflog=PatchBuilding **from day one** — - see PR 7 reflog item; others None). In this PR only "≠ None" is read (is + see PR 8 reflog item; others None). In this PR only "≠ None" is read (is this a diff main view). Refs: f470d870f (marker origin), a760f9ef5 (classifier final shape), N§21.25. 3. **The selection model** — `DiffSelectState` on `MainContext` @@ -309,7 +394,7 @@ Commits: content; hide when changes vanish (render-side hook in the panel's render-to-main + focus-side check via `ViewHasChangeLines`). e2e: `no_selection_when_no_changes`, `hide_selection_after_discarding_last_change` - (adapt: discard via files panel until PR 6). Ref: 7901de3d4, N§21.27 + (adapt: discard via files panel until PR 7). Ref: 7901de3d4, N§21.27 bug 4. 5. **Drag-to-range** — `dragAnchorViewLine` on MainContext; `MouseLeft` + `ModMotion` binding re-anchors at the mouse-down line. **Includes the gocui @@ -332,9 +417,9 @@ Commits: 9. **Copy the selection (`ctrl+o`)** — map the selected *view* range to buffer lines via `BufferLineForViewLine` (never `SelectedLines()` — it's wrapping-unaware, N§21.28), copy each wrapped line once, trailing `\n`; - `dropDiffPrefix` only when no pager is configured. e2e: + `dropDiffPrefix` only when no diff renderer is configured. e2e: `copy_from_main_view`. Ref: 99f14162c + its fixup. -10. **`narrowSelectionHighlight` per-pager config** — gocui +10. **`narrowSelectionHighlight` per-renderer config** — gocui `SelectedLineBgColorWidth` (left N columns only), gui maps bool→2; docs via `just generate`. Ref: cc90accde, N§21.34. @@ -342,14 +427,14 @@ Open item to resolve with the user during this PR: whether `n`/`N`/`f` get proper keybinding config entries (prototype used hardcoded literals, N§16.2) — lean: add config entries, matching lazygit convention. -Note: `space` is deliberately **not** bound here — staging arrives in PR 6. -Under a non-conforming restructuring pager, nav/e simply no-op until PR 6's -raw fallback lands; acceptable interim (same release). +Note: `space` is deliberately **not** bound here — staging arrives in PR 7. +Under a non-conforming restructuring renderer, nav/e simply no-op until +PR 7's raw fallback lands; acceptable interim (same release). -### PR 5 — Keep your position in the diff when changing context size or switching pagers +### PR 6 — Keep your position in the diff when changing context size or switching diff renderers The `RenderRestore` mechanism plus its two standalone consumers. After this -PR: `{`/`}` (context size) and `|`/`\` (pager cycle) keep your scroll +PR: `{`/`}` (context size) and `|`/`\` (renderer cycle) keep your scroll position and selection instead of jumping to the top. Commits: @@ -364,8 +449,9 @@ Commits: command-key changed`; **not cleared when a task starts** (survives stop-and-replace by the periodic refresh), cleared in `Apply` (found or not) — N§14.1; `Apply` work that touches gui state hops to `OnUIThread` - (it runs on the task goroutine, N§21.29 threading fix). Refs: 2e3a3ae5b - (mechanism parts), 3b597a0f2, N§14.1, N§20.5. + (it runs on the task goroutine, N§21.29 threading fix; origin writes are + UI-thread-only per §2.8 — the rebased prototype shows the exact shape). + Refs: 2e3a3ae5b (mechanism parts), 3b597a0f2, N§14.1, N§20.5. 2. **gocui: off-screen scan accessors** — `OffscreenDiffLineContents` / `OffscreenDiffLineContentsFrom(from)` (incremental — the O(n) load scan), `OffscreenLineCount`, `MiddleVisibleLineIdx`. Refs: 792c7a294, 3e5b52b8f, @@ -383,11 +469,11 @@ Commits: (same screen row); visibility guard (don't install on a hidden Normal view — merge-conflict edge, N§16.1). e2e: extend `staging/diff_context_change`-adjacent coverage. Ref: 24a95e965. -5. **Preserve position when switching pagers** — same one-liner in - `onPagerChanged`; fixes both the ext-diff top-jump and the wrong-line - "preserved by raw line number" cases (N§18.2); graceful no-op fallback for - unresolvable pagers. e2e: `diff/cycle_pagers` keeps passing. Ref: - a21c5841a. +5. **Preserve position when switching diff renderers** — same one-liner in + the renderer-cycle handler (prototype: `onPagerChanged`); fixes both the + ext-diff top-jump and the wrong-line "preserved by raw line number" cases + (N§18.2); graceful no-op fallback for unresolvable renderers. e2e: + `diff/cycle_pagers` keeps passing (rename per PR 3). Ref: a21c5841a. 6. **Preserve the selection's far end too** — `selectionFarEndIdentity` restored via `SetRangeSelectStart`; collapses to the cursor line when the far end didn't survive. Ref: 0412046c4, N§21.32(4). @@ -395,13 +481,13 @@ Commits: Known limitation (keep, document in PR): `NormalSecondary` is not preserved (N§16.1, N§18.3). -### PR 6 — Stage, unstage and discard changes directly from the focused main view +### PR 7 — Stage, unstage and discard changes directly from the focused main view The headline PR. After it: in the files panel's focused main view, `space` stages/unstages the selected line/range/hunk (multi-file, side-by-side aware), `d` discards, the split follows the acted-on side, the selection advances to -the next change, commit keys work there, and a non-conforming pager falls -back to a raw diff at focus time so staging always works. +the next change, commit keys work there, and a non-conforming diff renderer +falls back to a raw diff at focus time so staging always works. Commits: @@ -453,26 +539,29 @@ Commits: `DiffMainViewTypeStaging`; gate re-checked per press; `IsInStack`-guarded `NextInStack` lookup for cheatsheet generation. Ref: 4b54223f4. -9. **Raw-diff fallback for non-conforming pagers + the handshake probe** — - `ProbePagerEmitsDiffMetadata` (empty-input pager run / 7-arg ext-diff - convention on two empty temp files; `OSC1717=V1`; grep for the handshake); - verdict cached per pager signature; `useExternalDiffGitConfig` → always - raw when focused; `DiffMainViewShouldRenderRaw` read by every diff panel's - render-to-main; `ignoreExternalDiff` threaded through the diff-cmd - builders (`--no-ext-diff`, keep color); `types.NewMainViewDiffTask` routes - raw renders through `RunCommandTask` (bypasses `GIT_PAGER`); focus flow +9. **Raw-diff fallback for non-conforming diff renderers + the handshake + probe** — the probe (prototype: `ProbePagerEmitsDiffMetadata`; rename per + §1) runs the renderer on empty input (stdin route via `NewShell`; ext-diff + via git's 7-arg convention on two empty temp files), env `OSC1717=V1`, + greps for the handshake; verdict cached per renderer signature; + `useExternalDiffGitConfig` → always raw when focused; + `DiffMainViewShouldRenderRaw` read by every diff panel's render-to-main; + `ignoreExternalDiff` threaded through the diff-cmd builders + (`--no-ext-diff`, keep color); `types.NewMainViewDiffTask` routes raw + renders through `RunCommandTask` (bypasses `GIT_PAGER`); focus flow installs a restore to place the selection after the raw re-render; - click-to-focus replays the clicked view-line index (best effort). e2e: - `stage_from_main_view_with_unsupported_pager`, - `build`-variant comes with PR 7, `stage_from_main_view_with_conforming_pager` - (fake handshake pager). Refs: 98881fc9e, 17cfd567e, bf18778e9; the probe - detection is N§21.30 (the observe mechanism never lands — §3). + click-to-focus replays the clicked view-line index (best effort). e2e + (rename per PR 3): `stage_from_main_view_with_unsupported_pager`, the + `build`-variant comes with PR 8, + `stage_from_main_view_with_conforming_pager` (fake handshake renderer). + Refs: 98881fc9e, 17cfd567e, bf18778e9; the probe detection is N§21.30 + (the observe mechanism never lands — §3). 10. **Port the remaining prototype staging e2e tests** (whichever aren't already in earlier commits): `stage_hunk/range/range_spanning_files…`, `select_hunk_on_focusing_main_view`, `select_next_*`, `advance_to_next_hunk_after_staging_shifts_line_numbers`, `focus_follows…`/`focus_returns…`, `no_selection…`/`hide_selection…` (if - deferred from PR 4). + deferred from PR 5). Design seam to keep (separate-lists input, §7): the focus-follow decision and the "which side does this pane show" logic must stay **localized** (the @@ -480,12 +569,12 @@ handler + `diffSplitState`), not smeared across call sites — the parked separate-lists design will want to re-derive "side" from list-section membership and may want a different focus-follow rule. -### PR 7 — Build custom patches directly from a commit's diff view +### PR 8 — Build custom patches directly from a commit's diff view After it: `space` over a commit's diff (commit-files, commits, sub-commits, stash, reflog) toggles lines into a custom patch, a checkmark gutter shows -membership, the secondary pane previews the patch through your pager, `d` -removes lines from the commit, and moving/undoing patches keeps your +membership, the secondary pane previews the patch through your diff renderer, +`d` removes lines from the commit, and moving/undoing patches keeps your selection. Commits: @@ -501,7 +590,9 @@ Commits: three call sites after the rename rebase (N§21.36(1)); production looks up the `CommitFile` by path and passes `GetPreviousPath()`, mirroring `toggleForPatch`/`RefreshPatchBuildingPanel`. Refs: e57135979, b4270b7d9 - (accessor half), N§21.36(1). + (accessor half), N§21.36(1). Note master's `33b8d497c` added a mutex to + PatchBuilder (worker `Reset()` vs UI-thread readers) — keep the new + accessors within that locking discipline. 3. **Toggle from the commit-files main view** — `space` routes to the patch toggle (per the panel's `PrimaryAction`); decides add/remove from the first selected line; starts the builder if inactive (discard-confirm when @@ -531,19 +622,19 @@ Commits: (stash, other-branch sub-commits, mid-rebase) and in the secondary pane. e2e: `discard_lines_from_commit_main_view`. Refs: eaec32b2b (commit half), b4270b7d9 (secondary-disable). -7. **The secondary patch pane: correct removal + pager rendering** — +7. **The secondary patch pane: correct removal + renderer-based preview** — removal by **ordinal among shown change lines** (the aggregated patch renumbers additions — line numbers are wrong, N§21.35(1)); render the patch as a real diff: materialize `a/`+`b/` temp trees under lazygit's temp dir (from-side blobs; `git apply` of the patch; added files: empty `a/`, absent `b/`, `PatchToApply(false,false)`), render - `git diff --no-index --no-prefix a b` through the normal pager wiring; - generation counter drives lazy rebuilds. **Open sub-item: verify/handle - renames in the temp-tree rendering** (a renamed file materializes at two - paths; check what `--no-index` shows and whether `--find-renames` is - needed) — resolve during implementation, ask the user if it's ugly. - e2e: `remove_lines_from_main_view_secondary`. Refs: b4270b7d9, e0cde9b88, - 957952566, N§21.35. + `git diff --no-index --no-prefix a b` through the normal diff-renderer + wiring; generation counter drives lazy rebuilds. **Open sub-item: + verify/handle renames in the temp-tree rendering** (a renamed file + materializes at two paths; check what `--no-index` shows and whether + `--find-renames` is needed) — resolve during implementation, ask the user + if it's ugly. e2e: `remove_lines_from_main_view_secondary`. Refs: + b4270b7d9, e0cde9b88, 957952566, N§21.35. 8. **Preserve the selection across commit rewrites** — the command-agnostic net: the four commit-diff panels install an ordinal restore before `RenderToMainViews` when (main view focused + selection shown + no restore @@ -555,15 +646,15 @@ Commits: 9. **Allow changing context size during custom patch building** — ref: 10bb69d80 (read its message for the rationale/constraints). -Deferred, recorded not fixed (N§21.22(4)): a pager *switch* mid-build shifts -the checkmarks until the next refresh (needs a post-render recompute hook). -Note it in the PR description. +Deferred, recorded not fixed (N§21.22(4)): a renderer *switch* mid-build +shifts the checkmarks until the next refresh (needs a post-render recompute +hook). Note it in the PR description. -### PR 8 — Replace the staging and patch-building panels with the focused main view +### PR 9 — Replace the staging and patch-building panels with the focused main view The removal PR. Also the PR whose title tells users the big story — consider making *this* the umbrella release-notes headline ("staging now happens -directly in the diff view") since PRs 6/7 titles already describe the +directly in the diff view") since PRs 7/8 titles already describe the mechanics. Sequencing inside the PR (every commit green): @@ -580,13 +671,13 @@ Sequencing inside the PR (every commit green): panel: `enter` (and double-click on the file row) pushes the focused main view anchored at that file's diff (multi-file/directory diff → anchor at the file's first row via the jump-to-file landing logic). Selection - anchors per PR 4 rules. + anchors per PR 5 rules. 3. **Remove the explorer machinery** — contexts (`Staging`, `StagingSecondary`, `CustomPatchBuilder`), their views/windows in `context/setup.go` and layout, `StagingController`, `PatchBuildingController` (explorer half), `patch_exploring` package, `RefreshStagingPanel`/`RefreshPatchBuildingPanel` (keep/rewire the - *secondary patch panel* update path — PR 7's pager-rendered preview stays, + *secondary patch panel* update path — PR 8's renderer-based preview stays, fed by `secondaryPatchPanelUpdateOpts`), escape/`EscapeFromPatchExplorer` remnants, `IPatchExplorerContext`. Multiple commits: this is the risky demolition — go subsystem by subsystem. @@ -604,9 +695,9 @@ Risk note: this PR is where hidden couplings surface (things that push commands, `git bisect` edge flows). Grep for every reference to the removed contexts/views before starting; expect a long tail of small fixes. -### PR 9 — Alt- or shift-click a diff line to open it in your editor +### PR 10 — Alt- or shift-click a diff line to open it in your editor -Self-contained; after PR 3 (uses `GetDiffLineInfo`). Commits (N§19): +Self-contained; after PR 4 (uses `GetDiffLineInfo`). Commits (N§19): 1. **gocui: let a mouse binding opt into firing while a popup is focused** (`HandleWhenPopupPanelFocused`). Ref: ac85a90ed. @@ -622,9 +713,9 @@ Self-contained; after PR 3 (uses `GetDiffLineInfo`). Commits (N§19): Interactive sign-off: Ghostty, iTerm2, VS Code (already done once for the prototype; re-confirm the transcription). -### PR 10 — Open the selected diff line in the branch's GitHub PR +### PR 11 — Open the selected diff line in the branch's GitHub PR -Self-contained; after PR 4. One or two commits (N§5): +Self-contained; after PR 5. One or two commits (N§5): - `openPullRequestForSelectedLine` on `Commits.OpenPullRequestInBrowser` in the focused main view: URL `/changes/#diff-R`; @@ -640,21 +731,21 @@ Self-contained; after PR 4. One or two commits (N§5): ## 6. Interactive sign-off matrix -The headless harness cannot run real pagers, `LAZYGIT_SLOW_RENDER`, or the -pty path (N§13.1), and the gutter is draw-time-only. Each PR needs a user -pass before merge: +The headless harness cannot run real diff renderers, `LAZYGIT_SLOW_RENDER`, +or the pty path (N§13.1), and the gutter is draw-time-only. Each PR needs a +user pass before merge: | PR | What to verify interactively | |---|---| | 1 | Slow-render matrix (N§11/§13): flick commits/files scrolled down; 10 s auto-refresh (`refreshInterval: 3`) — no content/scrollbar flicker; **also re-test at normal speed** (N§20.5) | -| 3 | Patched delta/difftastic/diff-so-fancy emit + render cleanly; handshake swallowed (no phantom line) | -| 4 | Selection feel under delta (narrowSelectionHighlight); hunk-on-click; drag; nav under metadata delta incl. repeated `n` across files | -| 5 | `{`/`}` and pager-cycle scrolled down: no top-jump, offset preserved, both anchor cases; ext-diff pager route (difftastic) | -| 6 | Full staging matrix under no-pager / patched delta (unified + SxS) / difftastic; cross-pane focus-follow; raw fallback feel under stock delta / diff-so-fancy-without-metadata; binary-file focus stability (N§21.30 repro) | -| 7 | Gutter under delta/no-pager/difftastic; whole-commit path on LocalCommits (canRebase menu); secondary pane rendering per pager; metadata path resolves secondary removals to the right file (a/b masquerade — N§21.35 caveat) | -| 9 | Ghostty, iTerm2, VS Code | +| 4 | Patched delta/difftastic/diff-so-fancy emit + render cleanly; handshake swallowed (no phantom line) | +| 5 | Selection feel under delta (narrowSelectionHighlight); hunk-on-click; drag; nav under metadata delta incl. repeated `n` across files | +| 6 | `{`/`}` and renderer-cycle scrolled down: no top-jump, offset preserved, both anchor cases; ext-diff route (difftastic) | +| 7 | Full staging matrix under no-renderer / patched delta (unified + SxS) / difftastic; cross-pane focus-follow; raw fallback feel under stock delta / diff-so-fancy-without-metadata; binary-file focus stability (N§21.30 repro) | +| 8 | Gutter under delta/no-renderer/difftastic; whole-commit path on LocalCommits (canRebase menu); secondary pane preview per renderer; metadata path resolves secondary removals to the right file (a/b masquerade — N§21.35 caveat) | +| 10 | Ghostty, iTerm2, VS Code | -Patched pager builds: `cargo build` in delta/difftastic worktrees +Patched renderer builds: `cargo build` in delta/difftastic worktrees (`osc-1717-metadata` branches); diff-so-fancy is a script. ## 7. Compatibility with the parked separate-lists design @@ -684,48 +775,52 @@ review with the user when the relevant PR starts: | Gap | Disposition | |---|---| -| Rename support in the from-main-view patch paths (N§21.36(1)) | **Fix in PR 7 commit 2** (mandatory — regression vs master otherwise) | +| Rename support in the from-main-view patch paths (N§21.36(1)) | **Fix in PR 8 commit 2** (mandatory — regression vs master otherwise) | | patch pkg rename-aware Parse/Transform/FormatView (N§21.36(2)) | **Fix in PR 2 commit 1** (mandatory; `renamed_file_whole` guards it) | -| Reflog patch-building (N§21.24) | **Fix in PR 7 commit 5** | -| Renames in the custom-patch temp trees (new, this plan) | Resolve during PR 7 commit 7 | -| Diffing mode (`W`) not wired to the raw fallback → not stageable (N§21.29) | Defer; note in PR 6 description ("diffing-mode staging is its own question") | +| Reflog patch-building (N§21.24) | **Fix in PR 8 commit 5** | +| Renames in the custom-patch temp trees (new, this plan) | Resolve during PR 8 commit 7 | +| Diffing mode (`W`) not wired to the raw fallback → not stageable (N§21.29) | Defer; note in PR 7 description ("diffing-mode staging is its own question") | | `useExternalDiffGitConfig` always-raw when focused (N§21.30) | Keep; document | | Per-pane selection memory on `` (re-anchors each switch, N§21.9) | Defer; follow-up candidate | | `IsSingleHunkForWholeFile` hunk-default refinement (N§21.11) | Defer; follow-up candidate | -| `a` on a context line below the last hunk doesn't snap back like staging did (N§21.11) | Fix cheaply in PR 4 commit 3 if trivial (`ChangeBlockBounds` falls back to the block above); else defer | -| Deleted-file `MD`-vs-`D` staging special case (N§21.13) | Defer; record as follow-up in PR 6 description | -| `NormalSecondary` not preserved on `-U`/pager change (N§16.1) | Keep as documented limitation | +| `a` on a context line below the last hunk doesn't snap back like staging did (N§21.11) | Fix cheaply in PR 5 commit 3 if trivial (`ChangeBlockBounds` falls back to the block above); else defer | +| Deleted-file `MD`-vs-`D` staging special case (N§21.13) | Defer; record as follow-up in PR 7 description | +| `NormalSecondary` not preserved on `-U`/renderer change (N§16.1) | Keep as documented limitation | | Gutter marks for not-yet-loaded lines of huge diffs (N§21.20) | Keep (marks appear on next recompute); note | -| Pager switch mid-patch-build shifts checkmarks (N§21.22(4)) | Defer; note in PR 7 | -| Copy: metadata-typed prefix stripping under column-preserving pagers (N§21.28) | Defer | +| Renderer switch mid-patch-build shifts checkmarks (N§21.22(4)) | Defer; note in PR 8 | +| Copy: metadata-typed prefix stripping under column-preserving renderers (N§21.28) | Defer | | Nav only sees loaded content (deep targets in huge diffs, N§16.4) | Keep; note (openSearch-style ReadToEnd is the known shape if wanted) | | Toggle auto-advance: no "skip already-included" smarts (N§21.35) | Keep plain next-hunk | | difftastic token-vs-line `c`-at-new-line mismatch (M§10.2) | Protocol v2 candidate; nothing to do host-side | ## 9. Open questions (resolve before/during the marked PR) -1. **PR 4:** proper keybinding config entries for `n`/`N`/`f`? (lean: yes) -2. **PR 8:** new names for `useHunkModeInStagingView` / `wrapLinesInStagingView` +1. **PR 3:** does the per-entry `pager:` config field keep its name (it + selects the GIT_PAGER invocation route, vs `externalDiffCommand`), or is + it renamed too? +2. **PR 5:** proper keybinding config entries for `n`/`N`/`f`? (lean: yes) +3. **PR 9:** new names for `useHunkModeInStagingView` / `wrapLinesInStagingView` + config migration. -3. **PR 7:** renames in the custom-patch temp-tree rendering (see §8). -4. **PR titles**: drafts in §4 — the user finalizes wording at PR-open time +4. **PR 8:** renames in the custom-patch temp-tree rendering (see §8). +5. **PR titles**: drafts in §4 — the user finalizes wording at PR-open time (they're the release-notes lines). -5. **Cross-repo timing** (outside this plan): circulating the OSC 1717 spec, - upstreaming the three pager patches. lazygit ships fully functional - without them; revisit pitching once PRs 1–7 exist as evidence. +6. **Cross-repo timing** (outside this plan): circulating the OSC 1717 spec, + upstreaming the three renderer patches. lazygit ships fully functional + without them; revisit pitching once PRs 1–8 exist as evidence. ## 10. Progress - [ ] PR 1 — async render fixes - [ ] PR 2 — diff-line identity primitive -- [ ] PR 3 — OSC 1717 support -- [ ] PR 4 — selection & navigation -- [ ] PR 5 — position preserve -- [ ] PR 6 — staging from the main view -- [ ] PR 7 — custom patches from the main view -- [ ] PR 8 — panel removal -- [ ] PR 9 — alt/shift-click edit -- [ ] PR 10 — open PR at line +- [ ] PR 3 — rename pagers → diff renderers +- [ ] PR 4 — OSC 1717 support +- [ ] PR 5 — selection & navigation +- [ ] PR 6 — position preserve +- [ ] PR 7 — staging from the main view +- [ ] PR 8 — custom patches from the main view +- [ ] PR 9 — panel removal +- [ ] PR 10 — alt/shift-click edit +- [ ] PR 11 — open PR at line (Add per-commit checkboxes inside each PR section as work starts; record deviations from this plan inline, dated.)