Move to next stageable line when adding a line to a custom patch (#4675)

- **PR Description**

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.

Also, we change the visualization of what's included in the patch to
mark only the + and - lines of the patch; for all other lines it doesn't
make a difference whether they are included.

And finally, we make it so that only + and - lines are considered when
pressing space; previously it would also look at selected context lines,
which doesn't make much sense. This improves the experience for mouse
users who like to generously select hunks by dragging across them,
including some context lines above and below.

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))
+			}
 		}
 	}
 

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

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)
 	}

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
 }
 

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)
 	}
@@ -155,6 +161,8 @@ func (self *PatchBuildingController) toggleSelection() error {
 		state.SetLineSelectMode()
 	}
 
+	state.SelectNextStageableLineOfSameIncludedState(self.context().GetIncludedLineIndices(), firstSelectedChangeLineIsStaged)
+
 	return nil
 }
 

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])
 }
@@ -324,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])
+	}
+}

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"))

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"))