Fix highlighting the current search match (#6182)

Fixes several minor issues with how search results are highlighted. See
commit messages for details.

pkg/gocui/view.go

@@ -557,6 +557,11 @@ func (v *View) GetSearchStatus() (int, int) {
 	return v.searcher.currentSearchIndex, len(v.searcher.searchPositions)
 }
 
+// UpdateSearchResults runs the search for str again, for content or model search results
+// that have changed. It doesn't move the view. The match at the cursor becomes the
+// current one, in the same way as when the cursor moves; see SetNearestSearchPosition.
+// If the view doesn't show a selection, the current match stays the same.
+//
 // modelSearchResults is optional; pass nil to search the view. If non-nil,
 // these positions will be used for highlighting search results. Even in this
 // case the view will still be searched on a per-line basis, so that the caller
@@ -575,34 +580,56 @@ func (v *View) UpdateSearchResults(str string, modelSearchResults []SearchPositi
 	v.searcher.search(str, modelSearchResults)
 	v.updateSearchPositions()
 
-	if len(v.searcher.searchPositions) > 0 {
-		// get the first result past the current cursor
-		currentIndex := 0
-		if v.Highlight {
-			// ...but only if we're showing the highlighted line
-			adjustedY := v.oy + v.cy
-			adjustedX := v.ox + v.cx
-			for i, pos := range v.searcher.searchPositions {
-				if pos.Y > adjustedY || (pos.Y == adjustedY && pos.XStart > adjustedX) {
-					currentIndex = i
-					break
-				}
-			}
-		}
-		v.searcher.currentSearchIndex = currentIndex
+	if v.Highlight && len(v.searcher.searchPositions) > 0 {
+		v.searcher.currentSearchIndex = v.nearestSearchPosition()
 	}
 }
 
+// Search starts a search for str and goes to its first match after the cursor. See
+// UpdateSearchResults for what modelSearchResults is.
 func (v *View) Search(str string, modelSearchResults []SearchPosition) {
-	v.UpdateSearchResults(str, modelSearchResults)
-
-	if len(v.searcher.searchPositions) > 0 {
+	if v.startSearch(str, modelSearchResults) {
 		v.SelectSearchResult(v.searcher.currentSearchIndex)
 	} else {
 		v.renderSearchStatus(0, 0)
 	}
 }
 
+// startSearch runs a new search for str and makes its first match after the cursor the
+// current one. It returns whether there are any matches.
+func (v *View) startSearch(str string, modelSearchResults []SearchPosition) bool {
+	v.writeMutex.Lock()
+	defer v.writeMutex.Unlock()
+
+	v.searcher.search(str, modelSearchResults)
+	v.updateSearchPositions()
+
+	if len(v.searcher.searchPositions) == 0 {
+		return false
+	}
+	if v.Highlight {
+		v.searcher.currentSearchIndex = v.firstSearchPositionAfterCursor()
+	} else {
+		// If the view doesn't show a selection, the search starts at the first match.
+		v.searcher.currentSearchIndex = 0
+	}
+	return true
+}
+
+// firstSearchPositionAfterCursor returns the first match after the cursor, counting one
+// on the cursor's line if it starts right of the cursor. If there is none, it returns
+// the first match, so that the search wraps around.
+func (v *View) firstSearchPositionAfterCursor() int {
+	cursorY := v.oy + v.cy
+	cursorX := v.ox + v.cx
+	for i, pos := range v.searcher.searchPositions {
+		if pos.Y > cursorY || (pos.Y == cursorY && pos.XStart > cursorX) {
+			return i
+		}
+	}
+	return 0
+}
+
 func (v *View) ClearSearch() {
 	v.searcher.clearSearch()
 }
@@ -611,8 +638,15 @@ func (v *View) IsSearching() bool {
 	return v.searcher.searchString != ""
 }
 
+// nearestSearchPosition returns the current match if it is on the cursor's line.
+// Otherwise it returns the first match on that line, or else the last match above it,
+// or the first match if there is none above.
 func (v *View) nearestSearchPosition() int {
 	currentLineIndex := v.cy + v.oy
+	if v.searcher.searchPositions[v.searcher.currentSearchIndex].Y == currentLineIndex {
+		return v.searcher.currentSearchIndex
+	}
+
 	lastSearchPos := 0
 	for i, pos := range v.searcher.searchPositions {
 		if pos.Y == currentLineIndex {
@@ -702,11 +736,12 @@ func calculateNewOrigin(selectedLine int, oldOrigin int, lineCount int, viewHeig
 	return oldOrigin
 }
 
+// search sets what to search for. It leaves the current match for the caller to choose
+// once the positions have been worked out.
 func (s *searcher) search(str string, modelSearchResults []SearchPosition) {
 	s.searchString = str
 	s.searchPositions = []SearchPosition{}
 	s.modelSearchResults = modelSearchResults
-	s.currentSearchIndex = 0
 }
 
 func (s *searcher) clearSearch() {

pkg/integration/tests/commit/search_keeps_the_current_match_on_refresh.go

@@ -0,0 +1,57 @@
+package commit
+
+import (
+	"fmt"
+
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+var SearchKeepsTheCurrentMatchOnRefresh = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "With the selection between two search matches, a refresh keeps the match that the selection made the current one",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	SetupConfig: func(cfg *config.AppConfig) {
+		// So that R refreshes in the commits panel instead of rewording the commit
+		cfg.GetUserConfig().Keybinding.Commits.RenameCommitWithEditor = config.Keybinding{"<disabled>"}
+	},
+	SetupRepo: func(shell *Shell) {
+		shell.NewBranch("branch")
+		for i := 1; i <= 8; i++ {
+			message := fmt.Sprintf("commit %02d", i)
+			if i == 7 || i == 3 {
+				message += " needle"
+			}
+			shell.EmptyCommit(message)
+		}
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Commits().
+			Focus().
+			FilterOrSearch("needle").
+			SelectedLines(
+				Contains("commit 07 needle"),
+			).
+			NavigateToLine(Contains("commit 05")).
+			Tap(func() {
+				t.Views().Search().Content(Contains("matches for 'needle' (1 of 2)"))
+
+				t.GlobalPress(keys.Universal.Refresh)
+				t.Views().Search().Content(
+					Contains("matches for 'needle' (1 of 2)"))
+			}).
+			NavigateToLine(Contains("commit 01")).
+			Tap(func() {
+				t.Views().Search().Content(Contains("matches for 'needle' (2 of 2)"))
+
+				t.GlobalPress(keys.Universal.Refresh)
+				t.Views().Search().Content(
+					Contains("matches for 'needle' (2 of 2)"))
+			}).
+			// The next match wraps around to the first one
+			Press(keys.Universal.NextMatch).
+			SelectedLines(
+				Contains("commit 07 needle"),
+			)
+	},
+})

pkg/integration/tests/commit/search_keeps_the_second_match_on_a_line_on_refresh.go

@@ -0,0 +1,47 @@
+package commit
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+var SearchKeepsTheSecondMatchOnALineOnRefresh = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "With the second of two search matches on the selected commit as the current one, a refresh keeps it current",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	SetupConfig: func(cfg *config.AppConfig) {
+		// So that R refreshes in the commits panel instead of rewording the commit
+		cfg.GetUserConfig().Keybinding.Commits.RenameCommitWithEditor = config.Keybinding{"<disabled>"}
+	},
+	SetupRepo: func(shell *Shell) {
+		shell.NewBranch("branch")
+		shell.EmptyCommit("commit 01")
+		shell.EmptyCommit("commit 02 needle")
+		shell.EmptyCommit("commit 03")
+		shell.EmptyCommit("commit 04 needle and needle")
+		shell.EmptyCommit("commit 05")
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Commits().
+			Focus().
+			FilterOrSearch("needle").
+			SelectedLines(
+				Contains("commit 04 needle and needle"),
+			).
+			Press(keys.Universal.NextMatch).
+			SelectedLines(
+				Contains("commit 04 needle and needle"),
+			).
+			Tap(func() {
+				t.Views().Search().Content(Contains("matches for 'needle' (2 of 3)"))
+
+				t.GlobalPress(keys.Universal.Refresh)
+				t.Views().Search().Content(
+					Contains("matches for 'needle' (2 of 3)"))
+			}).
+			Press(keys.Universal.NextMatch).
+			SelectedLines(
+				Contains("commit 02 needle"),
+			)
+	},
+})

pkg/integration/tests/main_view/search_over_a_commit_log_keeps_the_current_match_on_refresh.go

@@ -0,0 +1,34 @@
+package main_view
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+var SearchOverACommitLogKeepsTheCurrentMatchOnRefresh = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "Searching a branch's commit log in the main view, which shows no selection, keeps the current match when a refresh renders the log again",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	SetupConfig:  func(config *config.AppConfig) {},
+	SetupRepo: func(shell *Shell) {
+		shell.CreateNCommits(3)
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Branches().
+			Focus().
+			Press(keys.Universal.FocusMainView)
+
+		t.Views().Main().
+			IsFocused().
+			SelectionIsHidden().
+			FilterOrSearch("commit-0").
+			Press(keys.Universal.NextMatch).
+			Tap(func() {
+				t.Views().Search().Content(Contains("matches for 'commit-0' (2 of 3)"))
+
+				t.GlobalPress(keys.Universal.Refresh)
+				t.Views().Search().Content(
+					Contains("matches for 'commit-0' (2 of 3)"))
+			})
+	},
+})

pkg/integration/tests/test_list.go

@@ -174,6 +174,8 @@ var tests = []*components.IntegrationTest{
 	commit.RevertWithConflictSingleCommit,
 	commit.Reword,
 	commit.Search,
+	commit.SearchKeepsTheCurrentMatchOnRefresh,
+	commit.SearchKeepsTheSecondMatchOnALineOnRefresh,
 	commit.SetAuthor,
 	commit.SetAuthorRange,
 	commit.StageRangeOfLines,
@@ -502,6 +504,7 @@ var tests = []*components.IntegrationTest{
 	main_view.ScrollDiffHorizontally,
 	main_view.SearchCollapsesTheSelection,
 	main_view.SearchFollowsTheSelection,
+	main_view.SearchOverACommitLogKeepsTheCurrentMatchOnRefresh,
 	main_view.SearchScrollsToAMatchOffTheEdge,
 	main_view.SelectBelowALongCommitMessage,
 	main_view.SelectBelowALongDiffstat,