Multi selection search

[haberdashPI]

Hi There!

First of all, thanks for the great extension! I love it, and use it daily. The clarity of your tutorial and documentation made this a joy to extend for my own purposes.

Here is a change that makes it possible to use multiple cursors with the search command. The idea is that it allows you to emulate commands like f and d in vim for each cursor. I am making use of it in my daily work now.

I also have been working on an ability to record "macros"; right now it only records normal mode keys, but seems like it should be possible to handle insert mode recording as well (though I don't know if it would be perfect). I will create a separate pull request for that once I am happy with the functionality.

This would close #5

[johtela]

Hi!

Thanks a lot for contributing! I checked out your pull request and spend quite a while testing the new feature. I have to say that while it kind of works, there are lot of edge cases I am not so happy with. When I first got issue #5, I thought about it briefly, and came up with problems that I could not figure out immediately, like:

1. How the multiple cursor search should behave if matches overlap (the next match for two cursor positions is at the same place)? Should it fail the search, or skip the match and try to find the next one?

2. What happens if the matches cannot be shown in the same screen, that is they are too far apart? Invariably some of them will be hidden in this case.

Your implementation answers question 1. by selecting the same match multiple times which effectively looses multiple cursors. That is a simple solution, but one could argue that it reduces the usefulness of the feature as this happens quite often. Jumping to previous or next match also looses multiple cursors - at the latest when a search string is not found any more. Skipping same matches would complicate the logic of the search considerably, so I understand why you didn't take that approach :-)

Regarding question 2, your implementation just shows the primary selection. Again, kinda works but editing a range of text you can't see is not something most users would do, so this also reduces the utility of the feature.

This was my reasoning back then why I gave up on the feature. I can see the value of it in a scenario, where you use modaledit.search to implement Vim's t or f commands; to jump to a same character in multiple lines, and then multiedit all the lines at the same time. There is, however, a simple workflow that achieves the same effect in most cases. Start with a single cursor, use modaledit.search to jump the text you want to edit on the first line, then user editor.action.addSelectionToNextFindMatch to add cursors to the following lines, and finally do the editing.

Third issue that I found is that nextMatch() function uses the primary selection (editor.selections[0]) to determine the value of the delta parameter for the highlightNextMathch() function. This will not work because the delta should shift the search start position for each range separately depending on whether it is before or after the cursor position. I spent quite a while trying to get the nextMatch() to work when the selection jumps across the search start position. The delta parameter is bit of a hack, but seemed to work in testing. Thinking of all the cases how nextMatch() should behave when you have multiple search start positions and cursor positions frankly gives me a headache, so I didn't test it extensively. All I know is that it would require a lot of testing to make sure that it behaves nicely.

So, for all these reasons, unfortunately I have to reject your pull request. The search command is by far the most complicated command in the extension as it is, so I would prefer not to make it any more complex.

I cannot give you an alternative solution how to achieve exactly the same feature, but here are some pointers. If you just want to implement Vim's t and f commands with multiple cursors, you could write a simpler command that just takes the search string as an argument, and jumps to the next occurrence of that string starting from all the cursor positions. This would not be incremental search, but a single command without any stored state - similar to modaledit.selectBetween command. Basically the search would start always from the current cursor positions, and there would not be a need to maintain starting positions or search strings.

You can get the searched character(s) by using a range in the key bindings. An example of this can be found here.

[haberdashPI]

Thanks for your very careful review and feedback. I have some thoughts on your comments.

TL;DR: I am open to trying to think through this more and generate a design that would be to both our likings w.r.t to the issues you raise. I think it is a worthwhile feature to have. However, if you're settled on this not being the right approach for you, we can just agree to disagree and I'll fork and add features on my own. Let me know what you think!

As I understand your comments, you raise 2 design questions and have identified a bug in my implementation. With respect to the design questions:

1. Handling overlapping matches: I could see something more sophisticated here, but yes, I think this (relatively simple) approach works well in practice (at least for the ways I personally have been using this). I could see an argument for improving this by halting next/prev match movement when you reach the limit of the file (or, wraparound, but that could be super confusing). The advantage being next/prev would be reversible.

2. Selections moving outside of the viewport: this seems like an inherent design limitation of multiple selections, and it is the primary reason they can't replace keyboard macros for all use cases. This problem can arise without the incremental search feature: just add more cursors for a given search term than can fit in the window. My expectation as a user would be that this would "work": even if I can't see the cursors, I wouldn't want an implementation to just drop cursors when they are out of the viewport. If one had infinite flexibility in UX I could image some sort of focused view showing only lines with a cursor on them (doubt that's reasonable as an extension to vscode, and would be more of a possible improvement to the core editor capabilities).

With respect to the bug you point out:

Basically, if I understand correctly, you're talking about a scenario in which there are a different number of search tokens in the neighborhood of each cursor, so when you search forwards and backwards the cursor could be at a different position relative to the start of the search. If I understand correctly, I think the immediate issue is fixable: rather than using a delta, represent the start of the search as a range rather than a position, and use the start or end of the range depending on the direction of the search, on a per cursor basis. Or did I miss something about how the delta works?

Though If I get your real problem with this, it is that you're not convinced all edge cases have been considered: i.e. there could be more bugs you haven't thought of.

However, I am trying to imagine a scenario in which multi-cursor searching would actually be useful for the sort of situation where the search goes awry in the way you describe. In general the benefit of searching with multiple selections is that you can identify tokens in the neighborhood of a symbol, on the basis of shared syntactic structure at the different locations (e.g. quickly move to the next parenthesis, move to the next if statement, move to the next else statement); if there are a different number of search tokens near one of the cursors, then the structure of the text near that cursor is necessarily different than the others, and so it's not clear that any mutations you made at those location could possibly useful.

My point is that any situation where the cursors cross paths or go in different directions would mean that you are editing text within each selection location that isn't similar in structure and the whole point of multi-selection would disappear. Searching is really only useful when the behavior at each cursor is uniform. Ideally, the implementation should handle those situations where it isn't uniform gracefully, so that reversing an action works as expected, but as a user I've already entered an undesired state if this happens in the first place and good old "cursor" or "soft" undo works just fine.

I don't think implementing this only for the use case of t or f from vim would be very helpful, due to the use cases mentioned above (e.g. searching for else or if). So in my view, it really does need to work for incremental searching. Just searching first doesn't help either, because the token you want to land on might be far more copious (e.g. there are only a few instances of mySpecialVariable but many ( or if tokens).

Clearly there is some work to improve these issues with this PR. But I'm happy to try out some approaches to make those improvements if you think the collaboration would be worthwhile. Just let me know!

[johtela]

Hi again!

Sorry for being unresponsive for a while. Life has kept me busy with other things than programming lately. This is a lengthy response, so I divide it to separate sections trying to make my points clearer.

Disappearing cursors

I have nothing against the idea of the PR in general. I just prefer that commands have clear specifications on how they work in all use cases. In this case the behavior of the search command needs to be specified so that it looks for the next occurrence of the search string starting from all cursor positions and selects the next match. If the matches overlap for some cursor positions, the selections will merge, and duplicate cursors will be removed.

Cursors out of view port

Also the second issue should be documented as a caveat that you cannot always see all cursor locations while the incremental search is active. That might spawn a feature request that you could jump between the cursors while the search is active using tab key, for example. You could bind the tab key to editor.action.moveSelectionToNextFindMatch command while the modaledit.searching context is on (By default this command is bound to ctrk+k ctrl+d).

General guidance about PRs

The point is that, instead of these features being accidental they should be acknowledged and documented. That's why I have spent the effort of writing the code in literate style, so that all of these assumptions would be stated in the documentation. As a practical guidance to submitting PR's my wish is that all of these changes in functionality would be documented in the comments. Then it would be easy to see that all of the aspects have been considered and taken care of.

The problem in the nextMatch function

This brings me to the delta argument which is currently not well specified. That's why I called it a hack. The problem I was trying to solve with it is the fact that modaledit.nextMatch and modaledit.previousMatch do not work as expected right now. I am currently working on a semi-complete Vim presets (#7) and implementing , and ; commands has proven to be very difficult using current functionality. The problem is that the cursor gets stuck, because the search string is actually under the cursor, and next/previousMatch does not jump to the next occurrence as expected.

The problem with current implementation of nextMatch is again that it is not well specified. I have an idea how to fix it, and I'll try that when I have time. If it works, I will write detailed description in comments, what the behavior is. So, I would wait until that is done before implementing the multiple cursor support. Then its operation would be consistent with behavior with a single cursor.

Adding wrap functionality

The last challenge I throw in is issue #8 which boils down to adding a new search parameter that would wrap the search around when the cursor reaches end or beginning of file. That is relatively easy to implement with a single cursor, but with multiple cursors the functionality would get quite confusing as only some of the cursors would wrap. This would kind of remedy the problem of cursors disappearing when the search reaches the end of file. But the fact that some of the cursors would be at the beginning of the file and some at the end would make tracking the cursors impossible without some way of jumping to them while the search is active.

How to proceed?

My suggestion for game plan regarding this feature would be:

1. Refactor the nextMatch function to have well specified behavior (I will do this anyway)
2. Implement search wrapping and close issue [Bug?] Search doesn't search entire file #8 (This is up to grabs, if you want to give it a shot).
3. Implement the support for multiple cursors. You can basically redo the changes on top of the updated code. Specify the behavior in the comments/documentation. The principle would be that each cursor would jump to the next/previous match exactly the same way, if it were the only cursor. I.e. the search does not consider where the other cursors go when computing the next search location. This can cause that the selections overlap, and some cursors to disappear.

How does that sound?

[haberdashPI]

Thanks again for the thorough response. I'm totally sympathetic to the time lag. That sounds like a reasonable plan to me.

I can pick things up for 2-3 once you are done with 1. In the revised PR, I understand that you want the literate documentation to explain the behavior and identify the limitations (e.g. selection overlap, cursors disappearing).

Later on, I'd be interested in implementing the feature to change which cursor is primary (and therefore where the viewport is located) with e.g. tab (or whatever binding a user prefers), that's something I'd been thinking of anyways (as I'm really inspired by Kakoune's approach to modal editing).

[haberdashPI]

If I understand correctly, you've now implemented this feature in the latest master. I think this is safe to close, right?