Prototype: Fold the staging functionality into the main view#5732
Draft
stefanhaller wants to merge 177 commits into
Draft
Prototype: Fold the staging functionality into the main view#5732stefanhaller wants to merge 177 commits into
stefanhaller wants to merge 177 commits into
Conversation
This was referenced Jun 26, 2026
|
I'm about 8 minutes into your demo video and I'm already very excited. Conceptually it seems simple enough. I just need to add some "hidden" escape codes into the d-s-f output to mark changed lines? |
I don't know enough about OSC codes to have a strong opinion on this. But 1717 seems fine to me.
This seems fine to me I think.
Not sure I have enough information to say on this.
I believe so. I'd definitely like to work closer with you to beta-test this feature as there are a lot of technical details I don't 100% follow quite yet.
I don't think so. d-s-f parses line by line, and retains header information so I should be able to call back to the last header when I encounter a line change. |
|
Having read the spec document and watched your video can I get clarification on a couple of things?
diff --git a/LICENSE b/LICENSE
index eb26539..028c6af 100644
--- a/LICENSE
+++ b/LICENSE
@@ -5,17 +5,18 @@ Copyright (c) 2016 So Fancy team
Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
-to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:
The above copyright notice and this permission notice shall be included in
all copies or substantial portions of the Software.
+FOOBAR
THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
-LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM;
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
THE SOFTWARE.Help me understand the fields for the deleted line. Would it be: For the added line. Would it be: In section 4.3 it says "context, new line 10" but I think it would be more clear as "context, line 10" because it's not new. |
|
Possible minor hiccup? While attempting to implement the basics of this I came across a minor problem: The default mode with d-s-f it to pipe through less as a pager, and it appears |
Collaborator
Author
I must not have made that clear enough: you don't have to do that, I did already. I just didn't post a (draft) PR yet because I first wanted to get feedback on the spec; but the changes are ready (apart from the necessary changes to the docs, if any, and to the changelog etc.). My branch is here if you want to try it.
That's too bad, but it's not a problem, because d-s-f will only emit the sequences when it is running inside lazygit. I'm taking your other questions over to #5731, that's where the spec should be discussed. |
|
Oh man... you didn't tell me you had most of the work done in d-s-f. You should have led with that. Your code looks pretty sane to me. Well done. |
Re-rendering a diff into a main view is asynchronous and lazy: the read loop fills the view a screenful at a time and refreshes as it goes. When debugging scroll-restore and flicker behaviour, the individual frames go by too fast to see. Setting LAZYGIT_SLOW_RENDER=<milliseconds> sleeps that long after each line is written, stretching the load out so the frames become visible. It has no effect when unset, so it's safe to leave in. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…building This was already possible, but only when a file was selected, and it woudln't always land on the right line when a pager was used. Now it's also possible to do this for directories, and it jumps to the right line. At the moment this is a hack that relies on delta's hyperlinks, so it only works on lines that have hyperlinks (added and context). The implementation is very hacky for other reasons too (e.g. the addition of the weirdly named ClickedViewRealLineIdx to OnFocusOpts).
… clicked line This involves first switching to the commit files view, and then entering the clicked file from there.
…ll the way back out I *think* I like it better this way, but it needs more testing.
…ction Instead of a user config that always shows a selection on focus, the focused main view starts without a selection (matching master). Pressing <space> shows a selection in the middle of the view (no scrolling); pressing <esc> hides it again before falling through to exiting the view.
HyperLinkInLine read v.lines/v.viewLines without holding writeMutex, so it could race a concurrent re-render rebuilding the buffer. It also indexed v.lines by viewLines[y].linesY after only checking y against len(viewLines); since refreshViewLinesIfNeeded overwrites viewLines in place without truncating, the tail can hold stale entries pointing past a shrunk v.lines, giving an out-of-range panic while a shorter diff is still loading. Take writeMutex (as the sibling view methods do) and bounds-check linesY against len(v.lines), returning "no link" rather than panicking. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The layout scrolls a view up if its origin is past the bottom of its content, to avoid showing blank space (e.g. after a resize). But it measures content height by the lines loaded so far, and command/pty tasks load asynchronously. So when a view is re-rendered while scrolled down, the layout would yank it to the top because only a fraction of the content has been read yet, then leave it there once loading finished. Track whether a command task is actively reading (set synchronously when the task is created, so a layout pass in between sees it; cleared at EOF, but not when stopped, since that means a newer task is taking over) and skip the scroll-up clamp for such views. onEndOfInput already re-clamps once loading completes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A task's read loop processes one LinesToRead request at a time. The initial request has a large line count and no Then callback; if the content is shorter than that, the loop hits EOF on the initial request and breaks out, abandoning any further requests still sitting in the readLines channel. So a ReadToEnd call that races a still-loading-but-shorter-than-its-initial-read view has its Then silently dropped: it isn't fired immediately (the channel was non-nil at call time) and it's never dequeued. On EOF, drain the queued requests and fire their Then callbacks before breaking out, since reaching EOF trivially satisfies any "read more" request. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Record the third pager emitter (diff-so-fancy, commit c397cd6 on its prototype-osc-metadata branch): the simplest of the three -- a line-oriented Perl filter, unified single-column only, the same #2 category as delta's default. Captures the diff-so-fancy-specific findings: sanitize_display strips OSC so the record is prepended not embedded; the path is derived from $file_1/$file_2 (not $last_file_seen, which is empty for a noprefix deletion); combined diffs skipped; counters are file-scope globals so a chunk-spanning hunk keeps counting; classification mirrors strip_leading_indicators so the no-newline marker is correctly skipped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
§10 now lists diff-so-fancy alongside delta and difftastic: the same #2 case as delta's default (strips +/- markers, conveys side by color) but a line-oriented Perl filter, unified-only, and -- the one wrinkle worth stating in the spec -- it strips terminal escapes from its content, so the record is prepended to the line rather than embedded. Fix the now-stale "all three"/"both pagers" counts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Focusing the main view by keyboard already selects a whole change block in hunk mode; a click, though, only ever placed a single-line selection, so the common "click the diff to stage this block" gesture still needed a follow-up `a`. Now a click on a change line selects that line's block too. A click on context still selects just that line, since the click points at it precisely (e.g. to edit it with `e`). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Once in hunk mode, clicking another change line reset the selection to a single line, so you had to press `a` again for each block you wanted to stage (a behaviour the main view inherited from the staging and patch- building panels). Preserve hunk mode across clicks instead: a click on a change line re-selects that whole block, while a click on context — or any click when we weren't in hunk mode — drops to a single line, where the click points precisely (e.g. to edit it). The two click handlers shared an identical selection body, so unify them into one helper and make the change in a single place. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Clicking the staged/unstaged pane you weren't focused on selected a single line the first time, even in hunk mode, because that pane's select mode was still its default until it had been focused once (tabbing to it and back was the workaround). Seed the clicked pane's mode from the one we're leaving so the very first click there behaves like every later one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ange When the diff re-renders for a new pager (or a changed context size), PreserveDiffPositionOnRerender restored the cursor by patch identity but left the selection's other end — the range anchor — pinned to its old view line. A pager that restructures the diff (delta side-by-side is the clearest case) then left the selection spanning the wrong range of patch lines. Remember the far end by patch identity too and put it back the same way, so the selection covers the same lines however the new pager lays them out. One mechanism now restores both ends, covering hunk and range selections alike; a single-line selection still needs only the cursor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Click-and-drag in the focused main view turns a hunk selection into a range, but it was anchored at the change block's far end — where selecting a hunk leaves the range anchor — rather than the clicked line. And on a context line, where the click leaves no range anchor at all, dragging just moved the single selected line instead of opening a range. Remember the line each mouse-down lands on (the click can show a whole hunk, so it can't be read back from the view) and, as the drag proceeds, anchor the range there while the cursor end follows the mouse as gocui already moves it. Works for a click that focuses the view and for one while it's already focused, on change and context lines alike. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When the mouse first moves after a left-button press, the tcell driver transitions MAYBE_DRAGGING -> DRAGGING but emitted that event with the default key (MouseRelease) rather than as a drag. The cursor still moved, so MouseLeft+ModMotion drag handlers didn't fire until the *second* moved-to line — the first line of every drag was mishandled. In the focused main view, where a click can leave the range anchor at a hunk's far end, that showed as a one-line range from the block end before the drag re-anchored at the click. Mark the first movement as a drag like every later one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…21.32) The driver fix is a no-op for the old staging panel, not a tightening: that panel anchors its drag at the click on mouse-down, so it never exposed the bug. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SHAs go stale on every history rewrite of this throwaway branch; describe commits by what they did instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Dropping a hunk with `d` re-establishes the focused main view's selection on the next surviving change, which feels great. But other operations that rewrite the commit under the focused main view — moving a custom patch out into the index, undoing right after a discard or a patch move — don't run through the focused-main-view action handlers, so nothing was preserving the selection. The stale gocui selection was left painted over the new content, often as a large, now-meaningless range. Rather than teach every such command to capture and restore the selection (move-patch, undo, redo, and any future one), the focused main view now preserves it itself, by its change-line ordinal, as the diff re-renders — the command-agnostic counterpart of revealSelectionAfterPrimaryAction. The diff side panels call it from their render-to-main before triggering the render, so the restore rides the re-render. It stands down unless the focused main view is current and shows a selection, no precise restore is already pending (escape / post-stage reveal / context-size place the selection more precisely), and the diff command is actually changing. That last gate matters: a plain background refresh re-renders the same commit's diff unchanged, and there the selection — range and all — must be left alone; only a command change (e.g. a rebase rewriting the commit's hash) means the content moved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Discarding lines from a commit via the focused main view installed its own revealSelectionAfterPrimaryAction before the rebase. That rebase rewrites the commit, so the selection-preserve net now re-establishes the selection as the diff re-renders — with the same anchor (the selection's first line), making this install redundant. Files-discard keeps its own reveal: its diff command is stable, so the net never fires there, and it has staging's focus-follow to the secondary pane besides. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…, disable its discard The focused-main-view rework made the secondary pane actionable, but the old patch-building explorer's secondary was inert, so its actions were never thought through. Pressing space there routed through the same toggle handler as the main pane, resolving the selection against the secondary's diff and mapping it to patch-builder indices by line number. But the secondary shows the *aggregated* custom patch, which renumbers included additions whenever an earlier addition in the same hunk is excluded (Transform recomputes each hunk's +start). So the shifted number resolved to the wrong line in the original diff — often adding an unrelated line instead of removing the selected one. Resolve the secondary selection by its *ordinal* among the change lines shown instead: the custom-patch view renders exactly the included change lines in order, so the k-th change line of a file is that file's k-th included change line (PatchBuilder.IncludedChangeLineIndices), independent of the renumbering. Space in the secondary now only ever removes, mirroring how space in the staging view's staged pane unstages. Discarding from the commit (the remove key) makes no sense in the custom-patch preview — it would act on lines shown only as the patch, and space already removes them — so it's disabled there. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…git diff The secondary pane showed the custom patch with a bespoke in-memory render (PatchBuilder.RenderAggregatedPatch / FormatView). That had two problems: it could never be fed to a pager the way the main view's diff is (a stdin pager might have worked, but an external diff tool like difftastic, which diffs two files rather than a unified diff, could not), and its hand-rolled hunk/context handling differed subtly from git's. Materialize the patch instead as two real file trees under a temp dir — a/ holds each patched file's "from"-side content, b/ that content with the patch applied — and render it with `git diff --no-index`, reusing the exact pager wiring the main view uses (stdin pager, external diff, or git's own colour as the raw fallback). `--no-index` honors both GIT_PAGER and --ext-diff, so every pager type now renders the custom patch like any other diff, and git computes the context, fixing the quirks. Because the secondary is now an async diff task, the post-removal selection reveal (which rides a task's restore) finally takes effect. The trees are named a/b so that with --no-prefix the diff shows the real repo-relative paths; added files are seeded empty in a/ so they pair up and show their real paths rather than git's directory-comparison "added in b" form. The patch builder owns the temp dir's lifetime (created on Start, removed on Reset) and bumps a generation counter on every change, so the trees are rebuilt only when the patch actually changes — covering the focused-main view and the old explorer alike, without rebuilding on mere navigation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rendering (§21.35)
Use osCommand.GetTempDir() (lazygit's own per-session temp dir, which it creates and cleans up) as the parent for the custom-patch diff trees, instead of the OS default — so it honors the configured temp dir and is cleaned up with the rest of lazygit's temp files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Staging a hunk from the focused main view advances the selection to the next hunk, because staging removes the acted-on lines from the diff so the preserved change-line ordinal lands on the next change. Toggling a hunk into a custom patch left the selection sitting on the just-toggled hunk instead — the toggle doesn't change the diff (only the inclusion set), so the same ordinal lands back where it was, and you had to navigate by hand to build a patch hunk by hunk. Give the toggle the same feel by advancing the reveal ordinal past the toggled change lines (RevealSelectionAfterStaging gains an advanceBy arg, fed the toggled change-line count). Staging and removal still pass 0 (their lines are consumed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cused The inclusion gutter (the ✓ markers on the commit diff showing which lines are in the custom patch) is painted on the Normal pane but is an affordance of the whole focused-main-view pair. It was gated on the Normal pane specifically being current, so tabbing to the secondary (custom-patch) pane hid it — even though both panes are visible and you're still building the patch. Show it whenever either pane of the focused main view holds focus, finding the side panel beneath whichever pane is current. GetOnFocusLost now re-evaluates the gutter (rather than unconditionally hiding) so it persists across a pane switch but still hides when focus leaves the pair; the new context is already current by then, so it decides correctly and doesn't flicker. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
stefanhaller
force-pushed
the
fold-staging-functionality-into-main-view
branch
from
f37084b to
ecdd436
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a prototype of a major change to how staging works:
In order to illustrate this better I made a video to demo it. Unfortunately it got a little longer than I had planned, I'm not experienced in producing content like this.
Prototype branches for the three mentioned pagers are here: delta, diff-so-fancy, difftastic.
For feedback on the OSC spec, please comment on #5731, not here.