Fix crash when deleting with multiple cursors

[pascalkuthe]

Fixes #6021
Fixes #5272
Fixes #6029

The delete_char_backward command delete indentation when at the start of the line. The problem is that this can cause multiple overlaing deletions with multiple cursors. For example in #6021 one of the cursors is after the 3. space and one after the 1. char. Because the indent unit is 4 spaces delete_char_backward deletes to the start of the line for both cursors. The generated transaction is therefore invalid which causes an integer underflow when the transaction is applied.

I fixed a similar crash in #5728. Mutlicursors potentially generating overlapping changes is a common pitfall and something we should be wary of. In that PR I simply discarded changes that overlapped a previous change. That seemed the right behaviour to me for the replacements created by completions. For deletions however we can simply merge the overlapping deletions together (otherwise the behaviour here would be incorrect). To that end I added a new delete and delete_by_selection function which does not accept a tendril for replacement and allows overlapping ranges.

In the process I noticed that the delete_by_selection_insert_mode function was implemented a bit weirdly:

• It requires a selection rather then a closure like Transaction::change_by_selection which requires cloneing the selection
• Calles generating transaction causes some weirdlooking code
• It doesn't automatically invoke lsp::signature_help_impl(cx, SignatureHelpInvoked::Automatic); eventough all callsites do (wihout doing that its barely wort moving the logic to a shared function)

Therefore I cleaned up that function a little in a second commit as I was touching that code anyways (and there were no open PRs that touch that code).
If we don't want these cosmetic changes I can easily drop that second commit.

[gabydd]

thanks for fixing this Pascal, it also fixes #5272

[pascalkuthe]

thanks for fixing this Pascal, it also fixes #5272

Thanks for confirming that 👍 That issue was on my todo too so I get two for one here 😄

[pascalkuthe]

I added a fix for #6029 here as the refactor in 3edcc5d made that easier to implement.
I added a detailed explanation in the commit message of f0f2dae.

The TLDR is:

Deleting forward in append mode used to delete one char forward and then delete backwards, because the selection was shrunk. I have changed the command to always delete forward. The following example of pressing <del> repeatedly in append mode explains it well I think.

What I expect to happen (and how it behaves with this PR):

|abcd|ef
|abce|f
|abcf|
|abc |

This is inline with how other editors like kakoune work.
However, helix currently moves the selection backwards leading to the
following behavior:

|abcd|ef
|abc|ef
|ab|ef
|e|f

[the-mikedavis]

Just a few minor comments, otherwise I think this looks good

[pascalkuthe]

(force pushed to keep the history intact since I did put some effort into commit messages here :D)

[dead10ck]

Ok the changes themselves look good to me. I would humbly request regression tests to codify expected behavior, but not sure if you or others feel this is worth blocking on.

I spent a little time thinking over the larger API—it doesn't quite feel right to fix the function that generates invalid transactions by adding another function that doesn't and changing specific call sites to that instead. But looking over the change function, I can see why we can't just move the from marker forward in the case of overlap, since this could just end up inserting text in the wrong place.

My first instinct was to suggest that we either

1. Just keep adding and deleting text separate, or
2. Fix this issue in change and just make delete a thin wrapper around it

to fix even the possibility of this happening through the API. But 1 would make cases like R very awkward, and 2 I can't think of a reasonable way to handle overlapping ranges when it handles both deletions and text replacement, if it's even possible, without making it super complex.

So at the end of the day, having another function that relaxes the overlapping ranges invariant for text deletions gets the job done.

[pascalkuthe]

@dead10ck I add some integration tests that was a great suggestions. We should really make more use of the integration test framework it works really great. I was pretty much just able to use the reproduction cases straight from the issues (plus some additional tests).

Regarding the API I actually think having a second function makes sense. For normal edits (replacements) there is no unambiguous way to merge overlapping edits. Instead callet must deal with that (or we panic). There is no correct way to handle this generally in change. Even separating the deleting and adding doesn't really fix the problem. The callers would map multiple positions to the same location in that case but there is still no unambiguous way to now add that new text (should it be concatenated? or spliced somehow? In whaynorder?). Ao again we would have just turned an explicit panic into weirdly/silently incorrect changes.

I spent a lot of time thinking about a better API here when I created this PR but I came to the conclusion that there really is none. Overlapping edits are fundamentally incorrect and must be prevented in the caller. The only exceptions are:

• deletions (where you can just merge the ranges as done here)
• "foreign" edits (like completions) over which we have no control. There we simply use the first range and discard all subsequent overlapping ranges (a decent fallback strategy but this doesn't work for deletions and some other cases)

When I created this PR I actually manually went trough all callsites to the change/change_by_selection and manually checked that this can't be a problem there. As far as I found all callsites were correct (in most cases edit ranges are just completions in which case overlapping edits are impossible and in the cases where they were possible the functions had explicit checks to deal with that). Only deletions were buggy.

[dead10ck]

Thanks a lot for adding tests! This will help a lot in the future for anyone who makes changes to deleting behavior.

Also, I agree with your points about the API and adding a function. It does make sense to have both.

[dead10ck]

Ok, this looks good to me! Thanks for adding tests and bearing with me. This will help when I get to auto pair deletions.