From 42bbe965fa3f6d800d383c47ec2e15cfc9db29e3 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 1 Jul 2025 17:06:12 +0200 Subject: [PATCH 1/4] Render only + and - lines as included in a patch It is confusing to get header lines, hunk headers, or context lines rendered as being included in a custom patch, when including these makes no difference to the patch. This is only a visual change; internally, we still record these non-patch lines as being included in the patch. It doesn't matter though; you can press space on a header line and nothing happens. It would probably be cleaner to only record + and - lines in the includedLines array, but that would be a bit more work, and doesn't seem worth it. --- pkg/commands/patch/format.go | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/pkg/commands/patch/format.go b/pkg/commands/patch/format.go index c61c8ef05..ccfc7924d 100644 --- a/pkg/commands/patch/format.go +++ b/pkg/commands/patch/format.go @@ -72,30 +72,22 @@ func (self *patchPresenter) format() string { lineIdx++ } - appendFormattedLine := func(line string, style style.TextStyle) { - formattedLine := self.formatLine( - line, - style, - lineIdx, - ) - - appendLine(formattedLine) - } for _, line := range self.patch.header { - appendFormattedLine(line, theme.DefaultTextColor.SetBold()) + // always passing false for 'included' here because header lines are not part of the patch + appendLine(self.formatLineAux(line, theme.DefaultTextColor.SetBold(), false)) } for _, hunk := range self.patch.hunks { appendLine( - self.formatLine( + self.formatLineAux( hunk.formatHeaderStart(), style.FgCyan, - lineIdx, + false, ) + // we're splitting the line into two parts: the diff header and the context - // We explicitly pass 'included' as false here so that we're only tagging the - // first half of the line as included if the line is indeed included. + // We explicitly pass 'included' as false for both because these are not part + // of the actual patch self.formatLineAux( hunk.headerContext, theme.DefaultTextColor, @@ -104,7 +96,12 @@ func (self *patchPresenter) format() string { ) for _, line := range hunk.bodyLines { - appendFormattedLine(line.Content, self.patchLineStyle(line)) + style := self.patchLineStyle(line) + if line.isChange() { + appendLine(self.formatLine(line.Content, style, lineIdx)) + } else { + appendLine(self.formatLineAux(line.Content, style, false)) + } } } From ef1a141347045b1c3d1d8ad36b510af92730ae03 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 3 Jul 2025 18:10:16 +0200 Subject: [PATCH 2/4] Export PatchLine.isChange --- pkg/commands/patch/format.go | 2 +- pkg/commands/patch/patch_line.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/commands/patch/format.go b/pkg/commands/patch/format.go index ccfc7924d..afd09df4b 100644 --- a/pkg/commands/patch/format.go +++ b/pkg/commands/patch/format.go @@ -97,7 +97,7 @@ func (self *patchPresenter) format() string { for _, line := range hunk.bodyLines { style := self.patchLineStyle(line) - if line.isChange() { + if line.IsChange() { appendLine(self.formatLine(line.Content, style, lineIdx)) } else { appendLine(self.formatLineAux(line.Content, style, false)) diff --git a/pkg/commands/patch/patch_line.go b/pkg/commands/patch/patch_line.go index 994d7c4e9..78eef3a19 100644 --- a/pkg/commands/patch/patch_line.go +++ b/pkg/commands/patch/patch_line.go @@ -18,7 +18,7 @@ type PatchLine struct { Content string // something like '+ hello' (note the first character is not removed) } -func (self *PatchLine) isChange() bool { +func (self *PatchLine) IsChange() bool { return self.Kind == ADDITION || self.Kind == DELETION } From ce9fbe58b2688846e7c1e1041d2e92b876e2cf71 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 3 Jul 2025 18:24:04 +0200 Subject: [PATCH 3/4] Toggle only added/deleted lines in patch building view This improves the experience when selecting a hunk generously with the mouse, by dragging over it including some context lines above and below. Previously we would consider the "moving end" of the selection range for whether things need to be added or removed, but this doesn't make sense if it's a context line. Now we consider the first actual change line that is included in the range. --- pkg/commands/patch/patch_builder.go | 16 ++++------------ .../controllers/patch_building_controller.go | 18 ++++++++++++------ pkg/gui/patch_exploring/state.go | 14 ++++++++++++++ 3 files changed, 30 insertions(+), 18 deletions(-) diff --git a/pkg/commands/patch/patch_builder.go b/pkg/commands/patch/patch_builder.go index 1edce0741..466b3da09 100644 --- a/pkg/commands/patch/patch_builder.go +++ b/pkg/commands/patch/patch_builder.go @@ -124,14 +124,6 @@ func (p *PatchBuilder) RemoveFile(filename string) error { return nil } -func getIndicesForRange(first, last int) []int { - indices := []int{} - for i := first; i <= last; i++ { - indices = append(indices, i) - } - return indices -} - func (p *PatchBuilder) getFileInfo(filename string) (*fileInfo, error) { info, ok := p.fileInfoMap[filename] if ok { @@ -152,24 +144,24 @@ func (p *PatchBuilder) getFileInfo(filename string) (*fileInfo, error) { return info, nil } -func (p *PatchBuilder) AddFileLineRange(filename string, firstLineIdx, lastLineIdx int) error { +func (p *PatchBuilder) AddFileLineRange(filename string, lineIndices []int) error { info, err := p.getFileInfo(filename) if err != nil { return err } info.mode = PART - info.includedLineIndices = lo.Union(info.includedLineIndices, getIndicesForRange(firstLineIdx, lastLineIdx)) + info.includedLineIndices = lo.Union(info.includedLineIndices, lineIndices) return nil } -func (p *PatchBuilder) RemoveFileLineRange(filename string, firstLineIdx, lastLineIdx int) error { +func (p *PatchBuilder) RemoveFileLineRange(filename string, lineIndices []int) error { info, err := p.getFileInfo(filename) if err != nil { return err } info.mode = PART - info.includedLineIndices, _ = lo.Difference(info.includedLineIndices, getIndicesForRange(firstLineIdx, lastLineIdx)) + info.includedLineIndices, _ = lo.Difference(info.includedLineIndices, lineIndices) if len(info.includedLineIndices) == 0 { p.removeFile(info) } diff --git a/pkg/gui/controllers/patch_building_controller.go b/pkg/gui/controllers/patch_building_controller.go index 94f987404..87328a15a 100644 --- a/pkg/gui/controllers/patch_building_controller.go +++ b/pkg/gui/controllers/patch_building_controller.go @@ -126,7 +126,6 @@ func (self *PatchBuildingController) toggleSelection() error { self.context().GetMutex().Lock() defer self.context().GetMutex().Unlock() - toggleFunc := self.c.Git().Patch.PatchBuilder.AddFileLineRange filename := self.c.Contexts().CommitFiles.GetSelectedPath() if filename == "" { return nil @@ -134,19 +133,26 @@ func (self *PatchBuildingController) toggleSelection() error { state := self.context().GetState() + // Get added/deleted lines in the selected patch range + lineIndicesToToggle := state.ChangeLinesInSelectedPatchRange() + if len(lineIndicesToToggle) == 0 { + // Only context lines or header lines selected, so nothing to do + return nil + } + includedLineIndices, err := self.c.Git().Patch.PatchBuilder.GetFileIncLineIndices(filename) if err != nil { return err } - currentLineIsStaged := lo.Contains(includedLineIndices, state.GetSelectedPatchLineIdx()) - if currentLineIsStaged { + + toggleFunc := self.c.Git().Patch.PatchBuilder.AddFileLineRange + firstSelectedChangeLineIsStaged := lo.Contains(includedLineIndices, lineIndicesToToggle[0]) + if firstSelectedChangeLineIsStaged { toggleFunc = self.c.Git().Patch.PatchBuilder.RemoveFileLineRange } // add range of lines to those set for the file - firstLineIdx, lastLineIdx := state.SelectedPatchRange() - - if err := toggleFunc(filename, firstLineIdx, lastLineIdx); err != nil { + if err := toggleFunc(filename, lineIndicesToToggle); err != nil { // might actually want to return an error here self.c.Log.Error(err) } diff --git a/pkg/gui/patch_exploring/state.go b/pkg/gui/patch_exploring/state.go index 53e120849..8c308b3d0 100644 --- a/pkg/gui/patch_exploring/state.go +++ b/pkg/gui/patch_exploring/state.go @@ -282,6 +282,20 @@ func (s *State) SelectedPatchRange() (int, int) { return s.patchLineIndices[start], s.patchLineIndices[end] } +// Returns the line indices of the selected patch range that are changes (i.e. additions or deletions) +func (s *State) ChangeLinesInSelectedPatchRange() []int { + viewStart, viewEnd := s.SelectedViewRange() + patchStart, patchEnd := s.patchLineIndices[viewStart], s.patchLineIndices[viewEnd] + lines := s.patch.Lines() + indices := []int{} + for i := patchStart; i <= patchEnd; i++ { + if lines[i].IsChange() { + indices = append(indices, i) + } + } + return indices +} + func (s *State) CurrentLineNumber() int { return s.patch.LineNumberOfLine(s.patchLineIndices[s.selectedLineIdx]) } From 039831a27adb49d6b299a6c3afa13303d9961ed7 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 1 Jul 2025 17:48:00 +0200 Subject: [PATCH 4/4] Move to next stageable line after adding a line to a custom patch While it's true that the behavior is a little different from the staging panel, where the staged lines are actually removed from the view and in many cases the selection stays more or less in the same place, it is still very useful to move to the next stageable thing in the custom patch building view too. --- pkg/commands/patch/patch.go | 28 +++++++++++----- .../controllers/patch_building_controller.go | 2 ++ pkg/gui/patch_exploring/state.go | 8 +++++ .../patch_building/move_to_index_partial.go | 1 - .../patch_building/specific_selection.go | 33 ++++++++++++------- 5 files changed, 52 insertions(+), 20 deletions(-) diff --git a/pkg/commands/patch/patch.go b/pkg/commands/patch/patch.go index f5efe95da..343eea54f 100644 --- a/pkg/commands/patch/patch.go +++ b/pkg/commands/patch/patch.go @@ -115,15 +115,22 @@ func (self *Patch) HunkContainingLine(idx int) int { return -1 } -// Returns the patch line index of the next change (i.e. addition or deletion). -func (self *Patch) GetNextChangeIdx(idx int) int { +// Returns the patch line index of the next change (i.e. addition or deletion) +// that matches the same "included" state, given the includedLines. If you don't +// care about included states, pass nil for includedLines and false for included. +func (self *Patch) GetNextChangeIdxOfSameIncludedState(idx int, includedLines []int, included bool) (int, bool) { idx = lo.Clamp(idx, 0, self.LineCount()-1) lines := self.Lines() + isMatch := func(i int, line *PatchLine) bool { + sameIncludedState := lo.Contains(includedLines, i) == included + return line.IsChange() && sameIncludedState + } + for i, line := range lines[idx:] { - if line.isChange() { - return i + idx + if isMatch(i+idx, line) { + return i + idx, true } } @@ -131,13 +138,18 @@ func (self *Patch) GetNextChangeIdx(idx int) int { // return the index of the last change for i := len(lines) - 1; i >= 0; i-- { line := lines[i] - if line.isChange() { - return i + if isMatch(i, line) { + return i, true } } - // should not be possible - return 0 + return 0, false +} + +// Returns the patch line index of the next change (i.e. addition or deletion). +func (self *Patch) GetNextChangeIdx(idx int) int { + result, _ := self.GetNextChangeIdxOfSameIncludedState(idx, nil, false) + return result } // Returns the length of the patch in lines diff --git a/pkg/gui/controllers/patch_building_controller.go b/pkg/gui/controllers/patch_building_controller.go index 87328a15a..faddbd5f8 100644 --- a/pkg/gui/controllers/patch_building_controller.go +++ b/pkg/gui/controllers/patch_building_controller.go @@ -161,6 +161,8 @@ func (self *PatchBuildingController) toggleSelection() error { state.SetLineSelectMode() } + state.SelectNextStageableLineOfSameIncludedState(self.context().GetIncludedLineIndices(), firstSelectedChangeLineIsStaged) + return nil } diff --git a/pkg/gui/patch_exploring/state.go b/pkg/gui/patch_exploring/state.go index 8c308b3d0..980f01fda 100644 --- a/pkg/gui/patch_exploring/state.go +++ b/pkg/gui/patch_exploring/state.go @@ -338,3 +338,11 @@ func wrapPatchLines(diff string, view *gocui.View) ([]int, []int) { view.Wrap, view.Editable, strings.TrimSuffix(diff, "\n"), view.InnerWidth(), view.TabWidth) return viewLineIndices, patchLineIndices } + +func (s *State) SelectNextStageableLineOfSameIncludedState(includedLines []int, included bool) { + _, lastLineIdx := s.SelectedPatchRange() + patchLineIdx, found := s.patch.GetNextChangeIdxOfSameIncludedState(lastLineIdx+1, includedLines, included) + if found { + s.SelectLine(s.viewLineIndices[patchLineIdx]) + } +} diff --git a/pkg/integration/tests/patch_building/move_to_index_partial.go b/pkg/integration/tests/patch_building/move_to_index_partial.go index 827a3aedb..409f86a57 100644 --- a/pkg/integration/tests/patch_building/move_to_index_partial.go +++ b/pkg/integration/tests/patch_building/move_to_index_partial.go @@ -48,7 +48,6 @@ var MoveToIndexPartial = NewIntegrationTest(NewIntegrationTestArgs{ Contains(`+third line2`), ). PressPrimaryAction(). - SelectNextItem(). PressPrimaryAction(). Tap(func() { t.Views().Information().Content(Contains("Building patch")) diff --git a/pkg/integration/tests/patch_building/specific_selection.go b/pkg/integration/tests/patch_building/specific_selection.go index 9027a34e6..5b4f62a03 100644 --- a/pkg/integration/tests/patch_building/specific_selection.go +++ b/pkg/integration/tests/patch_building/specific_selection.go @@ -66,18 +66,20 @@ var SpecificSelection = NewIntegrationTest(NewIntegrationTestArgs{ Contains(` 1f`), ). PressPrimaryAction(). - // unlike in the staging panel, we don't remove lines from the patch building panel - // upon 'adding' them. So the same lines will be selected SelectedLines( - Contains(`@@ -1,6 +1,6 @@`), - Contains(`-1a`), - Contains(`+aa`), - Contains(` 1b`), - Contains(`-1c`), - Contains(`+cc`), - Contains(` 1d`), - Contains(` 1e`), - Contains(` 1f`), + Contains(`@@ -17,9 +17,9 @@`), + Contains(` 1q`), + Contains(` 1r`), + Contains(` 1s`), + Contains(`-1t`), + Contains(`-1u`), + Contains(`-1v`), + Contains(`+tt`), + Contains(`+uu`), + Contains(`+vv`), + Contains(` 1w`), + Contains(` 1x`), + Contains(` 1y`), ). Tap(func() { t.Views().Information().Content(Contains("Building patch")) @@ -106,12 +108,21 @@ var SpecificSelection = NewIntegrationTest(NewIntegrationTestArgs{ Contains("+2a"), ). PressPrimaryAction(). + SelectedLines( + Contains("+2b"), + ). NavigateToLine(Contains("+2c")). Press(keys.Universal.ToggleRangeSelect). NavigateToLine(Contains("+2e")). PressPrimaryAction(). + SelectedLines( + Contains("+2f"), + ). NavigateToLine(Contains("+2g")). PressPrimaryAction(). + SelectedLines( + Contains("+2h"), + ). Tap(func() { t.Views().Information().Content(Contains("Building patch"))