diff --git a/AGENTS.md b/AGENTS.md index 9c3829af1..ea4374c53 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -63,7 +63,9 @@ Prefer a fine-grained commit history. Commits should be as small as possible while still being meaningful and self-contained. - **Every commit must compile and pass all tests.** No "WIP" commits, no - commits that leave the tree broken and rely on a follow-up to fix it. + commits that leave the tree broken and rely on a follow-up to fix it. A + `fixup!` is not such a follow-up; see "Iterate with `fixup!` commits" for + what one may leave broken until it is folded in. - **Every commit must be `gofumpt`-formatted.** Run `just format` before committing. - **Every commit must be lint-clean.** Run `just lint` before committing — @@ -134,6 +136,32 @@ target, make the change, `git commit --fixup=`, then branch. The fixup stays a separate, reviewable commit; only its position changes. +**A fixup may leave commits before it broken until it is folded in.** If a +`fixup!` on an early commit deletes something that a later commit still uses, +the later commit doesn't build until its own `fixup!`, right behind it, catches +up; the same goes for lint. That is expected. The rules above about every +commit compiling, testing and linting clean describe the history _after_ +autosquash, and I fold fixups in soon after reviewing them. Never amend a +commit directly, or edit the commits between two fixups, to keep every commit +of the un-squashed history green. The reviewable fixup is worth more than a +green intermediate state. Verify at each fixup instead, since the tree there +is what the folded-in history will have at that point, and say in the handoff +which commits stay broken until which fixup. + +**After a mid-stack fixup, check every branch tip above it, not just the stack +tip.** A fixup that deletes or renames something rewrites every commit replayed +above it, and a commit further up can hide the damage at the tip. A helper +whose last caller the fixup deleted is flagged as unused by `just lint` at the +tip of its own PR, but a later PR that calls it again makes the stack tip lint +clean. Each PR is reviewed and merged on its own, so each PR branch tip has to +be green on its own. After the replay, run `just build`, `just unit-test` and +`just lint` at each branch tip from the insertion point up. If the fixup deleted +or renamed a symbol, also build every replayed commit, for example with +`git -c rebase.autosquash=false rebase -x 'go build ./...' `; +unchanged commits are fast-forwarded, so their hashes stay, and the commits a +fixup is expected to leave broken stop it, so `git rebase --continue` past +those. + If the changes don't map cleanly onto existing commits — say they cut across several of them, or restructure something at a different layer than any existing commit naturally owns — stop and ask the user how to @@ -180,7 +208,7 @@ looks messy. The whole point of a fixup is that the iteration stays **visible and reviewable**; squashing it away yourself destroys exactly the artifact it exists to create. Collapsing fixups into their targets is the user's action, taken once they've reviewed the iterations. Every mention of -`--autosquash` in this section describes what the *user* will eventually +`--autosquash` in this section describes what the _user_ will eventually run, never a step for you to perform. If you think the history is ready to collapse, say so and leave it to them. @@ -320,7 +348,7 @@ refactor to an earlier commit (but don't do it without asking first). ## Don't read model state right after a `Refresh` A `Refresh` (or `RefreshFromWorker`) does its git work on a worker and then -*enqueues* the model update onto the UI thread. So when `Refresh` returns, the +_enqueues_ the model update onto the UI thread. So when `Refresh` returns, the model is **not** updated yet — the write is still queued. Reading a field synchronously right after refreshing its scope reads the stale, pre-refresh value (and this is true even for SYNC refreshes): @@ -406,7 +434,7 @@ column. Applies only to `pkg/i18n/english.go`. ## Code comments are for future readers, not development history -Comments in source code explain *why this code is shaped the way it is*. They +Comments in source code explain _why this code is shaped the way it is_. They are not the place to narrate the path we took during development — what was tried first, what didn't work, what's "more reliable" or "cleaner" than some alternative. That framing is interesting in the moment, but it's noise to @@ -422,7 +450,7 @@ Avoid phrasings like: - "X rather than Y", where Y is what the code did before the change The iteration story is sometimes worth preserving — but it belongs in the -commit message, which is the durable record of *why this change was made*. The +commit message, which is the durable record of _why this change was made_. The code comment should make sense to someone who has never seen any prior version and is just trying to understand the file as it currently exists. @@ -475,7 +503,7 @@ So: struct, run `just generate` and include the regenerated `docs-master/Config.md` (and `schema-master/config.json`) in your commit. - Don't hard-wrap the doc comments on `userConfig` fields. This applies - *only* to `userConfig`, because those comments are fed through the doc + _only_ to `userConfig`, because those comments are fed through the doc generator; comments on every other struct follow the normal Go wrapping conventions. For `userConfig` fields, write each sentence (or paragraph) as a single unwrapped line, however long — the generator re-wraps them for diff --git a/pkg/gocui/view.go b/pkg/gocui/view.go index 6f2ef869f..e0fdb4357 100644 --- a/pkg/gocui/view.go +++ b/pkg/gocui/view.go @@ -637,6 +637,13 @@ func (v *View) CancelRangeSelect() { v.rangeSelectStartY = -1 } +// HasRangeSelect reports whether a range selection is anchored, as opposed to the +// view showing a plain cursor. A range whose ends are on the same view line is still +// one, which SelectedLineRange alone can't tell you. +func (v *View) HasRangeSelect() bool { + return v.rangeSelectStartY != -1 +} + func calculateNewOrigin(selectedLine int, oldOrigin int, lineCount int, viewHeight int) int { if viewHeight >= lineCount { return 0 @@ -1996,9 +2003,60 @@ func (v *View) DiffLineContents() []DiffLineContent { v.writeMutex.Lock() defer v.writeMutex.Unlock() - contents := make([]DiffLineContent, len(v.buf.lines)) - for i := range v.buf.lines { - line := &v.buf.lines[i] + return diffLineContentsFrom(v.buf, 0) +} + +// OffscreenDiffLineContents is DiffLineContents for the content of a re-render in +// progress (see BeginOffscreenRender). A reader deciding where the new content +// should be shown has to work from this: it has to answer before the swap, since +// after the swap the content is already on screen. Returns nil when no re-render +// is underway. +func (v *View) OffscreenDiffLineContents() []DiffLineContent { + v.writeMutex.Lock() + defer v.writeMutex.Unlock() + + if v.offscreen == nil { + return nil + } + return diffLineContentsFrom(v.offscreen, 0) +} + +// OffscreenDiffLineContentsFrom is OffscreenDiffLineContents restricted to the lines +// from index `from` on (so result[0] is buffer line `from`). It lets a reader that +// follows a re-render as it loads look at each line once, rather than snapshotting +// the whole buffer again on every line — the difference between an O(n) and an O(n²) +// scan of a large diff. Returns nil when no re-render is underway, or when `from` is +// past the lines read so far. +func (v *View) OffscreenDiffLineContentsFrom(from int) []DiffLineContent { + v.writeMutex.Lock() + defer v.writeMutex.Unlock() + + if v.offscreen == nil || from < 0 || from >= len(v.offscreen.lines) { + return nil + } + return diffLineContentsFrom(v.offscreen, from) +} + +// OffscreenLineCount returns the number of unwrapped lines a re-render in progress +// has read so far, or 0 when none is underway. It tells a reader waiting for a +// particular line, cheaply, when a screenful below it has arrived too — so that the +// swap shows that line with content under it rather than at the bottom edge of a +// half-filled view. +func (v *View) OffscreenLineCount() int { + v.writeMutex.Lock() + defer v.writeMutex.Unlock() + + if v.offscreen == nil { + return 0 + } + return len(v.offscreen.lines) +} + +func diffLineContentsFrom(buf *viewBuffer, from int) []DiffLineContent { + lines := buf.lines[from:] + contents := make([]DiffLineContent, len(lines)) + for i := range lines { + line := &lines[i] var metadata []string for _, c := range line.cells { if c.metadata != "" && !slices.Contains(metadata, c.metadata) { @@ -2288,6 +2346,11 @@ func (v *View) SelectedLineIdx() int { return seletedLineIdx } +// IsLineVisible reports whether the given view line is one of those on screen. +func (v *View) IsLineVisible(viewLine int) bool { + return viewLine >= v.OriginY() && viewLine < v.OriginY()+v.InnerHeight() +} + // MiddleVisibleLineIdx returns the view line halfway down the visible content. It // stands in for a cursor in a view that has none: of the lines on screen, the one in // the middle is the likeliest to be the one being read. diff --git a/pkg/gui/controllers.go b/pkg/gui/controllers.go index 8668277c6..b7afdf918 100644 --- a/pkg/gui/controllers.go +++ b/pkg/gui/controllers.go @@ -67,8 +67,8 @@ func (gui *Gui) resetHelpersAndControllers() { worktreeHelper, searchHelper, ) - diffHelper := helpers.NewDiffHelper(helperCommon) diffLineHelper := helpers.NewDiffLineHelper(helperCommon) + diffHelper := helpers.NewDiffHelper(helperCommon, diffLineHelper) cherryPickHelper := helpers.NewCherryPickHelper( helperCommon, rebaseHelper, diff --git a/pkg/gui/controllers/context_lines_controller.go b/pkg/gui/controllers/context_lines_controller.go index 022364c07..f7077b299 100644 --- a/pkg/gui/controllers/context_lines_controller.go +++ b/pkg/gui/controllers/context_lines_controller.go @@ -83,6 +83,11 @@ func (self *ContextLinesController) applyChange() error { case context.STAGING_MAIN_CONTEXT_KEY, context.STAGING_SECONDARY_CONTEXT_KEY: self.c.Refresh(types.RefreshOptions{Scope: []types.RefreshableView{types.STAGING}}) default: + // The diff is about to be rendered again with more or less context around + // each change, which reads as the lines you were looking at moving up or down + // the view; keep them where they are instead. + self.c.Helpers().DiffLine.PreserveDiffPositionOnRerender(self.c.Contexts().Normal.GetView()) + self.c.Helpers().DiffLine.PreserveDiffPositionOnRerender(self.c.Contexts().NormalSecondary.GetView()) currentContext.HandleRenderToMain() } return nil diff --git a/pkg/gui/controllers/helpers/diff_helper.go b/pkg/gui/controllers/helpers/diff_helper.go index 02d8f005c..0547fafa3 100644 --- a/pkg/gui/controllers/helpers/diff_helper.go +++ b/pkg/gui/controllers/helpers/diff_helper.go @@ -14,12 +14,14 @@ import ( ) type DiffHelper struct { - c *HelperCommon + c *HelperCommon + diffLineHelper *DiffLineHelper } -func NewDiffHelper(c *HelperCommon) *DiffHelper { +func NewDiffHelper(c *HelperCommon, diffLineHelper *DiffLineHelper) *DiffHelper { return &DiffHelper{ - c: c, + c: c, + diffLineHelper: diffLineHelper, } } @@ -107,6 +109,11 @@ func (self *DiffHelper) RenderToMainAgain() { if currentSide.GetKey() == currentKey || currentKey == context.NORMAL_MAIN_CONTEXT_KEY || currentKey == context.NORMAL_SECONDARY_CONTEXT_KEY { + // Whatever changed can make the diff come out differently, such as a new + // renderer laying it out its own way, so the line you were looking at could + // end up anywhere in the view; keep it in front of you. + self.diffLineHelper.PreserveDiffPositionOnRerender(self.c.Contexts().Normal.GetView()) + self.diffLineHelper.PreserveDiffPositionOnRerender(self.c.Contexts().NormalSecondary.GetView()) currentSide.HandleRenderToMain() } } diff --git a/pkg/gui/controllers/helpers/diff_line_helper.go b/pkg/gui/controllers/helpers/diff_line_helper.go index 212cbff0b..b648ce207 100644 --- a/pkg/gui/controllers/helpers/diff_line_helper.go +++ b/pkg/gui/controllers/helpers/diff_line_helper.go @@ -32,48 +32,58 @@ func NewDiffLineHelper(c *HelperCommon) *DiffLineHelper { // ok is false when the row's identity can't be recovered, in which case the // caller must not act on the line at all. func (self *DiffLineHelper) GetDiffLineInfo(view *gocui.View, viewLineIdx int) (types.DiffLineInfo, bool) { + identities, ok := self.diffLineIdentitiesAt(view, viewLineIdx) + if !ok { + return types.DiffLineInfo{}, false + } + return identities[0], true +} + +// diffLineIdentitiesAt recovers every diff line the row at the given (wrapped) view +// line shows, left to right. It is GetDiffLineInfo's form for a reader that can't +// settle for the line the row leads with: an end of a selection covers its whole +// row, so where a rendering puts a modification's two halves side by side it covers +// both of them. ok is false when the row's identity can't be recovered at all. +func (self *DiffLineHelper) diffLineIdentitiesAt( + view *gocui.View, viewLineIdx int, +) ([]types.DiffLineInfo, bool) { // The cursor and clicks land on a view line, which counts wrapped segments; // the contents are indexed by unwrapped buffer line. bufferLineIdx, ok := view.BufferLineForViewLine(viewLineIdx) if !ok { - return types.DiffLineInfo{}, false + return nil, false } contents := view.DiffLineContents() if bufferLineIdx >= len(contents) { - return types.DiffLineInfo{}, false + return nil, false } if renderingStatesDiffLines(contents) { - if info, ok := self.diffLineInfoFromRecords(contents[bufferLineIdx].Metadata); ok { - return info, true + if identities := self.diffLineIdentitiesFromRecords(contents[bufferLineIdx].Metadata); len(identities) > 0 { + return identities, true } - return types.DiffLineInfo{}, false + return nil, false } parsed, ok := parseDiffLineFromBuffer(diffLineTexts(contents), bufferLineIdx) if !ok { - return types.DiffLineInfo{}, false + return nil, false } - return self.diffLineInfo(parsed), true + return []types.DiffLineInfo{self.diffLineInfo(parsed)}, true } -// diffLineInfoFromRecords recovers a row's identity from the records the diff -// renderer stated for it. ok is false when the row carries no record we understand. -// -// A row can carry more than one record, when the rendering puts two diff lines on it -// (a side-by-side row shows a deletion and the addition replacing it); the leftmost -// is the one a reader would call the row's own, so it is the row's identity. -func (self *DiffLineHelper) diffLineInfoFromRecords(metadata []string) (types.DiffLineInfo, bool) { - if len(metadata) == 0 { - return types.DiffLineInfo{}, false - } - parsed, ok := parseDiffLineMetadata(metadata[0]) - if !ok { - return types.DiffLineInfo{}, false - } - return self.diffLineInfo(parsed), true +// diffLineIdentitiesFromRecords recovers the identity of every diff line the row's +// records state, left to right. A row can carry more than one record, when the +// rendering puts two diff lines on it (a side-by-side row shows a deletion and the +// addition replacing it). Which of them a reader is after depends on the reader: the +// one the row leads with is the row's own identity (see GetDiffLineInfo and +// resolveDiffLines), while a reader looking for a particular line has to consider +// them all, since which of a modification's two halves leads a row is up to the +// rendering. +func (self *DiffLineHelper) diffLineIdentitiesFromRecords(metadata []string) []types.DiffLineInfo { + return self.diffLineInfos(parseDiffLineRecords(metadata)) } // resolvedDiffLine is one rendered row's recovered identity, plus whether it could @@ -86,36 +96,59 @@ type resolvedDiffLine struct { // resolveDiffLines recovers the identity of every row of a rendered diff in one // pass, indexed 1:1 with contents. It is the batch form of GetDiffLineInfo, for the // whole-buffer scans (which change lines are where, which file each row belongs -// to), and reads the rendering the same way: by the renderer's records, or by -// parsing it as a unified diff (see renderingStatesDiffLines). Resolving row by row -// would re-run the buffer parser's whole-section parse once per row — O(n²) on a -// large single-file diff — so the buffer parser runs once for the whole buffer. +// to). A row's identity is the line it leads with, of those resolveDiffLineIdentities +// finds on it. func (self *DiffLineHelper) resolveDiffLines(contents []gocui.DiffLineContent) []resolvedDiffLine { resolved := make([]resolvedDiffLine, len(contents)) - if renderingStatesDiffLines(contents) { - for i, content := range contents { - if info, ok := self.diffLineInfoFromRecords(content.Metadata); ok { - resolved[i] = resolvedDiffLine{info, true} - } - } - return resolved - } - - for i, parsed := range parseAllDiffLinesFromBuffer(diffLineTexts(contents)) { - if parsed.ok { - resolved[i] = resolvedDiffLine{self.diffLineInfo(parsed.parsed), true} + for i, identities := range self.resolveDiffLineIdentities(contents) { + if len(identities) > 0 { + resolved[i] = resolvedDiffLine{identities[0], true} } } return resolved } +// resolveDiffLineIdentities recovers every diff line each row of a rendered diff +// shows, in one pass, indexed 1:1 with contents. It reads the rendering the way +// GetDiffLineInfo does, by the renderer's records or by parsing it as a unified diff +// (see parseDiffLineIdentities), and is the form of the batch resolver for the +// readers that can't settle for the line a row leads with: looking for a remembered +// line in a new rendering has to consider both halves of a modification, since a +// side-by-side row leads with the deletion whose addition was what got remembered +// under a unified one. +func (self *DiffLineHelper) resolveDiffLineIdentities(contents []gocui.DiffLineContent) [][]types.DiffLineInfo { + identities := make([][]types.DiffLineInfo, len(contents)) + for i, parsed := range parseDiffLineIdentities(contents) { + if len(parsed) > 0 { + identities[i] = self.diffLineInfos(parsed) + } + } + return identities +} + // diffLineInfo turns a parser's result into the absolute-path identity consumers // work with. The path arrives repo-relative from the diff header, but a renderer // states it however it likes, absolute paths included. func (self *DiffLineHelper) diffLineInfo(parsed parsedDiffLine) types.DiffLineInfo { + return diffLineInfoIn(self.c.Git().RepoPaths.WorktreePath(), parsed) +} + +// diffLineInfos is diffLineInfo over every line of a row. +func (self *DiffLineHelper) diffLineInfos(parsed []parsedDiffLine) []types.DiffLineInfo { + infos := make([]types.DiffLineInfo, len(parsed)) + for i, line := range parsed { + infos[i] = self.diffLineInfo(line) + } + return infos +} + +// diffLineInfoIn is diffLineInfo against a given worktree, for the callers that can't +// ask which repo we are in where they run: a repo switch replaces it, so only the UI +// thread may read it. +func diffLineInfoIn(worktreePath string, parsed parsedDiffLine) types.DiffLineInfo { path := parsed.Path if !filepath.IsAbs(path) { - path = filepath.Join(self.c.Git().RepoPaths.WorktreePath(), path) + path = filepath.Join(worktreePath, path) } return types.DiffLineInfo{ diff --git a/pkg/gui/controllers/helpers/diff_line_parser.go b/pkg/gui/controllers/helpers/diff_line_parser.go index dd7270af2..8f8420d97 100644 --- a/pkg/gui/controllers/helpers/diff_line_parser.go +++ b/pkg/gui/controllers/helpers/diff_line_parser.go @@ -125,6 +125,46 @@ func renderingStatesDiffLines(contents []gocui.DiffLineContent) bool { }) } +// parseDiffLineRecords parses the records a row carries, left to right, leaving out +// the ones we don't understand. A row carries more than one when the rendering puts +// two diff lines on it, as a side-by-side row does with a deletion and the addition +// replacing it. +func parseDiffLineRecords(metadata []string) []parsedDiffLine { + parsed := make([]parsedDiffLine, 0, len(metadata)) + for _, record := range metadata { + if line, ok := parseDiffLineMetadata(record); ok { + parsed = append(parsed, line) + } + } + return parsed +} + +// parseDiffLineIdentities recovers, for every row of a rendering, the diff lines it +// shows, indexed 1:1 with contents; a row that shows none we can place gets an empty +// entry. The rendering is read the way renderingStatesDiffLines settles: by the +// renderer's records, every one a row carries, or else by parsing the rendering as a +// unified diff, where each row shows one line. Each file's section is parsed once; +// resolving row by row would re-run that parse once per row, O(n²) on a large +// single-file diff. +func parseDiffLineIdentities(contents []gocui.DiffLineContent) [][]parsedDiffLine { + identities := make([][]parsedDiffLine, len(contents)) + if renderingStatesDiffLines(contents) { + for i, content := range contents { + if parsed := parseDiffLineRecords(content.Metadata); len(parsed) > 0 { + identities[i] = parsed + } + } + return identities + } + + for i, parsed := range parseAllDiffLinesFromBuffer(diffLineTexts(contents)) { + if parsed.ok { + identities[i] = []parsedDiffLine{parsed.parsed} + } + } + return identities +} + // fileSectionBounds returns the half-open range [start, end) of the file section // containing targetIdx: the nearest line starting a section at or above it, up to // where that section ends. start is -1 when targetIdx is above the first file diff --git a/pkg/gui/controllers/helpers/diff_line_parser_test.go b/pkg/gui/controllers/helpers/diff_line_parser_test.go index f320a6ef5..5c5c41297 100644 --- a/pkg/gui/controllers/helpers/diff_line_parser_test.go +++ b/pkg/gui/controllers/helpers/diff_line_parser_test.go @@ -420,3 +420,61 @@ func TestRenderingStatesDiffLines(t *testing.T) { }) } } + +func TestParseDiffLineIdentities(t *testing.T) { + row := func(text string, records ...string) gocui.DiffLineContent { + return gocui.DiffLineContent{Text: text, Metadata: records} + } + + t.Run("a rendering with records is read by them alone", func(t *testing.T) { + // A renderer's picture of a commit that adds a test whose input is a diff. The + // test's "diff --git" line is an added line of the test's file and is shown on + // a row of its own, and the rows the renderer puts between hunks carry no + // record. Parsed as a diff, that row would open a section of a file the diff + // doesn't have and claim every untagged row below it. + contents := []gocui.DiffLineContent{ + row("src/parser.rs", "1;f;;;src/parser.rs"), + row(`let input = "\`, "1;c;10;;src/parser.rs"), + row("diff --git a/img.png b/img.png", "1;a;11;;src/parser.rs"), + row("Binary files a/img.png and b/img.png differ", "1;a;12;;src/parser.rs"), + row(""), + row("fn later() {}", "1;c;40;;src/parser.rs"), + } + + assert.Equal(t, [][]parsedDiffLine{ + {{Path: "src/parser.rs", Type: types.DiffLineFileHeader}}, + {{Path: "src/parser.rs", Type: types.DiffLineContext, NewLine: 10}}, + {{Path: "src/parser.rs", Type: types.DiffLineAdded, NewLine: 11}}, + {{Path: "src/parser.rs", Type: types.DiffLineAdded, NewLine: 12}}, + nil, + {{Path: "src/parser.rs", Type: types.DiffLineContext, NewLine: 40}}, + }, parseDiffLineIdentities(contents)) + }) + + t.Run("a row with two records shows both of their lines", func(t *testing.T) { + contents := []gocui.DiffLineContent{ + row("two │ TWO", "1;d;2;2;file1", "1;a;2;;file1"), + } + + assert.Equal(t, [][]parsedDiffLine{ + { + {Path: "file1", Type: types.DiffLineDeleted, NewLine: 2, OldLine: 2}, + {Path: "file1", Type: types.DiffLineAdded, NewLine: 2}, + }, + }, parseDiffLineIdentities(contents)) + }) + + t.Run("a rendering without records is parsed as a diff", func(t *testing.T) { + bufferLines := strings.Split(twoFileDiff, "\n") + contents := make([]gocui.DiffLineContent, len(bufferLines)) + for i, line := range bufferLines { + contents[i] = row(line) + } + + identities := parseDiffLineIdentities(contents) + for i, parsed := range parseAllDiffLinesFromBuffer(bufferLines) { + assert.True(t, parsed.ok, "line %d", i) + assert.Equal(t, []parsedDiffLine{parsed.parsed}, identities[i], "line %d", i) + } + }) +} diff --git a/pkg/gui/controllers/helpers/diff_line_restore.go b/pkg/gui/controllers/helpers/diff_line_restore.go new file mode 100644 index 000000000..7a35d7638 --- /dev/null +++ b/pkg/gui/controllers/helpers/diff_line_restore.go @@ -0,0 +1,426 @@ +package helpers + +import ( + "github.com/jesseduffield/lazygit/pkg/gocui" + "github.com/jesseduffield/lazygit/pkg/gui/types" + "github.com/jesseduffield/lazygit/pkg/tasks" + "github.com/samber/lo" +) + +// Keeping a diff view where it is when the same diff is rendered again differently. +// The line the user is on is remembered by identity (diff_line_helper.go), because a +// new rendering puts it on a different line of the view — and may not have it at all, +// which is what the fallbacks below are for. + +// diffLineAnchor is a line for a restore to land on: the identity to find it by in +// the new rendering, and the screen row it was on, so that it can be put back there. +type diffLineAnchor struct { + identity types.DiffLineInfo + row int +} + +// PreserveDiffPositionOnRerender remembers where a diff view is and puts it back +// there as it next re-renders, instead of leaving the user at the top of a new +// rendering of the diff they were already reading. Call it on the view about to be +// re-rendered, right before triggering the re-render — on both panes of the main +// window where both are being rendered again, since either of them may hold the diff +// being read; a pane that isn't showing is left alone. +// +// The line to keep is the end of the selection that is on screen, and the middle +// visible line when there is no selection or the whole of it has been scrolled out of +// sight — what the user is looking at, rather than the view's top edge or a selection +// they have long since left behind. It may not survive the re-render: a context line +// goes when the context size shrinks, and a whole hunk or file goes when whitespace +// stops counting. So the lines around it come along as fallbacks and the view lands on +// the nearest one that is still there, put back on the screen row it was on. With none +// of them left — and with a renderer that says nothing about its rows there is nothing +// to look for in the first place — the view keeps the scroll offset it had, which is +// still nearer to what was being read than the top of the diff. +// +// An off-screen selection is still put back on the diff line it was on, wherever the +// new rendering has that; it is only the view that stays where it is. +// +// A range or hunk selection has a second end, which is remembered the same way, so +// that it still covers the same lines of the diff afterwards. +func (self *DiffLineHelper) PreserveDiffPositionOnRerender(view *gocui.View) { + // A view that isn't the one its window is currently showing — the merge-conflicts + // view takes the main window over — isn't the one about to be re-rendered, so a + // restore installed on it would sit there and claim a later render instead. + if !view.Visible { + return + } + + // The re-render is produced by a different command from the one behind what is on + // screen — another context size, another renderer — so without being told otherwise + // it would be taken for content the user has never seen and shown from the top. + // Whether or not a line of the old rendering can be found in the new one, the offset + // into it is nearer to where they were reading than the top is. + if manager := self.c.GetViewBufferManagerForView(view); manager != nil { + manager.SetKeepScrollPositionForNextTask() + } + + showSelection := view.Highlight + anchorViewLine := view.MiddleVisibleLineIdx() + farEnd, hasFarEnd := types.DiffLineInfo{}, false + // A cursor that has been scrolled away from is put back by its own lines rather + // than by the anchor's, so that it comes out on the same line of the diff without + // the view having to go there. + var cursorCandidates []diffLineAnchor + if showSelection { + farEnd, hasFarEnd = self.selectionFarEndIdentity(view) + if end, ok := visibleSelectionEnd(view); ok { + anchorViewLine = end + } + if anchorViewLine != view.SelectedLineIdx() { + cursorCandidates = self.nearbyDiffLines(view, view.SelectedLineIdx()) + } + } + + self.restoreDiffLinePositionOnRerender(view, self.nearbyDiffLines(view, anchorViewLine), + func(anchor diffLineAnchor, viewLine int) { + // Put the line back on the screen row it was on, clamped into the view for + // the fallback lines, which can come from off screen. + row := lo.Clamp(anchor.row, 0, max(0, view.InnerHeight()-1)) + view.SetOrigin(0, max(0, viewLine-row)) + if showSelection { + // Put the far end back before the cursor, so that the selection covers + // the same lines again; a selection whose far end didn't survive the + // re-render is left as the single line we landed on. The origin is + // already where it should be, so moving the cursor mustn't scroll. + view.CancelRangeSelect() + cursorViewLine := self.selectionLine(view, cursorCandidates, viewLine) + if hasFarEnd { + if farEndViewLine, ok := self.findDiffLine(view, farEnd); ok { + cursorViewLine, farEndViewLine = coverWholeLines(view, cursorViewLine, farEndViewLine) + view.SetRangeSelectStart(farEndViewLine) + } + } + view.FocusPoint(0, cursorViewLine, false) + } + }) +} + +// coverWholeLines moves the two ends of a restored selection out to the edges of the +// diff lines they are on, so that the selection covers those lines whole. Both ends +// arrive on the first view line of their diff line, which is where looking one up by +// identity lands, and the view draws a line it wraps as several — of which a +// selection of that line means all. +func coverWholeLines(view *gocui.View, cursorViewLine int, farEndViewLine int) (int, int) { + if cursorViewLine <= farEndViewLine { + return cursorViewLine, lastViewLineOfSameDiffLine(view, farEndViewLine) + } + return lastViewLineOfSameDiffLine(view, cursorViewLine), farEndViewLine +} + +// lastViewLineOfSameDiffLine returns the last view line showing the same line of the +// diff as the given one, which is that line itself unless the view wrapped it. +func lastViewLineOfSameDiffLine(view *gocui.View, viewLine int) int { + bufferLine, ok := view.BufferLineForViewLine(viewLine) + if !ok { + return viewLine + } + if last, ok := view.LastViewLineForBufferLine(bufferLine); ok { + return last + } + return viewLine +} + +// visibleSelectionEnd returns the end of the selection to keep in place across a +// re-render: the selected line when it is on screen, and the range's other end when +// that is and the selected line isn't — a range can be long enough for the user to be +// looking at one end of it with the other far away. ok is false when the whole +// selection is off screen, and there is nothing of it to keep in place. +func visibleSelectionEnd(view *gocui.View) (int, bool) { + if view.IsLineVisible(view.SelectedLineIdx()) { + return view.SelectedLineIdx(), true + } + if farEnd, _, ok := selectionFarEndViewLine(view); ok && view.IsLineVisible(farEnd) { + return farEnd, true + } + return 0, false +} + +// selectionLine returns the line to put the cursor on once a re-render is on screen: +// the line the position anchor landed on, which is the selected one whenever it was +// on screen, and otherwise the nearest surviving line to where the selection was — +// found among its own candidates, since the anchor's are a search of the diff from +// somewhere else entirely. +func (self *DiffLineHelper) selectionLine( + view *gocui.View, candidates []diffLineAnchor, anchorViewLine int, +) int { + if len(candidates) == 0 { + return anchorViewLine + } + _, bufferLine := self.nearestSurvivingCandidate(view.DiffLineContents(), candidates) + if bufferLine == -1 { + return anchorViewLine + } + if viewLine, ok := view.ViewLineForBufferLine(bufferLine); ok { + return viewLine + } + return anchorViewLine +} + +// selectionFarEndIdentity returns the identity of the end of a range or hunk +// selection the cursor isn't on, so that a re-render can put it back. ok is false for +// a selection that is only a cursor, where restoring that is the whole job, and for +// an end that resolves to no diff line. +// +// An end covers the whole of its row, so where the row shows more than one diff line +// — a rendering that puts a modification's two halves side by side, or a word diff +// that puts both on the one line it changed — the end takes the outermost of them: +// the last for the range's lower end and the first for its upper one. Otherwise a +// rendering that splits them apart again would get back only the half the row led +// with, and half a change selected where a whole one was. +func (self *DiffLineHelper) selectionFarEndIdentity(view *gocui.View) (types.DiffLineInfo, bool) { + farEnd, isLowerEnd, ok := selectionFarEndViewLine(view) + if !ok { + return types.DiffLineInfo{}, false + } + identities, ok := self.diffLineIdentitiesAt(view, farEnd) + if !ok { + return types.DiffLineInfo{}, false + } + if isLowerEnd { + return identities[len(identities)-1], true + } + return identities[0], true +} + +// selectionFarEndViewLine returns the view line of the end of a range or hunk +// selection the cursor isn't on, and whether that is the lower of the two ends. ok +// is false when there is no range at all, only a cursor. +// +// A range whose two ends are on the same view line still has one, and is not the +// same thing as a cursor sitting there: it covers everything that row shows, which +// may be two lines of the diff at once. +func selectionFarEndViewLine(view *gocui.View) (int, bool, bool) { + if !view.HasRangeSelect() { + return 0, false, false + } + first, last := view.SelectedLineRange() + if view.SelectedLineIdx() == first { + return last, true, true + } + return first, false, true +} + +// findDiffLine returns the view line showing the given diff line in what view is +// displaying now, for placing a remembered line once the re-render is on screen. +func (self *DiffLineHelper) findDiffLine(view *gocui.View, identity types.DiffLineInfo) (int, bool) { + bufferLine, ok := self.patchLineRows(view.DiffLineContents())[patchLineOf(identity)] + if !ok { + return 0, false + } + return view.ViewLineForBufferLine(bufferLine) +} + +// restoreDiffLinePositionOnRerender arranges for view's next re-render to land on the +// first of the given candidate lines the new rendering still has, calling place with +// that candidate and the view line it ended up on. The candidates are in priority +// order (see nearbyDiffLines); if the rendering has none of them, place isn't called +// and the view re-renders as it otherwise would. +// +// The nearest candidate is looked for as the content loads, so that the re-render can +// be revealed at the right position as soon as that line and a screenful below it +// have arrived. Only the nearest one, because the candidates aren't in load order: a +// farther one can load first, and landing on it while a nearer one is still on its +// way would be settling for worse. The rest are considered together once the whole +// rendering is there. +func (self *DiffLineHelper) restoreDiffLinePositionOnRerender( + view *gocui.View, candidates []diffLineAnchor, place func(anchor diffLineAnchor, viewLine int), +) { + manager := self.c.GetViewBufferManagerForView(view) + if manager == nil || len(candidates) == 0 { + return + } + + // The readiness check below runs on the task's own goroutine, where neither the + // view's dimensions nor the repo we are in may be read — a repo switch replaces + // the latter — so take both here, on the UI thread, for it to work from. + viewHeight := view.InnerHeight() + worktreePath := self.c.Git().RepoPaths.WorktreePath() + + // What the search of the loading content has found, and how far it has looked, so + // that each line is looked at once. + found := diffLineAnchor{} + foundLine := -1 + scanned := 0 + + manager.SetRestoreForNextTask(&tasks.RenderRestore{ + FirstPaintReady: func() bool { + if foundLine == -1 { + rows := view.OffscreenDiffLineContentsFrom(scanned) + for i, row := range rows { + if rowShowsDiffLine(row, worktreePath, candidates[0].identity) { + found, foundLine = candidates[0], scanned+i + break + } + } + scanned += len(rows) + if foundLine == -1 { + return false + } + } + // Wait for a screenful below the line as well, so that the re-render isn't + // revealed with it stranded at the bottom of a half-filled view. + return view.OffscreenLineCount() >= foundLine+viewHeight + }, + Apply: func(swapIn func()) bool { + anchor, bufferLine := found, foundLine + if bufferLine == -1 { + anchor, bufferLine = self.nearestSurvivingCandidate(view.OffscreenDiffLineContents(), candidates) + } + + swapIn() + + if bufferLine == -1 { + return false + } + viewLine, ok := view.ViewLineForBufferLine(bufferLine) + if !ok { + return false + } + place(anchor, viewLine) + return true + }, + }) +} + +// nearbyDiffLines collects the lines of view's rendered diff as candidates for a +// restore to land on, ordered by proximity to the anchor line — the anchor itself +// first, then outward, preferring at-or-below on ties — each tagged with the screen +// row it is on. A restore lands on the first of them its re-render still has, so this +// order makes it land as near as possible to where the user was. +// +// The walk covers the whole diff rather than stopping at the change lines on either +// side of the anchor, which a context-size change always keeps: ignoring whitespace +// keeps nothing in particular, and can take a hunk or a whole file out of the diff, +// leaving the nearest surviving line in a neighbouring file. +func (self *DiffLineHelper) nearbyDiffLines(view *gocui.View, anchorViewLine int) []diffLineAnchor { + anchor, ok := view.BufferLineForViewLine(anchorViewLine) + if !ok { + return nil + } + resolved := self.resolveDiffLines(view.DiffLineContents()) + if anchor >= len(resolved) { + return nil + } + rows := screenRows(view, len(resolved)) + + candidates := make([]diffLineAnchor, 0, len(resolved)) + collect := func(bufferLine int) { + if line := resolved[bufferLine]; line.ok { + candidates = append(candidates, diffLineAnchor{identity: line.info, row: rows[bufferLine]}) + } + } + collect(anchor) + for below, above := anchor+1, anchor-1; below < len(resolved) || above >= 0; below, above = below+1, above-1 { + if below < len(resolved) { + collect(below) + } + if above >= 0 { + collect(above) + } + } + return candidates +} + +// screenRows maps each line of view's content to the screen row it is drawn on. The +// lines above the visible ones get -1 and those below them the view's height, so that +// putting one of them back where it was lands it at the top or bottom edge. +func screenRows(view *gocui.View, bufferLineCount int) []int { + height := view.InnerHeight() + originY := view.OriginY() + + rows := make([]int, bufferLineCount) + for i := range rows { + rows[i] = -1 + } + lastVisible := -1 + for y := originY; y < min(originY+height, view.ViewLinesHeight()); y++ { + bufferLine, ok := view.BufferLineForViewLine(y) + if !ok || bufferLine >= bufferLineCount { + continue + } + if rows[bufferLine] == -1 { + rows[bufferLine] = y - originY + } + lastVisible = bufferLine + } + for i := lastVisible + 1; i < bufferLineCount; i++ { + rows[i] = height + } + return rows +} + +// nearestSurvivingCandidate returns the first of the candidates that the given +// rendering still shows, and the line of it that does. The rendering is indexed +// first, rather than searched once per candidate: the candidate list is as long as +// the diff, and so is the rendering. +func (self *DiffLineHelper) nearestSurvivingCandidate( + contents []gocui.DiffLineContent, candidates []diffLineAnchor, +) (diffLineAnchor, int) { + rows := self.patchLineRows(contents) + for _, candidate := range candidates { + if line, ok := rows[patchLineOf(candidate.identity)]; ok { + return candidate, line + } + } + return diffLineAnchor{}, -1 +} + +// patchLineRows indexes a rendering by the diff lines it shows: for each of them, the +// first of its rows that does. A row can show more than one, and each is then a way +// of finding that row again. +func (self *DiffLineHelper) patchLineRows(contents []gocui.DiffLineContent) map[patchLine]int { + rows := map[patchLine]int{} + for i, identities := range self.resolveDiffLineIdentities(contents) { + for _, identity := range identities { + if _, seen := rows[patchLineOf(identity)]; !seen { + rows[patchLineOf(identity)] = i + } + } + } + return rows +} + +// rowShowsDiffLine reports whether the given row of a rendering shows the given diff +// line — among any others it shows, since a side-by-side rendering puts a deletion +// beside the addition replacing it. It only knows what the renderer states about the +// row, since the alternative, parsing the rendering as a diff, needs whole hunks and +// this is asked of content that is still loading. It takes the repo's worktree path +// rather than reading it, being asked off the UI thread. +func rowShowsDiffLine(row gocui.DiffLineContent, worktreePath string, target types.DiffLineInfo) bool { + return lo.SomeBy(row.Metadata, func(record string) bool { + parsed, ok := parseDiffLineMetadata(record) + return ok && patchLineOf(diffLineInfoIn(worktreePath, parsed)) == patchLineOf(target) + }) +} + +// patchLine records what stays the same about a diff line when the same diff is +// rendered again differently: which file it belongs to, the line number that +// identifies it on the side it belongs to, and what kind of line it is. +type patchLine struct { + path string + // Every kind of content line collapses into DiffLineContext, since an addition + // and the context line it turns into when whitespace stops counting are the same + // line of the same file. The header rows keep their kind: a file's header and the + // first line of the file it heads are not the same place. + kind types.DiffLineType + // The old file's line number for a deletion, since two consecutive deletions + // share a new-file position and differ only here; the new file's otherwise. + line int + isDeletion bool +} + +func patchLineOf(info types.DiffLineInfo) patchLine { + switch info.Type { + case types.DiffLineFileHeader, types.DiffLineHunkHeader: + return patchLine{path: info.Path, kind: info.Type, line: info.NewLine} + case types.DiffLineDeleted: + return patchLine{path: info.Path, kind: types.DiffLineContext, line: info.OldLine, isDeletion: true} + default: + return patchLine{path: info.Path, kind: types.DiffLineContext, line: info.NewLine} + } +} diff --git a/pkg/gui/controllers/toggle_whitespace_action.go b/pkg/gui/controllers/toggle_whitespace_action.go index 67bb59d86..33196182a 100644 --- a/pkg/gui/controllers/toggle_whitespace_action.go +++ b/pkg/gui/controllers/toggle_whitespace_action.go @@ -27,6 +27,12 @@ func (self *ToggleWhitespaceAction) Call() error { self.c.UserConfig().Git.IgnoreWhitespaceInDiffView = !self.c.UserConfig().Git.IgnoreWhitespaceInDiffView + // You toggle this to see whether what you are looking at is more than + // reindentation, so that is the thing to keep in front of you — even though + // ignoring whitespace, unlike the other ways of re-rendering a diff, can take + // the line away entirely along with the hunk or file it was in. + self.c.Helpers().DiffLine.PreserveDiffPositionOnRerender(self.c.Contexts().Normal.GetView()) + self.c.Helpers().DiffLine.PreserveDiffPositionOnRerender(self.c.Contexts().NormalSecondary.GetView()) self.c.Context().CurrentSide().HandleRenderToMain() return nil } diff --git a/pkg/integration/components/view_driver.go b/pkg/integration/components/view_driver.go index 332856eb6..3a354c777 100644 --- a/pkg/integration/components/view_driver.go +++ b/pkg/integration/components/view_driver.go @@ -300,6 +300,21 @@ func (self *ViewDriver) SelectedLines(matchers ...*TextMatcher) *ViewDriver { return self } +// SelectedViewLineRange asserts which view lines the selection covers. View lines +// count the wrapped segments a line is drawn as, so this can say whether a selection +// covers a wrapped line to its end; SelectedLines, which reports the lines of the +// content, cannot. +func (self *ViewDriver) SelectedViewLineRange(first int, last int) *ViewDriver { + self.t.assertWithRetries(func() (bool, string) { + actualFirst, actualLast := self.getSelectedRange() + return actualFirst == first && actualLast == last, + fmt.Sprintf("%s: Expected view lines %d-%d to be selected, but %d-%d were.", + self.context, first, last, actualFirst, actualLast) + }) + + return self +} + func (self *ViewDriver) validateMatchersPassed(matchers []*TextMatcher) { if len(matchers) < 1 { self.t.fail("'Lines' methods require at least one matcher to be passed as an argument. If you are trying to assert that there are no lines, use .IsEmpty()") diff --git a/pkg/integration/tests/main_view/keep_a_wrapped_line_covered_across_a_rerender.go b/pkg/integration/tests/main_view/keep_a_wrapped_line_covered_across_a_rerender.go new file mode 100644 index 000000000..9040b0e62 --- /dev/null +++ b/pkg/integration/tests/main_view/keep_a_wrapped_line_covered_across_a_rerender.go @@ -0,0 +1,60 @@ +package main_view + +import ( + "fmt" + "strings" + + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var KeepAWrappedLineCoveredAcrossARerender = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "A selection over a line too long for the view still covers all of it after a re-render", + ExtraCmdArgs: []string{}, + Skip: false, + Width: 80, + Height: 20, + SetupConfig: func(cfg *config.AppConfig) { + cfg.GetUserConfig().Gui.UseHunkModeInStagingView = true + }, + SetupRepo: func(shell *Shell) { + long := strings.Repeat("word ", 40) + lines := make([]string, 20) + for i := range lines { + lines[i] = fmt.Sprintf("line%02d", i+1) + } + before := strings.Join(lines[:10], "\n") + "\n" + after := strings.Join(lines[10:], "\n") + "\n" + + shell.CreateFileAndAdd("file1", before+long+"\n"+after) + shell.Commit("one") + + shell.UpdateFile("file1", before+"CHANGED "+long+"\n"+after) + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Files(). + IsFocused(). + Press(keys.Universal.FocusMainView) + + // The changed line is far too long for the view, so each half of the change + // is drawn as several view lines, and hunk mode selects all of them. + t.Views().Main(). + IsFocused(). + SelectedLines( + Contains("-word word"), + Contains("+CHANGED word"), + ). + SelectedViewLineRange(8, 16). + // The same two lines of the diff, wrapped the same way, are still covered + // to their ends once the diff has been rendered again. + Press(keys.Universal.IncreaseContextInDiffView). + Tap(func() { + t.ExpectToast(Equals("Changed diff context size to 4")) + }). + SelectedLines( + Contains("-word word"), + Contains("+CHANGED word"), + ). + SelectedViewLineRange(9, 17) + }, +}) diff --git a/pkg/integration/tests/main_view/keep_both_halves_of_a_change_selected.go b/pkg/integration/tests/main_view/keep_both_halves_of_a_change_selected.go new file mode 100644 index 000000000..87df911a2 --- /dev/null +++ b/pkg/integration/tests/main_view/keep_both_halves_of_a_change_selected.go @@ -0,0 +1,68 @@ +package main_view + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var KeepBothHalvesOfAChangeSelected = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "A change selected on the one row a renderer draws it as is selected on both rows of a renderer that splits it", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(cfg *config.AppConfig) { + cfg.GetUserConfig().Gui.UseHunkModeInStagingView = true + cfg.GetUserConfig().Git.DiffRenderers = []config.DiffRendererConfig{ + // Git's own diff, which has a row for each half of a change. It announces + // the metadata protocol, so lazygit acts on its output rather than + // replacing it; it states no records of its own, so the rows are located + // by parsing the text, which still looks like a diff. + {Name: "unified", Command: `printf '\033]1717;1\007'; cat`}, + // A renderer that puts the two halves of a change beside each other on one + // row. Only the records it states can say where those halves are; parsing + // the text could not. It ignores its input and prints this one. + {Name: "columns", Command: `printf '\033]1717;1\007'; ` + + `printf '\033]1717;1;f;;;file1\007file1\n'; ` + + `printf '\033]1717;1;h;1;;file1\007@@\n'; ` + + `printf '\033]1717;1;c;1;;file1\007one one\n'; ` + + `printf '\033]1717;1;d;2;2;file1\007two \033]1717;1;a;2;;file1\007TWO\n'; ` + + `printf '\033]1717;1;c;3;;file1\007three three\n'; ` + + `cat >/dev/null`}, + } + }, + SetupRepo: func(shell *Shell) { + shell.CreateFileAndAdd("file1", "one\ntwo\nthree\n") + shell.Commit("one") + + shell.UpdateFile("file1", "one\nTWO\nthree\n") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Files(). + IsFocused(). + Press(keys.Universal.FocusMainView) + + t.Views().Main(). + IsFocused(). + SelectedLines( + Contains("-two"), + Contains("+TWO"), + ). + // The change is one row here, and selecting it selects that row: both + // halves are on it. + Press(keys.Universal.CycleDiffRenderers). + Tap(func() { + t.ExpectToast(Equals("Diff renderer: columns (2 of 2)")) + }). + SelectedLines( + Contains("two TWO"), + ). + // Split apart again, the same change is the same two lines it was. + Press(keys.Universal.CycleDiffRenderers). + Tap(func() { + t.ExpectToast(Equals("Diff renderer: unified (1 of 2)")) + }). + SelectedLines( + Contains("-two"), + Contains("+TWO"), + ) + }, +}) diff --git a/pkg/integration/tests/main_view/keep_position_by_the_visible_end_of_a_selection.go b/pkg/integration/tests/main_view/keep_position_by_the_visible_end_of_a_selection.go new file mode 100644 index 000000000..0ac9e1f54 --- /dev/null +++ b/pkg/integration/tests/main_view/keep_position_by_the_visible_end_of_a_selection.go @@ -0,0 +1,102 @@ +package main_view + +import ( + "fmt" + "strings" + + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var KeepPositionByTheVisibleEndOfASelection = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "A re-render keeps the place by the end of a selected hunk that is on screen when its other end isn't", + ExtraCmdArgs: []string{}, + Skip: false, + Width: 120, + Height: 30, + SetupConfig: func(cfg *config.AppConfig) { + cfg.GetUserConfig().Gui.UseHunkModeInStagingView = true + // One line per scroll, so that the test can put the top of the view exactly + // where it wants it. + cfg.GetUserConfig().Gui.ScrollHeight = 1 + }, + SetupRepo: func(shell *Shell) { + lines := make([]string, 60) + for i := range lines { + lines[i] = fmt.Sprintf("line%02d", i+1) + } + shell.CreateFileAndAdd("file1", strings.Join(lines, "\n")+"\n") + shell.Commit("one") + + // A first change tall enough to be scrolled halfway out of the view, and more + // of them below it, so that a context-size change moves the lines further down + // the diff by more than it moves the first change. + for _, i := range []int{10, 11, 12, 13, 14, 15, 30, 45} { + lines[i-1] = strings.ToUpper(lines[i-1]) + } + shell.UpdateFile("file1", strings.Join(lines, "\n")+"\n") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Files(). + IsFocused(). + Press(keys.Universal.FocusMainView) + + t.Views().Main(). + IsFocused(). + SelectedLines( + Contains("-line10"), + Contains("-line11"), + Contains("-line12"), + Contains("-line13"), + Contains("-line14"), + Contains("-line15"), + Contains("+LINE10"), + Contains("+LINE11"), + Contains("+LINE12"), + Contains("+LINE13"), + Contains("+LINE14"), + Contains("+LINE15"), + ). + SelectedLineIdx(8). + // Scroll past the start of the selected block, leaving its last lines on + // screen and the cursor above the top of the view. + Press(keys.Universal.ScrollDownMain). + Press(keys.Universal.ScrollDownMain). + Press(keys.Universal.ScrollDownMain). + Press(keys.Universal.ScrollDownMain). + Press(keys.Universal.ScrollDownMain). + Press(keys.Universal.ScrollDownMain). + Press(keys.Universal.ScrollDownMain). + Press(keys.Universal.ScrollDownMain). + Press(keys.Universal.ScrollDownMain). + Press(keys.Universal.ScrollDownMain). + Press(keys.Universal.ScrollDownMain). + Press(keys.Universal.ScrollDownMain). + Press(keys.Universal.ScrollDownMain). + Press(keys.Universal.ScrollDownMain). + OriginY(14). + Press(keys.Universal.IncreaseContextInDiffView). + Tap(func() { + t.ExpectToast(Equals("Changed diff context size to 4")) + }). + // The block's last line was the fifth row of the screen, and one context + // line more above the block puts it a line further down the diff: the view + // follows it, rather than the middle visible line, which the hunks below + // have pushed further still. + OriginY(15). + SelectedLines( + Contains("-line10"), + Contains("-line11"), + Contains("-line12"), + Contains("-line13"), + Contains("-line14"), + Contains("-line15"), + Contains("+LINE10"), + Contains("+LINE11"), + Contains("+LINE12"), + Contains("+LINE13"), + Contains("+LINE14"), + Contains("+LINE15"), + ) + }, +}) diff --git a/pkg/integration/tests/main_view/keep_position_in_both_panes_when_changing_context_size.go b/pkg/integration/tests/main_view/keep_position_in_both_panes_when_changing_context_size.go new file mode 100644 index 000000000..ec686ede7 --- /dev/null +++ b/pkg/integration/tests/main_view/keep_position_in_both_panes_when_changing_context_size.go @@ -0,0 +1,71 @@ +package main_view + +import ( + "fmt" + "strings" + + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var KeepPositionInBothPanesWhenChangingContextSize = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Changing the diff's context size keeps the place in the lower pane too, not only in the upper one", + ExtraCmdArgs: []string{}, + Skip: false, + Width: 120, + Height: 30, + SetupConfig: func(cfg *config.AppConfig) { + cfg.GetUserConfig().Gui.UseHunkModeInStagingView = false + }, + SetupRepo: func(shell *Shell) { + lines := make([]string, 40) + for i := range lines { + lines[i] = fmt.Sprintf("line%02d", i+1) + } + shell.CreateFileAndAdd("file1", strings.Join(lines, "\n")+"\n") + shell.Commit("one") + + // Four staged changes, far enough apart that they stay four hunks as the + // context size grows, and one unstaged one to split the file's diff across + // both panes. + for _, i := range []int{5, 15, 25, 35} { + lines[i-1] = strings.ToUpper(lines[i-1]) + } + shell.UpdateFile("file1", strings.Join(lines, "\n")+"\n") + shell.GitAddAll() + + lines[39] = strings.ToUpper(lines[39]) + shell.UpdateFile("file1", strings.Join(lines, "\n")+"\n") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Files(). + IsFocused(). + Press(keys.Universal.FocusMainView) + + // The lower pane holds the staged changes; getting to the last of them scrolls + // it, so there is a position to lose. + t.Views().Main(). + IsFocused(). + PressTab() + + t.Views().Secondary(). + IsFocused(). + Press(keys.Main.NextHunk). + Press(keys.Main.NextHunk). + Press(keys.Main.NextHunk). + SelectedLines( + Contains("-line35"), + ). + SelectedLineIdx(35). + OriginY(14). + Press(keys.Universal.IncreaseContextInDiffView). + Tap(func() { + t.ExpectToast(Equals("Changed diff context size to 4")) + }). + SelectedLines( + Contains("-line35"), + ). + SelectedLineIdx(42). + OriginY(21) + }, +}) diff --git a/pkg/integration/tests/main_view/keep_position_in_both_panes_when_ignoring_whitespace.go b/pkg/integration/tests/main_view/keep_position_in_both_panes_when_ignoring_whitespace.go new file mode 100644 index 000000000..c84ccde5d --- /dev/null +++ b/pkg/integration/tests/main_view/keep_position_in_both_panes_when_ignoring_whitespace.go @@ -0,0 +1,70 @@ +package main_view + +import ( + "fmt" + "strings" + + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var KeepPositionInBothPanesWhenIgnoringWhitespace = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Ignoring whitespace keeps the place in the lower pane too, not only in the upper one", + ExtraCmdArgs: []string{}, + Skip: false, + Width: 120, + Height: 30, + SetupConfig: func(cfg *config.AppConfig) { + cfg.GetUserConfig().Gui.UseHunkModeInStagingView = false + }, + SetupRepo: func(shell *Shell) { + lines := make([]string, 60) + for i := range lines { + lines[i] = fmt.Sprintf("line%02d", i+1) + } + shell.CreateFileAndAdd("file1", strings.Join(lines, "\n")+"\n") + shell.Commit("one") + + // Staged: real changes at lines 5, 25 and 45, and a whitespace-only one at 15, + // whose hunk goes when whitespace stops counting. + lines[4] = strings.ToUpper(lines[4]) + lines[14] = " " + lines[14] + lines[24] = strings.ToUpper(lines[24]) + lines[44] = strings.ToUpper(lines[44]) + shell.UpdateFile("file1", strings.Join(lines, "\n")+"\n") + shell.GitAddAll() + + // And one unstaged change, to split the file's diff across both panes. + lines[59] = strings.ToUpper(lines[59]) + shell.UpdateFile("file1", strings.Join(lines, "\n")+"\n") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Files(). + IsFocused(). + Press(keys.Universal.FocusMainView) + + t.Views().Main(). + IsFocused(). + PressTab() + + t.Views().Secondary(). + IsFocused(). + Press(keys.Main.NextHunk). + Press(keys.Main.NextHunk). + Press(keys.Main.NextHunk). + SelectedLines( + Contains("-line45"), + ). + SelectedLineIdx(35). + OriginY(14). + Press(keys.Universal.ToggleWhitespaceInDiffView). + // The hunk above this one held nothing but a whitespace change, so it is + // gone and has taken nine lines of the lower pane's diff with it — leaving + // the line we were on where it was on the screen. + SelectedLines( + Contains("-line45"), + ). + SelectedLineIdx(26). + OriginY(5) + }, +}) diff --git a/pkg/integration/tests/main_view/keep_position_in_both_panes_when_switching_diff_renderers.go b/pkg/integration/tests/main_view/keep_position_in_both_panes_when_switching_diff_renderers.go new file mode 100644 index 000000000..9f562f7db --- /dev/null +++ b/pkg/integration/tests/main_view/keep_position_in_both_panes_when_switching_diff_renderers.go @@ -0,0 +1,78 @@ +package main_view + +import ( + "fmt" + "strings" + + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var KeepPositionInBothPanesWhenSwitchingDiffRenderers = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Switching to another diff renderer keeps the place in the lower pane too, not only in the upper one", + ExtraCmdArgs: []string{}, + Skip: false, + Width: 120, + Height: 30, + SetupConfig: func(cfg *config.AppConfig) { + cfg.GetUserConfig().Gui.UseHunkModeInStagingView = false + // Renderers that speak the metadata protocol, so that focusing the main view + // keeps their rendering rather than falling back to git's own diff. + cfg.GetUserConfig().Git.DiffRenderers = []config.DiffRendererConfig{ + {Name: "plain", Command: `printf '\033]1717;1\007'; cat`}, + // The same diff, three lines further down the view. (Lines before the + // diff's own header aren't part of it, so it still reads the same.) + {Name: "banner", Command: `printf '\033]1717;1\007'; printf 'rendered for you\n\n\n'; cat`}, + } + }, + SetupRepo: func(shell *Shell) { + lines := make([]string, 40) + for i := range lines { + lines[i] = fmt.Sprintf("line%02d", i+1) + } + shell.CreateFileAndAdd("file1", strings.Join(lines, "\n")+"\n") + shell.Commit("one") + + // Four staged changes to have a diff worth scrolling in the lower pane, and one + // unstaged one to split the file's diff across both panes. + for _, i := range []int{5, 15, 25, 35} { + lines[i-1] = strings.ToUpper(lines[i-1]) + } + shell.UpdateFile("file1", strings.Join(lines, "\n")+"\n") + shell.GitAddAll() + + lines[39] = strings.ToUpper(lines[39]) + shell.UpdateFile("file1", strings.Join(lines, "\n")+"\n") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Files(). + IsFocused(). + Press(keys.Universal.FocusMainView) + + t.Views().Main(). + IsFocused(). + PressTab() + + t.Views().Secondary(). + IsFocused(). + Press(keys.Main.NextHunk). + Press(keys.Main.NextHunk). + Press(keys.Main.NextHunk). + SelectedLines( + Contains("-line35"), + ). + SelectedLineIdx(35). + OriginY(14). + Press(keys.Universal.CycleDiffRenderers). + Tap(func() { + t.ExpectToast(Equals("Diff renderer: banner (2 of 2)")) + }). + // The banner pushed the whole diff three lines down, and the lower pane came + // along with it, just as the upper one would have. + SelectedLines( + Contains("-line35"), + ). + SelectedLineIdx(38). + OriginY(17) + }, +}) diff --git a/pkg/integration/tests/main_view/keep_position_when_changing_context_size.go b/pkg/integration/tests/main_view/keep_position_when_changing_context_size.go new file mode 100644 index 000000000..ba99eb072 --- /dev/null +++ b/pkg/integration/tests/main_view/keep_position_when_changing_context_size.go @@ -0,0 +1,97 @@ +package main_view + +import ( + "fmt" + "strings" + + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var KeepPositionWhenChangingContextSize = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Changing the diff's context size keeps the line you were looking at where it was", + ExtraCmdArgs: []string{}, + Skip: false, + Width: 120, + Height: 30, + SetupConfig: func(cfg *config.AppConfig) { + cfg.GetUserConfig().Gui.UseHunkModeInStagingView = false + }, + SetupRepo: func(shell *Shell) { + lines := make([]string, 40) + for i := range lines { + lines[i] = fmt.Sprintf("line%02d", i+1) + } + shell.CreateFileAndAdd("file1", strings.Join(lines, "\n")+"\n") + shell.Commit("one") + + // Four changes, far enough apart that they stay four hunks as the context + // size grows. + for _, i := range []int{5, 15, 25, 35} { + lines[i-1] = strings.ToUpper(lines[i-1]) + } + shell.UpdateFile("file1", strings.Join(lines, "\n")+"\n") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Files(). + IsFocused(). + Press(keys.Universal.FocusMainView) + + t.Views().Main(). + IsFocused(). + Press(keys.Main.NextHunk). + Press(keys.Main.NextHunk). + Press(keys.Main.NextHunk). + SelectedLines( + Contains("-line35"), + ). + // The diff is longer than the view, so getting to the last hunk scrolled + // it: the selected line sits 21 rows down the screen. + SelectedLineIdx(35). + OriginY(14). + Press(keys.Universal.IncreaseContextInDiffView). + Tap(func() { + t.ExpectToast(Equals("Changed diff context size to 4")) + }). + // A context line more on either side of each of the four hunks pushes the + // selected line seven lines further into the diff. The view follows it, so + // it is still the same line on the same screen row (42 - 21 = 21). + SelectedLines( + Contains("-line35"), + ). + SelectedLineIdx(42). + OriginY(21). + Press(keys.Universal.DecreaseContextInDiffView). + Tap(func() { + t.ExpectToast(Equals("Changed diff context size to 3")) + }). + Press(keys.Universal.DecreaseContextInDiffView). + Tap(func() { + t.ExpectToast(Equals("Changed diff context size to 2")) + }). + // And the same the other way (28 - 21 = 7). + SelectedLines( + Contains("-line35"), + ). + SelectedLineIdx(28). + OriginY(7). + // Leaving the view gives up the selection but not the scroll position, and + // with no selection to keep, it is the middle visible line that stays put. + PressEscape() + + t.Views().Files(). + IsFocused(). + Press(keys.Universal.IncreaseContextInDiffView). + Tap(func() { + t.ExpectToast(Equals("Changed diff context size to 3")) + }) + + // The middle visible line here is a hunk's header, and a context-size change + // rewrites those — they name the lines the hunk covers. So the restore falls + // back to the nearest line that does survive, the context line just below it, + // and puts that back on the row it was on. + t.Views().Main(). + SelectionIsHidden(). + OriginY(12) + }, +}) diff --git a/pkg/integration/tests/main_view/keep_position_when_ignoring_whitespace.go b/pkg/integration/tests/main_view/keep_position_when_ignoring_whitespace.go new file mode 100644 index 000000000..9b9479369 --- /dev/null +++ b/pkg/integration/tests/main_view/keep_position_when_ignoring_whitespace.go @@ -0,0 +1,85 @@ +package main_view + +import ( + "fmt" + "strings" + + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var KeepPositionWhenIgnoringWhitespace = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Ignoring whitespace keeps the line you were looking at where it was, even when it turns into a context line", + ExtraCmdArgs: []string{}, + Skip: false, + Width: 120, + Height: 30, + SetupConfig: func(cfg *config.AppConfig) { + cfg.GetUserConfig().Gui.UseHunkModeInStagingView = false + }, + SetupRepo: func(shell *Shell) { + lines := make([]string, 40) + for i := range lines { + lines[i] = fmt.Sprintf("line%02d", i+1) + } + shell.CreateFileAndAdd("file1", strings.Join(lines, "\n")+"\n") + shell.Commit("one") + + // Real changes at lines 5 and 25, whitespace-only ones at 15, 27 and 35. The + // one at 27 shares a hunk with the change at 25, so ignoring whitespace turns + // it into a context line rather than taking its hunk away. + lines[4] = strings.ToUpper(lines[4]) + lines[14] = " " + lines[14] + lines[24] = strings.ToUpper(lines[24]) + lines[26] = lines[26] + " " + lines[34] = " " + lines[34] + shell.UpdateFile("file1", strings.Join(lines, "\n")+"\n") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Files(). + IsFocused(). + Press(keys.Universal.FocusMainView) + + t.Views().Main(). + IsFocused(). + Press(keys.Main.NextHunk). + Press(keys.Main.NextHunk). + SelectedLines( + Contains("-line25"), + ). + SelectedLineIdx(26). + OriginY(14). + Press(keys.Universal.ToggleWhitespaceInDiffView). + // The hunk above this one held nothing but a whitespace change, so it is + // gone and has taken nine lines of diff with it. This is still the line we + // were on, on the row we were on (26 - 14 = 17 - 5). + SelectedLines( + Contains("-line25"), + ). + SelectedLineIdx(17). + OriginY(5). + // And back again, whitespace and all. + Press(keys.Universal.ToggleWhitespaceInDiffView). + SelectedLines( + Contains("-line25"), + ). + SelectedLineIdx(26). + OriginY(14). + // The whitespace-only change further down this hunk is a line of the file + // like any other: ignoring whitespace shows it as context instead of as a + // change, and that is still where we are. + Press(keys.Main.NextHunk). + Press(keys.Universal.NextItem). + SelectedLines( + Contains("+line27"), + ). + SelectedLineIdx(30). + OriginY(14). + Press(keys.Universal.ToggleWhitespaceInDiffView). + SelectedLines( + Contains(" line27"), + ). + SelectedLineIdx(20). + OriginY(4) + }, +}) diff --git a/pkg/integration/tests/main_view/keep_position_when_ignoring_whitespace_removes_it.go b/pkg/integration/tests/main_view/keep_position_when_ignoring_whitespace_removes_it.go new file mode 100644 index 000000000..1474d9042 --- /dev/null +++ b/pkg/integration/tests/main_view/keep_position_when_ignoring_whitespace_removes_it.go @@ -0,0 +1,81 @@ +package main_view + +import ( + "fmt" + "strings" + + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var KeepPositionWhenIgnoringWhitespaceRemovesIt = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Ignoring whitespace where that takes the line you were on out of the diff lands on the nearest line it kept", + ExtraCmdArgs: []string{}, + Skip: false, + Width: 120, + Height: 30, + SetupConfig: func(cfg *config.AppConfig) { + cfg.GetUserConfig().Gui.UseHunkModeInStagingView = false + }, + SetupRepo: func(shell *Shell) { + lines := make([]string, 40) + for i := range lines { + lines[i] = fmt.Sprintf("line%02d", i+1) + } + shell.CreateFileAndAdd("file1", strings.Join(lines, "\n")+"\n") + shell.CreateFileAndAdd("file2", "one\ntwo\nthree\n") + shell.Commit("one") + + // Real changes at lines 5, 15 and 25, and a whitespace-only one at 35, far + // enough apart to be hunks of their own. + for _, i := range []int{5, 15, 25} { + lines[i-1] = strings.ToUpper(lines[i-1]) + } + lines[34] = " " + lines[34] + shell.UpdateFile("file1", strings.Join(lines, "\n")+"\n") + + // Nothing but reindentation, so ignoring whitespace leaves no diff at all. + shell.UpdateFile("file2", " one\n two\n three\n") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Files(). + IsFocused(). + SelectNextItem(). + SelectedLine(Contains("file1")). + Press(keys.Universal.FocusMainView) + + t.Views().Main(). + IsFocused(). + Press(keys.Main.NextHunk). + Press(keys.Main.NextHunk). + Press(keys.Main.NextHunk). + SelectedLines( + Contains("-line35"), + ). + SelectedLineIdx(35). + OriginY(14). + Press(keys.Universal.ToggleWhitespaceInDiffView). + // That hunk was a whitespace change and nothing else, so ignoring + // whitespace takes it — and the context around it — out of the diff + // entirely. The nearest line the diff kept is the last line of the hunk + // above, so that is where the selection lands; it goes back on the row it + // was on itself, which leaves everything above it exactly where it was. + SelectedLines( + Contains(" line28"), + ). + SelectedLineIdx(30). + OriginY(14). + // The whole diff can go this way, and then there is nothing to land on. + Press(keys.Universal.ToggleWhitespaceInDiffView). + PressEscape() + + t.Views().Files(). + IsFocused(). + SelectNextItem(). + SelectedLine(Contains("file2")). + Press(keys.Universal.ToggleWhitespaceInDiffView) + + t.Views().Main(). + Content(Equals("")) + }, +}) diff --git a/pkg/integration/tests/main_view/keep_position_when_switching_diff_renderers.go b/pkg/integration/tests/main_view/keep_position_when_switching_diff_renderers.go new file mode 100644 index 000000000..7241ac9bf --- /dev/null +++ b/pkg/integration/tests/main_view/keep_position_when_switching_diff_renderers.go @@ -0,0 +1,75 @@ +package main_view + +import ( + "fmt" + "strings" + + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var KeepPositionWhenSwitchingDiffRenderers = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Switching to another diff renderer keeps the line you were looking at where it was", + ExtraCmdArgs: []string{}, + Skip: false, + Width: 120, + Height: 30, + SetupConfig: func(cfg *config.AppConfig) { + cfg.GetUserConfig().Gui.UseHunkModeInStagingView = false + cfg.GetUserConfig().Git.DiffRenderers = []config.DiffRendererConfig{ + {Name: "plain", Command: "cat"}, + // The same diff, three lines further down the view. (Lines before the + // diff's own header aren't part of it, so it still reads the same.) + {Name: "banner", Command: `printf 'rendered for you\n\n\n'; cat`}, + } + }, + SetupRepo: func(shell *Shell) { + lines := make([]string, 40) + for i := range lines { + lines[i] = fmt.Sprintf("line%02d", i+1) + } + shell.CreateFileAndAdd("file1", strings.Join(lines, "\n")+"\n") + shell.Commit("one") + + for _, i := range []int{5, 15, 25, 35} { + lines[i-1] = strings.ToUpper(lines[i-1]) + } + shell.UpdateFile("file1", strings.Join(lines, "\n")+"\n") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Files(). + IsFocused(). + Press(keys.Universal.FocusMainView) + + t.Views().Main(). + IsFocused(). + Press(keys.Main.NextHunk). + Press(keys.Main.NextHunk). + Press(keys.Main.NextHunk). + SelectedLines( + Contains("-line35"), + ). + SelectedLineIdx(35). + OriginY(14). + Press(keys.Universal.CycleDiffRenderers). + Tap(func() { + t.ExpectToast(Equals("Diff renderer: banner (2 of 2)")) + }). + // The banner pushed the whole diff three lines down, and the view came + // along with it: the same line on the same screen row (38 - 17 = 21). + SelectedLines( + Contains("-line35"), + ). + SelectedLineIdx(38). + OriginY(17). + Press(keys.Universal.CycleDiffRenderers). + Tap(func() { + t.ExpectToast(Equals("Diff renderer: plain (1 of 2)")) + }). + SelectedLines( + Contains("-line35"), + ). + SelectedLineIdx(35). + OriginY(14) + }, +}) diff --git a/pkg/integration/tests/main_view/keep_position_when_the_selection_is_off_screen.go b/pkg/integration/tests/main_view/keep_position_when_the_selection_is_off_screen.go new file mode 100644 index 000000000..d195cb7bb --- /dev/null +++ b/pkg/integration/tests/main_view/keep_position_when_the_selection_is_off_screen.go @@ -0,0 +1,65 @@ +package main_view + +import ( + "fmt" + "strings" + + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var KeepPositionWhenTheSelectionIsOffScreen = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "A re-render keeps the lines that are on screen where they are, not a selection scrolled away from", + ExtraCmdArgs: []string{}, + Skip: false, + Width: 120, + Height: 30, + SetupConfig: func(cfg *config.AppConfig) { + cfg.GetUserConfig().Gui.UseHunkModeInStagingView = false + // Half a diff per scroll, to leave the selection well behind in two presses. + cfg.GetUserConfig().Gui.ScrollHeight = 15 + }, + SetupRepo: func(shell *Shell) { + lines := make([]string, 60) + for i := range lines { + lines[i] = fmt.Sprintf("line%02d", i+1) + } + shell.CreateFileAndAdd("file1", strings.Join(lines, "\n")+"\n") + shell.Commit("one") + + for _, i := range []int{5, 15, 25, 35, 45, 55} { + lines[i-1] = strings.ToUpper(lines[i-1]) + } + shell.UpdateFile("file1", strings.Join(lines, "\n")+"\n") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Files(). + IsFocused(). + Press(keys.Universal.FocusMainView) + + t.Views().Main(). + IsFocused(). + SelectedLines( + Contains("-line05"), + ). + // Reading on past the selection leaves it far behind, off the top of the + // view. + Press(keys.Universal.ScrollDownMain). + Press(keys.Universal.ScrollDownMain). + OriginY(30). + Press(keys.Universal.DecreaseContextInDiffView). + Tap(func() { + t.ExpectToast(Equals("Changed diff context size to 2")) + }). + // A context line less on either side of the four hunks above what is on + // screen pulls it nine lines up the diff, and the view follows it there: the + // lines the user was reading are still on the rows they were on. + OriginY(21). + // The selection is where it always was, on its own line of the diff, rather + // than having been dragged back into view. + SelectedLines( + Contains("-line05"), + ). + SelectedLineIdx(7) + }, +}) diff --git a/pkg/integration/tests/main_view/keep_scroll_when_the_diff_cant_be_read.go b/pkg/integration/tests/main_view/keep_scroll_when_the_diff_cant_be_read.go new file mode 100644 index 000000000..9cda012c9 --- /dev/null +++ b/pkg/integration/tests/main_view/keep_scroll_when_the_diff_cant_be_read.go @@ -0,0 +1,58 @@ +package main_view + +import ( + "fmt" + "strings" + + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var KeepScrollWhenTheDiffCantBeRead = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Changing the context size under a diff renderer whose rows can't be placed keeps the scroll position rather than jumping to the top", + ExtraCmdArgs: []string{}, + Skip: false, + Width: 120, + Height: 30, + SetupConfig: func(cfg *config.AppConfig) { + cfg.GetUserConfig().Gui.UseHunkModeInStagingView = false + // A renderer that says nothing about which line of which file each row shows, + // and mangles the diff enough that it can't be read back as one either: no line + // of it can be looked for in the re-render. + cfg.GetUserConfig().Git.DiffRenderers = []config.DiffRendererConfig{ + {Name: "opaque", Command: `sed -e 's/^/| /'`}, + } + }, + SetupRepo: func(shell *Shell) { + lines := make([]string, 40) + for i := range lines { + lines[i] = fmt.Sprintf("line%02d", i+1) + } + shell.CreateFileAndAdd("file1", strings.Join(lines, "\n")+"\n") + shell.Commit("one") + + for _, i := range []int{5, 15, 25, 35} { + lines[i-1] = strings.ToUpper(lines[i-1]) + } + shell.UpdateFile("file1", strings.Join(lines, "\n")+"\n") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Files(). + IsFocused(). + Press(keys.Universal.ScrollDownMain). + Press(keys.Universal.ScrollDownMain). + Press(keys.Universal.ScrollDownMain) + + t.Views().Main(). + Content(Contains("| +LINE05")). + OriginY(6). + Tap(func() { + t.Views().Files().Press(keys.Universal.IncreaseContextInDiffView) + t.ExpectToast(Equals("Changed diff context size to 4")) + }). + // The re-render is a different command, and nothing in its output can be + // matched up with what was on screen, so the offset is all there is to keep — + // and it is a good deal closer than the top. + OriginY(6) + }, +}) diff --git a/pkg/integration/tests/main_view/keep_selected_range_when_changing_context_size.go b/pkg/integration/tests/main_view/keep_selected_range_when_changing_context_size.go new file mode 100644 index 000000000..8a64b8dab --- /dev/null +++ b/pkg/integration/tests/main_view/keep_selected_range_when_changing_context_size.go @@ -0,0 +1,123 @@ +package main_view + +import ( + "fmt" + "strings" + + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var KeepSelectedRangeWhenChangingContextSize = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "A range selection still covers the same lines of the diff after the context size changes", + ExtraCmdArgs: []string{}, + Skip: false, + Width: 120, + Height: 30, + SetupConfig: func(cfg *config.AppConfig) { + cfg.GetUserConfig().Gui.UseHunkModeInStagingView = false + }, + SetupRepo: func(shell *Shell) { + lines := make([]string, 40) + for i := range lines { + lines[i] = fmt.Sprintf("line%02d", i+1) + } + shell.CreateFileAndAdd("file1", strings.Join(lines, "\n")+"\n") + shell.Commit("one") + + for _, i := range []int{5, 15, 25, 35} { + lines[i-1] = strings.ToUpper(lines[i-1]) + } + shell.UpdateFile("file1", strings.Join(lines, "\n")+"\n") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Files(). + IsFocused(). + Press(keys.Universal.FocusMainView) + + // A range from a change down into the context below it, so that the cursor is + // on the last line of the selection and the other end is three lines above. + t.Views().Main(). + IsFocused(). + Press(keys.Main.NextHunk). + Press(keys.Main.NextHunk). + Press(keys.Universal.ToggleRangeSelect). + Press(keys.Universal.NextItem). + Press(keys.Universal.NextItem). + Press(keys.Universal.NextItem). + SelectedLines( + Contains("-line25"), + Contains("+LINE25"), + Contains(" line26"), + Contains(" line27"), + ). + // Both ends are still lines of the diff with more context around the + // change, so the selection still covers the same four. + Press(keys.Universal.IncreaseContextInDiffView). + Tap(func() { + t.ExpectToast(Equals("Changed diff context size to 4")) + }). + SelectedLines( + Contains("-line25"), + Contains("+LINE25"), + Contains(" line26"), + Contains(" line27"), + ). + // With a single line of context, the line the cursor was on is no longer in + // the diff. The end that survived stays put and the cursor lands on the + // nearest line that is left, so the selection shrinks with the diff. + Press(keys.Universal.DecreaseContextInDiffView). + Tap(func() { + t.ExpectToast(Equals("Changed diff context size to 3")) + }). + Press(keys.Universal.DecreaseContextInDiffView). + Tap(func() { + t.ExpectToast(Equals("Changed diff context size to 2")) + }). + Press(keys.Universal.DecreaseContextInDiffView). + Tap(func() { + t.ExpectToast(Equals("Changed diff context size to 1")) + }). + SelectedLines( + Contains("-line25"), + Contains("+LINE25"), + Contains(" line26"), + ). + // The other way round: a range extended upwards, so that it is the far end + // that the shrinking context takes away. There is no guessing which line + // inherits it, so what is left is the line the cursor is on. + PressEscape(). + Press(keys.Universal.IncreaseContextInDiffView). + Tap(func() { + t.ExpectToast(Equals("Changed diff context size to 2")) + }). + Press(keys.Universal.IncreaseContextInDiffView). + Tap(func() { + t.ExpectToast(Equals("Changed diff context size to 3")) + }). + SelectedLines( + Contains(" line26"), + ). + Press(keys.Universal.NextItem). + Press(keys.Universal.RangeSelectUp). + Press(keys.Universal.RangeSelectUp). + Press(keys.Universal.RangeSelectUp). + SelectedLines( + Contains("-line25"), + Contains("+LINE25"), + Contains(" line26"), + Contains(" line27"), + ). + Press(keys.Universal.DecreaseContextInDiffView). + Tap(func() { + t.ExpectToast(Equals("Changed diff context size to 2")) + }). + Press(keys.Universal.DecreaseContextInDiffView). + Tap(func() { + t.ExpectToast(Equals("Changed diff context size to 1")) + }). + SelectedLines( + Contains("-line25"), + ) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index e4df6b23f..ed89e9a03 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -371,6 +371,19 @@ var tests = []*components.IntegrationTest{ main_view.EscapeDismissesSelection, main_view.FileNavigationScrollsToTheTop, main_view.HideSelectionWhenChangesVanish, + main_view.KeepAWrappedLineCoveredAcrossARerender, + main_view.KeepBothHalvesOfAChangeSelected, + main_view.KeepPositionByTheVisibleEndOfASelection, + main_view.KeepPositionInBothPanesWhenChangingContextSize, + main_view.KeepPositionInBothPanesWhenIgnoringWhitespace, + main_view.KeepPositionInBothPanesWhenSwitchingDiffRenderers, + main_view.KeepPositionWhenChangingContextSize, + main_view.KeepPositionWhenIgnoringWhitespace, + main_view.KeepPositionWhenIgnoringWhitespaceRemovesIt, + main_view.KeepPositionWhenSwitchingDiffRenderers, + main_view.KeepPositionWhenTheSelectionIsOffScreen, + main_view.KeepScrollWhenTheDiffCantBeRead, + main_view.KeepSelectedRangeWhenChangingContextSize, main_view.KeepSelectionVisibleWhenDiffShrinks, main_view.NavigateByHunkAndFile, main_view.NoSelectionOverABinaryDiff, diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index 2a9fa4af5..427fd894f 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -98,6 +98,24 @@ type ViewBufferManager struct { // what that task was owed. newContentPending atomic.Bool + // When set, the next command task puts the view back where it was once it has + // re-rendered the content, instead of showing the new render from the top (see + // RenderRestore). It is installed just before the re-render is triggered. + // + // Like newContentPending it outlives the task it was installed for, and for the + // same reason: that task can be stopped and replaced before it ever paints, and + // the replacement, rendering the same content, is then the one that owes the + // user their position. It is cleared by whichever task applies it. Guarded by + // taskIDMutex, like the task key. + restoreForNextTask *RenderRestore + + // When set, the next command task leaves the view's scroll position alone even + // though it renders a different command's output, that output being the same + // content laid out differently (see SetKeepScrollPositionForNextTask). The task + // that starts consumes it, in place of noting that new content is on its way. + // Guarded by taskIDMutex, like the task key. + keepScrollForNextTask bool + // Whether a command task is currently reading content into the view. While // this is true the content is still growing, so callers (e.g. the layout) // must not clamp the view's scroll position to the amount loaded so far. @@ -152,6 +170,80 @@ type LinesToRead struct { Then func() } +// RenderRestore puts a view back where it was when it re-renders content the user +// is already looking at, laid out differently — a different context size, whitespace +// ignored, another diff renderer — instead of showing the new render from the top. +// +// The task reads the new content into an off-screen buffer; the restore says when +// enough of it has arrived to show the remembered position (FirstPaintReady), and +// then finds that position and reveals it (Apply). It is a pair of callbacks rather +// than a scroll position because a different layout of the same content puts the +// remembered line somewhere else, and only the new content itself says where. +type RenderRestore struct { + // FirstPaintReady reports whether enough of the new content has been read for + // the restore to show what it is looking for. It is consulted after each line + // is read, on the task's own goroutine. + FirstPaintReady func() bool + + // Apply runs once, on the UI thread, at the first paint. It finds its target in + // the off-screen content, calls swapIn to promote that content to the display, + // and places the view on the target — in that order, so that the search runs + // while the previous content is still displayed, and the new content is never + // drawn at the previous render's scroll position. + // + // It must call swapIn either way, and reports whether it placed the view: when + // it didn't, because what it was looking for is not in the new content, the + // task does what it would have done without a restore. + Apply func(swapIn func()) bool +} + +// SetRestoreForNextTask arranges for the next command task to put the view back +// where it is now once it has re-rendered. Call it right before triggering a +// re-render of the content the view is showing; see RenderRestore. +func (self *ViewBufferManager) SetRestoreForNextTask(restore *RenderRestore) { + self.taskIDMutex.Lock() + defer self.taskIDMutex.Unlock() + + self.restoreForNextTask = restore +} + +func (self *ViewBufferManager) getRestoreForNextTask() *RenderRestore { + self.taskIDMutex.Lock() + defer self.taskIDMutex.Unlock() + + return self.restoreForNextTask +} + +// SetKeepScrollPositionForNextTask arranges for the next command task to leave the +// view's scroll position alone, rather than showing its content from the top the way a +// render of different content does. Call it right before triggering a re-render of the +// content the view is showing, when the command producing it is not the one that +// produced what is on screen — a different context size, another diff renderer. +// +// It is the coarser sibling of SetRestoreForNextTask, for the same moment. The restore +// puts the view back on the line it remembers, which it can only do when the lines of +// the new rendering can be told apart. This one says merely "the content is a +// rearrangement of what is there, so the offset into it is nearer to where the user was +// than the top is". Both can be set at once, and then the restore has the first say. +func (self *ViewBufferManager) SetKeepScrollPositionForNextTask() { + self.taskIDMutex.Lock() + defer self.taskIDMutex.Unlock() + + self.keepScrollForNextTask = true +} + +// clearRestore drops a restore once a task has applied it, so that it rides exactly +// one re-render. One installed since — the user pressing the key again while this +// task was still reading — is left alone: it belongs to the render on its way. +func (self *ViewBufferManager) clearRestore(restore *RenderRestore) { + self.taskIDMutex.Lock() + defer self.taskIDMutex.Unlock() + + if self.restoreForNextTask == restore { + self.restoreForNextTask = nil + } +} + func (self *ViewBufferManager) GetTaskKey() string { self.taskIDMutex.Lock() defer self.taskIDMutex.Unlock() @@ -272,6 +364,10 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix onFirstPageShown() } + // Whatever position is owed to the user belongs to this render: it was + // remembered just before the re-render that led here was triggered. + restore := self.getRestoreForNextTask() + if self.throttle.Load() { self.Log.Info("throttling task") time.Sleep(THROTTLE_TIME) @@ -370,7 +466,12 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix // content is common (a background refresh over a repo with submodules // that have uncommitted changes, say). The pending flag isn't consumed // here; the first paint still owes the scroll reset. - if !loaded && self.newContentPending.Load() { + // + // A restore keeps the view too: it is there to make a re-render of what + // the user is looking at seamless, and blanking the view for a message + // before putting them back where they were is the flicker it exists to + // avoid. + if !loaded && restore == nil && self.newContentPending.Load() { self.beforeStart() // beforeStart cleared the previous content to show "loading...", so // put the view back at the top for it (beforeStart doesn't touch the @@ -431,6 +532,18 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix return } painted = true + if restore != nil { + // The restore does the swap itself, so that it can find where the + // user was in the new content before it is revealed. + placed := restore.Apply(self.swapInRender) + self.clearRestore(restore) + if placed { + // The view is where the user left it, which is exactly what the + // scroll reset would undo. + self.newContentPending.Store(false) + return + } + } self.swapInRender() if self.newContentPending.Swap(false) { self.resetOrigin() @@ -469,7 +582,13 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix linesToRead.Then() } } - for linesToRead.Total == -1 || linesRead < linesToRead.Total { + // A restore that hasn't painted yet keeps us reading past the lines + // asked for, all the way to the end of the input if need be. What it + // is looking for may be anywhere in the new content, and a rendering + // that has to be parsed as a diff to be searched at all can only be + // parsed whole — so stopping early would leave it nothing to find, + // and the view somewhere the user didn't put it. + for linesToRead.Total == -1 || linesRead < linesToRead.Total || (restore != nil && !painted) { if stopped() { callThen() break outer @@ -540,12 +659,22 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix time.Sleep(slowRenderPerLine) } - if linesRead == linesToRead.InitialRefreshAfter { - // We have read enough lines to fill the view, so do the first paint - // and refresh to show it. Continue reading and refresh again at the + if !painted { + // Do the first paint once we have read enough lines to fill the + // view — or, when a position is waiting to be restored, once the + // restore says it can show it, since where the view should be is + // its call. Continue reading afterwards and refresh again at the // end to make sure the scrollbar has the right size. - _ = self.onUIThread(firstPaint) - refreshViewIfStale() + var ready bool + if restore != nil { + ready = restore.FirstPaintReady() + } else { + ready = linesRead == linesToRead.InitialRefreshAfter + } + if ready { + _ = self.onUIThread(firstPaint) + refreshViewIfStale() + } } } refreshViewIfStale() @@ -672,10 +801,19 @@ func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error // newContentPending), so the previous content — left displayed until the // swap — doesn't visibly jump to the top before the new content appears. // Read taskKey directly: we already hold the mutex that guards it, and - // GetTaskKey would take it again. - if self.taskKey != key && self.resetOrigin != nil { + // GetTaskKey would take it again. A pending restore isn't dropped here + // either, even for a different command: the re-renders it rides are all + // different commands (a different context size, another diff renderer), and + // it validates itself against the content it lands in anyway. + // A task told to keep the scroll position renders the content the view is + // already showing, laid out differently, so the reset it would otherwise owe + // would take the user away from what they are reading — and the loading + // message, which the same flag governs, would blank content that is about to + // come back looking much the same. + if self.taskKey != key && self.resetOrigin != nil && !self.keepScrollForNextTask { self.newContentPending.Store(true) } + self.keepScrollForNextTask = false self.taskKey = key self.taskIDMutex.Unlock() diff --git a/pkg/tasks/tasks_test.go b/pkg/tasks/tasks_test.go index 6cc1cf9d6..4aa3a39d7 100644 --- a/pkg/tasks/tasks_test.go +++ b/pkg/tasks/tasks_test.go @@ -385,6 +385,231 @@ func TestLoadingIndicatorOnlyTakesOverForNewContent(t *testing.T) { 2*time.Second, 10*time.Millisecond) } +// A pending restore takes the first paint over: it says when enough of the new +// content has arrived to show the position it remembers, and does the swap itself so +// that it can look for that position while the previous content is still displayed. +// Having put the view where the user left it, it also keeps the scroll reset that new +// content would otherwise get. +func TestNewCmdTaskRestore(t *testing.T) { + writer := bytes.NewBuffer(nil) + linesWritten := func() int { return strings.Count(writer.String(), "\n") } + resetOrigin, getResetOriginCallCount := getCounter() + + swapped := false + applyCount := 0 + applyAtLines := -1 + swappedBeforeApply := false + swappedByApply := false + + manager := NewViewBufferManager( + utils.NewDummyLog(), + writer, + func() {}, // beforeStart + func() {}, // refreshView + func() {}, // onEndOfInput + resetOrigin, + func() {}, // beginRender + func() { swapped = true }, // swapInRender + func() gocui.Task { return gocui.NewFakeTask() }, + // no UI thread in the test; run the view mutations inline + func(f func()) error { f(); return nil }, + ) + + manager.SetRestoreForNextTask(&RenderRestore{ + // Ready once five lines have loaded — well before the view is filled (30). + FirstPaintReady: func() bool { return linesWritten() >= 5 }, + Apply: func(swapIn func()) bool { + applyCount++ + applyAtLines = linesWritten() + swappedBeforeApply = swappedBeforeApply || swapped + swapIn() + swappedByApply = swapped + return true + }, + }) + + done := make(chan struct{}) + start := func() (Cmd, io.Reader) { + // not actually starting this because it's not necessary + return ExecCmd{Cmd: exec.Command("blah")}, &BlankLineReader{totalLinesToYield: 50} + } + _ = manager.NewTask(manager.NewCmdTask(start, "", LinesToRead{100, 30, nil}, func() { close(done) }), "cmd") + <-done + + assert.Equal(t, 1, applyCount, "Apply should run exactly once") + assert.False(t, swappedBeforeApply, "the off-screen render should not be swapped in before Apply runs") + assert.True(t, swappedByApply, "Apply should swap the off-screen render in via swapIn") + // The first paint was driven by the restore, not by having read enough lines to + // fill the view. + assert.GreaterOrEqual(t, applyAtLines, 5) + assert.Less(t, applyAtLines, 30) + assert.Equal(t, 0, getResetOriginCallCount(), "a restore that placed the view leaves the scroll alone") +} + +// A restore that never finds what it is looking for keeps the task reading to the +// end of its input, since the line might have been anywhere in it. Once there is no +// more content to hope for, the render is revealed with the scroll reset that new +// content is owed. +func TestNewCmdTaskRestoreThatFindsNothing(t *testing.T) { + writer := bytes.NewBuffer(nil) + linesWritten := func() int { return strings.Count(writer.String(), "\n") } + resetOrigin, getResetOriginCallCount := getCounter() + + applyCount := 0 + swappedAtLines := -1 + + manager := NewViewBufferManager( + utils.NewDummyLog(), + writer, + func() {}, // beforeStart + func() {}, // refreshView + func() {}, // onEndOfInput + resetOrigin, + func() {}, // beginRender + func() { swappedAtLines = linesWritten() }, + func() gocui.Task { return gocui.NewFakeTask() }, + // no UI thread in the test; run the view mutations inline + func(f func()) error { f(); return nil }, + ) + + manager.SetRestoreForNextTask(&RenderRestore{ + FirstPaintReady: func() bool { return false }, + Apply: func(swapIn func()) bool { + applyCount++ + swapIn() + return false + }, + }) + + done := make(chan struct{}) + start := func() (Cmd, io.Reader) { + // not actually starting this because it's not necessary + return ExecCmd{Cmd: exec.Command("blah")}, &BlankLineReader{totalLinesToYield: 50} + } + _ = manager.NewTask(manager.NewCmdTask(start, "", LinesToRead{100, 30, nil}, func() { close(done) }), "cmd") + <-done + + assert.Equal(t, 1, applyCount, "Apply should still run, to swap the render in") + assert.Equal(t, 50, swappedAtLines, "the whole input should be read before giving up on the restore") + assert.Equal(t, 1, getResetOriginCallCount(), "new content the restore couldn't place starts at the top") +} + +// The task a restore was installed for can be stopped and replaced before it ever +// paints — a background refresh landing right after the key was pressed. The +// replacement renders the same content, so it is the one that owes the user their +// position. +func TestRestoreSurvivesTaskReplacement(t *testing.T) { + var applyCount atomic.Int32 + + manager := NewViewBufferManager( + utils.NewDummyLog(), + io.Discard, + func() {}, + func() {}, + func() {}, + func() {}, + func() {}, + func() {}, + func() gocui.Task { return gocui.NewFakeTask() }, + // no UI thread in the test; run the view mutations inline + func(f func()) error { f(); return nil }, + ) + + manager.SetRestoreForNextTask(&RenderRestore{ + FirstPaintReady: func() bool { return false }, + Apply: func(swapIn func()) bool { + applyCount.Add(1) + swapIn() + return true + }, + }) + + startTask := func(reader io.Reader, onDone func()) { + start := func() (Cmd, io.Reader) { + // not actually starting this because it's not necessary + return ExecCmd{Cmd: exec.Command("blah")}, reader + } + _ = manager.NewTask(manager.NewCmdTask(start, "", LinesToRead{100, 50, nil}, onDone), "cmd") + } + + // The task the restore was installed for stalls before it can paint. + stalled := BlockingLineReader{ + linesToYield: 3, + blocked: make(chan struct{}), + unblock: make(chan struct{}), + } + defer close(stalled.unblock) + startTask(&stalled, nil) + <-stalled.blocked + + done := make(chan struct{}) + startTask(&BlankLineReader{totalLinesToYield: 3}, func() { close(done) }) + <-done + + assert.EqualValues(t, 1, applyCount.Load(), "the replacement should apply the restore the stopped task couldn't") +} + +// A task told to keep the scroll position renders the content the view is showing +// under another command — the same diff with more context around it, say — so it +// neither resets the scroll nor blanks the view to say "loading...", both of which are +// for content the user hasn't seen. +func TestKeepScrollPositionForNextTask(t *testing.T) { + var beforeStartCount atomic.Int32 + resetOrigin, getResetOriginCallCount := getCounter() + + manager := NewViewBufferManager( + utils.NewDummyLog(), + io.Discard, + func() { beforeStartCount.Add(1) }, + func() {}, // refreshView + func() {}, // onEndOfInput + resetOrigin, + func() {}, // beginRender + func() {}, // swapInRender + func() gocui.Task { return gocui.NewFakeTask() }, + // no UI thread in the test; run the view mutations inline + func(f func()) error { f(); return nil }, + ) + + startTask := func(key string, reader io.Reader, onDone func()) { + start := func() (Cmd, io.Reader) { + // not actually starting this because it's not necessary + return ExecCmd{Cmd: exec.Command("blah")}, reader + } + _ = manager.NewTask(manager.NewCmdTask(start, "", LinesToRead{100, 50, nil}, onDone), key) + } + runTaskToCompletion := func(key string) { + done := make(chan struct{}) + startTask(key, &BlankLineReader{totalLinesToYield: 3}, func() { close(done) }) + <-done + } + + // Content the view wasn't showing, to have something to keep the position in. + runTaskToCompletion("cmd1") + assert.Equal(t, 1, getResetOriginCallCount()) + + manager.SetKeepScrollPositionForNextTask() + runTaskToCompletion("cmd2") + assert.Equal(t, 1, getResetOriginCallCount(), "the same content under another command keeps its position") + + // And the request rides one task only: the next different command is a different + // diff as far as anyone knows. + runTaskToCompletion("cmd3") + assert.Equal(t, 2, getResetOriginCallCount()) + + // The loading indicator goes by the same question, so it stays out of the way too. + manager.SetKeepScrollPositionForNextTask() + stalled := BlockingLineReader{ + blocked: make(chan struct{}), + unblock: make(chan struct{}), + } + defer close(stalled.unblock) + startTask("cmd4", &stalled, nil) + <-stalled.blocked + time.Sleep(500 * time.Millisecond) + assert.EqualValues(t, 0, beforeStartCount.Load()) +} + func TestNewCmdTaskRefresh(t *testing.T) { type scenario struct { name string