feat: nextMatchFromCursor, PrevSearchMatchFromCursor

[phanen]

PR Description

When in log/file panel, often I want to goto prevmatch/nextmatch from the cursor.

gocui pr: jesseduffield/gocui#91

This feature is not enabled by default, to enable it use:

keybinding:
  universal:
    nextMatch: ""
    prevMatch: ""
    nextMatchFromCursor: "n"
    prevMatchFromCursor: "N"

Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go generate ./...)
• Code has been formatted (see here)
• Tests have been added/updated (see here for the integration test guide)
• Text is internationalised (see here)
• If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
• Docs have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[stefanhaller]

I'm not sure a separate keybinding is the best solution for this. I could imagine always searching from the cursor downwards, for those views that have a selection. If you only press n, n, n without moving the selection in between, the behavior would be as today; but if you do move the selection, it will continue searching from there, which I would say is pretty much always the desired behavior. What do you think?

I'm a little less sure what to do about views that don't have a selection (e.g. the focused main view), but I think the behavior would be desirable there, as well. If you press / and see a lot of yellow highlighted results, and then scroll down the view with the wheel, and then press n, I think it is reasonable to expect searching from the topmost visible line downwards.

[phanen]

. If you only press n, n, n without moving the selection in between, the behavior would be as today; but if you do move the selection, it will continue searching from there, which I would say is pretty much always the desired behavior.

This sounds the same behavior as the new navigation in this pr, which is also Vim's default, also Emacs... don't seem allow cursor move without abort in once isearch.

I'm a little less sure what to do about views that don't have a selection (e.g. the focused main view), but I think the behavior would be desirable there, as well. If you press / and see a lot of yellow highlighted results, and then scroll down the view with the wheel, and then press n, I think it is reasonable to expect searching from the topmost visible line downwards.

Didn't consider it before, scroll only but not move selection?

[phanen]

push a new change to use bisect to get the nearest prev/next.
also fix when a line contain multiple match.

Test on linux repo, no noticable perfmance change.

If you like I can replace next/prev with this. I add a new key bind cause I'm also not sure if someone would prefer to old way.

[stefanhaller]

Yes, I'm reasonably sure that the new behavior is better, and that we don't need to make it configurable.

I'm wondering if it is better to update the "match x of y" status right away when changing the selection (including highlighting the new current match), I feel that this makes it more predictable. I made a draft PR trying this out: #5169. In general I like the behavior; there are just a few edge cases that are a bit weird, e.g. typing < to go to the first line; this will highlight the first search result, which may be a few pages down, in which case pressing n jumps to the second result rather than the first as you might expect. Not sure what to do about that.

Didn't consider it before, scroll only but not move selection?

No, that's not what I meant. If you only scroll with the mouse wheel but don't move the selection, nothing should change; this is consistent with e.g. scrolling the selection out of view and then pressing down, which moves to the next line from where it was, and makes it visible again.

No, I was talking about views that don't have a selection at all, like the diff view of a commit (which you can search in by pressing 0 to focus it). But maybe we can leave those simply as is.

[phanen]

No, I was talking about views that don't have a selection at all, like the diff view of a commit (which you can search in by pressing 0 to focus it). But maybe we can leave those simply as is.

right, this pr don't work in that case.

Tested #5169, it's different because currently if you update immediately, you have to update to previous entry to ensure n will goto next match below the cursor. So in that pr, N won't go to previous match, and if go to previous of the first match, n will also jump to the second one. To fix this maybe need two state: "true match" and "display match".

vim/nvim by default won't live update the match messsage on cursor move. With a plugin (https://github.com/kevinhwang91/nvim-hlslens) use another way to display: normally it show [current/total], but when you move your cursor it can show [n next/total] or [N prev/total] (n/N is determined by the last moving direction).

[stefanhaller]

So in that pr, N won't go to previous match, and if go to previous of the first match, n will also jump to the second one.

I added a commit that fixes these: 65c06fa. I think the jumping behavior is now the same as in your PR, but it still selects the nearest search result as you move the selection, which I guess is the best of both worlds.

but when you move your cursor it can show [n next/total] or [N prev/total] (n/N is determined by the last moving direction).

I feel that's overkill.

I also added two more commits that fixes existing bugs (bf2780b and fc19fb7), and then I added a commit (b7f0f85) that tries to select visible search results as you scroll a view that doesn't have a selection. As soon as the current search result leaves the visible area of the view, it tries to select the first one (or last one, if you scroll up) that is now visible. It might be a bit controversial whether this is the best behavior, but I kind of like it so far. Let me know how you feel about this.

[phanen]

Many Thanks. Two more advice:

We can highlight the nearest one (prefer belownext if equal from previous/next):

func (v *View) nearestSearchPosition() int {
	currentLineIndex := v.cy + v.oy
	index := sort.Search(len(v.searcher.searchPositions), func(i int) bool {
		return v.searcher.searchPositions[i].Y >= currentLineIndex
	})

	if index == 0 {
		return 0
	}
	if index == len(v.searcher.searchPositions) {
		return index - 1
	}

	deltaBefore := currentLineIndex - v.searcher.searchPositions[index-1].Y
	deltaAfter := v.searcher.searchPositions[index].Y - currentLineIndex

	if deltaBefore < deltaAfter {
		return index - 1
	}
	return index
}

bisect search, if it's ordered?

@@ -323,12 +323,10 @@ func (v *View) UpdateSearchResults(str string, modelSearchResults []SearchPositi
 			// ...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
-				}
-			}
+			currentIndex = sort.Search(len(v.searcher.searchPositions), func(i int) bool {
+				pos := v.searcher.searchPositions[i]
+				return pos.Y > adjustedY || (pos.Y == adjustedY && pos.XStart > adjustedX)
+			})
 		}
 		v.searcher.currentSearchIndex = currentIndex
 	}

[stefanhaller]

We can highlight the nearest one (prefer belownext if equal from previous/next):

We could do that, and I can see how it can sometimes make sense. However, there are cases where it feels very weird that the highlighted result changes as you move the cursor between two items (especially if one of them isn't visible), and in general it feels more stable and predictable to me if we always highlight the last match before the cursor, so I'd like to keep it that way.

bisect search, if it's ordered?

I would only do that if performance measurements suggest that it's a noticeable improvement. In many cases, linear search is actually faster than binary search (because of better cache locality), especially when the data set is small or you only need few iterations until you hit the target, as is typically the case here.

[phanen]

Ok. I thought go library should handle small scale case but actually it doesn't. Also search seems not the bottlenecks here. I think this can be closed now. Thanks again.