Implemented new actions SelectWord, SelectNextTextOccurrence, SelectPreviousTextOccurrence

[masmu]

This PR adds 3 new actions:

• SelectWord
  Just selects the word the cursor is currently on.
  If the character the cursor is currently on is not a word character or there is already a selection the actions returns false otherwise true.
  The code doing this was already existing, just the according action was added.
• SelectNextTextOccurrence
  In case there is already a selection this actions searches for the next text occurrence of the selected text and changes the selection to this location. In case there is no selection or there are multiple cursors this actions returns false.
• SelectPreviousTextOccurrence
  Same as for SelectNextTextOccurrence

So the following keybinding selects the word you might be on with the first press, the second press selects the next / previous text occurrence.

  "AltShiftLeft": "SelectWord|SelectPreviousTextOccurrence",
  "AltShiftRight": "SelectWord|SelectNextTextOccurrence",

Here the first press selects word (in case there is not selection already) and immediately selects the next / prev occurrence.

  "AltShiftLeft": "SelectWord,SelectPreviousTextOccurrence",
  "AltShiftRight": "SelectWord,SelectNextTextOccurrence",

There is one issue with the current implementation.
The original behavior of h.Cursor.SelectWord() extended the selection by one character to the right in case the cursor was not on a word character.

func (c *Cursor) SelectWord() {
	...
	if !util.IsWordChar(c.RuneUnder(c.X)) {
		c.SetSelectionStart(c.Loc)
		c.SetSelectionEnd(c.Loc.Move(1, c.buf))
		c.OrigSelection = c.CurSelection
		return
	}
	...
}

To embrace the power of chaining this was changed to just return in this case. But maybe there is a deeper thought behind this.

[Neko-Box-Coder]

I am not sure if I am missing something but what's the difference between SelectNextTextOccurrence and Find + FindNext. Aren't they doing the same thing (i.e. Find the next occurrence of selected text and selects it) ?

[masmu]

They are similar but SelectNextTextOccurrence and SelectPreviousTextOccurrence do this without opening a search prompt. They are entirely based on text selections. That's where SelectWord comes into play. It ensures (if possible) that there is a selection.
The first keystroke in the video executes SelectWord, which selects the word under the cursor (since there is no current text selection), and then the action chain is terminated. On all subsequent key presses there is already a selection, so SelectWord runs but does nothing and Select*TextOccurrence is executed.

  "AltShiftLeft": "SelectWord|SelectPreviousTextOccurrence",
  "AltShiftRight": "SelectWord|SelectNextTextOccurrence",

micro-SelectTextOccurrence.webm

[Neko-Box-Coder]

I understand the purpose of adding SelectWord and the fact that Find brings up a prompt even if there's a selection.

But apart from that, FindNext and FindPreviouis already do what SelectNextTextOccurrence and SelectPreviousTextOccurrence are doing, i.e. Find and select the next/previous occurrence.

Like for example if you duplicate/modify the Find behavior to not have prompt when there's a selection (and maybe not jump to the next occurrence), you essentially have what you are asking for, i.e.

"AltShiftLeft": "SelectWord&FindSelected&FindPrevious",
"AltShiftRight": "SelectWord&FindSelected&FindNext"

Unless I am understanding wrong, I think what you really want is to have an action that selects a word (SelectWord) and another action that populates the LastSearch in Buffer with the selection if any.

[masmu]

Actually Select*TextOccurrence is a replication of a very standard text editor function. It is called editor.action.nextSelectionMatchFindAction in vscode, vim::MoveToNext in zed, etc...

These actions must be distinguished from a search.
You are right that you could use a search to create something similar. But it would come with some downsides.

Depending on your color theme and your hlsearch setting you would highlight all other matches as well. And I would need to press ESC to reset the search and get rid of the highlighted matches.

I don't mind adding an additional action that sets the current selection as the search term. I am not 100% certain but I think this is not possible right now, not without opening the search prompt. So this might be a nice addition.

[Neko-Box-Coder]

Actually Select*TextOccurrence is a replication of a very standard text editor function. It is called editor.action.nextSelectionMatchFindAction in vscode, vim::MoveToNext in zed, etc...

Depending on your color theme and your hlsearch setting you would highlight all other matches as well. And I would need to press ESC to reset the search and get rid of the highlighted matches.

editor.action.nextSelectionMatchFindAction in vscode is doing what I mentioned where it populates the search with the selection and do FindNext/FindPrevious. You can see the search box popped up at the corner and the other matches are highlighted as well (at least for my vscode and settings).

I don't mind adding an additional action that sets the current selection as the search term. I am not 100% certain but I think this is not possible right now, not without opening the search prompt. So this might be a nice addition.

Or maybe an option to not prompt you if there's a selection and another option not to jump to it. Should be pretty straightforward to do.

I understand your use case but personally I am still not too sure about adding new actions (Select*TextOccurrence) that have mostly the same logic as existing actions (FindNext/FindPrevious), unless the use case justifies and unique enough where many people want it.

TL;DR
I am totally onboard with SelectWord and maybe adding more options to allow flexibility for the current Find action, but not too sure on Select*TextOccurrence since they are too similar to existing Find* actions.

But of course, the ultimate decisions are on the maintainers and I am just giving my opinions.

[masmu]

No question, Select*TextOccurrence was developed to implement the function without affecting the search. That was the goal. I really like the idea of FindSelected so that people who want to use the search (with all the consequences such as highlighting) can do so. This way we should get the best of both approches.

I get the impression that you are trying to minimize code duplication at all costs. But IMHO it's not necessarily a bad thing to have a little duplicated code here and there. It really helps to decouple things from each other and increases readability by not adding extra conditions or wrappers. Finding the right balance is key.

This can also be expressed in numbers: BufPane.find() already has a value of 21 for cognitive complexity (using gocognit), which can be considered complex. If you add two additional settings, this value rockets to 34 (extremely complex). In my experience, in those situations you should ask yourself if it's really worth it and usually refactor the whole thing to reduce the complexity to a minimum.
It also introduces other problems. What if I want to do a normal search? Do I have to change my settings for that?

My implementation adds two actions with the cognitive complexity of 4 each (which is good), keeps the search entirely decoupled but adds a bit of code duplication. From my point of view the better deal.

Please don't get me wrong. I greatly appreciate your commitment but I think your implementation has some serious flaws because of the mentioned reasons.

[masmu]

One of my biggest concerns is the naming of the two. I would like to call them SelectNext and SelectPrevious according to the current naming convention.
But that is so generalized that you actually do not know what is really being selected. Imaging some day we add treesitter and do get support for code symbols. I could think of actions like SelectSymbol and SelectNextSymbol.

[Neko-Box-Coder]

I get the impression that you are trying to minimize code duplication at all costs. But IMHO it's not necessarily a bad thing to have a little duplicated code here and there. It really helps to decouple things from each other and increases readability by not adding extra conditions or wrappers. Finding the right balance is key.

While it would be great to minimize code duplication, that was not my main concern. Nor the complexity or simplicity it adds.

My concern was does it makes sense to add the actions you are proposing given the niche use case and other editors (I know of) use the search mechanism to achieve the same thing which was what I was suggesting.

Would it make more sense to implement the Select*TextOccurrence actions as a plugin instead? Given you don't want to modify the search history.

All the suggestions I made were just trying to benefit a wider audience, not just your use case. They may not be perfect but would definitely be easier for people to utilize it for their own use case, just like yours, either with config json or lua script.

[masmu]

It's actually one of my mini plugins, and I like it so much that I thought it might be of use to others as well. But if you really think it's bloatware and not a contribution, I don't mind not adding it.

I really appreciate the work that has been done recently on ParagraphPrevious, ParagraphNext or SkipMultiCursor. I personally won't use them, but I like the fact that I can use them if I want to.
Isn't that one of the things that makes a good editor? That you can configure it the way you want without having to code everything yourself?

[Neko-Box-Coder]

It's actually one of my mini plugins, and I like it so much that I thought it might be of use to others as well. But if you really think it's bloatware and not a contribution, I don't mind not adding it.

I might be over-thinking this but I don't know, it just feels weird to me to have another find-like actions but more barebone. That being said...

Isn't that one of the things that makes a good editor? That you can configure it the way you want without having to code everything yourself?

yeah, you have a point and I agree with it. It doesn't hurt to add these actions in I think. It is good to have options as you said and pick & choose the ones you want.

One of my biggest concerns is the naming of the two. I would like to call them SelectNext and SelectPrevious according to the current naming convention.
But that is so generalized that you actually do not know what is really being selected. Imaging some day we add treesitter and do get support for code symbols. I could think of actions like SelectSymbol and SelectNextSymbol.

SelectNextTextOccurrence for me sounds like it will select the next text we are currently finding, because it doesn't explicitly say it will select the next text we are currently selecting.

Stealing vscode editor.action.nextSelectionMatchFindAction idea, would any of these be better?

• SelectNextSelectionMatch
• SelectNextMatchedSelection
• SelectNextSelection
• MatchSelectionSelectNext

or we can drop the select word since FindNext and FindPrevious is already selecting, maybe you can do the same?

• MatchSelectionNext
• MatchSelectedNext
• NextSelectionMatch
• NextMatchedSelection

[usfbih8u]

You can emulate this behavior using multicursors.

Use SpawnMultiCursor (default: Alt-n) to select the word under the cursor and then use SkipMultiCursor (default: Alt-x) to go to the next. And to go back use the SkipMultiCursorBack that you (@masmu) implemented in #3404 (thanks btw).