Improve hunk selection mode in staging view (#4684)

- **PR Description** Hunk selection mode is one of the features that many people don't know about, because it is not very discoverable. You can switch to it from line selection mode by pressing `a` in the staging view. The problem with this mode is that it selects entire hunks, where hunks are defined to be sections of the diff starting with `@@`. Very often, hunks consist of multiple distinct blocks of changes, separated by context lines. For example, with the default diff context size of 3 it takes at least 6 unchanged lines between blocks of changes for them to be separated into distinct hunks; if there are 5 or less unchanged lines between them, they are grouped into one hunk. And of course, if you increase the diff context size by pressing `}`, you will get even fewer hunks. Now, most of the time I want to navigate between the individual blocks of changes in a diff, regardless of how git groups them into hunks. That's what this PR does: when pressing `a`, the selection is extended to just the current group of changes, separated by context lines; you can easily stage it by pressing space, and the selection will move on to the next block of changes. Actual hunks no longer play a role here. Also, in line selection mode the right/left arrow keys now move between blocks of changes rather than actual hunks. I find this new behavior so useful that I almost always switch to hunk mode right away after entering the staging view. It saves a lot of keystrokes, since it is very rare that I want to select only some lines of a block of adjacent changes. This makes me wonder whether we should enable hunk mode by default when entering staging, but that's going to be another PR.

Author: stefanhaller

.vscode/tasks.json

@@ -24,22 +24,24 @@
  {
  "label": "Run current file integration test",
  "type": "shell",
- "command": "go generate pkg/integration/tests/tests.go && go run cmd/integration_test/main.go cli ${relativeFile}",
+ "command": "go run cmd/integration_test/main.go cli ${relativeFile}",
  "problemMatcher": [],
  "group": {
  "kind": "test",
  "isDefault": true
  },
  "presentation": {
+ "clear": true,
  "focus": true
  }
  },
  {
  "label": "Run current file integration test (slow)",
  "type": "shell",
- "command": "go generate pkg/integration/tests/tests.go && go run cmd/integration_test/main.go cli --slow ${relativeFile}",
+ "command": "go run cmd/integration_test/main.go cli --slow ${relativeFile}",
  "problemMatcher": [],
  "group": {
+ "clear": true,
  "kind": "test",
  },
  "presentation": {
@@ -49,12 +51,13 @@
  {
  "label": "Run current file integration test (sandbox)",
  "type": "shell",
- "command": "go generate pkg/integration/tests/tests.go && go run cmd/integration_test/main.go cli --sandbox ${relativeFile}",
+ "command": "go run cmd/integration_test/main.go cli --sandbox ${relativeFile}",
  "problemMatcher": [],
  "group": {
  "kind": "test",
  },
  "presentation": {
+ "clear": true,
  "focus": true
  }
  },

pkg/gui/controllers/patch_explorer_controller.go

@@ -223,13 +223,13 @@ func (self *PatchExplorerController) HandleNextLineRange() error {
 }
 
 func (self *PatchExplorerController) HandlePrevHunk() error {
-self.context.GetState().CycleHunk(false)
+self.context.GetState().SelectPreviousHunk()
 
 return nil
 }
 
 func (self *PatchExplorerController) HandleNextHunk() error {
-self.context.GetState().CycleHunk(true)
+self.context.GetState().SelectNextHunk()
 
 return nil
 }

pkg/gui/patch_exploring/state.go

@@ -125,6 +125,11 @@ func (s *State) ToggleSelectHunk() {
 s.selectMode = LINE
 } else {
 s.selectMode = HUNK
+
+// If we are not currently on a change line, select the next one (or the
+// previous one if there is no next one):
+s.selectedLineIdx = s.viewLineIndices[s.patch.GetNextChangeIdx(
+s.patchLineIndices[s.selectedLineIdx])]
 }
 }
 
@@ -203,25 +208,49 @@ func (s *State) DragSelectLine(newSelectedLineIdx int) {
 
 func (s *State) CycleSelection(forward bool) {
 if s.SelectingHunk() {
-s.CycleHunk(forward)
+if forward {
+s.SelectNextHunk()
+} else {
+s.SelectPreviousHunk()
+}
 } else {
 s.CycleLine(forward)
 }
 }
 
-func (s *State) CycleHunk(forward bool) {
-change := 1
-if !forward {
-change = -1
+func (s *State) SelectPreviousHunk() {
+patchLines := s.patch.Lines()
+patchLineIdx := s.patchLineIndices[s.selectedLineIdx]
+nextNonChangeLine := patchLineIdx
+for nextNonChangeLine >= 0 && patchLines[nextNonChangeLine].IsChange() {
+nextNonChangeLine--
 }
-
-hunkIdx := s.patch.HunkContainingLine(s.patchLineIndices[s.selectedLineIdx])
-if hunkIdx != -1 {
-newHunkIdx := hunkIdx + change
-if newHunkIdx >= 0 && newHunkIdx < s.patch.HunkCount() {
-start := s.patch.HunkStartIdx(newHunkIdx)
-s.selectedLineIdx = s.viewLineIndices[s.patch.GetNextChangeIdx(start)]
+nextChangeLine := nextNonChangeLine
+for nextChangeLine >= 0 && !patchLines[nextChangeLine].IsChange() {
+nextChangeLine--
+}
+if nextChangeLine >= 0 {
+// Now we found a previous hunk, but we're on its last line. Skip to the beginning.
+for nextChangeLine > 0 && patchLines[nextChangeLine-1].IsChange() {
+nextChangeLine--
 }
+s.selectedLineIdx = s.viewLineIndices[nextChangeLine]
+}
+}
+
+func (s *State) SelectNextHunk() {
+patchLines := s.patch.Lines()
+patchLineIdx := s.patchLineIndices[s.selectedLineIdx]
+nextNonChangeLine := patchLineIdx
+for nextNonChangeLine < len(patchLines) && patchLines[nextNonChangeLine].IsChange() {
+nextNonChangeLine++
+}
+nextChangeLine := nextNonChangeLine
+for nextChangeLine < len(patchLines) && !patchLines[nextChangeLine].IsChange() {
+nextChangeLine++
+}
+if nextChangeLine < len(patchLines) {
+s.selectedLineIdx = s.viewLineIndices[nextChangeLine]
 }
 }
 
@@ -259,11 +288,34 @@ func (s *State) CurrentHunkBounds() (int, int) {
 return start, end
 }
 
+func (s *State) selectionRangeForCurrentBlockOfChanges() (int, int) {
+patchLines := s.patch.Lines()
+patchLineIdx := s.patchLineIndices[s.selectedLineIdx]
+
+patchStart := patchLineIdx
+for patchStart > 0 && patchLines[patchStart-1].IsChange() {
+patchStart--
+}
+
+patchEnd := patchLineIdx
+for patchEnd < len(patchLines)-1 && patchLines[patchEnd+1].IsChange() {
+patchEnd++
+}
+
+viewStart, viewEnd := s.viewLineIndices[patchStart], s.viewLineIndices[patchEnd]
+
+// Increase viewEnd in case the last patch line is wrapped to more than one view line.
+for viewEnd < len(s.patchLineIndices)-1 && s.patchLineIndices[viewEnd] == s.patchLineIndices[viewEnd+1] {
+viewEnd++
+}
+
+return viewStart, viewEnd
+}
+
 func (s *State) SelectedViewRange() (int, int) {
 switch s.selectMode {
 case HUNK:
-start, end := s.CurrentHunkBounds()
-return s.viewLineIndices[start], s.viewLineIndices[end]
+return s.selectionRangeForCurrentBlockOfChanges()
 case RANGE:
 if s.rangeStartLineIdx > s.selectedLineIdx {
 return s.selectedLineIdx, s.rangeStartLineIdx

pkg/integration/tests/patch_building/specific_selection.go

@@ -55,31 +55,13 @@ var SpecificSelection = NewIntegrationTest(NewIntegrationTestArgs{
 ).
 Press(keys.Main.ToggleSelectHunk).
 SelectedLines(
-Contains(`@@ -1,6 +1,6 @@`),
 Contains(`-1a`),
 Contains(`+aa`),
-Contains(` 1b`),
-Contains(`-1c`),
-Contains(`+cc`),
-Contains(` 1d`),
-Contains(` 1e`),
-Contains(` 1f`),
 ).
 PressPrimaryAction().
 SelectedLines(
-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`),
+Contains(`-1c`),
+Contains(`+cc`),
 ).
 Tap(func() {
 t.Views().Information().Content(Contains("Building patch"))
@@ -154,8 +136,7 @@ var SpecificSelection = NewIntegrationTest(NewIntegrationTestArgs{
 Contains(`-1a`),
 Contains(`+aa`),
 Contains(` 1b`),
-Contains(`-1c`),
-Contains(`+cc`),
+Contains(` 1c`),
 Contains(` 1d`),
 Contains(` 1e`),
 Contains(` 1f`),

pkg/integration/tests/staging/diff_context_change.go

@@ -52,67 +52,40 @@ var DiffContextChange = NewIntegrationTest(NewIntegrationTestArgs{
 IsFocused().
 Press(keys.Main.ToggleSelectHunk).
 SelectedLines(
-Contains(`@@ -1,6 +1,6 @@`),
-Contains(` 1a`),
-Contains(` 2a`),
 Contains(`-3a`),
 Contains(`+3b`),
-Contains(` 4a`),
-Contains(` 5a`),
-Contains(` 6a`),
 ).
 Press(keys.Universal.IncreaseContextInDiffView).
 Tap(func() {
 t.ExpectToast(Equals("Changed diff context size to 4"))
 }).
 SelectedLines(
-Contains(`@@ -1,7 +1,7 @@`),
-Contains(` 1a`),
-Contains(` 2a`),
 Contains(`-3a`),
 Contains(`+3b`),
-Contains(` 4a`),
-Contains(` 5a`),
-Contains(` 6a`),
-Contains(` 7a`),
 ).
 Press(keys.Universal.DecreaseContextInDiffView).
 Tap(func() {
 t.ExpectToast(Equals("Changed diff context size to 3"))
 }).
 SelectedLines(
-Contains(`@@ -1,6 +1,6 @@`),
-Contains(` 1a`),
-Contains(` 2a`),
 Contains(`-3a`),
 Contains(`+3b`),
-Contains(` 4a`),
-Contains(` 5a`),
-Contains(` 6a`),
 ).
 Press(keys.Universal.DecreaseContextInDiffView).
 Tap(func() {
 t.ExpectToast(Equals("Changed diff context size to 2"))
 }).
 SelectedLines(
-Contains(`@@ -1,5 +1,5 @@`),
-Contains(` 1a`),
-Contains(` 2a`),
 Contains(`-3a`),
 Contains(`+3b`),
-Contains(` 4a`),
-Contains(` 5a`),
 ).
 Press(keys.Universal.DecreaseContextInDiffView).
 Tap(func() {
 t.ExpectToast(Equals("Changed diff context size to 1"))
 }).
 SelectedLines(
-Contains(`@@ -2,3 +2,3 @@`),
-Contains(` 2a`),
 Contains(`-3a`),
 Contains(`+3b`),
-Contains(` 4a`),
 ).
 PressPrimaryAction().
 Press(keys.Universal.TogglePanel)
@@ -121,18 +94,14 @@ var DiffContextChange = NewIntegrationTest(NewIntegrationTestArgs{
 IsFocused().
 Press(keys.Main.ToggleSelectHunk).
 SelectedLines(
-Contains(`@@ -2,3 +2,3 @@`),
-Contains(` 2a`),
 Contains(`-3a`),
 Contains(`+3b`),
-Contains(` 4a`),
 ).
 Press(keys.Universal.DecreaseContextInDiffView).
 Tap(func() {
 t.ExpectToast(Equals("Changed diff context size to 0"))
 }).
 SelectedLines(
-Contains(`@@ -3,1 +3 @@`),
 Contains(`-3a`),
 Contains(`+3b`),
 ).
@@ -141,24 +110,16 @@ var DiffContextChange = NewIntegrationTest(NewIntegrationTestArgs{
 t.ExpectToast(Equals("Changed diff context size to 1"))
 }).
 SelectedLines(
-Contains(`@@ -2,3 +2,3 @@`),
-Contains(` 2a`),
 Contains(`-3a`),
 Contains(`+3b`),
-Contains(` 4a`),
 ).
 Press(keys.Universal.IncreaseContextInDiffView).
 Tap(func() {
 t.ExpectToast(Equals("Changed diff context size to 2"))
 }).
 SelectedLines(
-Contains(`@@ -1,5 +1,5 @@`),
-Contains(` 1a`),
-Contains(` 2a`),
 Contains(`-3a`),
 Contains(`+3b`),
-Contains(` 4a`),
-Contains(` 5a`),
 )
 },
 })